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

Update Truth to 0.44. #8150

Closed
wants to merge 1 commit into from
Closed

Update Truth to 0.44. #8150

wants to merge 1 commit into from

Conversation

cpovirk
Copy link
Contributor

@cpovirk cpovirk commented Apr 25, 2019

This is necessary for some forthcoming changes that migrate to methods introduced in that version:
https://github.com/google/truth/releases/tag/release_0_44

While here, move diffutils from runtime_deps to deps. This is unlikely to matter in practice, but in principle it's the right thing, and it might matter if static analysis or other build tools (-Xlint:all?) went digging into the deps of Truth at compile time. Such problems (albeit when the dependency's API is publicly visible through the using library) are what prompted the creation of java_import#deps (internal Google bug 23781162). For other problems (that might also not apply here), see google/guava#2721

This is necessary for some forthcoming changes that migrate to methods introduced in that version:
https://github.com/google/truth/releases/tag/release_0_44

While here, move diffutils from runtime_deps to deps. This is unlikely to matter in practice, but in principle it's the right thing, and it *might* matter if static analysis or other build tools (-Xlint:all?) went digging into the deps of Truth at compile time. Such problems (albeit when the dependency's API is publicly visible through the using library) are what prompted the creation of java_import#deps (internal Google bug 23781162). For other problems (that might also not apply here), see google/guava#2721
@katre
Copy link
Member

katre commented Apr 25, 2019

@philwo was working on this, in #8144.
He ran into the following problems:

We're currently using Truth 0.42. Since Truth 0.43, it needs Guava >= 27.0.1. We're currently using Guava 26.1.
However, Guava 27.0 split out a few classes into separate JARs. No problem, we can just import them.

Except our *wonderful* Android Desugar tests do magic command-line stuff that depends on Guava consisting of exactly one JAR.

And then:

we have a version of another Android thing checked in that still uses CharMatcher.JAVA_LETTER, which was deprecated since Guava 19.0 and has been removed in Guava 27:1) testFilterOldLanguageFormatWithAdditionalQualifiers(com.google.devtools.build.lib.rules.android.ResourceFilterFactoryTest)java.lang.NoSuchFieldError: JAVA_LETTER	at com.android.ide.common.resources.configuration.LocaleQualifier.isValidAlpha2Code(LocaleQualifier.java:105)	at com.android.ide.common.resources.configuration.LocaleQualifier.getQualifier(LocaleQualifier.java:171)	at com.android.ide.common.resources.configuration.LocaleQualifier.checkAndSet(LocaleQualifier.java:423)	at com.android.ide.common.resources.configuration.FolderConfiguration.getConfigFromQualifiers(FolderConfiguration.java:233)	at com.android.ide.common.resources.configuration.FolderConfiguration.getConfigFromQualifiers(FolderConfiguration.java:137)	at com.android.ide.common.resources.configuration.FolderConfiguration.getConfigForQualifierString(FolderConfiguration.java:335)

To which @ahumesky replied:

That class is in here:third_party/android_common/com.android.tools_sdk-common_25.0.0.jar(and com.android.tools_sdk-common_25.0.0-stripped.jar)

Updating the sdk tools to a later version (assuming newer versions are even updated to use guava 27) will be a significant effort. It's probably easier to patch that one class somehow.

@cpovirk
Copy link
Contributor Author

cpovirk commented Apr 25, 2019

Thanks. And oops, I thought I had noted somewhere public that Truth doesn't actually require Guava 27.0.1: It just uses it for good hygiene. I'll see if I can dig up what I wrote.

Still, if you'd prefer to wait for a full upgrade, that's OK with me. FWIW, tests do appear to be passing.

@cpovirk
Copy link
Contributor Author

cpovirk commented Apr 25, 2019

Eh, I can't find what I wrote, but: Truth does require 25.1 or newer (for Strings.lenientFormat), but that's been the case since 0.41, and as expected, you have a new enough Guava already. Guava hasn't added much recently, despite the other "interesting" changes you've encountered.

@katre
Copy link
Member

katre commented Apr 25, 2019

You're right, the tests pass, which is the important bit. I will merge this shortly, and then we can unblock your other changes.

Thanks for helping out with this!

@katre
Copy link
Member

katre commented Apr 25, 2019

This was merged as 568048b

@katre katre closed this Apr 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants