codeconsole commented on code in PR #39:
URL:
https://github.com/apache/grails-gradle-publish/pull/39#discussion_r4116562416
##########
plugin/src/main/groovy/org/apache/grails/gradle/publish/GrailsPublishGradlePlugin.groovy:
##########
@@ -806,6 +931,28 @@ Note: if project properties are used, the properties must
be defined prior to ap
] : null
}
+ /**
+ * The extra artifact usually lives in a classes directory, such as
META-INF/grails-plugin.xml. Publishing it from
+ * there would write its signature into that directory, which other tasks
read, such as the jar tasks, so publish a
+ * copy instead.
+ */
+ private static void addExtraArtifact(Project project, MavenPublication
publication, Map<String, String> extraArtifact) {
+ File source = new File(extraArtifact.source)
+ TaskProvider<Copy> copyTask =
project.tasks.register('grailsPublishExtraArtifact', Copy) { Copy copy ->
+ copy.from(source)
+
copy.into(project.layout.buildDirectory.dir('grails-publish/extra-artifact'))
+ // the file is written by the task producing the classes directory
it lives in, and the jar tasks package
+ // those directories, so depending on them builds it
+ copy.dependsOn(project.tasks.withType(Jar))
Review Comment:
Looks good, thanks. The dry-run test is a good guard, since it fails with
the old `withType(Jar)` dependency.
##########
plugin/src/main/groovy/org/apache/grails/gradle/publish/GrailsPublishGradlePlugin.groovy:
##########
@@ -137,89 +171,127 @@ The credentials and connection url must be specified as
a project property or an
When using `NEXUS_PUBLISH`, either the property `signing.secretKeyRingFile`
must be set to the path of the GPG keyring file or local gpg must be configured
to sign artifacts.
-Note: if project properties are used, the properties must be defined prior to
applying this plugin.
+Note: properties are read from the root project's gradle.properties, the one
in the Gradle user home, -P or ORG_GRADLE_PROJECT_ environment variables, or
from the project applying this plugin: its own gradle.properties, or its build
script before the plugin is applied. Properties set on parent projects,
including in the gradle.properties of a parent project's directory, are not
read.
"""
}
+ /**
+ * Finds a property set via `ext` on the given project itself, or a Gradle
property.
+ *
+ * The project's own extra properties are checked first, so a value set in
its build script overrides a Gradle property,
+ * as it does with {@link Project#findProperty}. Properties set on parent
projects are deliberately not read: resolving
+ * them implicitly is removed in Gradle 10, and reading them explicitly is
not allowed with Isolated Projects.
+ */
+ @PackageScope
+ static Object findProjectProperty(Project project, String name) {
+ def extraProperties = project.extensions.extraProperties
+ if (extraProperties.has(name)) {
+ return extraProperties.get(name)
+ }
+ project.providers.gradleProperty(name).orNull
+ }
+
+ /**
+ * Finds a publish type property. These fall back to a default when unset,
so a value that is only set on a parent
+ * project, which is no longer read, would silently change where artifacts
are published. Fail the build instead.
+ */
+ private Object findPublishTypeProperty(Project project, String name) {
+ Object value = findProjectProperty(project, name)
+ // with Isolated Projects, parent projects cannot be inspected, and
such builds never relied on reading them
+ if (value == null && !buildFeatures.isolatedProjects.active.get()) {
+ for (Project parent = project.parent; parent != null; parent =
parent.parent) {
+ if (parent.extensions.extraProperties.has(name)) {
+ throw new InvalidUserDataException("The property `${name}`
is set on ${parent} but not on ${project}. " +
+ 'The Grails Publish plugin does not read
properties from parent projects. ' +
+ "Set `${name}` in the root project's
gradle.properties, with -P${name}=..., or on ${project} " +
+ 'itself, in its gradle.properties or in its build
script before applying the plugin.')
+ }
+ }
+ }
+ value
+ }
+
@Override
void apply(Project project) {
- project.rootProject.logger.info("Applying Grails Publish Gradle Plugin
for `${project.name}`...");
+ LOG.info('Applying Grails Publish Gradle Plugin for `{}`...',
project.name)
if (project.extensions.findByName('grailsPublish') == null) {
project.extensions.create('grailsPublish', GrailsPublishExtension)
}
- final String nexusPublishUrl = project.findProperty('nexusPublishUrl')
?: System.getenv('NEXUS_PUBLISH_URL') ?: ''
- final String nexusPublishSnapshotUrl =
project.findProperty('nexusPublishSnapshotUrl') ?:
System.getenv('NEXUS_PUBLISH_SNAPSHOT_URL') ?: ''
- final String nexusPublishUsername =
project.findProperty('nexusPublishUsername') ?:
System.getenv('NEXUS_PUBLISH_USERNAME') ?: ''
- final String nexusPublishPassword =
project.findProperty('nexusPublishPassword') ?:
System.getenv('NEXUS_PUBLISH_PASSWORD') ?: ''
- final String nexusPublishStagingProfileId =
project.findProperty('nexusPublishStagingProfileId') ?:
System.getenv('NEXUS_PUBLISH_STAGING_PROFILE_ID') ?: ''
- final String nexusPublishDescription =
project.findProperty('nexusPublishDescription') ?:
System.getenv('NEXUS_PUBLISH_DESCRIPTION') ?: ''
+ final String nexusPublishUrl = findProjectProperty(project,
'nexusPublishUrl') ?: System.getenv('NEXUS_PUBLISH_URL') ?: ''
+ final String nexusPublishSnapshotUrl = findProjectProperty(project,
'nexusPublishSnapshotUrl') ?: System.getenv('NEXUS_PUBLISH_SNAPSHOT_URL') ?: ''
Review Comment:
Thanks, this covers the case I was worried about. One follow-up: the check
runs even when the project doesn't publish through Nexus, and the new test
shows it. `properties-from-parent-project` is a snapshot with the default
publish types, so it publishes with Maven, yet `:subproject:assemble` fails. A
build that keeps `ext.nexusPublishUrl` on its root would fail every snapshot
and local build, such as `test`, not only the release that uses the URL.
The URLs are only used inside `if (useNexusPublish)`, so they could be read
there, once `useNexusPublish` is known. A release would still fail before
anything reaches oss.sonatype.org. Failing on every build does surface the
migration sooner, though, so I'll leave that call to you.
A smaller one: for these two properties, the error message could mention
`NEXUS_PUBLISH_URL` / `NEXUS_PUBLISH_SNAPSHOT_URL`, since setting the
environment variable fixes it too.
Neither blocks the merge for me.
##########
plugin/src/main/groovy/org/apache/grails/gradle/publish/GrailsPublishExtension.groovy:
##########
@@ -194,7 +197,9 @@ class GrailsPublishExtension {
testRepositoryPath = objects.directoryProperty().convention(null as
Directory)
pomCustomization = objects.property(Closure).convention(null as
Closure)
addComponents = objects.property(Boolean).convention(true)
- publicationName = objects.property(String).convention('maven')
+ publicationName = objects.property(String).convention(project.provider
{
+ isGradlePluginProject(project) ? 'pluginMaven' : 'maven'
Review Comment:
Looks good, thanks. The README and the Javadoc match the new check, and the
`automatedPublishing = false` test covers it.
##########
plugin/src/functionalTest/groovy/org/apache/grails/gradle/publish/ReleaseSigningSpec.groovy:
##########
@@ -0,0 +1,189 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.grails.gradle.publish
+
+import org.gradle.testkit.runner.GradleRunner
+import spock.lang.Requires
+import spock.lang.Shared
+
+import java.nio.file.Files
+import java.nio.file.Path
+import java.util.concurrent.TimeUnit
+
+/**
+ * Signs and publishes releases with a throwaway GPG key, to cover the sign
and publish task wiring.
+ */
+@Requires({ ReleaseSigningSpec.gpgAvailable() })
+class ReleaseSigningSpec extends GradleSpecification {
+
+ @Shared
+ Path gnupgHome
+
+ @Shared
+ String keyId
+
+ List<File> toCleanup = []
+
+ void setupSpec() {
+ // keep the path short, since gpg-agent's socket path is limited in
length
+ gnupgHome = Files.createTempDirectory('gpg')
+ gpg('--batch', '--passphrase', '', '--quick-gen-key', 'Throwaway Test
Key <[email protected]>', 'rsa2048', 'sign', 'never')
+ keyId = gpg('--list-keys', '--with-colons').readLines()
+ .find { it.startsWith('pub:') }
+ .split(':')[4]
+ .takeRight(8)
+ }
+
+ void cleanup() {
+ toCleanup.each { it.deleteDir() }
+ }
+
+ void cleanupSpec() {
+ runGpgconf('--kill', 'gpg-agent')
+ gnupgHome.toFile().deleteDir()
+ }
+
+ def "a signed release signs every published file - #description"() {
+ given:
+ File repository = File.createTempDir('release-repository')
+ File mavenLocal = File.createTempDir('release-maven-local')
+ toCleanup << repository << mavenLocal
+
+ and:
+ GradleRunner runner = setupTestResourceProject('other-artifacts',
fixture)
+ runner = setGradleProperty('projectVersion', '0.0.1', runner)
+ runner = setGradleProperty('releasePublishType', 'MAVEN_PUBLISH',
runner)
+ runner = setGradleProperty('mavenPublishUrl', repository.absolutePath,
runner)
+ runner = addEnvironmentVariable('GRAILS_PUBLISH_RELEASE', 'true',
runner)
+ // the build environment is otherwise empty, and signing runs the gpg
command
+ runner = addEnvironmentVariable('PATH', System.getenv('PATH'), runner)
+ runner = addEnvironmentVariable('GNUPGHOME',
gnupgHome.toAbsolutePath().toString(), runner)
+ runner = addEnvironmentVariable('SIGNING_KEY', keyId, runner)
+ environment.each { String key, String value ->
+ runner = addEnvironmentVariable(key, value, runner)
+ }
+
+ when:
+ executeTask('publish', ['publishToMavenLocal',
"-Dmaven.repo.local=${mavenLocal.absolutePath}".toString()], runner)
+
+ then:
+ List<File> published = publishedFiles(repository)
+ published
+ published.findAll { !signatureVerifies(it) } == []
+
+ and:
+ List<File> publishedLocally = publishedFiles(mavenLocal)
+ publishedLocally
+ publishedLocally.findAll { !signatureVerifies(it) } == []
+
+ where:
+ fixture | environment |
description
+ 'simple-project' | [:] |
'one publication'
+ 'additional-publication' | [:] |
'an additional publication'
+ 'gradle-plugin-project' | [:] |
'Gradle plugin project, java-gradle-plugin applied last'
+ 'gradle-plugin-project' | [APPLY_JAVA_GRADLE_PLUGIN_FIRST: 'true'] |
'Gradle plugin project, java-gradle-plugin applied first'
+ 'gradle-plugin-project' | [PUBLISH_SHARED_ARTIFACTS: 'true'] |
'publications sharing artifacts'
+ }
+
+ def "a signed release publishes the grails-plugin.xml without signing it
inside the classes directory"() {
+ given:
+ File repository = File.createTempDir('release-repository')
+ toCleanup << repository
+
+ and:
+ GradleRunner runner = setupTestResourceProject('other-artifacts',
'grails-plugin-project')
+ runner = setGradleProperty('projectVersion', '0.0.1', runner)
+ runner = setGradleProperty('releasePublishType', 'MAVEN_PUBLISH',
runner)
+ runner = setGradleProperty('mavenPublishUrl', repository.absolutePath,
runner)
+ runner = addEnvironmentVariable('GRAILS_PUBLISH_RELEASE', 'true',
runner)
+ runner = addEnvironmentVariable('PATH', System.getenv('PATH'), runner)
+ runner = addEnvironmentVariable('GNUPGHOME',
gnupgHome.toAbsolutePath().toString(), runner)
+ runner = addEnvironmentVariable('SIGNING_KEY', keyId, runner)
+
+ and: 'the grails-plugin.xml exists, as the plugin only publishes it
when it does at configuration time'
+ executeTask('classes', runner)
+
+ when:
+ executeTask('publish', runner)
+
+ then: 'the grails-plugin.xml is published and signed'
+ File publishedPluginXml = new File(repository,
'org/grails/example/grails-plugin-project/0.0.1/grails-plugin-project-0.0.1-plugin.xml')
+ publishedPluginXml.text == '<plugin name="grails-plugin-project"/>'
+ signatureVerifies(publishedPluginXml)
+
+ and: 'its signature is not written into the classes directory, which
other tasks read'
+ !new File(runner.projectDir,
'build/classes/groovy/main/META-INF/grails-plugin.xml.asc').exists()
+ }
+
+ private static List<File> publishedFiles(File directory) {
+ List<File> files = []
+ directory.eachFileRecurse { File file ->
+ if (file.name ==~ /.*\.(jar|pom|module)/) {
+ files << file
+ }
+ }
+ files
+ }
+
+ /**
+ * Whether the file's .asc signature exists and verifies against the
throwaway key.
+ */
+ private boolean signatureVerifies(File file) {
+ File signature = new File("${file.path}.asc")
+ if (!signature.exists()) {
+ return false
+ }
+ try {
+ gpg('--batch', '--verify', signature.path, file.path)
+ true
+ } catch (IllegalStateException ignored) {
+ false
+ }
+ }
+
+ private String gpg(String... arguments) {
+ run(['gpg', '--homedir', gnupgHome.toString()] + arguments.toList())
+ }
+
+ private void runGpgconf(String... arguments) {
+ try {
+ run(['gpgconf', '--homedir', gnupgHome.toString()] +
arguments.toList())
+ } catch (Exception ignored) {
+ // the agent is gone with the temporary directory either way
+ }
+ }
+
+ private static String run(List<String> command) {
+ Process process = new
ProcessBuilder(command).redirectErrorStream(true).start()
+ String output = process.inputStream.text
+ if (!process.waitFor(2, TimeUnit.MINUTES) || process.exitValue() != 0)
{
+ throw new IllegalStateException("${command.join(' ')}
failed:\n${output}")
+ }
+ output
Review Comment:
Looks good, and thanks for checking the timeout path too.
--
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]
