jdaugherty commented on PR #37:
URL:
https://github.com/apache/grails-gradle-publish/pull/37#issuecomment-5779075718
Notes from a review pass over this change, since the branch was force-pushed
with a different approach than it opened with.
### Changed as a result
**The exclude-based fix could publish an empty javadoc jar.** The first
version of this PR left Gradle's `from(javadoc)` wiring in place and filtered
the javadoc destination directory back out with `jar.exclude { ... }`. But
`exclude` applies to the jar's whole copy spec, not just the javadoc
contribution. With a `javadoc.destinationDir` that is an ancestor of the
groovydoc output — `build/docs`, say — the path check matches the groovydoc as
well, and the jar is published containing nothing but the manifest, silently.
Reproduced before rewriting.
That is what moved this to registering `javadocJar` from the groovydoc up
front. Gradle only wires `from(javadoc)` into a `javadocJar` it registers
itself, so there is nothing to filter and the failure mode cannot arise. It
also removes ~17 lines rather than adding them, and it covers cases the exclude
missed — `-x javadoc`, `javadoc { onlyIf { false } }` — because the jar no
longer depends on *why* the javadoc task did not run.
`tasks.named('javadoc', Javadoc)` went back to the untyped
`tasks.named('javadoc')` it was before; the type was only needed for
`destinationDir`, which the new approach does not read.
**A test assertion was vacuous.** `readJarFileEntry("help-doc.html",
javadocJar) != staleMarker` passes just as well when the entry is missing
entirely, so it could not have caught the empty-jar regression above. It now
asserts the entry is present as well, and there is a second case covering the
ancestor-`destinationDir` configuration directly.
### Considered and not changed
**Configuration cache.** Raised as a likely failure — capturing a
`TaskProvider` in a `Spec` resolved at execution time — with the note that
`org.gradle.configuration-cache=false` in this repo means CI would not catch
it. It was tested against the fixture both before and after: the entry stores
and is reused, and the jar is byte-identical. `project.provider {}` is resolved
to a plain value when the cache entry is written, so no task reference is
serialized. The suggested cleanup of inlining `javadocTask.get()` into the spec
would have been the thing to actually break it. Moot either way now that the
predicate is gone, but worth recording.
**Two items better handled separately, rather than widening a bugfix:**
- `project.files(groovyDocTask.destinationDir)` reads the groovydoc
destination eagerly at configuration time, so a `groovydoc.destinationDir` set
afterwards leaves the jar pointed at a directory groovydoc never writes to.
Pre-existing, and the same latent bug class as this issue.
- `findJarFileEntry` is duplicated verbatim in `GrailsPublishPluginSpec` and
`AdditionalPublicationSpec`; both it and the new `readJarFileEntry` belong in
the shared `GradleSpecification`.
--
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]
