You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@groovy.apache.org by GitBox <gi...@apache.org> on 2020/09/29 14:45:30 UTC
[GitHub] [groovy] melix opened a new pull request #1382: Cc/gradle/update publishing
melix opened a new pull request #1382:
URL: https://github.com/apache/groovy/pull/1382
This is a draft PR which refactors the build in order to:
- cleanup bad practices
- introduce configuration avoidance
- remove deprecations
- add support for the `maven-publish` plugin instead of the old one
Initially I only wanted to focus on the last point but I realized there's a LOT of hackery in the build which makes it complicated to migrate without doing the cleanup first.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] paulk-asert commented on pull request #1382: Make the Gradle build more idiomatic
Posted by GitBox <gi...@apache.org>.
paulk-asert commented on pull request #1382:
URL: https://github.com/apache/groovy/pull/1382#issuecomment-706121593
Merged, Thanks!! Still a few glitches to unravel but with master continually updating, I think merging and some post amendments will be quickest path to get this in.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] melix commented on a change in pull request #1382: Make the Gradle build more idiomatic
Posted by GitBox <gi...@apache.org>.
melix commented on a change in pull request #1382:
URL: https://github.com/apache/groovy/pull/1382#discussion_r499256703
##########
File path: buildSrc/src/main/groovy/org/apache/groovy/gradle/GroovyLibraryExtension.groovy
##########
@@ -64,16 +66,18 @@ class GroovyLibraryExtension {
final TaskContainer tasks
final ProjectLayout layout
+ @Inject
GroovyLibraryExtension(ObjectFactory factory,
+ ProjectLayout layout,
SharedConfiguration sharedConfiguration,
JavaPluginConvention javaPluginConvention,
JavaPluginExtension javaPluginExtension,
SoftwareComponentContainer components,
ConfigurationContainer configurations,
- TaskContainer tasks,
- ProjectLayout layout
+ TaskContainer tasks
) {
this.objects = factory
+ this.layout = layout
this.sharedConfiguration = sharedConfiguration
this.includeInGroovyAll = factory.property(Boolean).convention(true)
this.grooid = factory.property(Boolean).convention(false)
Review comment:
I could but given that I need to inject other properties in any case, I prefer keeping the things consistent and not have a mix of abstract properties and injected properties
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] melix commented on pull request #1382: Make the Gradle build more idiomatic
Posted by GitBox <gi...@apache.org>.
melix commented on pull request #1382:
URL: https://github.com/apache/groovy/pull/1382#issuecomment-701697763
@paulk-asert @danielsun1106 I'm setting this for review despite CI failing, I'm not 100% sure why the `dist` command on GH action fails, maybe it's just too late.
As you can see this PR ended up being much larger than I initially wanted it to be. In order to migrate to the `maven-publish` plugin, I ended up refactoring the build to make it cleaner, more idiomatic. Work is not complete but I could spend weeks on that.
The new build is, IMO, much easier to understand. I tried to remove bad practices I found, and I tested that I could publish on a local artifactory server with signing. The new publications are richer and use Gradle Module Metadata.
Also I took the liberty to remove the "backports" modules. Reintroducing wouldn't be too hard, but AFAIK they haven't really been used, and now there's only a 2.3 backport, and we're talking about Groovy 4.
I'm going to continue a bit on this but I'd appreciate that you play a bit with this branch to find out if they are obvious problems: during the refactoring it's possible I've slightly changed the outputs.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] wolfs commented on a change in pull request #1382: Make the Gradle build more idiomatic
Posted by GitBox <gi...@apache.org>.
wolfs commented on a change in pull request #1382:
URL: https://github.com/apache/groovy/pull/1382#discussion_r499184531
##########
File path: buildSrc/src/main/groovy/org/apache/groovy/gradle/GroovyLibraryExtension.groovy
##########
@@ -64,16 +66,18 @@ class GroovyLibraryExtension {
final TaskContainer tasks
final ProjectLayout layout
+ @Inject
GroovyLibraryExtension(ObjectFactory factory,
+ ProjectLayout layout,
SharedConfiguration sharedConfiguration,
JavaPluginConvention javaPluginConvention,
JavaPluginExtension javaPluginExtension,
SoftwareComponentContainer components,
ConfigurationContainer configurations,
- TaskContainer tasks,
- ProjectLayout layout
+ TaskContainer tasks
) {
this.objects = factory
+ this.layout = layout
this.sharedConfiguration = sharedConfiguration
this.includeInGroovyAll = factory.property(Boolean).convention(true)
this.grooid = factory.property(Boolean).convention(false)
Review comment:
I think you should get away with declaring the `grooid` property abstract and then calling the abstract getter in the constructor setting it to a convention. This would allow you to get rid of using the `ObjectFactory`, if you wouldn't need it to instantiate the attributes.
So something like this should work:
```
class GroovyLibraryExtension {
abstract Property<Boolean> getGrooid()
GroovyLibraryExtension(...) {
grooid.convention(false)
}
}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] paulk-asert commented on pull request #1382: Make the Gradle build more idiomatic
Posted by GitBox <gi...@apache.org>.
paulk-asert commented on pull request #1382:
URL: https://github.com/apache/groovy/pull/1382#issuecomment-702452165
I will look at it soon. Might be a few days. Nice work! Thanks so much.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] asfgit closed pull request #1382: Make the Gradle build more idiomatic
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #1382:
URL: https://github.com/apache/groovy/pull/1382
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] paulk-asert commented on pull request #1382: Make the Gradle build more idiomatic
Posted by GitBox <gi...@apache.org>.
paulk-asert commented on pull request #1382:
URL: https://github.com/apache/groovy/pull/1382#issuecomment-706121593
Merged, Thanks!! Still a few glitches to unravel but with master continually updating, I think merging and some post amendments will be quickest path to get this in.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] melix commented on pull request #1382: Make the Gradle build more idiomatic
Posted by GitBox <gi...@apache.org>.
melix commented on pull request #1382:
URL: https://github.com/apache/groovy/pull/1382#issuecomment-702426140
PR looks fine from my POV. Let me know how we can make progress on merging this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] asfgit closed pull request #1382: Make the Gradle build more idiomatic
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #1382:
URL: https://github.com/apache/groovy/pull/1382
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org