Discover all diagnostic manifests

[pkoenig10]

Our internal diagnostic annotation processor generates the diagnostic manifest in the class directory, not the resource directory.

I've confirmed that this works on one of our internal projects.

[changelog-app]

Generate changelog in changelog/@unreleased

Type

• Feature
• Improvement
• Fix
• Break
• Deprecation
• Manual task
• Migration

Description

The diagnostic manifest plugin now correctly discovers diagnostic manifest generates from places other than the resource directory.

Check the box to generate changelog(s)

• Generate changelog entry

[carterkozak]

Beautiful, thank you, @pkoenig10!

[carterkozak]

👍 looks great, feel free to merge+release when you're happy with it. Thanks again!

Dismissed because the approval was invalidated by another commit

[pkoenig10]

I was a bit confused why we set the org.gradle.libraryelements attribute, since it didn't seem necessary.

Then I discovered that @iamdanfox's implementation had some good comments in #1035 that were removed before the PR merged.

sls-packaging/gradle-sls-packaging/src/main/groovy/com/palantir/gradle/dist/service/DiagnosticsManifestPlugin.java

Lines 95 to 96 in 614f8a2

	// It's not _strictly_ necessary to define the mapping for the 'org.gradle.libraryelements' attribute,
	// but it looks a bit weird for something to still be marked as a 'jar' when it's clearly not.

Adding this back so future devs understand why we do this.

[pkoenig10]

My initial approach did not work because we were registering multiple artifact transforms. Both the resources directory and the classes directory transform would produce a dependency with the variant:

artifactType=diagnostics,org.gradle.libraryelements=diagnostics

During resolution, Gradle must select one. There are some docs that vaguely describe how this works. In my testing, the resources directory was chosen fairly consistently over the classes directory.

https://docs.gradle.org/7.6/userguide/artifact_transforms.html#artifact_transform_selection_and_execution

The only way to make this work would be to define separate attribute values, so the variants are different. For example, a dependency would produce both:

// These are from the resources directory
artifactType=diagnostics,org.gradle.libraryelements=resource-diagnostics

// These are from the class directory
artifactType=diagnostics,org.gradle.libraryelements=class-diagnostics

Interestingly, because a local dependency also produces a JAR, I suspect the JAR transform also conflicts with the resources directory transform.

This solution would also require us to define a separate artifact view for each variant and pass those all into the classpath for the mergeDiagnosticsTask, which is a bit gross.

Looking back at code comments from #1035, it looks like this was done purely as an optimization to avoid needing to create a JAR to extract the diagnostic manifest.

sls-packaging/gradle-sls-packaging/src/main/groovy/com/palantir/gradle/dist/service/DiagnosticsManifestPlugin.java

Lines 107 to 109 in 614f8a2

	// (2) this 'java-resources -> extracted file' thing is just a 'shortcut' so that gradle can complete this
	// task without needing to compile the java source files from any local projects. If we didn't define this,
	// then gradle would turn src dirs into a compiled jar, then run that through the transform (1) above.

It feels reasonable to just eliminate this optimization, with the following justification:

• It is undesirable to have conflicting artifact transforms
• Removing it makes the plugin logic simpler and more consistent between libraries and local projects
• If we want to use the output from an annotation processor, we're going to have to perform compilation
• Most of the builds that run createManifest are probably also going to perform compilation

[carterkozak]

I agree, this is entirely reasonable. We have several other components which tie into distribution/manifest creation (e.g. minimum dependency calculation based on referenced rpc methods) which also require compilation.

[CRogers]

I'd need to check, but my one fear with making createManifest depend on compilation is that dev-env might delay the resolveSlsConfigurations task, which will slow down builds by minutes as we can't pull containers early (requires resolveSlsConfigurations to be run). It also means resolveSlsConfigurations and it successors (gofigure etc) cannot be run in parallel with compilation and must come after it.

[CRogers]

We have several other components which tie into distribution/manifest creation (e.g. minimum dependency calculation based on referenced rpc methods) which also require compilation.

I'm pretty all the scary code involving artifacts transforms inside this repo was to avoid that.

[CRogers]

I confirmed by looking at a build scan that createManifest does not currently depend on compilation, and this would cause some slowdown in builds.

[pkoenig10]

I don't see how we can avoid compilation if we want to support the annotation processor.

I don't completely understand how we have this guarantee today. It's not obvious to me what would make Gradle reliably choose the resources directory artifact transform over the JAR artifact transform - I assume we're just getting lucky with a Gradle implementation detail.

Just looking at this code and the Gradle docs, I would have thought that we'd fall into the "selection fails and an error is reported" case. But that's obviously not the case.

Of all the found transform chains, Gradle tries to select the best one:

• If there is only one transform chain, it is selected.
• If there are two transform chains, and one is a suffix of the other one, it is selected.
• If there is a shortest transform chain, then it is selected.
• In all other cases, the selection fails and an error is reported.

[CRogers]

Talking @carterkozak, we're rolling out the conjure per-endpoint minimum version plugin atm, which will force compilation to occur anyway. This will slow down builds, but I think I can make another task in sls-packaging to spit out the pdeps to file, then have dev-env read that rather than createManifest. This way we can make builds fast again. So I think this is fine.

[pkoenig10]

Note that you cannot have a diagnostic manifest in both the resources directory and class directory.

Both files will have the same path in the generated JAR, so only one must be chosen. In my testing, the resource manifest would override the generated one - I assume the resource manifest wins because resources are copied into the classes directory after compilation.

While this may seem undesirable, there's not really an alternative and it makes the behavior more consistent with the behavior when consuming manifest from libraries (ie. we just extract from the JAR).

[svc-autorelease]

Released 7.31.0