Explicit list of packaged libraries

[jglick]

Currently this plugin bundles all compile-scoped dependencies whose trail does not include a plugin in WEB-INF/lib/*.jar. #140 / #172 / #192 shows that there are subtleties here. #130 at least makes it clear what is being bundled and (usually) why to a developer paying attention to the build log, but of course most of the time no one is looking.

As suggested in jenkinsci/blueocean-plugin#2433 (comment) and jenkinsci/plugin-pom#705 (comment), it would be more reliable to simply require that the plugin pom explicitly list the JARs it expects to bundle so there needs to be a conscious decision to start bundling something and this decision is apparent to reviewers.

Some more examples of why this would be safer:

• Revert "Update hamcrest" google-compute-engine-plugin#398 / Make awaitility and hamcrest dependencies compile-scope so they are bundled in the plugin for use by gcp-client google-compute-engine-plugin#402
• [JENKINS-62767] Do not include JGit in hpi file workflow-remote-loader-plugin#18
• https://groups.google.com/g/jenkinsci-dev/c/POxcfeUTRzo/m/bRiyei3ZAgAJ
• Ban Jenkins test harness in compile scope plugin-pom#864
• published incremental poms have dependencyManagement stripped causing incorrect dependency resolution plugin-pom#705 (comment)

Suggested implementation:

• Add a new mojo parameter for a sorted list of artifactIds corresponding to dependency JARs which ought to be bundled. (version is obvious from dependency:tree and other things; groupId could be included for clarity, though the default basename in the WAR just uses artifactId.)
• If the actual list as computed by the current algorithm differs from the declared list, fail the build, printing the computed list. Otherwise proceed as now (including logging from [JENKINS-53957] Note in the build log when JARs are being bundled, and why #130, perhaps toned down).
• Define a POM property for the list in plugin-pom (probably meaning the mojo parameter must be String-valued, unless Maven has some clever way to bind a variable expression to a List<String>). The default value should be empty. Make sure that the mojo failure message shows you the exact line you need to add to <properties>.
• Prominently announce this as a breaking change for the corresponding POM release.
• File PRs for at least the most commonly maintained plugins in @jenkinsci adding the property with the current computed value. Harmless against an older POM, but makes sure that the Dependabot POM bump will pass uneventfully.

[basil]

I feel neutral about this change overall because while it does avoid some pathological cases, it also adds a bit of unnecessary extra work for normal cases. However, let me state from the outset that I will not support any breaking change that does not include a clear migration plan for, say, the top 200 plugins or similar. The work can possibly be shared, especially if others in the community are interested in helping, but I feel strongly that the work should not be deferred to "someone, sometime," because that is likely to create friction that will slow down unrelated future efforts. As far as my own personal interest in helping with the migration is concerned, I will participate to the degree that others participate. So, for example, if others migrate 5 plugins then I will also migrate 5 plugins, and if others migrate 0 plugins then I will also migrate 0 plugins.

[jglick]

a bit of unnecessary extra work

Yes of course, but small I think (one line of XML which you would be prompted to add to the POM); and the alternative is not just the risk of a regression (occasionally causing linkage errors, size bloat, licensing issues, security scanner flags, etc.) but actually more work to manually validate whether changes like jenkinsci/aws-java-sdk-plugin#1159 or jenkinsci/pipeline-model-definition-plugin#684 are actually doing what they claim.

Upon reflection I guess it should be possible to do this compatibly (set the default value to some “undeclared” token) so that we can start by having selected plugins opt into declaring their bundled libraries, and get some practical validation of the approach, before ultimately making it mandatory.

[daniel-beck]

File PRs for at least the most commonly maintained plugins in https://github.com/jenkinsci adding the property with the current computed value.

Will there be a goal to compute the list even for a plugin with older parent POM hpi dependency, so it's a basic mvn call to obtain the list for any (reasonably maintained) plugin?

[jglick]

Seems easy enough to just update the parent (or run with -Dhpi-plugin.version=…), but TBD.

[basil]

Seems easy enough to just update the parent

Have you ever tried to do this for the top 200 plugins? I would love to know, when you finish, how easy it was.

[jtnord]

I would generally be in favour of an approach that would fail a build. I have been hit once too often by dependabot bumps.
Bonus points if we can get it to find libraries that should be plugins too (with an opt out/in).