Skip to content
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

Forcing guava JRE versions in Android projects with new metadata is too tedious #6801

Open
ZacSweers opened this issue Oct 24, 2023 · 13 comments

Comments

@ZacSweers
Copy link

ZacSweers commented Oct 24, 2023

We have an Android project with minSdk 29 and target Java 17, which means we can freely use newer APIs and the guava JRE artifact. With the introduction of the new Gradle metadata though, this has become an insurmountable a battle that actually breaks our infrastructure with libraries that also use Guava.

The ideal goal is to force use of JRE regardless. However, in my testing I've found that's fundamentally not possible anymore possible but with lots of tedium. The release notes advice was not helpful (and in some places invalid code), below is everything I attempted.

1. Dependency constraints

My first attempt was to apply dependency constraints like so

dependencies.constraints {
  testImplementation("com.google.guava:guava") {
    attributes {
      attribute(
        TargetJvmEnvironment.TARGET_JVM_ENVIRONMENT_ATTRIBUTE,
        objects.named(TargetJvmEnvironment.STANDARD_JVM)
      )
    }
  }
}

However, this results in the following failed resolution

* What went wrong:
Execution failed for task ':libraries:slack-kit:slack-kit:mapReleaseUnitTestSourceSetPaths'.
> Could not resolve all files for configuration ':libraries:slack-kit:slack-kit:releaseUnitTestRuntimeClasspath'.
   > Could not resolve com.google.guava:guava.
     Required by:
         project :libraries:slack-kit:slack-kit
      > Module 'com.google.guava:guava' has been rejected:
           Cannot select module with conflict on capability 'com.google.collections:google-collections:32.1.3-jre' also provided by [com.google.guava:guava:32.1.3-jre(jreRuntimeElements), com.google.guava:guava:32.1.3-jre(androidRuntimeElements)]
   > Could not resolve com.google.guava:guava:32.1.3-jre.
     Required by:
         project :libraries:slack-kit:slack-kit > project :tooling:slack-platform
         project :libraries:slack-kit:slack-kit > com.google.guava:guava-bom:32.1.3-jre
         project :libraries:slack-kit:slack-kit > project :libraries:binders:core > project :libraries:foundation:slack-commons
      > Module 'com.google.guava:guava' has been rejected:
           Cannot select module with conflict on capability 'com.google.collections:google-collections:32.1.3-jre' also provided by [com.google.guava:guava:32.1.3-jre(jreRuntimeElements), com.google.guava:guava:32.1.3-jre(androidRuntimeElements)]
   > Could not resolve com.google.guava:guava:32.0.1-android.

2. Resolution strategies

This approach adds on to the above by also configuring a resolution strategy to force the JRE version.

configurations.configureEach {
  resolutionStrategy.capabilitiesResolution.withCapability("com.google.guava:guava") {
    select(candidates.single { it.variantName.contains("jreRuntimeElements") })
  }
}

This results in another error about missing google-collections

* What went wrong:
Execution failed for task ':libraries:slack-kit:slack-kit:mapReleaseUnitTestSourceSetPaths'.
> Could not resolve all files for configuration ':libraries:slack-kit:slack-kit:releaseUnitTestRuntimeClasspath'.
   > Could not resolve com.google.guava:guava.
     Required by:
         project :libraries:slack-kit:slack-kit
      > Module 'com.google.guava:guava' has been rejected:
           Cannot select module with conflict on capability 'com.google.collections:google-collections:32.1.3-jre' also provided by [com.google.guava:guava:32.1.3-jre(jreRuntimeElements), com.google.guava:guava:32.1.3-jre(androidRuntimeElements)]
   > Could not resolve com.google.guava:guava:32.1.3-jre.
     Required by:
         project :libraries:slack-kit:slack-kit > project :tooling:slack-platform
         project :libraries:slack-kit:slack-kit > com.google.guava:guava-bom:32.1.3-jre
         project :libraries:slack-kit:slack-kit > project :libraries:binders:core > project :libraries:foundation:slack-commons
      > Module 'com.google.guava:guava' has been rejected:
           Cannot select module with conflict on capability 'com.google.collections:google-collections:32.1.3-jre' also provided by [com.google.guava:guava:32.1.3-jre(jreRuntimeElements), com.google.guava:guava:32.1.3-jre(androidRuntimeElements)]
   > Could not resolve com.google.guava:guava:32.0.1-android.

So I fix this by adding a strategy for that too

configurations.configureEach {
  resolutionStrategy.capabilitiesResolution.withCapability("com.google.guava:guava") {
    select(candidates.single { it.variantName.contains("jreRuntimeElements") })
  }
  resolutionStrategy.capabilitiesResolution.withCapability(
    "com.google.collections:google-collections"
  ) {
    select(candidates.single { it.variantName.contains("jreRuntimeElements") })
  }
}

This gets farther, but now I get a failure with AGP's CheckAarMetadataTask, which sees invalid metadata in the produced library jar. So, I disable that task to try to get further.

tasks.withType<CheckAarMetadataTask>().configureEach { enabled = false }

3. JVM SIGSEGV Nevermind, this part is fixed

Finally, I'm able to proceed running tests, only to find that after all the above it actually crashes the JVM entirely. I've attached a crash log of this, but the tombstone looks like this.

#
# A fatal error has been detected by the Java Runtime Environment:
#
#  SIGSEGV (0xb) at pc=0x00000001468b3304, pid=21676, tid=8451
#
# JRE version: OpenJDK Runtime Environment Zulu20.32+11-CA (20.0.2+9) (build 20.0.2+9)
# Java VM: OpenJDK 64-Bit Server VM Zulu20.32+11-CA (20.0.2+9, mixed mode, sharing, tiered, compressed oops, compressed class ptrs, g1 gc, bsd-aarch64)
# Problematic frame:
# C  [libandroid_runtime.dylib+0x2cb304]  android::uirenderer::skiapipeline::SkiaHostPipeline::setSurface(ANativeWindow*, android::uirenderer::renderthread::SwapBehavior)+0x9c
#
# No core dump will be written. Core dumps have been disabled. To enable core dumping, try "ulimit -c unlimited" before starting Java again
#
# If you would like to submit a bug report, please visit:
#   http://www.azul.com/support/
# The crash happened outside the Java Virtual Machine in native code.
# See problematic frame for where to report the bug.
#

hs_err_pid21676.log

Conclusion

At this point, we cannot update Guava 32.0.0 because of the introduction of these metadata rules. I completely understand the intention and goal behind it, and it may even be that Guava is not really at fault here vs Gradle tooling. However, the end result is that it's made Guava in android projects at-best annoying and at-worst impossible. I articulated this at length here: #6612 (comment)

Quoting here with emphasis on the ask again :)

This change to guava has been incredibly annoying to deal with across multiple repos. Some gradle plugins use guava as a transitive dependency in opaque configurations that then require project maintainers to go do silly workarounds like have been articulated in the above comments.

The number of PRs linking back to this with various workarounds I feel is indicative of this being a recurring papercut now. I strongly feel the original goal of making guava dependency management easier has not been met, and actually made worse by this change. Would you be open to rolling back this change for now until a better solution is found?

@ZacSweers
Copy link
Author

I'm able to work around the 3rd issue by forcing tests to fork every 1 test and think that bit's actually unrelated to guava. The result is this configuration appears to work

tasks.withType<CheckAarMetadataTask>().configureEach { enabled = false }

dependencies.constraints {
  testImplementation("com.google.guava:guava") {
    attributes {
      attribute(
        TargetJvmEnvironment.TARGET_JVM_ENVIRONMENT_ATTRIBUTE,
        objects.named(TargetJvmEnvironment.STANDARD_JVM)
      )
    }
  }
}

configurations.configureEach {
  resolutionStrategy.capabilitiesResolution.withCapability("com.google.guava:guava") {
    select(candidates.single { it.variantName.contains("jreRuntimeElements") })
  }
  resolutionStrategy.capabilitiesResolution.withCapability(
    "com.google.collections:google-collections"
  ) {
    select(candidates.single { it.variantName.contains("jreRuntimeElements") })
  }
}

This is a lot of tedium to have to do though 😬

@ZacSweers ZacSweers changed the title No way to force guava JRE versions in Android projects with new metadata Forcing guava JRE versions in Android projects with new metadata is too tedious Oct 24, 2023
@cpovirk
Copy link
Member

cpovirk commented Oct 24, 2023

Thanks for all the details, and sorry for not making the time for this yet.

In hindsight, one question that I should have looked into before merging the metadata is the question of whether a rollback of the metadata could itself lead to problems. Maybe @jjohannes has thoughts?

I predict that @jjohannes's first thought is that we should avoid rolling back... and to be fair, that's still my hope, as well :) But that is hard to feel great about that when things are broken for some users and I'm not sure how soon I can do much about it.

I will say that I should probably have anticipated that some users would use the -jre variant under Android, but I did not. I recently discouraged that in #6725, but you're now the third person to do it or at least ask about it, so I should probably put some of that discouragement on the Guava landing page. And then there's the question of whether to do more than just discourage: Maybe it's better for us to somehow outright break -jre for Android users soon (so that it doesn't accumulate more users who might break in the future—at, of course, the cost of creating real breakages preemptively). Or maybe it's better to try to keep it working as best we can (which might limit our options for the JVM flavor in the future but at least helps now).

(I do wonder if your experiences here imply that forcing the Android flavor is also more difficult than our instructions suggest. Maybe it works OK for a single simple library (which is the primary use case—a cross-platform library that can be used from either Android or JRE) but falls apart for more complex projects. This is the kind of thing that I will hopefully get a better sense of when I look back through the various commits and issues linked from the various threads here.)

If we were to break -jre usage under Android (well, more than we already have), I would want to get Java 8 APIs into the -android flavor as much as we can. Do you have a sense of how many different Java 8 APIs you'd need in order for guava-android to work for your use case? If so, can you weigh in on #6567? I made some recent progress in testing ImmutableSet.toImmutableSet in our internal Android code, and I think I've got enough fixed for us to try it there and eventually release it if all goes well. "Eventually" is not a great story for your situation, but if you have a set of APIs that you care about, I can push for those first and with at least a bit more urgency.

I recognize that there's also still the larger question of whether the aggregate disruption we're seeing with the metadata is worth the cost, even assuming that we successfully add Java 8 APIs to guava-android. But the discussion of Java 8 APIs gives me something specific to think about until I get the chance to either look into the rest more or just give up and push the big red button.

@ZacSweers
Copy link
Author

I think that breaking -jre for android users would be the wrong move. As I mentioned in our case — we actively don't want the android variant and do want the -jre version because, speaking frankly, the android variant offers no benefits to us with our current minSdk version and library desugaring. If you want a simpler explanation – unit tests can run on -jre versions and often use test libraries that depend on -jre APIs (which is what prompted me to enter this rabbit hole, ref).

I think the right move is to remove the metadata, it's overly prescriptive to the point of being an active and invasive issue to consumers, and gradle's cumbersome API surface area makes it too tedious for people without deep domain knowledge to do anything about the issues it leads to.

@cpovirk
Copy link
Member

cpovirk commented Nov 2, 2023

The thing I should have mentioned in my last post is that the Android flavor has a number of implementation changes that reduce memory usage and binary size. So, if we could get all the APIs you need into that flavor, it would provide the best of both worlds. It is of course possible that those changes aren't significant in your case. And it is indisputable that we haven't made the APIs you need available yet. I wouldn't want you to sit down and compile a list of exactly what you need, but do you have a sense of whether there's more than the various Collector APIs? I would like to at least move in the right direction in case some future change to guava-jre does cause problems (say, a change to use MethodHandle so that we can keep compiling against a future JDK or something?).

My hope for the Gradle metadata has been that users will get the appropriate version automatically, avoiding problems like cashapp/paparazzi#552 from Truth's dependency on guava-android. I suspect that it's mostly helped, but I don't know any way to know for sure, and anyway, that's little consolation to the people who we forced to confront some obscure misconfiguration that never mattered before or who are using guava-jre from Android. I will still try to sort through the various reports we've gotten to see what's left and what might happen if we were to remove the metadata. I don't know exactly when, though.

@ZacSweers
Copy link
Author

I don't know all the little bits and pieces, but my perception at a broad level is that D8/R8/L8 on android tooling are pretty aggressively paced and outpacing guava's efforts to backport functionality to a separate -android artifact. I'd also argue that making ImmutableMap implementations behave similar to ArrayMap in 2023 are much less impactful than, say, 5 years ago. In fact, I'd argue it's much more likely that an app that cares about this degree of performance/binary size would avoid using guava in the first place.

I don't want to get tooooo close into the realm of "should guava android still exist", but it does feel like it's circling around that. To that end, I'd argue that it shouldn't.

copybara-service bot pushed a commit that referenced this issue Nov 6, 2023
(somewhat relevant to #6725 and #6801, but this CL is about the good things about the Android flavor, not the risks of the JRE flavor)

RELNOTES=n/a
PiperOrigin-RevId: 579831040
cpovirk added a commit that referenced this issue Nov 6, 2023
(somewhat relevant to #6725 and
#6801, but this CL is about the
good things about the Android flavor, not the risks of the JRE flavor)
copybara-service bot pushed a commit that referenced this issue Nov 6, 2023
(somewhat relevant to #6725 and #6801, but this CL is about the good things about the Android flavor, not the risks of the JRE flavor)

RELNOTES=n/a
PiperOrigin-RevId: 579886456
@ZacSweers
Copy link
Author

Another way this issue manifests: some tools resolve dependencies that use guava via Gradle configurations in projects that are not inherently jvm or android projects, such as gradle tasks that invoke formatters or generate code (i.e. Spotless). In this case, it will always fail to resolve due to gradle not being able to decide between available variants.

A problem occurred configuring root project 'slack-android-ng'.
> Could not resolve all files for configuration ':spotless865458226'.
   > Could not resolve com.google.guava:guava:32.1.1-jre.
     Required by:
         project : > com.google.googlejavaformat:google-java-format:1.18.1
      > The consumer was configured to find attribute 'org.gradle.category' with value 'library', attribute 'org.gradle.dependency.bundling' with value 'external'. However we cannot choose between the following variants of com.google.guava:guava:32.1.1-jre:
          - androidRuntimeElements
          - jreRuntimeElements

@jjohannes
Copy link
Contributor

For the "problem" posted above: Projects that do not yet apply any Java/Android/JVM plugin have to apply the id("jvm-ecosystem") plugin to have the conflict resolution rules of the "JVM World" available. This is not a Guava specific problem. It is just that Guava is one of the libraries that can cause such an error. Other libraries with "simpler" metadata just happen to work because of fallbacks Gradle has to stay compatible with (very) old Gradle versions. But Guava is not the only library that can cause this. I think if anything should be done, it's on the Gradle side (better error, better documentation, remove some of the old fallbacks, ...). I already talked about that in other places (like here #6612 (comment)).


For the problem posted in the description: could you please provide a reproducer for that problem @ZacSweers? We should find out why the situation(s) are in which dependencies.constraints { ... } is not just working and why. It should IMO and it did in all projects I tried this in so far.


General feedback on the topic of "Gradle Metadata for Guava" (my personal opinion and impressions): When I read through this discussions and some of #6612 it sounds like things are broken. But so far all concrete problems I have seen, analyzed, reproduced were either fixed by the patch releases or came down to misconfiguration/misunderstanding in user projects that 'by chance' worked with older Guava versions with less precise metadata. What I find particularly frustrating is folks asking about why it is broken for them but then do not even share the whole error message (happened on the other issue several times) let alone a reproducer. This also gives a bad and wrong impression for others looking at these issues, where the actual helpful advice is buried between a lot of noise. I also would appreciate if you could avoid wording like "silly workarounds" @ZacSweers. I don't think any of this is silly.


In hindsight, one question that I should have looked into before merging the metadata is the question of whether a rollback of the metadata could itself lead to problems. Maybe @jjohannes has thoughts?

@cpovirk I personally don't see the evidence for why this should be rolled back unless I fully understand the problems that currently exist (like the one described in this issue, which I would need to reproduce first). But it's not my call and since you asked: If you would roll back the change (remove the Gradle Metadata) in the next release it should not cause any problems. (Only for me because I have to adjust my metadata patching plugin again 😃.) Gradle selects one version and only then looks at the metadata. If a version without Gradle Metadata is selected, no Gradle Metadata is used.
If you really consider that, I think it would be great if a precise list of reasons why you consider it is assembled. And then it would also be great if someone from the Gradle team can be involved and comment on it. To learn from this and improve things in Gradle. I am not in the team anymore since almost three years, but I can help to establish contact.

@ZacSweers
Copy link
Author

Hi @jjohannes. There is a lot to unpack there, but unfortunately my takeaway here is that you are taking this issue personally and it makes it hard to know how to engage with most of what you said. So, I'll just address the two issues.

For the issue in the description: attempt to use guava JRE in an android project that runs paparazzi tests, observe that they all fail because paparazzi requires guava JRE but the metadata choice forces guava android. It's easy to repro, but if you're serious about looking into it I can spin up a repro project for you.

For the issue with spotless: I don't really know what to tell you. This error message

A problem occurred configuring root project 'slack-android-ng'.
> Could not resolve all files for configuration ':spotless865458226'.
   > Could not resolve com.google.guava:guava:32.1.1-jre.
     Required by:
         project : > com.google.googlejavaformat:google-java-format:1.18.1
      > The consumer was configured to find attribute 'org.gradle.category' with value 'library', attribute 'org.gradle.dependency.bundling' with value 'external'. However we cannot choose between the following variants of com.google.guava:guava:32.1.1-jre:
          - androidRuntimeElements
          - jreRuntimeElements

in no way leads one to this (great!) explanation you offered

Projects that do not yet apply any Java/Android/JVM plugin have to apply the id("jvm-ecosystem") plugin to have the conflict resolution rules of the "JVM World" available

You can explain it because you know happened to know about it. You used to work at Gradle and are a well known Gradle build tool expert in the industry. Most people that would see that error message are not those things. The string jvm-ecosystem doesn't even appear anywhere in gradle's docs.

Sure, I'm holding it wrong, but the error message didn't exactly tell me how to hold it or where to learn either. That's why I filed this issue, because when this system falls over, it's going to result in most users just not updating guava instead.

@big-guy
Copy link

big-guy commented Nov 14, 2023

@ZacSweers which version of Gradle is this with?

It's easy to repro, but if you're serious about looking into it I can spin up a repro project for you.

This would be very helpful to me to understand where this is going wrong. I can barely spell paparazzi. :)

This error message

100% agree, these error messages are not helpful. We're going to land a small change to provide breadcrumbs to documentation for this in 8.6, but ultimately, these messages should go away and said something useful.

@ZacSweers
Copy link
Author

Here you go: slackhq/circuit#993

@jjohannes
Copy link
Contributor

Thank you for the project. Unfortunately, I cannot reproduce the problem. If I only add this part back and remove the rest of the "workaround block" it works as I would expect:

dependencies.constraints {
  add("testImplementation", "com.google.guava:guava") {
    attributes {
      attribute(TargetJvmEnvironment.TARGET_JVM_ENVIRONMENT_ATTRIBUTE, 
        objects.named(TargetJvmEnvironment.STANDARD_JVM))
    }
  }
}

The tests are running (https://scans.gradle.com/s/c4hsvnqakzbzq) and it gets to the same point as when you have the whole "workaround block" (https://scans.gradle.com/s/gxfcsn7xhsdx4).

This is, in my mind, the correct way now to explicitly select the jre variant. This is also what @cpovirk documented in the release notes.

@ZacSweers
Copy link
Author

I suspect the added work we have to do internally is because we apply the guava bom internally, which is mentioned in the doc on that workaround section but not sure if you saw.

In my mind, the "correct" way to explicitly select the -jre would be to add the -jre artifact as a dependency, which is... what I did. And what most people will try to do. This defeats that convention in favor of a new, only-obvious-to-gradle-internals convention.

In a way, it reminds me of how kotlin multiplatform works, where there is a common target and then platform targets that gradle automatically selects based on the source set. However, if I depended on a -jvm target dependency directly, it wouldn't try to switch me over to -android under my feet. Maybe what's missing here is a common guava target (sans -jre/-android), then consumers could depend on that and let gradle disambiguate the automatic way for them, but get out of the way when someone explicitly depends on the -jre target.

@TWiStErRob
Copy link

Dropping this here, because I think it's tightly related:

Also contains an isolated (no Paparazzi, @big-guy ;) repro for a similar (if not the same) issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants