codeconsole commented on code in PR #43:
URL:
https://github.com/apache/grails-gradle-publish/pull/43#discussion_r4190860618
##########
plugin/src/main/groovy/org/apache/grails/gradle/publish/GrailsPublishGradlePlugin.groovy:
##########
@@ -389,16 +429,22 @@ Note: properties are read as Gradle properties (the root
project's gradle.proper
pe.repositories { RepositoryHandler repoHandler ->
repoHandler.maven { MavenArtifactRepository repo ->
+ repo.name = MAVEN_REPOSITORY_NAME
+ repo.url = mavenPublishUrl
final String mavenPublishUsername =
findProjectProperty(project, 'mavenPublishUsername') ?:
System.getenv('MAVEN_PUBLISH_USERNAME')
final String mavenPublishPassword =
findProjectProperty(project, 'mavenPublishPassword') ?:
System.getenv('MAVEN_PUBLISH_PASSWORD')
if (mavenPublishUsername && mavenPublishPassword) {
+ // explicit credentials: Gradle runs builds
publishing with them without storing a
+ // configuration cache entry
repo.credentials { PasswordCredentials
credentials ->
credentials.username = mavenPublishUsername
credentials.password = mavenPublishPassword
}
+ } else if (isHttpRepository(repo) &&
hasRepositoryCredentialProperties(project)) {
Review Comment:
**This checks `mavenUsername`, but Gradle reads the credentials of the
repository's final name, which can be `maven2`**
`credentials(PasswordCredentials)` binds to the repository's name lazily
(`AbstractAuthenticationSupportedRepository` passes `provider(this::getName)`).
`repoHandler.maven { }` runs this block first and only then makes the name
unique (`DefaultArtifactRepositoryContainer.addWithUniqueName`). So if the
build script already declared a publishing repository named `maven`, this one
becomes `maven2`. `maven` is the default name of a plain `repositories { maven
{ } }` block, and the usual owner of `mavenUsername`/`mavenPassword`. The check
here then finds that other repository's properties, and Gradle looks for
`maven2Username`/`maven2Password`, which are missing, so a `publish` that
includes this repository fails. Before this change the repository got no
credentials, so publishing to a repository that doesn't need them worked.
Checking after the repository is added, with its final name, avoids it:
```groovy
MavenArtifactRepository repo = repoHandler.maven { MavenArtifactRepository
it ->
// name, url and explicit credentials as now
}
if (!explicitCredentials && isHttpRepository(repo) &&
hasRepositoryCredentialProperties(project, repo.name)) {
repo.credentials(PasswordCredentials)
}
```
The README line saying the repository is named `maven` would need the same
caveat. A narrow case, not blocking.
##########
plugin/src/functionalTest/groovy/org/apache/grails/gradle/publish/ReleaseSigningSpec.groovy:
##########
@@ -78,12 +84,20 @@ class ReleaseSigningSpec extends GradleSpecification {
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'
+ fixture | environment |
sharedCoordinates | description
+ 'simple-project' | [:] |
false | 'one publication'
+ 'additional-publication' | [:] |
false | 'an additional publication'
+ 'gradle-plugin-project' | [:] |
false | 'Gradle plugin project, java-gradle-plugin applied last'
+ 'gradle-plugin-project' | [APPLY_JAVA_GRADLE_PLUGIN_FIRST: 'true'] |
false | 'Gradle plugin project, java-gradle-plugin applied first'
+ 'gradle-plugin-project' | [PUBLISH_SHARED_ARTIFACTS: 'true'] |
true | 'publications sharing artifacts'
+ }
+
+ /** Whether the first task had finished before the second one ran, the
order the build reports them in */
+ private static boolean publishedBefore(BuildResult result, String first,
String second) {
Review Comment:
**This check compares start order, so it can pass with the race still
present**
TestKit adds a task to `BuildResult.tasks` when the task's start event
arrives (`ToolingApiGradleExecutor.TaskExecutionProgressListener`), so the
index is start order. The check shows the first task started before the second,
not that it finished before the second ran, as the javadoc says. In this
release, every publish task `mustRunAfter` every `Sign` task, so the two
publish tasks become ready at the same moment. Their start order then reflects
Gradle's scheduling more than the plugin's ordering, and this assertion can
pass without `orderPublishTasksSharingCoordinates`. The unit test on
`mustRunAfter` is what actually covers the fix.
To make this prove the tasks don't overlap, the fixture could print a marker
at the start and end of each publish task, and the spec could check that one
task's end comes before the other's start:
```groovy
tasks.withType(AbstractPublishToMaven).configureEach { task ->
String taskName = task.name
task.doFirst { println "PUBLISH-START $taskName" }
task.doLast { println "PUBLISH-END $taskName" }
}
```
Otherwise, reword the javadoc to say it checks start order only. Not
blocking.
--
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]
