build: Added configurations to apply constraints to dependencies resolves #134#159
build: Added configurations to apply constraints to dependencies resolves #134#159Thedrogon wants to merge 5 commits intoMovingBlocks:developfrom Thedrogon:add-constraint-branch
resolves #134#159Conversation
…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
Thedrogon
changed the title
Thedrogon
changed the title
resolves #134
BenjaminAmos
left a comment
BenjaminAmos
left a comment
There was a problem hiding this comment.
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.
...alt-asset-core/src/main/java/org/terasology/gestalt/assets/AbstractFragmentDataProducer.java
Outdated
Show resolved
Hide resolved
gestalt-asset-core/src/main/java/org/terasology/gestalt/assets/Asset.java
Outdated
Show resolved
Hide resolved
gestalt-asset-core/src/main/java/org/terasology/gestalt/assets/AssetType.java
Outdated
Show resolved
Hide resolved
gestalt-asset-core/src/main/java/org/terasology/gestalt/assets/AssetType.java
Show resolved
Hide resolved
… 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 commit 43923eb.
…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
Outdated
Show resolved
Hide resolved
|
@BenjaminAmos does this look good to you now? apart from commented stuff which imo should be better removed ... |
BenjaminAmos
left a comment
BenjaminAmos
left a comment
There was a problem hiding this comment.
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.
| 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.
Despite being deprecated, I'm afraid that for the time being we do still need to use the AccessController APIs. Gestalt sandboxes modules using SecurityManager as an additional precaution and until it's completely unusable I'd rather have that safety net than not. The AccessController.doPrivileged calls are needed when running with a SecurityManager active.
There was a problem hiding this comment.
Then for the requirement I will revert them in the PR.
| * 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.
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. |
3 participants
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