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

Make it easier to use a different JVM target #297

Closed
wants to merge 2 commits into from

Conversation

jtnord
Copy link
Member

@jtnord jtnord commented Aug 4, 2022

Whilst Jenkins defaults will now be java11 some libraries may want to
stay on java8.

Doing so makes backports of any issues much easier (hopefully not
needed!) and the library can take advantage of all the updated plugins
with their fixes.

Whilst this is less than optimal for java8 as it still required a java9+
JVM to be used to compile (as that was when the release flag was
introduced) it is much better than either

  • pinning the parent to 1.76 (the last supporting java8)
  • having to overwrite all the properties to remove the maven.compiler.release and testRelease ones as well as
    overwriting the enforcer configurations (albeit this is simplier with
    combine.children)
  • 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

Whilst Jenkins defaults will now be java11 some libraries may want to
stay on java8.

Doing so makes backports of any issues much easier (hopefully not
needed!) and the library can take advantage of all the updated plugins
with their fixes.

Whilst this is less than optimal for java8 as it still required a java9+
JVM to be used to compile (as that was when the `release` flag was
introduced it is much better than either pinning the parent to 1.76 (the
last supporting java8), or having to overwrite all the properties to
remove the maven.compiler.release and testRelease ones as well as
overwriting the enforcer configurations (albeit this is simplier with
combine.children)
@jtnord jtnord marked this pull request as draft August 4, 2022 11:29
@jtnord jtnord marked this pull request as ready for review August 4, 2022 11:40
@jtnord jtnord requested review from jglick and basil August 4, 2022 11:41
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.

On the one hand, this is relatively simple and there have already been a few components that updated this POM only to laboriously override Java levels.

On the other hand, I tend to prefer to keep things simple and just drop Java 8 support from new versions of components. If and when a backport is needed for some emergency bug fix—and 2.346.x is even still under support—it is very simple to create a backport branch and publish fixes from it. In fact I would strongly advise using a backport branch anyway, since trunk can accumulate all sorts of incidental changes unrelated to your emergency which might cause regressions.

In the case motivating this PR, jenkinsci/trilead-api-plugin#55 seems more straightforward. There is no particular reason to offer new versions of the plugin for older Jenkins versions.

pom.xml Outdated Show resolved Hide resolved
@jglick
Copy link
Member

jglick commented Aug 4, 2022

Amends #209 for the record. (Would have mentioned that in the review comment, but alas GitHub neglects to create a backlink in that case—a bug I thought I reported long ago.)

Co-authored-by: Jesse Glick <jglick@cloudbees.com>
Copy link
Member

@basil basil left a comment

Choose a reason for hiding this comment

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

I concur with #297 (review). This implicitly encourages consumers to stay on Java 8 by adding first-class API support for it in the form of the javaTarget Jenkins-specific property. Once such an API is exposed, consumers will start expecting generic support for arbitrary Java versions from the parent POM, which is not something we are committing to providing. Seems safer not to expose any such commitment at the API level and to implicitly discourage consumers from staying on Java 8 by making them carry their own workarounds (or a backport branch).

@daniel-beck
Copy link
Member

Could we enforce a minimum Jenkins dependency of 2.357 for Java 11 targets in the plugin POM to prevent a situation in which a plugin requires Java 11, yet claims compatibility with Jenkins <2.357 which are supposed to still support Java 8?

@jtnord
Copy link
Member Author

jtnord commented Aug 4, 2022

Could we enforce a minimum Jenkins dependency of 2.357 for Java 11 targets in the plugin POM to prevent a situation in which a plugin requires Java 11, yet claims compatibility with Jenkins <2.357 which are supposed to still support Java 8?

portably the wrong place for that discussion but https://github.com/jenkinsci/plugin-pom/blob/master/pom.xml#L1001 is likely the reason this went undetected.

@jglick
Copy link
Member

jglick commented Aug 4, 2022

Yes, we went with jenkinsci/plugin-pom#563 currently. Something along the lines of jenkinsci/plugin-pom#478 would be a longer-term change. Probably you can arrange to continue to have plugin-pom support both pre- and post-2.357 jenkins.versions with 100% safety but at the cost of complexity. (all IIUC)

@daniel-beck
Copy link
Member

Right, there's a comment explaining it in detail just above, thanks for the reference. Too bad this isn't possible 😦

@basil
Copy link
Member

basil commented Nov 2, 2022

I filed #331 with an alternative.

@basil basil closed this in #331 Nov 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants