- Notifications
You must be signed in to change notification settings - Fork22
build: Added configurations to apply constraints to dependenciesresolves #134#159
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
…ent `gestalt` pulling in dependencies with known CVEs, directly or transitively through things like Reflections. Added some suppressions in various files and as well removed some unnecessary imports
resolves #134There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks for this!
Constraining the dependencies is a useful adjustment but I would rather it was not bundled with other wide-sweeping changes. Suppressing warnings is a last resort as well, since usually we want to see warnings.
Uh oh!
There was an error while loading.Please reload this page.
...alt-asset-core/src/main/java/org/terasology/gestalt/assets/AbstractFragmentDataProducer.java OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
gestalt-asset-core/src/main/java/org/terasology/gestalt/assets/Asset.java OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
gestalt-asset-core/src/main/java/org/terasology/gestalt/assets/AssetType.java OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
gestalt-asset-core/src/main/java/org/terasology/gestalt/assets/AssetType.javaShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
… to prevent `gestalt` pulling in dependencies with known CVEs, directly or transitively through things like Reflections. Added some suppressions in various files and as well removed some unnecessary imports"This reverts commit43923eb.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
reverted the changes .
…s of (deprecated)access-controller so that it can be removed without errors
gestalt-asset-core/src/main/java/org/terasology/gestalt/assets/AssetType.java OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
@BenjaminAmos does this look good to you now? apart from commented stuff which imo should be better removed ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I appreciate you trying to make changes for the better but not all warnings are actionable here for reasons that I have tried to explain.
If you could constrain the changes to that tiny but beneficial gradle constraint improvement then this would be much easier to take in. Removing logic in the code without understanding why it is there (this library is quite a central piece of our games) just makes this pull request unviable as-is, I'm afraid.
| @@ -328,16 +330,16 @@ Optional<T> createInstance(Asset<U> asset) { | |||
| Optional<? extends Asset<U>> result = asset.createCopy(asset.getUrn().getInstanceUrn()); | |||
| if (!result.isPresent()) { | |||
| try { | |||
| return AccessController.doPrivileged((PrivilegedExceptionAction<Optional<T>>) () -> { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Despite being deprecated, I'm afraid that for the time being we do still need to use theAccessController APIs. Gestalt sandboxes modules usingSecurityManager as an additional precaution and until it's completely unusable I'd rather have that safety net than not. TheAccessController.doPrivileged calls are needed when running with aSecurityManager active.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Then for the requirement I will revert them in the PR.
| @@ -131,7 +133,7 @@ public synchronized void close() { | |||
| /** | |||
| * Disposes any assets queued for disposal. This occurs if an asset is no longer referenced by anything. | |||
| */ | |||
| @SuppressWarnings("unchecked") | |||
| //@SuppressWarnings("unchecked") | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You can either remove those suppressions or leave them be but commenting them out like that in committed code is rather messy.
@BenjaminAmos what you think now of this? |
My comments from before are still valid, I think? The changes remove code needed for gestalt to work with |
This also has merge conflicts now, so it would have to be updated regardless. |
Added configurations to apply constraints to the
build.gradledependencies to preventgestaltpulling in dependencies with known CVEs, directly or transitively through things like Reflections. Added some suppressions in various files and as well removed some unnecessary imports