[JENKINS-45047] Migrate from PCT `org.jenkins.tools.test.hook.TransformPom` to HPI plugin `overrideWar` #1336

basil · 2022-07-26T23:00:01Z

Fixes #657 by moving us from an incremental build of PCT, which should never have been accepted, to a release build of PCT.

Fixes #821, a regression introduced by JENKINS-47498, by suppressing the org.jenkins.tools.test.hook.TransformPom hook from PCT in favor of the overrideWar option from jenkinsci/maven-hpi-plugin#336 to pin each plugin to the version in the fat WAR; i.e., the version of the plugin specified in the version of the BOM under test, as opposed to current behavior of the version of the plugin specified in the version of the BOM specified by the plugin under test. This should eliminate the need to run around the Jenkins ecosystem bumping BOM versions and doing plugin releases every time an incompatible change is made. It should also help us catch regressions sooner. Actually, it already has, as while preparing this PR I found several such regressions (both the first time around in March, and recently while preparing the present PR).

Does not fix #895, but brings us closer to a solution. Not only is the release build of PCT is more strict than the incremental build we were using before, but also using a release build of PCT should make it easier to test the likes of jenkinsci/plugin-compat-tester#348. Pending the completion of the latter it should be easier to reëvaluate /pct.sh#L40-L71 in favor of a more robust alternative.

See the self-review for more explanation.

Last but not least, some well-deserved acknowledgements:

@Dohbedoh for releasing recent backports to the Mina SSHD API plugins that were blocking the present effort.
@imonteroperez for merging and releasing Correctly split Maven properties with equals signs plugin-compat-tester#353 and Support for excluding hooks plugin-compat-tester#372, which were critical for the present effort.
@jetersen for his recent heroic effort to update the Mina SSHD API plugins, including releasing recent fixes to the Variant plugin and kindly incorporating a properties fix that was blocking the present effort.
@jglick for the design of JENKINS-47498, starting the implementation in [JENKINS-45047] Support for plugin-to-plugin integration tests maven-hpi-plugin#66, a recent bug fix to workflow-job that was blocking this effort, merging and releasing Support running with Java 17 plugin-compat-tester#352 and Teach PCT about Mina SSHD API plugins plugin-compat-tester#375, and much other help that currently escapes my memory.
@jtnord for the helpful feedback in [JENKINS-45047] Support for plugin-to-plugin integration tests maven-hpi-plugin#336.
@timja for releasing recent backports to the JUnit plugin that were blocking the present effort. I am truly grateful for your helpful involvement in nearly every project I work on.

I am sure I have forgotten others. I will edit this post if I remember. Thank you to all!

…rmPom` to HPI plugin `overrideWar`

basil · 2022-07-26T23:00:19Z

pct.sh

+JENKINS_VERSION=$(grep Jenkins-Version META-INF/MANIFEST.MF | awk '{print $2}')
+popd
+rm -rf pct-work
+MAVEN_PROPERTIES+=":jenkins.version=${JENKINS_VERSION}:overrideWar=$(pwd)/megawar.war:useUpperBounds=true"


The main substance of this change: adding overrideWar from JENKINS-45047, along with its prerequisite jenkins.version.

The use of its companion option useUpperBounds in this context does not usually affect plugin versions, as would often be the case in the original use case described in JENKINS-45047. This is because the entire tree of plugin versions is already specified in the BOM and therefore the megawar, so there should not be any upper bounds errors here in the common case. However, this option is effective at dealing with some mundane technical issues, like aligning the version of SpotBugs annotations used between core and some library used by a plugin. It might also be effective at helping deal with libraries that have been detached into plugins, although in at least the case of Jakarta Mail it was not effective for this purpose due to the Jakarta Mail maintainers using the same group ID and artifact ID for multiple releases of their library, one with javax packages and one with jakarta packages. If you observe the actual upper bounds errors fixed in a test run, they all represent TODOs that would be "nice to fix" in most cases (e.g. SpotBugs, Joda Time, etc) but not serious enough to warrant the effort in many cases, and it is better to deal with them automatically rather than to allow these mundane issues to block a test run.

basil · 2022-07-26T23:00:33Z

pct.sh

+# TODO When all plugins in the managed set are using a plugin parent POM with HPI Plugin 3.29 or
+# later (i.e., plugin parent POM 4.44 or later), this can be deleted.


Taking on a medium amount of technical debt here, which I felt was the more reasonable alternative to updating the plugin POM and spinning releases for 100 or so plugins, a massive exercise. I plan to keep this up-to-date as new versions of HPI plugin are released. Hopefully in a year or so the vast majority of plugins will have been updated over the course of routine work, at which point we can sweep through the stragglers and eliminate this technical debt.

basil · 2022-07-26T23:00:41Z

pct.sh

+# TODO When these plugins are using a plugin parent POM with test harness 1657.vf8a824e79147 or
+# later (i.e., plugin parent POM 4.32 or later), this can be deleted.


Taking on a small amount of technical debt here, which I felt was reasonable in the short term because these five plugins are currently unmaintained. I will try and nag the maintainers a little more to get these plugins updated before I give up and adopt them myself. Help would certainly be appreciated if anyone is interested. I feel that this technical debt could be reasonably dealt with in the next couple of months, which is why I am choosing to proceed with this PR regardless.

I guess you mean that this could become a problem when plugins start to rely on JTH features (rather more likely than maven-hpi-plugins which I mentioned elsewhere), so yeah we will need to deal with this in the short-to-medium term.

I did not see anything in https://github.com/jenkinsci/plain-credentials-plugin/pulls and it looks like it is not yet set up with Dependabot or even JEP-305, oops. Will make a note to refresh it soon. I probably have rights to some of the others too, do not recall.

I did not see anything in https://github.com/jenkinsci/plain-credentials-plugin/pulls

Did you expect to? If you did, should I have expected to see a PR from you in https://github.com/jenkinsci/git-server-plugin/pulls after detaching instance-identity? 😀 Come on Jesse, at least I am being upfront about taking a shortcut here. 🤣

basil · 2022-07-26T23:00:49Z

pct.sh

+# TODO When all plugins in the managed set are using plugin parent POM 4.42 or later, this can be
+# deleted.


org.jenkins.tools.test.hook.TransformPom already skipped Enforcer so I should get a pass here, as this is not introducing new technical debt but just moving the existing technical debt to a new location. However I should note that it should be easy to address this once more plugins are moved to a recent plugin POM, which we already hope to do over the next year or so to address the TODO relating to HPI plugin. So hopefully we should be able to kill two birds with one stone and clean this up at the same time.

I think it is pretty reasonable to permanently suppress Enforcer in the PCT context.

basil · 2022-07-26T23:01:01Z

pct.sh

-	--add-opens java.base/java.lang.reflect=ALL-UNNAMED \
-	--add-opens java.base/java.text=ALL-UNNAMED \
-	--add-opens java.base/java.util=ALL-UNNAMED \
-	--add-opens java.desktop/java.awt.font=ALL-UNNAMED \


No longer needed as of jenkinsci/plugin-compat-tester#352.

basil · 2022-07-26T23:01:23Z

pct.sh

-# TODO various problems with PCT itself (e.g. https://github.com/jenkinsci/bom/pull/338#issuecomment-715256727)
-# and anyway the tests in PluginAutomaticTestBuilder are generally uninteresting in a PCT context
-# We always try to run this test rather than adding it to excludes.txt in order
-# to be able to detect if PCT failed to run tests at all a few lines above.
-rm -fv pct-work/*/{,*/}target/surefire-reports/TEST-InjectedTest.xml


No longer needed as of jenkinsci/maven-hpi-plugin#336.

Wow, OK. We should be comfortable readding this exclusion at the first sign of trouble, though—this test suite really does not add value in the PCT context.

basil · 2022-07-26T23:01:32Z

prep.sh

@@ -32,7 +32,7 @@ for LINE in $LINEZ; do
 	echo '# nothing' >jenkins/split-plugins.txt
 	cp -r jenkins-for-test "megawar-${LINE}"
 	jar uvf megawar-$LINE/WEB-INF/lib/jenkins-core-*.jar jenkins/split-plugins.txt
-	rm -rfv `# TODO delete all but instance-identity? megawar-$LINE/WEB-INF/detached-plugins` megawar-$LINE/META-INF/*.{RSA,SF}


No longer needed as of jenkinsci/maven-hpi-plugin#336.

Hopefully? I failed to code/test this properly the first time around, fixed in ace1efa but not yet tested. If the tests fail I will just revert this hunk.

Worked like a champ 💪

(FTR, reverts a workaround added in #1160)

That is a good sign!

basil · 2022-07-26T23:01:52Z

prep.sh

@@ -41,7 +41,7 @@ for LINE in $LINEZ; do
 done

 # TODO find a way to encode this in some POM so that it can be managed by Dependabot
-version=1152.vafc19b26d5aa # TODO https://github.com/jenkinsci/plugin-compat-tester/pull/345
+version=1178.vbef3c43d0e69


One of the most significant lines of this patch: the use of a release build of PCT.

Do you happen to know what obviated the need for jenkinsci/plugin-compat-tester@ef1d7a2? useUpperBounds? Would be happy to close jenkinsci/plugin-compat-tester#345.

I have no idea and I am scared to find out.

jetersen

LGTM

As long as we get #821 fixed I am happy 😅

Thanks for your effort @basil much appreciated!

timja

great work 👍

basil · 2022-07-27T15:04:55Z

Any objections @jglick? I plan to merge this later today if everyone is OK with it.

jglick

Excellent! Left some minor comments and suggestions but probably best to just get this merged sooner rather later.

pct.sh

jglick · 2022-07-27T17:38:25Z

pct.sh

+# TODO When all plugins in the managed set are using a plugin parent POM with HPI Plugin 3.29 or
+# later (i.e., plugin parent POM 4.44 or later), this can be deleted.
+#
+MAVEN_PROPERTIES+=:hpi-plugin.version=3.30


At some point it may happen that we cut a maven-hpi-plugin release with some new feature that some managed plugins begin relying upon (in a way that matters for test runs). Does not seem especially likely, but if that happens we would need to amend this to

mvn -Dexpression=hpi-plugin.version -q -DforceStdout help:evaluate

and pick whichever is newer.

Or we could just update to the newer version =)

pct.sh

jglick · 2022-07-27T17:47:07Z

pct.sh

+
+#
+# Testing plugins against a version of Jenkins that requires Java 11 exposes
+# jenkinsci/plugin-pom#563. This was fixed in plugin parent POM 4.42, but many plugins under test


jenkinsci/plugin-pom#563 👀

jglick · 2022-07-27T17:48:01Z

pct.sh

+# TODO When all plugins in the managed set are using plugin parent POM 4.42 or later, this can be
+# deleted.


I think it is pretty reasonable to permanently suppress Enforcer in the PCT context.

jglick · 2022-07-27T17:49:20Z

pct.sh

-# TODO various problems with PCT itself (e.g. https://github.com/jenkinsci/bom/pull/338#issuecomment-715256727)
-# and anyway the tests in PluginAutomaticTestBuilder are generally uninteresting in a PCT context
-# We always try to run this test rather than adding it to excludes.txt in order
-# to be able to detect if PCT failed to run tests at all a few lines above.
-rm -fv pct-work/*/{,*/}target/surefire-reports/TEST-InjectedTest.xml


Wow, OK. We should be comfortable readding this exclusion at the first sign of trouble, though—this test suite really does not add value in the PCT context.

jglick · 2022-07-27T17:50:24Z

prep.sh

@@ -32,7 +32,7 @@ for LINE in $LINEZ; do
 	echo '# nothing' >jenkins/split-plugins.txt
 	cp -r jenkins-for-test "megawar-${LINE}"
 	jar uvf megawar-$LINE/WEB-INF/lib/jenkins-core-*.jar jenkins/split-plugins.txt
-	rm -rfv `# TODO delete all but instance-identity? megawar-$LINE/WEB-INF/detached-plugins` megawar-$LINE/META-INF/*.{RSA,SF}


That is a good sign!

jglick · 2022-07-27T17:52:51Z

prep.sh

@@ -41,7 +41,7 @@ for LINE in $LINEZ; do
 done

 # TODO find a way to encode this in some POM so that it can be managed by Dependabot
-version=1152.vafc19b26d5aa # TODO https://github.com/jenkinsci/plugin-compat-tester/pull/345
+version=1178.vbef3c43d0e69


Do you happen to know what obviated the need for jenkinsci/plugin-compat-tester@ef1d7a2? useUpperBounds? Would be happy to close jenkinsci/plugin-compat-tester#345.

Co-authored-by: Jesse Glick <jglick@cloudbees.com>

[JENKINS-45047] Migrate from PCT `org.jenkins.tools.test.hook.Transfo…

a593be2

…rmPom` to HPI plugin `overrideWar`

basil added the enhancement New feature or request label Jul 26, 2022

basil commented Jul 26, 2022

View reviewed changes

jetersen approved these changes Jul 26, 2022

View reviewed changes

Whoops!

ace1efa

timja approved these changes Jul 27, 2022

View reviewed changes

jetersen requested a review from jglick July 27, 2022 15:07

jglick approved these changes Jul 27, 2022

View reviewed changes

basil and others added 5 commits July 27, 2022 11:58

Merge remote-tracking branch 'origin/master' into JENKINS-45047

51c0a93

More correct parsing

2b59ab1

Update pct.sh

2fb55a2

Co-authored-by: Jesse Glick <jglick@cloudbees.com>

Update pct.sh

4f07b1a

Co-authored-by: Jesse Glick <jglick@cloudbees.com>

Update pct.sh

36a2507

Co-authored-by: Jesse Glick <jglick@cloudbees.com>

basil enabled auto-merge (squash) July 27, 2022 19:03

basil merged commit c795072 into jenkinsci:master Jul 27, 2022

basil mentioned this pull request Jul 28, 2022

Build passes even after PCT fails to run on a plugin #895

Closed

This was referenced Jul 28, 2022

Enable CD jenkinsci/plain-credentials-plugin#31

Merged

Enable CD jenkinsci/jdk-tool-plugin#41

Merged

jglick mentioned this pull request Nov 11, 2022

Bump jenkins.version from 2.371 to 2.377 for bom-weekly #1479

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-45047] Migrate from PCT `org.jenkins.tools.test.hook.TransformPom` to HPI plugin `overrideWar` #1336

[JENKINS-45047] Migrate from PCT `org.jenkins.tools.test.hook.TransformPom` to HPI plugin `overrideWar` #1336

basil commented Jul 26, 2022 •

edited by jetersen

basil Jul 26, 2022 •

edited

basil Jul 26, 2022

basil Jul 26, 2022

jglick Jul 27, 2022

basil Jul 27, 2022

basil Jul 26, 2022

jglick Jul 27, 2022

basil Jul 26, 2022

basil Jul 26, 2022

jglick Jul 27, 2022

basil Jul 26, 2022

basil Jul 26, 2022

basil Jul 27, 2022

basil Jul 27, 2022

jglick Jul 27, 2022

basil Jul 26, 2022

jglick Jul 27, 2022

basil Jul 27, 2022

jetersen left a comment •

edited

timja left a comment

basil commented Jul 27, 2022

jglick left a comment

jglick Jul 27, 2022

basil Jul 27, 2022

jglick Jul 27, 2022

jglick Jul 27, 2022

jglick Jul 27, 2022

jglick Jul 27, 2022

jglick Jul 27, 2022

		# TODO When all plugins in the managed set are using a plugin parent POM with HPI Plugin 3.29 or
		# later (i.e., plugin parent POM 4.44 or later), this can be deleted.

		# TODO When these plugins are using a plugin parent POM with test harness 1657.vf8a824e79147 or
		# later (i.e., plugin parent POM 4.32 or later), this can be deleted.

		# TODO When all plugins in the managed set are using plugin parent POM 4.42 or later, this can be
		# deleted.

[JENKINS-45047] Migrate from PCT org.jenkins.tools.test.hook.TransformPom to HPI plugin overrideWar #1336

[JENKINS-45047] Migrate from PCT org.jenkins.tools.test.hook.TransformPom to HPI plugin overrideWar #1336

Conversation

basil commented Jul 26, 2022 • edited by jetersen

basil Jul 26, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jetersen left a comment • edited

Choose a reason for hiding this comment

timja left a comment

Choose a reason for hiding this comment

basil commented Jul 27, 2022

jglick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[JENKINS-45047] Migrate from PCT `org.jenkins.tools.test.hook.TransformPom` to HPI plugin `overrideWar` #1336

[JENKINS-45047] Migrate from PCT `org.jenkins.tools.test.hook.TransformPom` to HPI plugin `overrideWar` #1336

basil commented Jul 26, 2022 •

edited by jetersen

basil Jul 26, 2022 •

edited

jetersen left a comment •

edited