Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[JENKINS-58771] Avoid more cases of bundling libs incorrectly #172

Merged
merged 6 commits into from
Mar 13, 2020
Merged

[JENKINS-58771] Avoid more cases of bundling libs incorrectly #172

merged 6 commits into from
Mar 13, 2020

Conversation

Vlatombe
Copy link
Member

@Vlatombe Vlatombe commented Mar 5, 2020 •
edited
Loading

Expands #140

Sometimes some dependencies gets pulled through test or provided scope
trails, because in fact they are being pulled in compile scope through a plugin with a longer trail.

Inspired by jenkinsci/kubernetes-plugin#729

@Vlatombe
[JENKINS-58771] Avoid more cases of bundling libs incorrectly
bb133bc 
Sometimes some dependencies gets pulled through test or provided scope
trails, because in fact they are being pulled in compile scope through a plugin with a longer trail.
@Vlatombe Vlatombe requested a review from jglick March 5, 2020 16:55
@Vlatombe Vlatombe mentioned this pull request Mar 5, 2020
Merged
@jtnord
Copy link
Member

jtnord commented Mar 9, 2020 •
edited
Loading

also seeing cases of

[WARNING] Bundling transitive dependency jsr305-3.0.2.jar (via spotbugs-annotations)
[INFO] Bundling direct dependency rate-limited-logger-2.0.0.jar
[WARNING] Bundling transitive dependency eddsa-0.3.0.jar (via jenkins-war)
[WARNING] Bundling transitive dependency slf4j-api-1.7.26.jar (via rate-limited-logger)

I only see eddsa as test scope from jenkins-war so looks like this will also help.

[INFO] +- org.jenkins-ci.main:jenkins-war:executable-war:2.222.1-cb-1:test
[INFO] |  +- org.jenkins-ci.modules:ssh-cli-auth:jar:1.7:test
[INFO] |  +- org.jenkins-ci.modules:slave-installer:jar:1.7:test
[INFO] |  +- org.jenkins-ci.modules:windows-slave-installer:jar:1.12:test
[INFO] |  +- org.jenkins-ci.modules:launchd-slave-installer:jar:1.2:test
[INFO] |  +- org.jenkins-ci.modules:upstart-slave-installer:jar:1.1:test
[INFO] |  +- org.jenkins-ci.modules:systemd-slave-installer:jar:1.1:test
[INFO] |  +- org.jenkins-ci.modules:sshd:jar:2.6:test
[INFO] |  |  +- org.apache.sshd:sshd-core:jar:1.7.0:test
[INFO] |  |  \- net.i2p.crypto:eddsa:jar:0.3.0:test

slf4j is being promoted via an optional dependency

[INFO] com.cloudbees.operations-center.server:operations-center-server:hpi:2.204.2.112-SNAPSHOT
[INFO] +- org.jenkins-ci.plugins:support-core:jar:2.64:compile (optional)
[INFO] |  +- net.sf.uadetector:uadetector-resources:jar:2013.09:compile (optional)
[INFO] |  |  +- net.sf.uadetector:uadetector-core:jar:0.9.9:compile (optional)
[INFO] |  |  |  \- net.sf.qualitycheck:quality-check:jar:1.3:compile (optional)
[INFO] |  |  \- org.slf4j:slf4j-api:jar:1.7.26:compile

@oleg-nenashev oleg-nenashev self-requested a review March 9, 2020 14:52
@jtnord
Copy link
Member

jtnord commented Mar 9, 2020

This PR fixes the above issues for me.

jglick
jglick reviewed Mar 9, 2020
View reviewed changes
Copy link
Member

@jglick jglick left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Would like some more context before I feel comfortable approving.

src/it/JENKINS-58771-packaged-plugins-2/pom.xml Outdated Show resolved Hide resolved
src/it/JENKINS-58771-packaged-plugins-3/verify.groovy Show resolved Hide resolved
src/main/java/org/jenkinsci/maven/plugins/hpi/AbstractHpiMojo.java Outdated Show resolved Hide resolved
@jtnord
Copy link
Member

jtnord commented Mar 9, 2020

also seeing cases of

[WARNING] Bundling transitive dependency jsr305-3.0.2.jar (via spotbugs-annotations)
[INFO] Bundling direct dependency rate-limited-logger-2.0.0.jar
[WARNING] Bundling transitive dependency eddsa-0.3.0.jar (via jenkins-war)
[WARNING] Bundling transitive dependency slf4j-api-1.7.26.jar (via rate-limited-logger)

I only see eddsa as test scope from jenkins-war so looks like this will also help.

[INFO] +- org.jenkins-ci.main:jenkins-war:executable-war:2.222.1-cb-1:test
[INFO] |  +- org.jenkins-ci.modules:ssh-cli-auth:jar:1.7:test
[INFO] |  +- org.jenkins-ci.modules:slave-installer:jar:1.7:test
[INFO] |  +- org.jenkins-ci.modules:windows-slave-installer:jar:1.12:test
[INFO] |  +- org.jenkins-ci.modules:launchd-slave-installer:jar:1.2:test
[INFO] |  +- org.jenkins-ci.modules:upstart-slave-installer:jar:1.1:test
[INFO] |  +- org.jenkins-ci.modules:systemd-slave-installer:jar:1.1:test
[INFO] |  +- org.jenkins-ci.modules:sshd:jar:2.6:test
[INFO] |  |  +- org.apache.sshd:sshd-core:jar:1.7.0:test
[INFO] |  |  \- net.i2p.crypto:eddsa:jar:0.3.0:test

slf4j is being promoted via an optional dependency

[INFO] com.cloudbees.operations-center.server:operations-center-server:hpi:2.204.2.112-SNAPSHOT
[INFO] +- org.jenkins-ci.plugins:support-core:jar:2.64:compile (optional)
[INFO] |  +- net.sf.uadetector:uadetector-resources:jar:2013.09:compile (optional)
[INFO] |  |  +- net.sf.uadetector:uadetector-core:jar:0.9.9:compile (optional)
[INFO] |  |  |  \- net.sf.qualitycheck:quality-check:jar:1.3:compile (optional)
[INFO] |  |  \- org.slf4j:slf4j-api:jar:1.7.26:compile

eddsa..

image

promoted to compile due to dependency on maven plugin which depends on ssh-credentials which depends on... whilst correct at the maven level, maven-plugin is a HPI and should break the cycle.

image

@atanasenko
Copy link

atanasenko commented Mar 10, 2020 •
edited
Loading

I found usage of getDependencyTrail() to be flawed to begin with. It is always the shortest path to dependency and the reason why maven included that particular dependency.
Logic behind jenkins/hpi conflicts with the way maven performs resolution, shorter trail of, for example, dep1 -> commons-exec will always win over plugindep -> dep2 -> commons-exec and it will get bundled (irregardless of version differences), though it will never be used unless you mask classes from it.

@Vlatombe
Copy link
Member Author

I found usage of getDependencyTrail() to be flawed to begin with. It is always the shortest path to dependency and the reason why maven included that particular dependency.

Indeed, not using a specific type (hpi or jpi) when pulling jenkins plugins dependencies is making it impossible to rely on Maven to do the right thing.

jglick
jglick approved these changes Mar 10, 2020
View reviewed changes
Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

Clearer, thank you.

src/main/java/org/jenkinsci/maven/plugins/hpi/AbstractHpiMojo.java Outdated Show resolved Hide resolved
src/main/java/org/jenkinsci/maven/plugins/hpi/AbstractHpiMojo.java Outdated Show resolved Hide resolved
src/main/java/org/jenkinsci/maven/plugins/hpi/AbstractHpiMojo.java Outdated Show resolved Hide resolved
@Vlatombe @jglick
Use blocks
86b71de 
Co-Authored-By: Jesse Glick <jglick@cloudbees.com>
@jglick
Copy link
Member

jglick commented Mar 10, 2020

JENKINS-58771-packaged-plugins-2 failed

@Vlatombe
Copy link
Member Author

Re-running build

@Vlatombe Vlatombe closed this Mar 11, 2020
@Vlatombe Vlatombe reopened this Mar 11, 2020
@jglick jglick merged commit 8dbb9f1 into jenkinsci:master Mar 13, 2020
@Vlatombe Vlatombe deleted the JENKINS-58771-2 branch March 13, 2020 15:40
@jglick jglick mentioned this pull request Mar 13, 2020
Merged
@jglick jglick mentioned this pull request Apr 15, 2020
Closed
@jetersen
Copy link
Member

jetersen commented Jun 10, 2020 •
edited
Loading

I seen a few more cases of odd bundling when it comes to plugin pom v4 and Jenkins Core bom.
This is using plugin pom v4.2 which uses hpi v3.14.

seems Guava, commons-io, commons-lang are chosen to be bundled in transitive dependencies even though Jenkins core provide these versions.

This PR needed to use some Avoid bundling technique: jfrog/jenkins-artifactory-plugin#285

@Vlatombe
Copy link
Member Author

@jetersen I looked at the PR you referenced and hpi plugin is behaving as expected. If you depend on a jar that pulls transitive dependencies that may be provided through other means in the Jenkins ecosystem, you have to exclude these transitive dependencies, unless you have a very good reason to include them (like pulling a more recent version).

@jetersen
Copy link
Member

jetersen commented Jun 10, 2020 •
edited
Loading

I added jackson-api plugin expecting those dependencies would not be transitively bundled from both buildinfo-api and buildinfo-client

@jglick jglick mentioned this pull request Jun 26, 2020
Merged
@MarkEWaite MarkEWaite mentioned this pull request Jul 4, 2020
Closed
7 tasks
@jglick jglick mentioned this pull request Nov 27, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amuniz amuniz amuniz approved these changes

@jtnord jtnord jtnord approved these changes

@jglick jglick jglick approved these changes

@oleg-nenashev oleg-nenashev Awaiting requested review from oleg-nenashev

Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Vlatombe @jtnord @atanasenko @jglick @jetersen @amuniz @oleg-nenashev