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 using project.copy when possible, and fix serialization #406

Merged
merged 1 commit into from
Jun 2, 2020

Conversation

gavra0
Copy link
Contributor

@gavra0 gavra0 commented Jun 1, 2020

Since Gradle 6.0 there is FileSystemOperations API which is compatible
with configuration caching. This PR introduces CopyActionFacade which
exposes the copy method, but implementation is using the new API whne
possible, and reverts to project.copy when Gradle version is lower than 6.0.

This PR also starts using Gradle 6.0 to build the plugin. It is needed in
order to use FileSystemOperations API.

In GenerateProtoTask, SourceSet is now not serialized as part of the
task state, and a provider capturing what is needed is created instead.

Integration test for the Java plugin is updated to Gradle 6.5-rc-1.

@gavra0 gavra0 force-pushed the configuration-caching-pt1 branch 3 times, most recently from af318da to 405ca49 Compare June 1, 2020 13:37
@gavra0
Copy link
Contributor Author

gavra0 commented Jun 1, 2020

Update to Gradle 6.0 updated the Groovy compiler, so let me upload a separate PR to fix all CodeNarc issues that are now reported.

Edit: this is now done in #407.

@gavra0 gavra0 force-pushed the configuration-caching-pt1 branch from 405ca49 to b514908 Compare June 1, 2020 15:07
@voidzcy voidzcy self-requested a review June 1, 2020 17:02
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.

This looks good. Once the Gradle 6 PR goes in this can be rebased on top of it.

Copy link
Collaborator

@voidzcy voidzcy left a comment

Choose a reason for hiding this comment

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

Generally LGTM, although IMO, cleaning up and wrapping the SourceSet field and fields that uses Project object with Property/Provider seem to be a cleaner way.

@@ -89,8 +92,7 @@ class ProtobufJavaPluginTest extends Specification {
.withProjectDir(projectDir)
.withArguments(
'build', '--stacktrace',
'-Dorg.gradle.unsafe.instant-execution=true',
'-Dorg.gradle.unsafe.instant-execution.fail-on-problems=true'
'--configuration-cache=warn'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want the test to be stricter by '--configuration-cache=on', although real users can definitely ignore configuration cache problems.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd love to do that, but the issue is that the OsDetectorPlugin plugin is not fully compliant and it uses JDK APIs to access system properties, instead of the Gradle API exposed with the ProviderFactory.

Copy link
Collaborator

@voidzcy voidzcy Jun 2, 2020

Choose a reason for hiding this comment

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

Sure, we can live with it now. I was wondering that the OsDetectorPlugin is so small that we can just turn it into a build service instead of directly using it as a plugin.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would be great. With that and new Gradle APIs for tarTree and zipTree in 6.6 we could run with --configuration-cache=on.

@ejona86
Copy link
Collaborator

ejona86 commented Jun 2, 2020

This needs to be rebased on top of master and remove the first commit.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
Since Gradle 6.0 there is FileSystemOperations API which is compatible
with configuration caching. This PR introduces CopyActionFacade which
exposes the copy method, but implementation is using the new API whne
possible, and reverts to project.copy when Gradle version is lower than 6.0.

This PR also starts using Gradle 6.0 to build the plugin. It is needed in
order to use FileSystemOperations API.

In GenerateProtoTask, SourceSet is now not serialized as part of the
task state, and a provider capturing what is needed is created instead.

Integration test for the Java plugin is updated to Gradle 6.5-rc-1.
@gavra0 gavra0 force-pushed the configuration-caching-pt1 branch from b514908 to a221693 Compare June 2, 2020 20:02
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.

None yet

3 participants