Skip to content

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

resolve temporary constraint exclude of errorprone("com.google.guava:guava") #5039

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 4 additions & 13 deletions gradle/plugins/common/src/main/kotlin/junitbuild.java-nullability-conventions.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,6 @@ plugins {
dependencies {
errorprone(dependencyFromLibs("errorProne-core"))
errorprone(dependencyFromLibs("nullaway"))
constraints {
errorprone("com.google.guava:guava") {
version {
require("33.4.8-jre")
}
because("Older versions use deprecated methods in sun.misc.Unsafe")
Copy link
Contributor Author

@Pankraz76 Pankraz76 Oct 8, 2025 •
edited
Loading

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.

Copy link
Member

@marcphilipp marcphilipp Oct 8, 2025

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

Copy link
Contributor Author

@Pankraz76 Pankraz76 Oct 9, 2025

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...

}
}
}

nullaway {
Expand All @@ -27,8 +19,9 @@ nullaway {

tasks.withType<JavaCompile>().configureEach {
options.errorprone {
val shouldDisableErrorProne = java.toolchain.implementation.orNull == JvmImplementation.J9
if (name == "compileJava" && !shouldDisableErrorProne) {
disableWarningsInGeneratedCode = true
disableAllChecks = !(name == "compileJava" && java.toolchain.implementation.orNull != JvmImplementation.J9)
Copy link
Contributor Author

@Pankraz76 Pankraz76 Oct 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.

if (!disableAllChecks.get()) {
disable(

// This check is opinionated wrt. which method names it considers unsuitable for import which includes
Expand All @@ -52,11 +45,9 @@ tasks.withType<JavaCompile>().configureEach {
"MissingSummary",
)
error("PackageLocation")
} else {
disableAllChecks = true
}
nullaway {
if (shouldDisableErrorProne) {
if (disableAllChecks.get()) {
Copy link
Member

@marcphilipp marcphilipp Oct 8, 2025

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.

Copy link
Contributor Author

@Pankraz76 Pankraz76 Oct 9, 2025

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?

Copy link
Member

@marcphilipp marcphilipp Oct 9, 2025

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

Copy link
Contributor Author

@Pankraz76 Pankraz76 Oct 9, 2025 •
edited
Loading

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?

disable()
} else {
enable()
Expand Down