Avoid failures on removal of published artifacts

[ljacomet]

Summary

In the Gradle sigstore plugin:

• Makes handling an artifact removal from the publication conditional to the plugin having derived a signature from it.

Unfortunately, testing this is quite messy as it would require a dual signing setup: sigstore + pgp. If required, I can add such a test. Ideally in a follow up PR as getting a version of the plugin released with this fix would be awesome. Conference driven development 😉

Result with the 0.4.0 sigstore version can be seen in this GH action run.

I have tested locally that the fix here allows to skip GPG signing of *.sigstore artifacts

Release Note

• Fixes Gradle plugin: Logic to unregister derived artifacts from a publication is too eager #414

Documentation

NONE

[vlsi]

That would break multi-module concurrent execution. There's cleanLocalRepository to wipe the repo, and every module would run it at random, so you'll end up with half-empty local-maven-repo.

[ljacomet]

Oh, you are saying that folks run ./gradlew cleanLocalRepository publishAllPublicationsToTmp-mavenRepository in a single invocation?

I did not think of that indeed.

Let me see about figuring it out. Because the current setup really is messy for testing changes that require new versions of the whole stack.

[vlsi]

Let me see about figuring it out. Because the current setup really is messy for testing changes that require new versions of the whole stack.

Could you clarify "messy"?
running with composite builds or publishToMavenLocal should work if you mean testing the changes for external project.

I would say the current setup is just fine for testing changes.
The only required bit is to add a PGP key and pass it to the existing test SigstorePublishSignTest

[ljacomet]

Ok, I missed that the publication depends on the clean up task always.

Accidentally, the problem you describe cannot happen right now: Gradle marks all subsequent runs of the clean task as up-to-date. I believe this is because for Gradle the fingerprint of those tasks is a single value. But that is brittle anyway.

Looking for a better solution.

[ljacomet]

publishToMavenLocal and unicorns die 😉

I really want Gradle to get rid of that option and instead recommend usage of local repositories. That's my attempt in tweaking this.

But I see that you have a different perspective. I'll back out this change for now as it is unrelated.

[ljacomet]

(thank you GH for not showing me the update when I replied on my before last message)

[vlsi]

I really want Gradle to get rid of that option and instead recommend usage of local repositories

I do not like publishToMavenLocal either, however, I think it would be nice if Gradle provided something like publishToRootProjectRepo.

[vlsi]

Suggested change

	// Ignore artifacts that we have not added a signature for
	artifacts.remove(publishableArtifact)?.also {
	signTask.configure {
	signatures.findByName(publishableArtifact.file.name)
	?.takeIf { publishableArtifact in it.builtBy }
	?.let {
	signatures.remove(it)
	return@configure
	}
	// Slow path just in case
	signatures.removeIf { publishableArtifact in it.builtBy }
	}
	publication.removeDerivedArtifact(it)
	val artifact = artifacts.remove(publishableArtifact) ?: return@whenPublishableArtifactRemoved
	publication.removeDerivedArtifact(artifact)
	signTask.configure {
	signatures.findByName(file.name)
	?.takeIf { publishableArtifact in it.builtBy }
	?.let {
	signatures.remove(it)
	return@configure
	}
	// Slow path just in case
	signatures.removeIf { publishableArtifact in it.builtBy }
	}

[ljacomet]

I am not a proficient Kotlin user. But from a personal perspective, I find following the flow with different return@ a bit harder.

Do you have a strong preference for your version @vlsi ?

[vlsi]

Early exit allows reducing the nesting of the blocks, and it enables easier reasoning.

In this case, we check the artifact, and skip everything if the artifact is unknown. The code like if (..) { if (...) { ...} } else {... } is hard to read.

[ljacomet]

I had an if based version at first and I agree that the less likable of all. The also method feels more natural to me. But that's a style thing effectively.
Will move to your proposal.

[vlsi]

Testing would indeed require adding PGP key and signing with that (see

sigstore-java/sigstore-gradle/sigstore-gradle-sign-plugin/src/test/kotlin/dev/sigstore/gradle/SigstorePublishSignTest.kt

Line 36 in e1b8824

id("maven-publish")

), so I think it might be ok if we keep manual tests only.

I wonder if we should exclude pgp-signing .sigstore files by default though (e.g. flagged with Gradle property for cases when users' repository requires .asc for everything), so the users apply sigstore plugin and deploy without signatures.

[ljacomet]

@vlsi I have updated the PR to focus on the issue only and applied your suggestion.

Would it be an option to release a 0.4.1 of the plugin once this is merged?

[vlsi]

@ljacomet , I've pushed the plugin as 0.4.1: https://plugins.gradle.org/plugin/dev.sigstore.sign