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

ProtobufExtract after upgrading to Gradle 8.1 causes unexpected inputs to configuration cache #711

Closed
liutikas opened this issue Jun 16, 2023 · 6 comments

Comments

@liutikas
Copy link

When we upgrade to Gradle 8.1 in github.com/androidx/androidx we found that com.google.protobuf.gradle.ProtobufExtract$_instantiateFilteredProtos_closure4 causes ../../out/androidx/datastore/datastore-sampleapp/build/intermediates/compile_app_classes_jar/debug/classes.jar to be an input to the configuration cache calculation.

image

What this means, if you run a build on a clean CI, then run a second build, then the second build will not hit configuration cache and will instead recalculate it from scratch again. This is very problematic for our CI usecase, but it is generally bad as it will cause a lot of invalidations of configuration cache when it is not expected.

In other similar issues with Gradle 8.1 new input to configuration cache, it has been usage of File.exists() that caused Gradle to consider it as an input to the configuration cache. I do not see usage of that in ProtobufExtract.groovy so maybe it is the use of isDirectory() that is triggering it.

@ejona86
Copy link
Collaborator

ejona86 commented Jun 20, 2023

Given "closure4" in the screenshot, that seems likely to be:

zipTree.each { entry ->
protoInputs.add(archiveFacade.zipTree(entry).matching(protoFilter))
}

I'm not sure how best to resolve that, without looking at it more.

@liutikas
Copy link
Author

@gavra0 👋

@gavra0
Copy link
Contributor

gavra0 commented Jun 28, 2023

IIUC this method does not need to run on <init> it can be lazy. I'll try that approach.

gavra0 added a commit to gavra0/protobuf-gradle-plugin that referenced this issue Jun 28, 2023
Rely on FileCollection.getElements() (available in Gradle 5.6+) to
ensure no files are accessed eagerly during configuration. This also
ensures configuration cache key does not depend on them.

Fixes issue google#711.

Test: Added "testProjectDependent proto extraction with configuration cache" with Gradle 8.1
gavra0 added a commit to gavra0/protobuf-gradle-plugin that referenced this issue Jun 28, 2023
Rely on FileCollection.getElements() (available in Gradle 5.6+) to
ensure no files are accessed eagerly during configuration. This also
ensures configuration cache key does not depend on them.

Fixes issue google#711.

Test: Added "testProjectDependent proto extraction with configuration cache" with Gradle 8.1
@gavra0
Copy link
Contributor

gavra0 commented Jun 28, 2023

I've uploaded a PR that fixes this issue (confirmed in a test).

ejona86 pushed a commit that referenced this issue Jun 28, 2023
Rely on FileCollection.getElements() (available in Gradle 5.6+) to
ensure no files are accessed eagerly during configuration. This also
ensures configuration cache key does not depend on them.

Fixes issue #711.

Test: Added "testProjectDependent proto extraction with configuration cache" with Gradle 8.1
@ejona86
Copy link
Collaborator

ejona86 commented Jun 28, 2023

Fixed by #713.

And now I know the question is "when will we do a release?" We should probably fix #708 as well; it seems quick. @YifeiZhuang, let's try to do a release in the next week or two.

@ejona86 ejona86 closed this as completed Jun 28, 2023
YifeiZhuang pushed a commit to YifeiZhuang/protobuf-gradle-plugin that referenced this issue Jul 13, 2023
Rely on FileCollection.getElements() (available in Gradle 5.6+) to
ensure no files are accessed eagerly during configuration. This also
ensures configuration cache key does not depend on them.

Fixes issue google#711.

Test: Added "testProjectDependent proto extraction with configuration cache" with Gradle 8.1
@liutikas
Copy link
Author

Any updates on a release of a fixed version?

YifeiZhuang added a commit that referenced this issue Jul 13, 2023
Rely on FileCollection.getElements() (available in Gradle 5.6+) to
ensure no files are accessed eagerly during configuration. This also
ensures configuration cache key does not depend on them.

Fixes issue #711.

Test: Added "testProjectDependent proto extraction with configuration cache" with Gradle 8.1

Co-authored-by: Ivan Gavrilovic <gavra0@users.noreply.github.com>
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

No branches or pull requests

3 participants