Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
98 changes: 84 additions & 14 deletions pct.sh
Original file line number Diff line number Diff line change
Expand Up @@ -18,23 +18,99 @@ if [[ -n ${EXTRA_MAVEN_PROPERTIES-} ]]; then
fi

#
# Plugin Compatibility Tester (PCT) requires custom --add-opens directives when running on Java 17.
# As a temporary workaround, we pass in these directives when invoking PCT. When
# jenkinsci/plugin-compat-tester#352 is merged and released, and when this repository has upgraded
# to that release, this workaround can be deleted.
# Grab the Jenkins version from the WAR file so that we can pass it in via jenkins.version. This is
# needed because HPI Plugin requires the version of the WAR passed in via overrideWar to be
# identical to jenkins.version. If we do not explicitly pass in jenkins.version, then the
# jenkins.version defined in the plugin's pom.xml file will be used, which may not match the version
# of the WAR under test.
#
mkdir pct-work
pushd pct-work
jar xf ../megawar.war META-INF/MANIFEST.MF
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"
Copy link
Member Author

@basil basil Jul 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


#
# The overrideWar option is available in HPI Plugin 3.29 or later, but many plugins under test
# still use an older plugin parent POM and therefore an older HPI plugin version. As a temporary
# workaround, we override the HPI plugin version to the latest version.
#
# 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.
Comment on lines +40 to +41
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

#
MAVEN_PROPERTIES+=:hpi-plugin.version=3.30
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or we could just update to the newer version =)


#
# Define the excludes for upper bounds checking. We define these excludes in a separate file and
# pass it in via -mavenPropertiesFile rather than using -mavenProperties because -mavenProperties
# uses a colon as the separator and these values contain colons.
#
if [[ $LINE == '2.319.x' ]]; then
#
# com.sun.mail needs to be excluded because it is still provided by core on this line (using
# pre-Jakarta imports) and we do not want it to be upgraded to a version that uses Jakarta
# imports (which is not a realistic test scenario) just because the Jakarta Mail API plugin
# happens to be on the class path and triggers an upper bounds violation.
#
# javax.servlet:servlet-api comes from core at version 0, which is an intentional trick to
# prevent this library from being used, and we do not want it to be upgraded to a nonzero
# version (which is not a realistic test scenario) just because it happens to be on the
# class path of some plugin and triggers an upper bounds violation. JENKINS-68696 tracks the
# removal of this trick.
#
echo upperBoundsExcludes=com.sun.mail:jakarta.mail,javax.servlet:servlet-api >"$(pwd)/maven.properties"
else
#
# javax.servlet:servlet-api comes from core at version 0, which is an intentional trick to
# prevent this library from being used, and we do not want it to be upgraded to a nonzero
# version (which is not a realistic test scenario) just because it happens to be on the
# class path of some plugin and triggers an upper bounds violation. JENKINS-68696 tracks the
# removal of this trick.
#
echo upperBoundsExcludes=javax.servlet:servlet-api >"$(pwd)/maven.properties"
fi

#
# Apache HttpComponents Client 4.x API, Oracle Java SE Development Kit Installer, JSch, OkHttp, and
# Plain Credentials use an older plugin parent POM and therefore an older test harness that predates
# compatibility with the removal of JNR in recent cores in jenkinsci/jenkins-test-harness#350. As a
# temporary workaround, we override the test harness to a recent version. Note that we cannot use a
# test harness newer than 1812.v6d4e97d91fd8, because later releases of the test harness require
# changes to the plugin parent POM for JUnit 5 support.
#
# 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.
Comment on lines +77 to +78
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 🤣

#
if [[ $PLUGINS =~ apache-httpcomponents-client-4-api ]] ||
[[ $PLUGINS =~ jdk-tool ]] ||
[[ $PLUGINS =~ jsch ]] ||
[[ $PLUGINS =~ okhttp-api ]] ||
[[ $PLUGINS =~ plain-credentials ]]; then
MAVEN_PROPERTIES+=:jenkins-test-harness.version=1812.v6d4e97d91fd8
fi

#
# 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

# still use an older plugin parent POM. As a temporary workaround, we skip Enforcer.
#
# TODO When all plugins in the managed set are using plugin parent POM 4.42 or later, this can be
# deleted.
Comment on lines +93 to +94
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

#
MAVEN_PROPERTIES+=:enforcer.skip=true

java \
--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 \
Comment on lines -27 to -30
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

-jar pct.jar \
-war "$(pwd)/megawar.war" \
-includePlugins "${PLUGINS}" \
-workDirectory "$(pwd)/pct-work" \
-reportFile "$(pwd)/pct-report.xml" \
$PCT_S_ARG \
-mavenProperties "${MAVEN_PROPERTIES}" \
-excludeHooks org.jenkins.tools.test.hook.TransformPom \
-mavenPropertiesFile "$(pwd)/maven.properties" \
-skipTestCache true

if grep -q -F -e '<status>INTERNAL_ERROR</status>' pct-report.xml; then
Expand Down Expand Up @@ -70,10 +146,4 @@ elif grep -q -F -e '<status>TEST_FAILURES</status>' pct-report.xml; then
done
fi

# 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
Comment on lines -73 to -77
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


# produces: **/target/surefire-reports/TEST-*.xml
4 changes: 2 additions & 2 deletions prep.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(FTR, reverts a workaround added in #1160)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a good sign!

rm -rfv megawar-$LINE/META-INF/*.{RSA,SF}
mkdir "megawar-${LINE}/WEB-INF/plugins"
cp -rv test-classes/test-dependencies/*.hpi "megawar-${LINE}/WEB-INF/plugins"
cd "megawar-${LINE}"
Expand All @@ -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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

pct=$HOME/.m2/repository/org/jenkins-ci/tests/plugins-compat-tester-cli/${version}/plugins-compat-tester-cli-${version}.jar
[ -f "${pct}" ] || $MVN dependency:get -Dartifact=org.jenkins-ci.tests:plugins-compat-tester-cli:${version}:jar -DremoteRepositories=https://repo.jenkins-ci.org/public/,https://repo.jenkins-ci.org/incrementals/ -Dtransitive=false
cp "${pct}" target/pct.jar
Expand Down