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

guava:32.1.1-jre causes issues with mockito-core:3.x #6654

Closed
amitlpande opened this issue Jul 26, 2023 · 11 comments · May be fixed by #6659
Closed

guava:32.1.1-jre causes issues with mockito-core:3.x #6654

amitlpande opened this issue Jul 26, 2023 · 11 comments · May be fixed by #6659
Assignees
Labels
P2 package=general type=defect Bug, not working as expected

Comments

@amitlpande
Copy link

amitlpande commented Jul 26, 2023

Hello,

In our project, we use mockito 3.x (3.8.0) for unit testing.
When we attempted to upgrade guava-jre to 32.1.1, we're running into odd issues.

Below is build.gradle file snip:

dependencies {

implementation 'org.mockito:mockito-core:3.8.0' // this is done just for testing ..
implementation 'com.google.guava:guava:32.1.1-jre'
testImplementation 'org.junit.jupiter:junit-jupiter-api:5.8.1'
testRuntimeOnly 'org.junit.jupiter:junit-jupiter-engine:5.8.1'

}

When code is compiled - running into this:

Task :compileJava FAILED
/tmp/mytestprojects/test/src/main/java/org/example/Main.java:8: error: cannot find symbol
org.mockito.Matchers.matches("abc");
^
symbol: class Matchers
location: package org.mockito
1 error

FAILURE: Build failed with an exception.

However, the code just compiles fine when we switch to 32.0.1-jre.

Appreciate your help in triaging this.

@cpovirk
Copy link
Member

cpovirk commented Jul 26, 2023

I would hope that the Guava version doesn't affect Mockito somehow, but if it did, it would almost certainly mean that some kind of unusual bug is being triggered by our new Gradle Module Metadata.

There is probably not much we can do to investigate unless you're able to share a repo that reproduces the problem (or maybe a Gradle build scan if someone more experienced than I with Gradle has a look).

@amitlpande
Copy link
Author

Thank you for quick response!
Here is sample project -https://github.com/amitlpande/test
In build.gradle if you change to any earlier version of guava (e.g. 32.0.1), the compilation succeeds.
Kindly let me know if anything is needed from my end.

Thanks,
Amit

@cpovirk
Copy link
Member

cpovirk commented Jul 26, 2023

Thanks! That's pretty interesting :)

It looks like, with 32.0.1, Gradle gives you mockito-core:3.8.0, as you requested. And with 32.1.1, Gradle gives you mockito-core:4.11.0, the version used by the separate artifact guava-tests (but not by guava) and specified in guava-parent both for 32.0.1 and for 32.1.1.

I wouldn't have expected the dependencyManagement section of guava-parent to affect the dependencies you see, at least not when guava doesn't use those dependencies itself. It does seem likely that this is fallout from Gradle Module Metadata. @jjohannes

You might be able to work around the problem immediately by migrating from Matchers to ArgumentMatchers (which I'm hoping is at least usually as simple as changing imports) and seeing if the rest of Mockito 4.11.0 is compatible with your project. Or you might be able to force Gradle to choose Mockito 3.8.0 again. (I assume that's easy, but I don't use Gradle enough to know how without looking it up.)

@amitlpande
Copy link
Author

Thank you, yes forcing specific mockito version (in this case 3.8.0) helps resolve the issue.

Thanks for quick response!

@jjohannes
Copy link
Contributor

This is unfortunate. The background here is that Gradle is based on composition, while Maven is based on inheritance. The concept of a "parent POM" does not exist in Gradle's model. Gradle can read a parent POM if it was published by Maven (effectively merging the POMs) but never publishes "parent POMs" itself.

If Guava would be published by Gradle, the versions would probably end up directly in the POMs – which are generated during publishing only as metadata (and not as project description).

What is possible is to use a parent POM – or more precisely the <dependencyManagemnt> block as a BOM (also called platform in Gradle) and integrate this via a dependency (doing composition instead of inheritance).

That's this part in the Gradle Metadata:

        {
          "group": "com.google.guava",
          "module": "guava-parent",
          "version": {
            "requires": "${pom.version}"
          },
          "attributes": {
            "org.gradle.category": "platform"
          }
        }

However, all entries are picked up by this approach. Also <scope>test</scope> – which is a detail I forgot about (see gradle/gradle#4765).

The better option would then maybe be to put the versions of the non-test dependencies directly in the Gradle Metadata and not do the dependency to guava-parent as BOM/platform. Maybe this can somehow be automated.

I could have a look at that.

@cpovirk
Copy link
Member

cpovirk commented Jul 31, 2023

Thanks. Would it work, then, for us to just stop using dependencyManagement in the parent pom at all? (We'd still keep the parent pom so that we can specify plugin configuration and list the other poms as modules.) Nowadays, we generally update deps in bulk with versions-maven-plugin, so it wouldn't really hurt for us to have to update 2-4 pom.xml files instead of 1.

Or does this also/instead need to be changed specifically in the Gradle metadata, as you'd offered to investigate?

@cpovirk cpovirk self-assigned this Jul 31, 2023
@cpovirk cpovirk added type=defect Bug, not working as expected package=general P2 labels Jul 31, 2023
copybara-service bot pushed a commit that referenced this issue Jul 31, 2023
Fixes #6654, I hope? If `guava` and its parent pom don't refer to `mockito-core`, then `guava` should no longer affect which version of `mockito-core` is selected by Gradle. (Really, it "shouldn't" even now, but there's a mismatch between Maven's model and Gradle's that causes it to do so.)

As part of resolving a merge conflict in the `android` flavor, I also noticed that I had never added a direct `checker-qual` dependency to that flavor's `guava-testlib` or `guava-tests` configuration, as I should have done back in cl/522315614 or thereabouts. So I added it. (Of course, it matters little because Maven lets us use the `checker-qual` dependency of `guava` transitively.)

It would be nice if we could still declare our dependency versions only once, now by using `properties`. (In fact, my attempt to use `properties` made me notice that our version of the Error Prone _plugin_ is older than our version of the Error Prone _annotations_.) However, if we were to make that change, then we'd lose the ability to update dependencies with `versions-maven-plugin` (`update-properties` + `use-latest-releases`), I assume because the properties are declared in one `pom.xml` and used in another. (It's possible that Dependabot is better about this, but we've had trouble getting it to work with our unusual 2-flavor, Google-repo-source-of-truth setup.)

Tested:
  ```
  $ mvn dependency:tree -Dverbose -X -U &> /tmp/pre

  # made changes

  $ mvn dependency:tree -Dverbose -X -U &> /tmp/post

  $ strip() { cat "$@" | grep -v -e 'Dependency collection stats' -e 'Downloading from' -e 'Progress' -e 'Using connector' -e 'Using transporter' -e 'Using mirror' -e maven-dependency-plugin -e SUCCESS -e 'Total time' -e 'Finished at' | sed -e 's/ (.*managed from.*)//'; }; vimdiff <(strip /tmp/pre) <(strip /tmp/post)
  ```

RELNOTES=Changed our Maven project to avoid [affecting which version of Mockito our Gradle users see](#6654).
PiperOrigin-RevId: 552479838
@cpovirk
Copy link
Member

cpovirk commented Jul 31, 2023

And only after trying something else do I read carefully enough to get the point of #6654 (comment): We're getting away without version numbers in the Gradle metadata only because we're getting dependencyManagement from guava-parent. So, while getting rid of that dependencyManagement might fix the Mockito problem, it would break everything else :)

Or at least it would break everything else until @jjohannes is able to have a look at automating adding the versions to the Gradle metadata. But once that happens, do we actually gain anything by getting rid of dependencyManagement? Maybe my attempt to do so in #6657 is somehow of help if it simplifies that automation? But if not, I can probably just discard that change.

But, to the extent that this is "a problem with our Mockito dep," I can probably still solve it by removing Mockito from our dependencyManagement section. (I'm not sure I'd expect similar problems with any dep other than Mockito and maybe Truth, which both have removed APIs within the past several years.)

copybara-service bot pushed a commit that referenced this issue Jul 31, 2023
…arent pom.

Fixes #6654, I hope? If `guava` and its parent pom don't refer to `mockito-core`, then `guava` should no longer affect which version of `mockito-core` is selected by Gradle. (Really, it "shouldn't" even now, but there's a mismatch between Maven's model and Gradle's that causes it to do so.)

I had initially attempted (in cl/552479838) to declare versions of _all_ our dependencies inline, but that [didn't work](#6657 (comment)): We [really need `dependencyManagement` for Gradle purposes, at least until we specify versions for Gradle explicitly](#6654 (comment)).

It would be nice if we could still declare our dependency versions only once, now by using `properties`. (In fact, my attempt to use `properties` made me notice that our version of the Error Prone _plugin_ is older than our version of the Error Prone _annotations_.) However, if we were to make that change, then we'd lose the ability to update dependencies with `versions-maven-plugin` (`update-properties` + `use-latest-releases`), I assume because the properties are declared in one `pom.xml` and used in another. (It's possible that Dependabot is better about this, but we've had trouble getting it to work with our unusual 2-flavor, Google-repo-source-of-truth setup.) I notice that we have this problem even today with `truth.version`....

Tested:
  ```
  $ mvn dependency:tree -Dverbose -X -U &> /tmp/pre

  # made changes

  $ mvn dependency:tree -Dverbose -X -U &> /tmp/post

  $ strip() { cat "$@" | grep -v -e 'Dependency collection stats' -e 'Downloading from' -e 'Progress' -e 'Using connector' -e 'Using transporter' -e 'Using mirror' -e maven-dependency-plugin -e SUCCESS -e 'Total time' -e 'Finished at' | sed -e 's/ (.*managed from.*)//'; }; vimdiff <(strip /tmp/pre) <(strip /tmp/post)
  ```

RELNOTES=Changed our Maven project to avoid [affecting which version of Mockito our Gradle users see](#6654).
PiperOrigin-RevId: 552508216
@cpovirk
Copy link
Member

cpovirk commented Jul 31, 2023

I'll release a new version of Guava to address the problem with the Mockito dep specifically.

I still don't have a full model for what's going on with dependency selection, and it seems from #6657 that we might be better off having dependency versions in the Gradle Metadata directly instead of pulling them in through guava-parent. So, despite closing this issue, I'm definitely interested in how to make that happen.

@cpovirk
Copy link
Member

cpovirk commented Aug 1, 2023

https://github.com/google/guava/releases/tag/v32.1.2 should fix this (and any similar problems that users might see with Truth). Thanks again for the report. Sorry that I'm using all your builds as a learning experience....

jjohannes added a commit to jjohannes/guava that referenced this issue Aug 2, 2023
This makes the version handling in POM and Gradle Metadata more similar.
See google#6654 (comment)
@jjohannes
Copy link
Contributor

@cpovirk I think this works: #6664

Inside the POMs, move the versions into properties. Then use the properties in the module metadata.

That's probably the better solution. Feel free to take it from my draft PR and integrate it (if you also prefer that solution).

@cpovirk
Copy link
Member

cpovirk commented Aug 2, 2023

That looks excellent. Thank you.

copybara-service bot pushed a commit that referenced this issue Aug 2, 2023
This makes the version handling in POM and Gradle Metadata more similar.
See #6654 (comment)

We expect this to fix #6657, though we don't fully understand it.

Fixes #6664

RELNOTES=Changed Gradle Metadata to include dependency versions directly. This may address ["Could not find `some-dependency`" errors](#6657) that some users have reported (which might be a result of users' excluding `guava-parent`).
PiperOrigin-RevId: 553180806
copybara-service bot pushed a commit that referenced this issue Aug 2, 2023
This makes the version handling in POM and Gradle Metadata more similar.
See #6654 (comment)

We expect this to fix #6657, though we don't fully understand it.

Fixes #6664

RELNOTES=Changed Gradle Metadata to include dependency versions directly. This may address ["Could not find `some-dependency`" errors](#6657) that some users have reported (which might be a result of users' excluding `guava-parent`).
PiperOrigin-RevId: 553215970
@google google deleted a comment from SteveBombardier Feb 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 package=general type=defect Bug, not working as expected
Projects
None yet
4 participants
@cpovirk @jjohannes @amitlpande and others