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-54828] Dynamically configure Java compiler based on core #323

Merged
merged 2 commits into from Mar 31, 2022

Conversation

basil
Copy link
Member

@basil basil commented Mar 30, 2022

See JENKINS-54828. When combined with jenkinsci/plugin-pom#523 (which this PR doesn't require, but rather benefits from if present), this dynamically configures the Java compiler settings based on the core version in use. Although this sort of dynamic configuration may confuse IDEs, the benefit is that it avoids an awkward flag day when we bump the minimum required Java version to 11. Of course, we may still choose to update the plugin parent POM at that point in time for the benefit of IDEs, but having this change widely adopted beforehand would soften the blow.

I've tested this with a minimum Java version of 8 and a minimum Java version of 11, with a version 8 compiler and a version 11 compiler (matrix of 4 test configurations). When using a minimum Java version of 11, I verified that I could use Java 11 features in the plugin. When using a minimum Java version of 11 and a Java 8 compiler, I verified that I got an error. When using a Java 11 compiler, I verified that we were only setting release and testRelease regardless of whether the minimum Java version was 8 or 11. When using a Java 8 compiler, I verified that we were setting source, target, testSource, and testTarget but not release or testRelease.

@basil basil requested a review from jglick March 30, 2022 21:39
@basil basil mentioned this pull request Mar 30, 2022
@basil
Copy link
Member Author

basil commented Mar 31, 2022

Sample output from "migration mode" (minimum Java version faked to 11), compiling with Java 11:

maven-hpi-plugin:3.27-SNAPSHOT:initialize (default-initialize) @ pipeline-stage-tags-metadata ---
[INFO] Setting maven.compiler.release to 11
[INFO] Setting maven.compiler.testRelease to 11
[INFO] Unsetting maven.compiler.source
[INFO] Unsetting maven.compiler.target
[INFO] Unsetting maven.compiler.testSource
[INFO] Unsetting maven.compiler.testTarget

The intention is that when we do decide to drop support for Java 8, we would merge and release jenkinsci/plugin-pom#478 which would eliminate the above sketchy dynamic adjustments, but in the meantime this helps make the transition smoother.

@basil basil changed the title Dynamically configure Java compiler based on core [JENKINS-54828] Dynamically configure Java compiler based on core Mar 31, 2022
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.

This worries me a bit as too magical.

What's wrong with just requiring a plugin pom update to build and compile Java 11+?

Plugins should still compile fine on Java 8 (as long as they're built against older cores), and will be able to run fine on Java 11 and newer cores.


Is this to allow compiling with Java release level 11 in CI before upgrading the pom to a version that requires Java 11?


(Non blocking to be clear)

Comment on lines +23 to +24
if (!project.getProperties().containsKey("maven.compiler.source")
&& !project.getProperties().containsKey("maven.compiler.release")) {
Copy link
Member

Choose a reason for hiding this comment

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

This presumes that the POM is configuring these mojo parameters via properties, which is not necessary, and in fact discouraged—best configured via <configuration> in <plugin>.

More robust to look up the effective configuration of the mojo. Do not recall the idiom for that offhand.

Copy link
Member Author

Choose a reason for hiding this comment

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

More robust to look up the effective configuration of the mojo. Do not recall the idiom for that offhand.

Declined. If you want to understand the reason why then just try writing the code and you will quickly find out.

@jglick
Copy link
Member

jglick commented Mar 31, 2022

What's wrong with just requiring a plugin pom update to build and compile Java 11+?

And not allowing you to the newer POM to build and compile at level 8—so that would be a breaking update for the plugin POM. I also think this would be acceptable given the special circumstances. jenkinsci/plugin-pom#478 (comment) has more detailed discussion. I guess I am -0 on this—seems like asking for trouble, and not exactly necessary, but also not blocking either (since this magic would not be expected to be used most of the time).

@basil
Copy link
Member Author

basil commented Mar 31, 2022

This worries me a bit as too magical.

It is complex, but this is intended as a short-term compatibility shim rather than a long-term solution.

What's wrong with just requiring a plugin pom update to build and compile Java 11+?

Because the flag day is a rough migration that breaks compatibility. This PR maintains compatibility (at the cost of complexity) during the migration period, while allowing us to drop the complexity in favor of a cleaner solution once the migration has been completed.

This is standard practice for our community; see, for example, the Acegi to Spring Security migration, or the Guava migration.

Is this to allow compiling with Java release level 11 in CI before upgrading the pom to a version that requires Java 11?

As I wrote above, it is to make the migration smoother, not a long term solution.

@basil
Copy link
Member Author

basil commented Mar 31, 2022

I guess I am -0 on this

Comments like this really confuse me. In the other PR, you wrote about how breaking compatibility was a concern and how we should try to find ways to avoid a flag day. Well, this is it: signed, sealed, and delivered. And it took a considerable amount of effort to reach this solution. And now you are saying "I'd rather we not do this." No matter which way I attempt to address your concerns, it seems that I cannot win.

Yes this is temporary glue code. Most backward compatibility shims are not pretty. This code can be removed later on, when Mark breaks his silence about a date and we cross over into a hypothetical glorious future by dropping support for Java 8 in plugin-pom. I think that a smoother transition for developers is worth the added complexity in the short term.

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.

Makes me nervous, but so long as this is considered a temporary trick it is probably worth it to avoid friction during the transitional period.

@basil
Copy link
Member Author

basil commented Mar 31, 2022

Thanks. And I think it should go without saying at this point that if there are any problems that I will take responsibility for them in a timely fashion, up to and including a revert if necessary.

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

3 participants