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

Bump Truth 1.1.5 => 1.2.0 #1206

Merged
merged 2 commits into from
Jan 8, 2024
Merged

Bump Truth 1.1.5 => 1.2.0 #1206

merged 2 commits into from
Jan 8, 2024

Conversation

jrodbx
Copy link
Collaborator

@jrodbx jrodbx commented Jan 5, 2024

Relates to #906. Closes #1195.

Guava's publishing changed slightly in v33.0.0, using Gradle Module Metadata to help guide consumers' resolution of the appropriate variant, i.e., -jre vs -android: google/guava#3683

Truth 1.2.0 bumps to Guava 33.0.0, so the preceding PR was failing because that new publishing mechanism by default forces the -android variant, which doesn't have the methods we expect and that are present in the -jre variant.

This is a frustrating ongoing problem and this PR is only solving the most recent manifestation of this issue. See the discussion in google/guava#6801 for the latest discussion.

Comment on lines +30 to +31
assertThat(Build.DISPLAY).isEqualTo("sdk_phone_armv7-userdebug UpsideDownCakePrivacySandbox URD9.231106.004.A2 11164158 test-keys")
assertThat(Build.ID).isEqualTo("URD9.231106.004.A2")
Copy link
Contributor

Choose a reason for hiding this comment

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

How did truth/guava affect these?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good point, i think just non-hermetic update from Google's side. I can move into a separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm happy either way, was just surprised.

Do I understand it correctly that this change depends on the Android SDK via compileSdk?

Copy link
Collaborator Author

@jrodbx jrodbx Jan 6, 2024

Choose a reason for hiding this comment

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

Yes, but now this change is leading me to pull on a long thread haha. When I push an empty commit, CI passes, but if i bump Truth, this test fails due to an updated sdk preview....why? Is our Gradle caching setup on CI busted in some way?

Copy link
Contributor

Choose a reason for hiding this comment

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

Very likely

Based on "empty commit" mention, it sounds like the second, assuming that when you push empty commit no test gets re-executed (which should be the case if every task is cached by gradle-build-action).

How this relates to truth upgrade is beyond me still though. You can try modifying this file in a non-meaningful way (e.g. add a comment) and see if it fails or passes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Did a deep search of the SDK and found that this is located in: platforms\android-34\build.prop, then noticed that in the Paparazzi codebase this is mentioned here:

val platformDataRoot = System.getProperty("paparazzi.platform.data.root")
?: throw RuntimeException("Missing system property for 'paparazzi.platform.data.root'")
val platformDataDir = File(platformDataRoot, "data")
val fontLocation = File(platformDataDir, "fonts")
val nativeLibLocation = File(platformDataDir, getNativeLibDir())
val icuLocation = File(platformDataDir, "icu" + File.separator + "icudt70l.dat")
val keyboardLocation = File(platformDataDir, "keyboards" + File.separator + "Generic.kcm")
val buildProp = File(environment.platformDir, "build.prop")
val attrs = File(platformDataResDir, "values" + File.separator + "attrs.xml")
val systemProperties = DeviceConfig.loadProperties(buildProp) + mapOf(
// We want Choreographer.USE_FRAME_TIME to be false so it uses System_Delegate.nanoTime()
"debug.choreographer.frametime" to "false"
)

So the fix for this is to add all of those files as inputs to the test task, otherwise Gradle thinks that nothing has changed and load from cache.

Copy link
Collaborator Author

@jrodbx jrodbx Jan 7, 2024

Choose a reason for hiding this comment

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

nice find!

Ok, so PrepareResourcesTask will re-run when compileSdkVersion changes:

  abstract val compileSdkVersion: Property<String>

which will cause the test task to re-run again

        test.systemProperties["paparazzi.test.resources"] =
          writeResourcesTask.flatMap { it.paparazziResources.asFile }.get().path

the issue here is 99% of the time $ANDROID_HOME/platforms/android-${compileSdkVersion} will have static contents, but 1% (when using sandbox preview sdks), this will not be the case, as we're seeing here because build.prop is changed, but not being detected.

I'd hate to change the plugin to crawl through the entire platformDir checking for file content changes for the 1% test case, especially given initial findings that we might be caching huge payloads, so I think the "right fix" here is to disable caching for that specific plugin test.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Another option -- if this comes up again -- would be to force the PrepareResourcesTask to rerun when compileSdkVersion doesn't parse to an integer value.

Comment on lines +8 to +17
add("testImplementation", "com.google.guava:guava") {
attributes {
attribute(
TargetJvmEnvironment.TARGET_JVM_ENVIRONMENT_ATTRIBUTE,
objects.named(TargetJvmEnvironment, TargetJvmEnvironment.STANDARD_JVM)
)
}
because("LayoutLib and sdk-common depend on Guava's -jre published variant." +
"See https://github.com/cashapp/paparazzi/issues/906.")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could/should this be somehow done in a way that it affects all of Paparazzi's users? If someone uses Paparazzi on testImplementation, they'll likely run into issues (we have so many reported in this repo).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can't think of a way, without force bundling Guava/Truth, which I'm reluctant to do. @JakeWharton, any thoughts?

@jrodbx jrodbx changed the base branch from master to jrod/2024-01-06/sandbox-previews January 7, 2024 16:35
Base automatically changed from jrod/2024-01-06/sandbox-previews to master January 8, 2024 21:29
Copy link
Collaborator

@luis-cortes luis-cortes left a comment

Choose a reason for hiding this comment

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

LGTM

@jrodbx jrodbx merged commit ab9928f into master Jan 8, 2024
14 checks passed
@jrodbx jrodbx deleted the jrod/2024-01-04/bump-truth branch January 8, 2024 21:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants