-
Notifications
You must be signed in to change notification settings - Fork 276
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
Conversation
af318da
to
405ca49
Compare
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. |
405ca49
to
b514908
Compare
There was a problem hiding this 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.
There was a problem hiding this 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' |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
This needs to be rebased on top of master and remove the first commit. |
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.
b514908
to
a221693
Compare
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.