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

Improve validation mojo #319

Merged
merged 6 commits into from Mar 30, 2022
Merged

Improve validation mojo #319

merged 6 commits into from Mar 30, 2022

Conversation

basil
Copy link
Member

@basil basil commented Mar 29, 2022

The validation mojo currently checks for Java 6 or later and Jenkins 1.420 or later. These seem like pretty old minimum requirements.

This PR bumps the minimum supported core version to 2.204 and enforces minimum Java version based on the value encoded by core, at whatever version of core the plugin is running against. The only place this is encoded right now is in java.level in core-bom, which is where we're reading it (well, MANIFEST.MF in jenkins.war encodes a Build-Jdk-Spec, but that could be problematic if you did a local build with Java 11 and <release>8</release>, so I am not relying on it). I don't feel strongly about encoding it in this particular way, just that it is encoded somewhere in the artifacts published by core and that we read it out here. If we want to encode it some other way I am fine with that.

I tested this by building core with a java.level of 11 and verifying that a plugin gave us the proper error message. No integration tests yet, because it is too hard to integration test all of this stuff across multiple repositories when stuff hasn't been released. When releases have been made and everything is updated, I think it will be far easier to add test coverage.

@jglick Curious what you think in the context of our other discussions elsewhere. No pressure to review this though.

Comment on lines 108 to 110
if (version == null || version.isEmpty()) {
throw new MojoExecutionException("java.level not defined in " + bom);
}
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 probably the most controversial part of this change, as it commits us to an encoding of java.level in the <properties> section of core-bom. Might not be the ideal encoding. As I wrote above, my point here is just to rely on an encoding somewhere. It doesn't have to be this specific encoding.

Copy link
Member

Choose a reason for hiding this comment

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

Can we inspect the Java bytecode level of some class in jenkins-core.jar? More work, but less sensitive to how core is built.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

That's an interesting idea. I'll try it out. I like it on a high level. As the late Roger Faulkner once opined:

Roger believed that terrible things were sometimes required to create beautiful abstractions, and his trailblazing work on /proc embodies this burden: the innards may be delicate and nasty (‘vile,’ as Roger might say in his distinguished Carolinian accent) — but the resulting abstractions are breathtaking in their power, scope and robustness.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in commit 34cf73b. Marking as draft due to the incremental in the POM file, but from my perspective this is ready for review, just not ready for merge until the release of lib-version-number.

@jglick
Copy link
Member

jglick commented Mar 29, 2022

context: jenkinsci/plugin-pom#478

@basil basil marked this pull request as draft March 30, 2022 02:13
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.

Looks good. Do you also plan to verify in a mojo that the plugin’s Java release is the same as that of core?

Co-authored-by: Jesse Glick <jglick@cloudbees.com>
pom.xml Outdated Show resolved Hide resolved
Co-authored-by: Jesse Glick <jglick@cloudbees.com>
@basil basil marked this pull request as ready for review March 30, 2022 16:18
@basil basil merged commit a6c22b3 into jenkinsci:master Mar 30, 2022
@basil basil deleted the validation branch March 30, 2022 16:30
@basil
Copy link
Member Author

basil commented Mar 30, 2022

Do you also plan to verify in a mojo that the plugin’s Java release is the same as that of core?

I need to think about this some more.

@basil
Copy link
Member Author

basil commented Mar 30, 2022

Do you also plan to verify in a mojo that the plugin’s Java release is the same as that of core?

In #323 I am not verifying it, but rather dynamically adjusting it as needed to allow for a smoother transition. Once the majority of plugins have crossed the flag day, we can likely change this from a dynamic adjustment to a hard enforcement, but this makes the transition smoother.

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

2 participants