[JENKINS-65757] only use pluginFirst classloader when you have no other option (and never for guice!) #151

jtnord · 2021-06-07T20:43:57Z

JENKINS-65757 The dependencies was a mess due to pulling libraries in that where comletely un-needed.

Even plexus could probably be removed with a bit more effort - but at least plexus is safe.

Maven etc was pulling in guva and a misguided attempt to bundle it along with the plugin first classloaded caused all hell to break loose as guice saw things loaded from separate classloaders and it was not happy

Added a proper realistic E2E test (there where not even any semi realistic ones) using RealJenkinsRule to ensure this still works with a FreeStyleProject creating jacoco coverage via maven (a tag of this project)

Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
Ensure that the pull request title represents the desired changelog entry
Please describe what you did
Link to relevant issues in GitHub or Jira
Link to relevant pull requests, esp. upstream and downstream changes
Ensure you have provided tests - that demonstrates feature works or fixes the issue

FileFilter just joined the passed in excludes as a comma separated list and added defaults if the list was empty or null, But the code already handles this so just call the Plex util directly

jtnord · 2021-06-07T20:45:30Z

pom.xml

@@ -530,7 +361,6 @@ THE SOFTWARE.
                    </execution>
                </executions>
                <configuration>
-                    <pluginFirstClassLoader>true</pluginFirstClassLoader>


we do not need this, we are safe with maskClasses now after removing other conflicting libraries

jtnord · 2021-06-07T20:45:56Z

src/main/java/hudson/plugins/jacoco/ExecutionFileLoader.java

@@ -19,7 +19,6 @@
 import org.jacoco.core.data.ExecutionDataReader;
 import org.jacoco.core.data.ExecutionDataStore;
 import org.jacoco.core.data.SessionInfoStore;
-import org.jacoco.maven.FileFilter;


this caused a whole slew of the maven dependencies and for no real reason.

jtnord · 2021-06-07T20:46:39Z

src/main/java/hudson/plugins/jacoco/ExecutionFileLoader.java

@@ -122,9 +121,8 @@ private IBundleCoverage analyzeStructure() throws IOException {
 				excludes = ITEM_ZERO;


so this is the default exclude - but I have no idea why. the default in maven in the empty String, but here we use "{0}" 🤷

jtnord · 2021-06-07T20:47:20Z

src/test/java/hudson/plugins/jacoco/e2e/E2ETest.java

+public class E2ETest {
+
+    @Rule
+    public RealJenkinsRule rjr = new RealJenkinsRule();


https://javadoc.jenkins.io/component/jenkins-test-harness/org/jvnet/hudson/test/RealJenkinsRule.html

jtnord · 2021-06-07T20:49:03Z

src/test/java/hudson/plugins/jacoco/e2e/E2ETest.java

+    }
+
+    private static List<Builder> createJacocoProjectBuilders() {
+        String[] commands = { "git clone --branch jacoco-3.2.0 https://github.com/jenkinsci/jacoco-plugin.git .",


whilst running maven inside a test is not the best, we are using this project as the test (and a tag) so we at least have most of the dependencies already local.

It will diverge over time, so we may want to try and keep this somewhat in sync

So binaries "git" and "mvn" need to be available on the CI node for this to work, right?

yes, but maven already is as it is what runs the build that runs this test :)

as for git the CI could have cloned using jgit rather than the git CLI so it may not be available, but ci.jenkins.io is not setup like that.
A developer could work exclusively with IntelliJ or Eclipse (again jgit) but I am not convinced that is likely.

For a more reproducible environment:

store a small test project in src/test/resources/sampleproj/

use dependency:copy to unpack a specific release of Maven into target/test-classes/

but still use the Java in path I guess.

JDK11 and JDK8 compilers generate different bytecode and so asserting the bytecode instruction coverage would need to be different on potentially different compilers (e.g eclipse vs jdk 8 vs jdk11 vs ...)

the shade plugin was bound to a phase that did not exist in the hpi packaging type. even if it did it would be completely unclear what it did as the shade plugin does not play nicely with hpis.

MarkEWaite · 2021-06-07T22:27:33Z

I loaded the PR build https://ci.jenkins.io/job/Plugins/job/jacoco-plugin/job/PR-151/4/ into my Jenkins instance and confirmed that it reports JaCoCo coverage as expected for the platformlabeler plugin test job that uses it. Thanks so much for your work on this @jtnord !

centic9 · 2021-06-08T09:46:26Z

Thanks for the suggested simplification, would be nice to be able to merge this. Some of these dependencies were necessary to make "validate" succeed and not complain about mismatched versions, did you try running that maven target as well?

e.g. the following needs to succeed to be able to release:
mvn clean package && mvn validate

Also some tests seem to be failing in CI, is this caused by some of the changes?

jtnord · 2021-06-08T11:38:16Z

validate is one of the first phases and is run before the test phase, so this is covered by existing Jenkins jobs and yes there are no upperBounds errors.

jtnord · 2021-06-08T11:41:32Z

Also some tests seem to be failing in CI, is this caused by some of the changes?

@centic9 any link? - all the statuses are green after 40f5613 and the failures I saw on ci.jenkins.io (which caused me to retrigger the build) where due to agent infrastructure issues and not test failures.
Please let me know if I missed something.

centic9

Ok, thanks for confirming, I was looking at CI-results of 91837c8 which showed some failing tests, it seems some newer build now succeeds fully again.

centic9 · 2021-06-08T12:40:36Z

src/test/java/hudson/plugins/jacoco/e2e/E2ETest.java

+    }
+
+    private static List<Builder> createJacocoProjectBuilders() {
+        String[] commands = { "git clone --branch jacoco-3.2.0 https://github.com/jenkinsci/jacoco-plugin.git .",


So binaries "git" and "mvn" need to be available on the CI node for this to work, right?

MarkEWaite · 2021-06-08T14:23:33Z

So binaries "git" and "mvn" need to be available on the CI node for this to work, right?

@centic9 yes, the ci.jenkins.io agents used for buildPlugin must include mvn and git. The git plugin, the git-client plugin, and many others rely on that assumption. It is safe to rely on that in the jacoco plugin as well.

jglick · 2021-06-08T15:22:39Z

pom.xml

-        <maven.version>3.8.1</maven.version>
-        <powermock.version>2.0.7</powermock.version>
+        <!--  until the plugin-pom picks up a newer version -->
+        <jenkins-test-harness.version>1589.vc23fca066d5c</jenkins-test-harness.version>


https://github.com/jenkinsci/jenkins-test-harness/releases/tag/1589.vc23fca066d5c OK

jglick · 2021-06-08T15:26:07Z

src/test/java/hudson/plugins/jacoco/e2e/E2ETest.java

+    }
+
+    private static List<Builder> createJacocoProjectBuilders() {
+        String[] commands = { "git clone --branch jacoco-3.2.0 https://github.com/jenkinsci/jacoco-plugin.git .",


For a more reproducible environment:

store a small test project in src/test/resources/sampleproj/

use dependency:copy to unpack a specific release of Maven into target/test-classes/

but still use the Java in path I guess.

basil

Much nicer!

maven-hpi-plugin output before

[INFO] --- maven-hpi-plugin:3.17:hpi (default) @ jacoco ---
[INFO] Generating src/jenkinsci/jacoco-plugin/target/jacoco/META-INF/MANIFEST.MF
[INFO] Checking for attached .jar artifact ...
[INFO] Generating jar src/jenkinsci/jacoco-plugin/target/jacoco.jar
[INFO] Building jar: src/jenkinsci/jacoco-plugin/target/jacoco.jar
[INFO] Exploding webapp...
[INFO] Copy webapp webResources to src/jenkinsci/jacoco-plugin/target/jacoco
[INFO] Assembling webapp jacoco in src/jenkinsci/jacoco-plugin/target/jacoco
[WARNING] Bundling transitive dependency guice-4.2.1.jar (via jacoco-maven-plugin)
[WARNING] Bundling transitive dependency javax.inject-1.jar (via jacoco-maven-plugin)
[INFO] Bundling direct dependency httpcore-4.4.13.jar
[WARNING] Bundling transitive dependency maven-reporting-api-3.0.jar (via jacoco-maven-plugin)
[WARNING] Bundling transitive dependency maven-resolver-api-1.6.2.jar (via jacoco-maven-plugin)
[WARNING] Bundling transitive dependency maven-resolver-impl-1.6.2.jar (via jacoco-maven-plugin)
[WARNING] Bundling transitive dependency maven-resolver-spi-1.6.2.jar (via jacoco-maven-plugin)
[WARNING] Bundling transitive dependency maven-resolver-util-1.6.2.jar (via jacoco-maven-plugin)
[WARNING] Bundling transitive dependency file-management-1.2.1.jar (via jacoco-maven-plugin)
[WARNING] Bundling transitive dependency maven-shared-io-1.1.jar (via jacoco-maven-plugin)
[WARNING] Bundling transitive dependency maven-shared-utils-3.2.1.jar (via jacoco-maven-plugin)
[WARNING] Bundling transitive dependency wagon-provider-api-1.0-alpha-6.jar (via jacoco-maven-plugin)
[WARNING] Bundling transitive dependency maven-artifact-manager-2.0.2.jar (via jacoco-maven-plugin)
[WARNING] Bundling transitive dependency maven-builder-support-3.8.1.jar (via jacoco-maven-plugin)
[WARNING] Bundling transitive dependency maven-core-3.8.1.jar (via jacoco-maven-plugin)
[WARNING] Bundling transitive dependency maven-model-builder-3.8.1.jar (via jacoco-maven-plugin)
[WARNING] Bundling transitive dependency maven-repository-metadata-3.8.1.jar (via jacoco-maven-plugin)
[WARNING] Bundling transitive dependency maven-resolver-provider-3.8.1.jar (via jacoco-maven-plugin)
[WARNING] Bundling transitive dependency maven-settings-builder-3.8.1.jar (via jacoco-maven-plugin)
[INFO] Bundling direct dependency plexus-utils-3.2.1.jar
[WARNING] Bundling transitive dependency org.eclipse.sisu.inject-0.3.4.jar (via jacoco-maven-plugin)
[INFO] Bundling direct dependency jacoco-maven-plugin-0.8.7.jar
[WARNING] Bundling transitive dependency org.jacoco.agent-0.8.7.jar (via jacoco-maven-plugin)
[WARNING] Bundling transitive dependency org.jacoco.core-0.8.7.jar (via org.jacoco.report)
[INFO] Bundling direct dependency org.jacoco.report-0.8.7.jar
[WARNING] Bundling transitive dependency asm-analysis-9.1.jar (via org.jacoco.report)
[WARNING] Bundling transitive dependency asm-commons-9.1.jar (via org.jacoco.report)
[WARNING] Bundling transitive dependency asm-tree-9.1.jar (via org.jacoco.report)
[WARNING] Bundling transitive dependency asm-9.1.jar (via org.jacoco.report)
[INFO] Bundling direct dependency xml-apis-1.4.01.jar
[INFO] Generating hpi src/jenkinsci/jacoco-plugin/target/jacoco.hpi
[INFO] Building jar: src/jenkinsci/jacoco-plugin/target/jacoco.hpi

maven-hpi-plugin output after

[INFO] --- maven-hpi-plugin:3.17:hpi (default-hpi) @ jacoco ---
[INFO] Generating src/jenkinsci/jacoco-plugin/target/jacoco/META-INF/MANIFEST.MF
[INFO] Checking for attached .jar artifact ...
[INFO] Found attached .jar artifact: src/jenkinsci/jacoco-plugin/target/jacoco.jar
[INFO] Exploding webapp...
[INFO] Copy webapp webResources to src/jenkinsci/jacoco-plugin/target/jacoco
[INFO] Assembling webapp jacoco in src/jenkinsci/jacoco-plugin/target/jacoco
[INFO] Bundling direct dependency plexus-utils-3.2.1.jar
[WARNING] Bundling transitive dependency org.jacoco.core-0.8.7.jar (via org.jacoco.report)
[INFO] Bundling direct dependency org.jacoco.report-0.8.7.jar
[WARNING] Bundling transitive dependency asm-analysis-9.1.jar (via org.jacoco.report)
[WARNING] Bundling transitive dependency asm-commons-9.1.jar (via org.jacoco.report)
[WARNING] Bundling transitive dependency asm-tree-9.1.jar (via org.jacoco.report)
[WARNING] Bundling transitive dependency asm-9.1.jar (via org.jacoco.report)
[INFO] Generating hpi src/jenkinsci/jacoco-plugin/target/jacoco.hpi
[INFO] Building jar: src/jenkinsci/jacoco-plugin/target/jacoco.hpi

I noticed we are still bundling ASM 9:

[WARNING] Bundling transitive dependency asm-analysis-9.1.jar (via org.jacoco.report)
[WARNING] Bundling transitive dependency asm-commons-9.1.jar (via org.jacoco.report)
[WARNING] Bundling transitive dependency asm-tree-9.1.jar (via org.jacoco.report)
[WARNING] Bundling transitive dependency asm-9.1.jar (via org.jacoco.report)

This also seems to be the reason for maskClasses. This is unnecessary since 2.274, which bundles ASM 9 in the WAR via jnr-posix (formalized in the core BOM in jenkinsci/jenkins#5525). I suggest updating jenkins.version to 2.277.1 or later, then excluding the ASM dependencies as is done in token-macro. Then maskClasses wouldn't be needed at all and this plugin would be slimmer as well.

jtnord · 2021-06-09T08:45:25Z

@centic9 would you be able to merge and release this sometime this week please?

batmat · 2021-06-09T08:46:12Z

@basil to be clear, I suppose you mean we should do this in general, not necessarily in this PR right? Thanks
(I am strongly in general in favor to have separate PRs when possible, easier to review and land, etc. :-))

jtnord · 2021-06-09T09:31:16Z

This also seems to be the reason for maskClasses. This is unnecessary since 2.274, which bundles ASM 9 in the WAR via jnr-posix (formalized in the core BOM in jenkinsci/jenkins#5525). I suggest updating jenkins.version to 2.277.1 or later, then excluding the ASM dependencies as is done in token-macro. Then maskClasses wouldn't be needed at all and this plugin would be slimmer as well.

using an unshaded ASM is core is probably a mistake and it should not really happen. Jenkins core should not be pulling moar libraries into Jenkins core, and ASM specifically is known to not be compatible between versions, so this plugin could break at a moments notice if we updated jnr-posix to a version that say included ASM10 and where not providing our own ASM. Seems risky.

MRamonLeon · 2021-06-11T08:36:58Z

@centic9 , @ognjenb do you consider merging and releasing this change please? Thank you!

batmat · 2021-06-16T10:19:58Z

@centic9 hi, can you please consider merging & relasing?
As users mentioned on the linked jira, this issue even prevents Jenkins from starting, so it looks like a release would be good :-).

We can help on this if you need it. Just let us know.

Thanks a lot!

centic9 · 2021-06-16T13:43:33Z

Sorry, I was not aware of the impact of the changes as there was no mentioning in the PR.

Unfortunately in general I can not spend much time on this plugin, only the bare minimum to keep it alive :(

I can roll a release today or tomorrow.

If there is someone who is interested in the plugin enough to step in and spend time on general maintenance, please contact me or simply apply for access rights!

Otherwise it will probably soon go towards "unmaintained" state as there are other alternatives by now anyway...

centic9 · 2021-06-17T12:10:33Z

I pushed out 3.3.0 now with these changes.

jtnord added 5 commits June 7, 2021 17:56

Add a full e2e test using realistic class loading

f705075

remove the need for maven / guice

44a9081

FileFilter just joined the passed in excludes as a comma separated list and added defaults if the list was empty or null, But the code already handles this so just call the Plex util directly

httpcomponents was declared as a dependency but unused and bundled

03819b3

xml-apis was declared bundled and unused

92279d9

final clean up of unrequired stuff

91837c8

jtnord commented Jun 7, 2021

View reviewed changes

skip the instruction coverage as it is compiler specific

d8d300a

JDK11 and JDK8 compilers generate different bytecode and so asserting the bytecode instruction coverage would need to be different on potentially different compilers (e.g eclipse vs jdk 8 vs jdk11 vs ...)

jtnord closed this Jun 7, 2021

jtnord reopened this Jun 7, 2021

remove shade configuration - it did nothing.

40f5613

the shade plugin was bound to a phase that did not exist in the hpi packaging type. even if it did it would be completely unclear what it did as the shade plugin does not play nicely with hpis.

jtnord closed this Jun 7, 2021

jtnord reopened this Jun 7, 2021

batmat requested review from basil and centic9 June 8, 2021 08:45

rsandell approved these changes Jun 8, 2021

View reviewed changes

jtnord mentioned this pull request Jun 8, 2021

bump to maven 3.8.1 #148

Merged

6 tasks

centic9 approved these changes Jun 8, 2021

View reviewed changes

jglick approved these changes Jun 8, 2021

View reviewed changes

basil approved these changes Jun 8, 2021

View reviewed changes

centic9 merged commit e54a545 into jenkinsci:master Jun 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-65757] only use pluginFirst classloader when you have no other option (and never for guice!) #151

[JENKINS-65757] only use pluginFirst classloader when you have no other option (and never for guice!) #151

jtnord commented Jun 7, 2021 •

edited

Loading

jtnord Jun 7, 2021

jtnord Jun 7, 2021

jtnord Jun 7, 2021 •

edited

Loading

jtnord Jun 7, 2021

jtnord Jun 7, 2021

centic9 Jun 8, 2021

jtnord Jun 8, 2021 •

edited

Loading

jglick Jun 8, 2021

MarkEWaite commented Jun 7, 2021

centic9 commented Jun 8, 2021

jtnord commented Jun 8, 2021

jtnord commented Jun 8, 2021

centic9 left a comment

centic9 Jun 8, 2021

MarkEWaite commented Jun 8, 2021

jglick Jun 8, 2021

jglick Jun 8, 2021

basil left a comment

jtnord commented Jun 9, 2021

batmat commented Jun 9, 2021

jtnord commented Jun 9, 2021 •

edited

Loading

MRamonLeon commented Jun 11, 2021

batmat commented Jun 16, 2021

centic9 commented Jun 16, 2021

centic9 commented Jun 17, 2021

		@@ -122,9 +121,8 @@ private IBundleCoverage analyzeStructure() throws IOException {
		excludes = ITEM_ZERO;

[JENKINS-65757] only use pluginFirst classloader when you have no other option (and never for guice!) #151

[JENKINS-65757] only use pluginFirst classloader when you have no other option (and never for guice!) #151

Conversation

jtnord commented Jun 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtnord Jun 7, 2021 • edited Loading

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

jtnord Jun 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarkEWaite commented Jun 7, 2021

centic9 commented Jun 8, 2021

jtnord commented Jun 8, 2021

jtnord commented Jun 8, 2021

centic9 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarkEWaite commented Jun 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

basil left a comment

Choose a reason for hiding this comment

jtnord commented Jun 9, 2021

batmat commented Jun 9, 2021

jtnord commented Jun 9, 2021 • edited Loading

MRamonLeon commented Jun 11, 2021

batmat commented Jun 16, 2021

centic9 commented Jun 16, 2021

centic9 commented Jun 17, 2021

jtnord commented Jun 7, 2021 •

edited

Loading

jtnord Jun 7, 2021 •

edited

Loading

jtnord Jun 8, 2021 •

edited

Loading

jtnord commented Jun 9, 2021 •

edited

Loading