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.

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

Support using Jenkins core BOM available since Jenkins 2.195 #229

Merged
merged 17 commits into from
Nov 13, 2019

Conversation

jtnord
Copy link
Member

@jtnord jtnord commented Aug 13, 2019

Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

Would it make sense to put BOM into a profile with a useJenkinsBOM activator property (or whatever?). In such case we can define different behaviors for cases when BOM is defined or not so that we can apply change retrospectively to plugins which require Jenkins versions without Core BOM

@jtnord
Copy link
Member Author

jtnord commented Aug 14, 2019

apply change retrospectively to plugins which require Jenkins versions without Core BOM

I was thinking of publishing some BOMs retrospectivly for some common versions like 2.138.1 etc)?

Both approaches are equally valid (but would like to keep things simple at the same time (the pom is getting a bit over grown and is in need of a prune in other areas (to be followed up later)).

Let's see if anyone else has any opinions on the way to go here. @jenkinsci/code-reviewers

@markawm
Copy link

markawm commented Aug 23, 2019

Here's an example of using the BOM in a plugin (cloudbees-folder-plugin): https://github.com/markawm/cloudbees-folder-plugin/tree/use-jenkins-bom

@jetersen
Copy link
Member

conflicts 😓

Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

It cannot be merged until there is a solution for Jenkins core versions which do not provide BOM (profiles?)... or until BOM is available for all LTS releases since 2.60.1.

@jglick jglick self-requested a review August 27, 2019 12:40
@jglick
Copy link
Member

jglick commented Aug 27, 2019

Is there a way to make this opt-in somehow, like jenkinsci/bom is? The problem as I understand it is that dependency management from a parent overrides imported dependency management; so could the dependency management in the parent which is being deleted here be moved into a profile which is active by default but which you can suppress while also importing a BOM?

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.

I suspect this is all doing too much and all we actually need to align is SLF4j, which may be an easier problem.

</dependency>

<!--
Recommended versions of Mockito and PowerMock for build with Java 11 (JENKINS-55098)
Copy link
Member

Choose a reason for hiding this comment

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

Why would these need to be aligned with versions used by Jenkins core? I would revert all this and let the plugin POM manage test frameworks independently.

@@ -143,21 +136,12 @@
<artifactId>test-annotations</artifactId>
<version>1.3</version>
</dependency>
<dependency>
<groupId>junit</groupId>
<artifactId>junit</artifactId>
Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

pom.xml Outdated
<dependency> <!-- used in JTH and jenkins core > 2.x -->
<groupId>javax.servlet</groupId>
<artifactId>javax.servlet-api</artifactId>
<version>3.1.0</version>
</dependency>
<dependency>
<groupId>org.codehaus.mojo</groupId>
<artifactId>animal-sniffer-annotations</artifactId>
Copy link
Member

Choose a reason for hiding this comment

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

Is there any particular reason annotations need to be aligned in version with Jenkins core? I do not believe so. Recommend reverting.

@oleg-nenashev
Copy link
Member

Yes. My expectation is to have profiles as well. Can be easily done via useBOM property. We can also hide https://github.com/jenkinsci/bom under the profile once there is a clear mapping for jenkins.version -> bom.version

@markawm
Copy link

markawm commented Sep 5, 2019

Updated now to add a no-jenkins-bom profile that captures the current/previous behaviour. Also a use-jenkins-bom profile that imports the BOM for when that is being used.
Either one or the other is activated according to the property use-jenkins-bom, and the default is to recreate the old behaviour by having no-jenkins-bom active.

@markawm
Copy link

markawm commented Sep 5, 2019

Couple of draft PRs showing usage of this version with and without bom:
jenkinsci/cloudbees-folder-plugin#130 - example of latest plugin-pom with bom.
jenkinsci/cloudbees-folder-plugin#131 - example of latest plugin-pom without bom.

pom.xml Outdated Show resolved Hide resolved
Co-Authored-By: Tim Jacomb <[email protected]>
pom.xml Show resolved Hide resolved
@markawm
Copy link

markawm commented Oct 7, 2019

Added some short info. to the README and referenced longer core bom usage information here.

@markawm
Copy link

markawm commented Oct 10, 2019

@oleg-nenashev, @jglick: documentation for this is now merged (thanks); can we merge this? If not, what can I do to get it in the right shape?

@markawm
Copy link

markawm commented Oct 14, 2019

@oleg-nenashev, I think I've addressed all the comments on this now; can we get it merged? Thanks.

@oleg-nenashev oleg-nenashev self-requested a review October 15, 2019 17:25
Copy link

@EstherAF EstherAF left a comment

Choose a reason for hiding this comment

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

lgtm

@timja
Copy link
Member

timja commented Oct 20, 2019

Are we able to move ahead with this?
It's already documented on the website =/

https://jenkins.io/doc/developer/plugin-development/dependency-management/

Copy link
Member

@jetersen jetersen left a comment

Choose a reason for hiding this comment

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

LGTM, one minor nit for documentation.


Since version 2.195, Jenkins provides a [Maven Bill Of Materials (BOM)](https://maven.apache.org/guides/introduction/introduction-to-dependency-mechanism.html#Importing_Dependencies)
that centrally defines versions of various libraries used by Jenkins Core.
The default behaviour of `plugin-pom` is to _not_ use the BOM, but when jenkins.version>=2.195 (and plugin-pom>=4.0) you can switch on Jenkins BOM support by setting the Maven property `use-jenkins-bom`.
Copy link
Member

Choose a reason for hiding this comment

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

nit: suggest to use .mvn/maven.config for adding -Duse-jenkins-bom

Copy link

Choose a reason for hiding this comment

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

Agreed, its the preferred way and what the docs describe - was just keeping it brief here and referencing the docs for details.

Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

I am releasing the new Plugin POM now to clear a runway for this change.
Please cleanup the merge conflict so that we could cut a release this week or early next week

@jetersen
Copy link
Member

Looking forward to!
🚢 📦 🚀

Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

Just 2 comments, but it is ready to go overall

pom.xml Outdated Show resolved Hide resolved
@@ -14,7 +14,7 @@
<packaging>pom</packaging>
<properties>
<jenkins.version>2.7.3</jenkins.version>
<java.level>7</java.level>
<java.level>8</java.level>
Copy link
Member

Choose a reason for hiding this comment

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

Phasing out Java 7 is already confirmed in https://groups.google.com/forum/#!msg/jenkinsci-dev/SdMOMKs7XIQ/_XADhodoBQAJ , no need in extra approvals for it.

If the Jenkins Version in this test is bumped to 2.60.3, everything will be fine IMHO

Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

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

LGTM

@oleg-nenashev
Copy link
Member

I plan to merge it tomorrow if no negative feedback

@oleg-nenashev oleg-nenashev merged commit fcedbce into jenkinsci:master Nov 13, 2019
@oleg-nenashev oleg-nenashev changed the title Use BOM from Jenkins Support using Jenkins core BOM available since Jenkins 2.195 Nov 13, 2019
@oleg-nenashev
Copy link
Member

Sorry for the squash merge, was not intended here

@jglick
Copy link
Member

jglick commented Nov 14, 2019

This seems to break all incrementalified plugins. Do not use 3.52.

<!--
The 'no-jenkins-bom' profile captures the properties & dependencies when _not_ using the Jenkins BOM:
https://github.com/jenkinsci/jenkins/blob/master/bom/pom.xml
When the BOM is not being used, this profile is adding back the <dependencyManagement/> entries that were
Copy link
Member

Choose a reason for hiding this comment

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

BTW use <dependencyManagement /> since otherwise MRP will make this change for you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants