-
-
Notifications
You must be signed in to change notification settings - Fork 77
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-68568] Require Java 11 or newer #478
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting rid of Animal Sniffer makes sense. We can require that the build be run on JDK 11 (or newer) if you are using a sufficiently new parent, even if the plugin is still targeting pre-switch core versions and Java 8. Just means that buildPlugin
should offer only JDK 11 builds (for compatibility automatically pruning/editing configurations requesting JDK 8—tough).
java.level
seems problematic, though. You may not define it to 11 when using a pre-switch jenkins.version
. You could keep it as 8 with a post-switch jenkins.version
if there was some reason to do so—you are too lazy to stop using _
as an identifier?
Probably this property should never have been defined. Perhaps we can deprecate it—ignore any value and issue a warning to that effect. Instead figure out what Java level to pass to tools based on the jenkins.version
(somehow).
pom.xml
Outdated
@@ -515,7 +490,7 @@ | |||
</requirePluginVersions> | |||
--> | |||
<enforceBytecodeVersion> | |||
<maxJdkVersion>1.${java.level}</maxJdkVersion> | |||
<maxJdkVersion>${java.level}</maxJdkVersion> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to break for java.level=8
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good thing this PR explicitly drops support for java.level=8
, then, as stated in the PR description:
Drop support for Java 8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah but we cannot just do that because then people cannot simply accept updates to the parent POM without also updating jenkins.version
to something very new.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, we could, but then we have plugin POM versions that work with older cores but are not well suited to newer cores, and POM versions that require newer cores, without any overlap. So this seems pretty drastic when we normally encourage people to update the parent even when the core version is a few LTS lines behind. For example, anyone staying with an older line would not be able to accept new POM versions for totally unrelated reasons (new tools or JTH).
If we really go with the flag day approach in this PR, then we should definitely get rid of the java.level
property as it serves no purpose. Add a profile making it an error to set it, and hard-code 11 as the Java level. And make sure that the build fails early and clearly when your jenkins.version
is too old.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can. We can always do a backport branch if we want to deliver plugin parent POM updates to plugins that do not want to upgrade jenkins.version
to something very new.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this seems pretty drastic
Yes it is drastic but nevertheless feasible without changing the current semantics of java.level
being defined in a <properties>
section in plugin pom.xml
files. Other proposals like #133 are not feasible as far as I can tell so long as feedback like this remain unaddressed.
Instead figure out what Java level to pass to tools based on the jenkins.version (somehow).
That approach could be feasible (and less drastic!), but it would be a lot more work and (as of today) I am explicitly not volunteering to do that work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with Basil, because of how this was done you can't keep compat.
We can run a 4.x branch if we need to backport changes...
alternatively plugins that want to stay on old core could just set this to 1.8?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
set this to 1.8
IIRC that will break other places which expect 8
.
pom.xml
Outdated
@@ -736,7 +690,7 @@ | |||
<webApp> | |||
<contextPath>/jenkins</contextPath> | |||
</webApp> | |||
<minimumJavaVersion>1.${java.level}</minimumJavaVersion> | |||
<minimumJavaVersion>${java.level}</minimumJavaVersion> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should just get rid of this in maven-hpi-plugin
I think. Jenkins core does not really deal with plugins requiring a newer version of Java and we do not really want to offer such a system.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should just get rid of this in maven-hpi-plugin I think.
I tend to agree, but I am not tackling that cleanup in this PR.
pom.xml
Outdated
@@ -950,26 +904,9 @@ | |||
</file> | |||
</activation> | |||
<properties> | |||
<java.level>8</java.level> | |||
<java.level>11</java.level> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No this must be reverted so long as
Line 64 in 3325bb8
<jenkins.version>2.249</jenkins.version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It cannot be reverted while also accomplishing the purpose of this PR, which is stated in the PR description:
Drop support for Java 8
The minimum Jenkins version cannot yet be updated because we do not know which version will drop support for Java 8, but the intention is that when we do we would update the minimum Jenkins version to that version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I misunderstood the intent of this PR.
src/it/benchmark/pom.xml
Outdated
<jenkins.version>2.249</jenkins.version> | ||
<java.level>8</java.level> | ||
<java.level>11</java.level> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revert these unless also updating jenkins.version
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It cannot be reverted while also accomplishing the purpose of this PR, which is stated in the PR description:
Drop support for Java 8
The minimum Jenkins version cannot yet be updated because we do not know which version will drop support for Java 8, but the intention is that when we do we would update the minimum Jenkins version to that version.
Right, which is why this PR drops support for |
To summarize the status at this time, there is:
Based on the use of the imperative mood in the above comments, I would like to clarify my intention when filing this PR. My intention was to do the minimum amount of work to reach a feasible result, which I believe I have accomplished. I am not against anyone doing a more complete and ideal implementation in another PR; however, my goal for this PR was merely to reach a minimum viable product (MVP). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM once jenkins.version
is updated (when we figure out what that is)
Since it seems like the easiest option to implement, and cleans up tech debt, I am OK with a “flag day” switch like this, so that people updating to a post-switch I do recommend that as part of this we drop the One other question. If we use the incompatible approach here, do we release it once the first weekly requiring Java 11 is released? Or wait for the first LTS after that? The former risks confusing devs with failing Dependabot PRs that would require a |
Is there any particular reason why cleaning up |
Indeed no; my advice was more that it be done no later than this PR, so that devs picking up a new baseline & this PR are not forced to explicitly edit POMs to select Java 11 when that is really their only option anyway.
Yes it could. Then this PR would just switch the hard-coded value from 8 to 11. Again pending some confirmation of real-world impact, that there is not a legitimate reason to override this to a different value than what would be implied by |
OK, so from a project management point of view we could define two work items: a cleanup task to remove the technical debt of This PR represents the RFE. I think the cleanup task belongs in a separate PR which, as you note (and as is characteristic of technical debt), needs some careful research before it can be deemed safe, along with someone able and willing to deal with any potential regressions. Whether the cleanup task should be considered a blocker for the RFE is perhaps a matter of subjective opinion. Certainly doing it first would result in a better overall outcome. But in my experience there is an emotional cost with doing such cleanup tasks and being on the receiving end of the associated regressions. Perhaps that is why some people call them "chores". I think these tasks are best distributed evenly among members of a team to avoid having a few individuals shoulder the bulk of the emotional cost. As you know, I do not shy away from such tasks, but I am cautious not to take on too many at once lest I become discouraged. Right now I am dealing with a |
Fine with me. Anyway we have some months (I guess?) before this is even eligible for merge. |
This can be done in
Agreed.
This might be possible with a new |
This would likely work for Maven builds as such, but could well confuse tools such as IDEs which expect to evaluate a POM to understand basic aspects of project configuration. (In particular, what |
Well I cannot come up with anything better so I am going to run with it. |
Bear with me, I am trying to sort through the different options relating to
3 seems simplest and friendliest to me. If we want to remove |
Yes, you read my mind regarding the last paragraph. That is exactly the conclusion I had come to.
I thought it was you proposing that? 😄 I have lost track at this point. But on a high level I think there is a valid point here: the minimum Java version is conceptually tied to the Jenkins core version, so encoding it in the plugin POM (which is in principle agnostic to core version, within a supported range at least) seems like the wrong place. And as a result of this impedance mismatch, you end up with the problem of incompatible flag days and more. Let me give another example of the above: it's not just a matter of Java level. With Java 17 support, we're adding So I think dynamically setting things like Java level and Surefire arguments based on the core version makes sense at a high level. It avoids the flag days, and it avoids duplication of information. Core can encode official values of things like the minimum Java level and the The challenge comes with tools, like you said. I wonder if we could blend both approaches: define a Java level and Surefire arguments in the plugin parent POM (corresponding to the minimum core version that the plugin parent POM supports) as a lowest common denominator and for the benefit of tools, but override those values dynamically in an initializer mojo based on the plugin's (not plugin POM's!) minimum core version. Maybe that way we could get the best of both worlds: precision for official builds, but degrading gracefully for GUI tools. Speaking of which, do you think we have consensus on proceeding with #478 (comment) (i.e., a revert of JENKINS-20679)? I've prepared the commits for core, |
Yeah I did speculate idly in #478 (review), though it does not seem like there is any way to do this in Maven Core (i.e. with a cleverly constructed POM). I am not even positive whether you can do this from a mojo early enough. The “blend” approach would be nice if it worked. I am not too concerned about defining Surefire arguments dynamically since it is unlikely that tools would attempt to parse it out, unless there are some IDEs that bypass Maven to run tests yet read the POM (statically, without executing mojos) as the source of truth? But Lines 675 to 678 in d2b4d19
mvn ) but would be scrambled in IDEs. I guess we would push out a POM update around the same time as the core release that switches the POM default, so IDEs would work again after a while.
IMO an incompatible release of the plugin POM is tolerable if it corresponds to a new Java major version requirement in core, which could arrive every couple years on average if we trail a fixed distance behind Java LTS releases. I agree that we would not want to be forcing a plugin POM range every time we adjust A more radical option would be to revert JENKINS-32493: resume packaging the plugin pom as part of core. This would avoid the mismatch problems not just of Lines 573 to 585 in faf83a7
maven-hpi-plugin and jenkins-test-harness and making it harder to test against alternate core versions in PCT and buildPlugin() .
I am not confident at this point. I have not personally searched for examples of plugins doing this, nor do I know of any examples offhand. |
That I am fairly certain about, having tried many ways of doing this. I do not think it is possible.
Neither am I. Halfway through a prototype and it is challenging. 😅
Yeah agreed on all points.
Perhaps a bit too radical for me as part of this project. 😄
OK so what are the next steps for getting people to agree about changes like this? Developer list thread? It is really frustrating to attempt a large body of work only to get nitpicked to death in code review from people who don't entirely agree with every minute detail and have no constructive suggestions. |
Why was this closed? The last I understood from conversations in February was that we would release this in either June (right after the proposed weekly dropping Java 8 support) or September (the first LTS based on it). Did something change? |
This question implies that there is a problem with the PR (to be corrected by reinlining the profile). It does not state what the problem is or the reason why there is believed to be a problem. I will not participate in code reviews where reasoning is not provided. |
I was asking whether the change was intentional. Especially if you are not using [merge]
conflictstyle = diff3 (which GitHub Web does not!) it is easy to apply a hunk to the wrong side. But maybe the change was done for a good reason and just got included in a merge commit without a separate commit message. If I follow correctly, until 95cd21e (https://github.com/jenkinsci/plugin-pom/pull/478/files/22d4b09f2ea55dcf9b95e647a3ab2a401ccd0dee) we were switching (unconditionally, since no more I am still confused about why this PR is closed. Is there some replacement elsewhere? |
That question usually implies that the person asking the question believes there is a (potential) problem. If the person asking the question did not believe there was a (potential) problem, they would likely not be asking the question. If you believe there is a problem with one of my PRs, please state what you believe the problem is and the reason why you believe it to be a problem. If the reason is valid, I will be happy to accommodate any changes. Do not ask me questions that imply there is a problem without stating it. Do not issue imperative statements requesting that I change X or Y without giving the reasoning for the request. I am being very clear about these boundaries.
I could ask the same thing about jenkinsci/jenkins#5304. As I wrote above, the reason this PR is closed is because you continue to comment on my PRs without providing reasoning. I am not interested in participating in such reviews. |
The `java.level` property is deprecated and will be removed from a future plugin parent pom. See jenkinsci/plugin-pom#522 for the plugin pom deprecation of `java.level`. See the following additional references: * jenkinsci/plugin-pom#478 (comment) * jenkinsci/plugin-pom#478 (comment) * jenkinsci/plugin-pom#522 (comment) Also uses newer Jenkins version in the example. Would have liked to use the automatic replacement as is done in the "Choosing a Jenkins version" baseline page, but that was a larger change than I'm ready to make in this pull request.
@@ -58,18 +58,23 @@ | |||
<project.build.outputEncoding>UTF-8</project.build.outputEncoding> | |||
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding> | |||
|
|||
<maven.compiler.release>11</maven.compiler.release> | |||
<maven.compiler.testRelease>11</maven.compiler.testRelease> | |||
<!-- Work around openjdk/jdk11u-dev#919. TODO When we upgrade to OpenJDK 11.0.16, this should be deleted. --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Developer mailing list thread