jdaugherty commented on code in PR #39:
URL:
https://github.com/apache/grails-gradle-publish/pull/39#discussion_r4105670810
##########
gradle/signing-config.gradle:
##########
@@ -28,7 +28,7 @@ if (isReleaseVersion) {
afterEvaluate {
signing {
- required { isReleaseVersion && gradle.taskGraph.hasTask('publish')
}
+ required { rootProject.isReleaseVersion &&
gradle.taskGraph.hasTask('publish') }
useGpgCmd()
Publication[] publications = new
Publication[publishing.publications.size()]
Review Comment:
Along the same lines as `matching`: rather than copying the container into
an array with Groovy's `findAll()`, this could use the signing extension's
`DomainObjectCollection` overload directly:
```groovy
sign publishing.publications
```
That is a live collection, so it also signs publications added later.
##########
gradle/signing-config.gradle:
##########
@@ -38,7 +38,7 @@ if (isReleaseVersion) {
}
tasks.withType(Sign).configureEach {
- onlyIf { isReleaseVersion }
+ onlyIf { rootProject.isReleaseVersion }
Review Comment:
Nit (also predates the PR): this whole block is already inside `if
(rootProject.isReleaseVersion)`, so this `onlyIf` is always true, and so is the
first half of `required` above. Both could go.
##########
gradle/signing-config.gradle:
##########
@@ -28,7 +28,7 @@ if (isReleaseVersion) {
afterEvaluate {
signing {
- required { isReleaseVersion && gradle.taskGraph.hasTask('publish')
}
+ required { rootProject.isReleaseVersion &&
gradle.taskGraph.hasTask('publish') }
Review Comment:
This predates the PR, but since the line is touched: `required` can never be
true. `TaskExecutionGraph.hasTask(String)` matches full task paths (e.g.
`:grails-publish:publish`), so `'publish'` never matches. The release workflow
also runs `publishToSonatype`
([release.yaml](https://github.com/apache/grails-gradle-publish/blob/8b7eb295ae50a385f35b9c9a227f9d8077cdf589/.github/workflows/release.yaml#L102)),
not `publish`. The effect is that if the signing key is missing during a
release, the Sign tasks are skipped instead of failing the build.
Since this block only runs for release builds, `required = true` may be
enough. Fine as a follow-up.
##########
gradle/publish-config.gradle:
##########
@@ -18,7 +18,7 @@
*/
publishing {
- if (!isReleaseVersion) {
+ if (!rootProject.isReleaseVersion) {
Review Comment:
Nit: `rootProject.isReleaseVersion` fixes the warning, but it still reads
the root project's `ext`, which Gradle calls out as a problem for Isolated
Projects. The value only comes from `GRAILS_PUBLISH_RELEASE`, so each script
could read it directly, e.g.
`providers.environmentVariable('GRAILS_PUBLISH_RELEASE').map {
Boolean.parseBoolean(it) }.getOrElse(false)`. Fine to leave for now.
##########
plugin/src/main/groovy/org/apache/grails/gradle/publish/GrailsPublishGradlePlugin.groovy:
##########
@@ -902,7 +902,7 @@ Note: if project properties are used, the properties must
be defined prior to ap
GrailsPublishExtension gpe =
project.extensions.getByType(GrailsPublishExtension)
Set<String> additionalPublicationSourceSets =
gpe.additionalPublications
.collect { it.sourceSetName.get() } as Set<String>
- Collection<SourceSet> publishedSourceSets = sourceSets.findAll {
SourceSet sourceSet ->
+ Collection<SourceSet> publishedSourceSets = sourceSets.matching {
SourceSet sourceSet ->
!(sourceSet.name in additionalPublicationSourceSets)
}
jar.from publishedSourceSets.collect { it.allSource }
Review Comment:
Nit (predates the PR): `jar.from` already registers these as task inputs, so
the `jar.inputs.files(...)` on the next line looks redundant. Computing
`publishedSourceSets.collect { it.allSource }` once would also avoid iterating
the filtered view twice.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
