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

Avoid eagerly resolving input files in ProtobufExtract #713

Merged
merged 1 commit into from
Jun 28, 2023

Conversation

gavra0
Copy link
Contributor

@gavra0 gavra0 commented 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

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
Copy link
Collaborator

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

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

It isn't clear how this improves the situation, as in both cases we have a provider and it does does similar things. Best guess I have is that providerFactory.provider didn't include dependency information which required it to be loaded more eagerly whereas getElements().map does. It doesn't seem like it could be because s/File/FileSystemLocation/.

In any case, I believe you that this fixes it. That's just my way of saying I'm really happy you made a test.

Copy link
Collaborator

@rougsig rougsig left a comment

Choose a reason for hiding this comment

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

LGTM

@gavra0
Copy link
Contributor Author

gavra0 commented Jun 29, 2023

@ejona86 You analysis is correct: during task graph building Gradle had to compute return value of providerFactory.provider and to invoke get() on this provider. With FileCollection.getElements() the dependency information is already in that provider and it does not need to do a get() on it.

Thanks for a quick review :)

YifeiZhuang pushed a commit to YifeiZhuang/protobuf-gradle-plugin that referenced this pull request 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
YifeiZhuang added a commit that referenced this pull request 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

Successfully merging this pull request may close these issues.

3 participants