-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
resolve temporary constraint exclude of errorprone("com.google.guava:guava")
#5039
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
version { | ||
require("33.4.8-jre") | ||
} | ||
because("Older versions use deprecated methods in sun.misc.Unsafe") |
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 remember recently seeing this sun.misc.Unsafe
to be an issue seen in spot, but not here, despite using the same version.
Assuming this is not needed anymore, right?
Its passing assemble
, which is failing when removing some of the disable
items.
Its just an warning not an error so the cases are different.
WARNING: A terminally deprecated method in sun.misc.Unsafe has been called
WARNING: sun.misc.Unsafe::objectFieldOffset has been called by com.google.common.util.concurrent.AbstractFuture$UnsafeAtomicHelper (file:/Users/vincent.potucek/.gradle/caches/modules-2/files-2.1/com.google.guava/guava/33.4.0-jre/3fcc0a259f724c7de54a6a55ea7e26d3d5c0cac/guava-33.4.0-jre.jar)
WARNING: Please consider reporting this to the maintainers of class com.google.common.util.concurrent.AbstractFuture$UnsafeAtomicHelper
WARNING: sun.misc.Unsafe::objectFieldOffset will be removed in a future release
vs
FAILURE: Build failed with an exception.
* What went wrong:
Execution failed for task ':lib:compileJava'.
> Compilation failed; see the compiler output below.
/Users/vincent.potucek/IdeaProjects/spotless/lib/src/main/java/com/diffplug/spotless/java/ModuleHelper.java:37: warning: Unsafe is internal proprietary API and may be removed in a future release
import sun.misc.Unsafe;
^
1 error
100 warnings
BUILD FAILED in 22s
14:58:12: Execution finished 'clean assemble'.
maybe then keeping it makes sense again.
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.
The constraint is not an "exclude" but an update of a transitive dependency that's still in action:
https://ge.junit.org/s/miswateogm3po/dependencies?dependencies=guava&expandAll&focusedDependencyView=versions
Without it, noisy warnings are printed during the build:
https://ge.junit.org/s/mvdx45wqr7tom/console-log#L77-L80
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.
Thanks yes updated the doc to prevent any furhter...
9190617
to
f531277
Compare
options.errorprone { | ||
val shouldDisableErrorProne = java.toolchain.implementation.orNull == JvmImplementation.J9 | ||
if (name == "compileJava" && !shouldDisableErrorProne) { | ||
disableAllChecks = !(name == "compileJava" && java.toolchain.implementation.orNull != JvmImplementation.J9) |
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.
disableAllChecks = !(name == "compileJava" && java.toolchain.implementation.orNull != JvmImplementation.J9) | |
disableAllChecks = !(name == "compileJava" && java.toolchain.implementation.orNull != JvmImplementation.J9) |
these braces over the whole term cost me lots of time. SOC is always best.
This way we can reduce the field overhead and be straight forward, avoiding the extra context of shouldDisableErrorProne and disableAllChecks is kind of the same story.
f531277
to
864451f
Compare
} | ||
nullaway { | ||
if (shouldDisableErrorProne) { | ||
if (disableAllChecks.get()) { |
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.
This would disable NullAway for compileTestJava
etc.
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.
do we need the name == "compileJava"
argument, as its also running without?
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.
We want NullAway
for all compile tasks but the other Error Prone rules only for compileJava
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.
thanks for clarification. Bad combo leads to failing build anyways, so this should be kind of the same without the coupling right?
version { | ||
require("33.4.8-jre") | ||
} | ||
because("Older versions use deprecated methods in sun.misc.Unsafe") |
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.
The constraint is not an "exclude" but an update of a transitive dependency that's still in action:
https://ge.junit.org/s/miswateogm3po/dependencies?dependencies=guava&expandAll&focusedDependencyView=versions
Without it, noisy warnings are printed during the build:
https://ge.junit.org/s/mvdx45wqr7tom/console-log#L77-L80
I hereby agree to the terms of the JUnit Contributor License Agreement.
Definition of Done
@API
annotations