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

Dependency resolution issues caused by com.google.guava:listenablefuture:9999.0-empty-to-avoid-conflict-with-guava hack #7385

Open
2 tasks done
hungvietnguyen opened this issue Sep 2, 2024 · 5 comments
Assignees
Labels
P3 type=defect Bug, not working as expected

Comments

@hungvietnguyen
Copy link

hungvietnguyen commented Sep 2, 2024

Please upvote this request to signal your interest in having this issue resolved.

Guava Version

33.3.0 and earlier

Description

The split of com.google.guava:listenablefuture:1.0/com.google.guava:listenablefuture:9999.0-empty-to-avoid-conflict-with-guava from the com.google.guava:guava library has been known to cause various dependency resolution issues.

This was discussed at length in the following bugs, but it seems that the issue was never fully resolved:

I'm filing this request so Guava authors can settle this issue once and for all.

Example

Please see https://issuetracker.google.com/300760566#comment14.

Current Behavior

  • com.google.guava:guava:31.1-android contains the ListenableFuture interface.
  • com.google.guava:guava:31.1-android depends on com.google.guava:listenablefuture:9999.0-empty-to-avoid-conflict-with-guava, which is empty.
  • com.google.guava:listenablefuture:1.0 also contains the ListenableFuture interface.

Expected Behavior

The com.google.guava:listenablefuture:9999.0-empty-to-avoid-conflict-with-guava hack should be removed.

Proposal 1 (fix the split)

  • Remove ListenableFuture interface from com.google.guava:guava:31.1-android
  • Let com.google.guava:guava:31.1-android depend on com.google.guava:listenablefuture:1.0
  • Remove com.google.guava:listenablefuture:9999.0-empty-to-avoid-conflict-with-guava

I'm scoping this request to the android variant. (The jre variant can be discussed separately.)

Note that @cpovirk actually once proposed this change at https://issuetracker.google.com/131431257#comment11.

Proposal 2 (revert the split)

  • Remove com.google.guava:listenablefuture:1.0
  • Remove com.google.guava:listenablefuture:9999.0-empty-to-avoid-conflict-with-guava

Packages

com.google.common.util.concurrent

Platforms

Android

Checklist

  • I agree to follow the code of conduct.

  • I can reproduce the bug with the latest version of Guava available.

@hungvietnguyen hungvietnguyen added the type=defect Bug, not working as expected label Sep 2, 2024
@cpovirk
Copy link
Member

cpovirk commented Sep 3, 2024

Thanks for reporting this on our issue tracker, as well.

I've had some more time to think about it, so now I have some more concrete concerns with this proposal:

  • If users are manually listing their transitive deps to be downloaded (instead of having a build system identify and download everything they need based on direct deps), then they'll now need to pull in listenablefuture. This is at least a simple problem to describe and fix—though the underlying problem might be hard to understand for a person who suddenly sees some random missing Guava class but who hasn't read the release notes. Of course, it's also yet another Guava dependency annoyance, and you may need to be careful not to add the dep until after you've updated to a new enough version of Guava.
  • If users are relying on their build system to compute transitive deps, I worry somewhat about having a dependency graph that differs in this way, Could there be some way in which Gradle or another build system selects guava-android (presumably because it knows that it's performing "an Android build") but has trouble reconsidering the idea that it should use the empty 9999 version of listenablefuture (since that version is "newer," which would normally be good)? Maybe we'd want to set up Gradle module metadata for listenablefuture similar to what we already have for guava?
  • I'm not sure how much trouble this will cause for cross-platform (i.e., Android+JRE) libraries that depend on Guava. (We have some of these, like Truth, but I'd be more worried about the rest of the world.) If they depend on guava-android (as Truth does now), then they would not longer be compatible with the Java module system (because c.g.c.util.concurrent would be split across two artifacts). But if they depend on guava-jre, then they wouldn't be compatible with Android. One possibility would be to encourage libraries like that to release two separate "versions," just as Guava does (as touched on for Truth previously). But even if that's something we'd consider for Truth, it's not something that I'd want to inflict on the ecosystem.
  • While we've been careful not to change ListenableFuture "significantly," we have added some annotations since the initial release: Compare this version(?) with the current version. The versions remain binary-compatible in both directions, but they may be treated differently by static-analysis tools. If Android users fall back to the old version, they may see those slight changes in treatment. Or we could release a new version of listenablefuture, but we've at least hoped to avoid that, if only for reasons of general churn.

All that might still add up to less trouble than Android Gradle Plugin users have started seeing again since the Gradle changes. But I'm going to end up feeling bad whether we make a change or not :(

(@jodastephen, @jbduncan as frequent correspondents on this topic)

@jodastephen
Copy link
Contributor

There are three jar files under discussion:

  • guava
  • failureaccess - either a direct or transient dependency
  • listenablefuture - either a direct or transient dependency

Each jar has versions:

  • guava - normal versioning with either jre or android suffix
  • failureaccess - normal versioning
  • listenablefuture - normal versioning or 9999-hack
  • (any combination of versions can be setup, deliberately or by accident)

Each dependency can be specified to be in a scope, eg. runtime, compile time, test etc. Theoretically you could depend on guava-jre in compile scope and guava-android in test scope.

Dependencies can be run on the classpath, module path, in OSGI and other class loader systems.

Dependencies can be loaded by Maven, Gradle, another build tool or manually, and each tool has multiple versions.

Other dependencies in your application may also depend directly or indirectly on one of the three jar files, potentially with an unexpected version/jar-hell situation.

In summary, there are lots of possible combinations, many of which are invalid - it is not realistic to try and support all possible combinations.

@jodastephen
Copy link
Contributor

Fixing this is hard, but ultimately I still feel that the best approach is to effectively revert the change.

Proposal (JRE):

  • Alter jre variant to include code from failureaccess and remove both failureaccess and listenablefuture dependencies.
    • Guava users will get two less dependencies.
    • Guava users that have a direct dependency on failureaccess will need to remove it and only depend on guava-jre iff they care about duplicate classes
    • Guava users that have an indirect dependency on failureaccess will need to use an exclusion mechanism iff they care about duplicate classes
    • Guava users that have a direct dependency on listenablefuture v1 will need to remove it iff they care about duplicate classes (presumably they don't care as their system already has the duplicates)
    • Guava users that have an indirect dependency on listenablefuture v1 will need to use an exclusion mechanism iff they care about duplicate classes (presumably they don't care as their system already has the duplicates)
    • Guava users that have a direct or indirect dependency on listenablefuture v9999 do not need to do anything, but would benefit from removing the useless dependency
    • Users that depend on listenablefuture and/or failureaccess without depending on Guava are unaffected

Proposal (Android):

  • Exactly as per JRE
    • (are there really that many uses of listenablefuture as a standalone jar file? enough to warrant the complexity of the odd exclusion or duplicate?)
  • (I did consider suggesting releasing a third variant of Guava - android-full - but overall I think that confuses things further)

Bump the major version to emphasize the change

@hungvietnguyen
Copy link
Author

Thanks for the great insights!

(are there really that many uses of listenablefuture as a standalone jar file? enough to warrant the complexity of the odd exclusion or duplicate?)

@cpovirk: Chris, could you comment on this? What is the original motivation that triggered this split, and now that we know the cost, should we consider reverting it?

@cpovirk
Copy link
Member

cpovirk commented Sep 5, 2024

Thanks.

I am trying to remember where I've seen usage statistics for Maven packages before.... mvnrepository.com has listenablefuture-1.0 with 150 usages, and libraries.io has it with 119. The former includes a list of those usages, and it appears to be various Android libraries, as I'd expect.

The background for listenablefuture (and failureaccess) is that some Android projects were looking for a new concurrency primitive. CompletableFuture wasn't widely available enough yet (and probably still isn't quite there, though it's getting close). ListenableFuture was what Google had been using for the better part of a decade before CompletableFuture was even added to Java, so it already had an ecosystem around it inside Google and supporting libraries in Guava. Some Android projects already found themselves having to adapt between ListenableFuture and other concurrency primitives, so they wanted to converge on ListenableFuture. (This convergence happened to some extent, but I don't have a broad enough view to judge just how much. Perhaps we would have seen more convergence if not for the rise of Kotlin?)

The concern was that Guava is a heavyweight dependency for an Android app. The natural solution would be for libraries to depend on Guava and then exercise care in using only ListenableFuture and perhaps a few supporting classes. However, the Android folks told us that we could not rely on all downstream Android apps to use Proguard or similar tooling to strip unused classes (primarily because of use of reflection, though it's possible that the owners would additionally have concerns about build-time cost or something). That pushed us toward producing a separate artifact with just ListenableFuture in it. (And then the failureaccess work was an attempt to ensure that no one would be prevented from using ListenableFuture out of performance concerns. In hindsight, we should probably have waited for any such concerns to materialize... though users might have routed around us instead of raising such concerns.)

(We might have been able to do this in a simpler way if we'd chosen for that separate artifact to be guava-1.0-listenablefutureonly—that is, a "new" "version" of "Guava" rather than a separate artifact ID. I don't know that that would have avoided the problem we're discussing here, though.)

I have never had a great read on the Android ecosystem, so the big question in my mind is whether apps are now reliably set up to strip unused classes. If they are, then we could encourage users of listenablefuture to move to depend on plain guava. Each time a user migrates, that reduces the chance that people will hit the classpath-mismatch problem.

Even if we can't do that, we should think further about at least the possibility of removing the listenablefuture dependency from guava. I would expect that to lead to new duplicate-class errors for users who depend on both listenablefuture-1.0 and guava, since guava would no longer magically promote the listenablefuture dep to the empty 9999 library. But those errors are likely easier to suppress than the errors that users are seeing now, and maybe AGP could even be made to do so automatically.

At the moment, I'm interested to see where the discussion goes on https://issuetracker.google.com/300760566.

@hungvietnguyen hungvietnguyen changed the title com.google.guava:guava:X.Y-android should depend on com.google.guava:listenablefuture:1.0 Dependency resolution issues caused by com.google.guava:listenablefuture:9999.0-empty-to-avoid-conflict-with-guava hack Sep 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 type=defect Bug, not working as expected
Projects
None yet
Development

No branches or pull requests

4 participants