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

[JENKINS-60474] streamline the pom and support the jenkins-bom #269

Merged
merged 36 commits into from
Dec 24, 2019

Conversation

jtnord
Copy link
Member

@jtnord jtnord commented Dec 16, 2019

JENKINS-60474 Streamlines the plugin pom by removing support for some older settings as per https://groups.google.com/d/topic/jenkinsci-dev/tm3F_vgK5vA/discussion

I did not remove the frontend parts for now - I have kicked that can down the road. but there are some other areas that have been streamlined.

downstream PRs

changelog suggestions

  • [internal] always run the integration tests in this module
  • libraries that are shipped with jenkins-core now have their dependency version unconditionally managed from the jenkins-bom. With this change the libaries used will be correct for a given jenkins.version however this requires a version of Jenkins that was published with the bom (2.195 or higher), or has been explicitly published (see here for a list of supported versions. The jenkins-bom and no-jenkins-bom profiles have been removed
  • findbugs properties have been removed, use the equivalent spotbugs property
  • support for compiling plugins with java < 7 has been removed. currently the only supported java.level is 8
  • concurrency property has been removed. use the forkCount surefire option directly from the command line (or pom) (e.g. mvn -DforkCount=4 verify)
  • support for the jgit provider of the release plugin has been removed. the git executable is now required to be installed
  • an extra enforcer rule has been added to prevent releases containing SNAPSHOT versions
  • the automatic re-running of failing tests has been removed. if this is desired it should be set per project by configuring surefire's rerunFailingTestsCount parameter
  • jmh benchmarks are now run by activating the jmh-benchmark directly rather than via a property. (mvn -P jmh-benchmark test)
  • skipping tests using surefire's skipTest property no longer skips other mojos execution.
  • a quick-build profile has been introduced that disabled things not related to just producing the desired artifacts. This is activated on the command line in the standard maven way (e.g. mvn -P quick-build package)

<jenkins.version>2.138.4</jenkins.version>
<!-- Should only need to override these if using timestamped snapshots (but better to use Incrementals instead): -->

<jenkins.version>2.204</jenkins.version>
Copy link
Member Author

@jtnord jtnord Dec 16, 2019

Choose a reason for hiding this comment

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

closest thing to an LTS with the bom. 2.204.1 will be around in a couple of days.

@@ -606,7 +635,7 @@
<webApp>
<contextPath>/jenkins</contextPath>
</webApp>
<minimumJavaVersion>${plugin.minimumJavaVersion}</minimumJavaVersion>
<minimumJavaVersion>1.${java.level}</minimumJavaVersion>
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 the reason to make this a property was so that you could override it and set 11. With it here the only way to do that is to override the plugin configuration

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 don't se how. If you require 11 at run time, then you should also compile with java 11..

compiling with a different java version that you require at run-time is highly likely to make things go bang at run-time - its such an advanced use case I do not believe we should hand a loaded gun to non advanced people so they can shoot themselves in the foot. Either that or I am missing a use case.

Copy link
Member Author

@jtnord jtnord Dec 17, 2019

Choose a reason for hiding this comment

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

I'm not actually sure what the state of Java 11 is here. #133 is still open indicating that the only option today should be java 8. (I still to remove Java7 support to simplify)

maybe @oleg-nenashev could shed some light when he comes back from PTO?

Copy link
Member Author

Choose a reason for hiding this comment

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

(there are also no annimal-snifffer signatures for Java11 - at least defined in this pom)

@jtnord jtnord marked this pull request as ready for review December 18, 2019 13:17
@@ -5,7 +5,7 @@ node('maven') {
// TODO Azure mirror
ansiColor('xterm') {
withEnv(['MAVEN_OPTS=-Djansi.force=true']) {
sh 'mvn -B -Dstyle.color=always -ntp -Prun-plugin-pom-its clean verify'
sh 'mvn -B -Dstyle.color=always -ntp clean verify'
Copy link
Member Author

Choose a reason for hiding this comment

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

this is no longer a profile it is in the main definition

jtnord added a commit to jtnord/pipeline-library that referenced this pull request Dec 23, 2019
do not be scared of profiles - activate them directly so you know what is happening (also removed in plugin-pom 4.0)
jenkinsci/plugin-pom#269
pom.xml Outdated Show resolved Hide resolved
Co-Authored-By: Oleg Nenashev <o.v.nenashev@gmail.com>
@jtnord
Copy link
Member Author

jtnord commented Dec 23, 2019

@oleg-nenashev

Should we create a new integration branch for Plugin POM 4.x so that we incrementally integrate all changes there ?

I'm not sure what you are suggesting?

if you mean a new branch instead of master - I'm not sure, I would just like to roll forward quickly. the v4 should be the new, do we want a legacy branch for the current version incase some fixes are needed for it - but once I have published some older boms (2.138.[123], 2.164.[123]) I'm not sure if there would be a reason for people to stay on the old lines.
at the end of the day no one is forcing a parent update on plugins?

@oleg-nenashev
Copy link
Member

@jtnord There are basically two ways:

  1. We create a new branch for 4.x while it is in Beta. 3.x stays in master
  2. We keep 4.x in master, 3.x branch will be created if needed due to whatever reason

Either way is fine with me, with slight preference towards (2)

@jtnord
Copy link
Member Author

jtnord commented Dec 23, 2019

@oleg-nenashev option 2 sounds perfect.

@oleg-nenashev
Copy link
Member

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.

Approving it for 4.0 Beta. 3.55 has been released few minutes ago, so we can start integrating changes IMHO.

@jtnord could you please prepare a list of Removals and breaking changes in the PR summary? I will use it for the changelog

@jtnord
Copy link
Member Author

jtnord commented Dec 23, 2019

@oleg-nenashev release note suggestions provided.

@oleg-nenashev
Copy link
Member

Taking the number of changes, I will probably cut beta-1 just with this PR so that it can be evalyated without the JTH breaking changes

@oleg-nenashev
Copy link
Member

I will merge it once the build passes

@@ -31,6 +31,7 @@ Then you can just replace the Plugin POM version in your downstream PRs by this
### Managing release notes

* Changelog drafts are automatically by link:https://github.com/toolmantim/release-drafter[Release Drafter]
** See the documentation about Jenkins Release Drafter configuration link:https://github.com/jenkinsci/.github/blob/master/.github/release-drafter.adoc[here]
Copy link
Member Author

@jtnord jtnord Dec 24, 2019

Choose a reason for hiding this comment

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

@oleg-nenashev why did you restore this it is garbage URL.

Copy link
Member

Choose a reason for hiding this comment

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

I fixed it

@oleg-nenashev
Copy link
Member

OK, will proceed with the merge and beta-1 release preparation

Comment on lines -64 to -65
<concurrency>1</concurrency> <!-- DEPRECATED use forkCount -->
<forkCount>${concurrency}</forkCount> <!-- may use e.g. 2C for 2 × (number of cores) -->
Copy link
Member

Choose a reason for hiding this comment

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

Cleanup from #80 FTR

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.

None yet

5 participants