-
Notifications
You must be signed in to change notification settings - Fork 87
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-20679] Try to declare the minimum Java version in the manifest #75
Conversation
Unfortunately hpi-plugin is not using jar-plugin. Does maven-jar-plugin put such info? Could you compare with generic maven project with jar packaging manifest? Is it missing something else? |
@KostyaSha Not sure what you mean, could you elaborate? Also, note that I don't want to mechanically pick up whatever is used to build the plugin (may be too recent, or there may be reasons to require a newer JRE to actually run), so the |
Checked myself. Seems maven-jar is not setting this information. I thought it includes more information in manifest :/ |
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.
🐝 . There may be a need to check if the dependencies are compliant with the declared version, but IIRC it's already done by other tools
@oleg-nenashev Animal sniffer and enforcer use the |
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.
Too magical, not the Maven way.
src/main/java/org/jenkinsci/maven/plugins/hpi/AbstractHpiMojo.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jenkinsci/maven/plugins/hpi/AbstractHpiMojo.java
Outdated
Show resolved
Hide resolved
f5c07ea
to
1ccd722
Compare
This change + the downstream change to plugin POM should probably done sooner rather than later. The corresponding core change is only needed when core supports Java 9 at all, and the infra change can be implemented even later (theoretically). |
Where is that? |
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.
OK so far as it goes, but needs to be evaluated in context of matching PRs.
src/main/java/org/jenkinsci/maven/plugins/hpi/AbstractHpiMojo.java
Outdated
Show resolved
Hide resolved
Is there something filed downstream of this? |
@jglick See original PR comment. |
I think on hold while discussions in the downstreams continue. |
@jenkinsci/java11-support |
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, but I commented in https://github.com/jenkinsci/maven-hpi-plugin/pull/75/files#r236275067 about my main concern currently.
And then Maven integration tests failed... as designed, I believe. Will add flags there |
There is a chicken-and-egg problem in integration tests. They fail, because option 1: I could release a snapshot of the Parent POM, update integration tests to such timestamped snapshot (upper bounds, yey), and then the tests would be green. But it requires a circular dependency in the release. It should be Okay in this case option 2: Reconsider the approach and do not require @jglick @daniel-beck @jenkinsci/java11-support Would appreciate an advice how to proceed |
IMO if we step back:
So, for the issue at stake here IMO this is why we should go for option 1, because requiring the field is the most important and the right long-term decision. |
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 prefer the option 1.
Alternately, make the Java version optional (with a warning) for now, release everything, then immediately afterward go back and make it mandatory by switching the parent POMs in the ITs to use the prior release. |
Deployed the current state as |
Going forward with option 1 |
…eaking change we introduce, go to 3.x
@jenkinsci/java11-support @jglick The rest of integration tests are failing with...
I cannot do much about that, so I suggest merging as is and then fixing the PR builder after releasing the downstream patch and bumping to the released version of Plugin POM. Are you fine with it? |
You mean releasing the |
Inject `Minimum-Java-Version` into the manifest. | ||
* It is set by a new mandatory `minimumJavaVersion` parameter in `hpi:hpi`, `hpi:jar` and `hpi:hpl` | ||
* Format: `java.specification.version` according to [Java JEP-223](https://openjdk.java.net/jeps/223). | ||
Examples: `1.6`, `1.7`, `1.8`, `6`, `7`, `8`, `9`, `11`, ... |
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.
Huh? That JEP seems to say the format should be 6
, 7
, etc., and not 1.6
, 1.7
, etc. as before. So which is the version format expected by this feature?
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.
https://openjdk.java.net/jeps/223 . 1.6
and 1.7
are the old formats of Java specification, we should not maintain them going forward imho
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, so why would we support them at all in this newly added feature?
Maven release failed due to the duplicate metadata file, but I see it for other Maven Plugins in Jenkins org as well. The release itself should be fine
|
@oleg-nenashev nope not duplicated! but you cannot write the maven-metadata.xml (error message says |
The patterns used for upload permissions from https://github.com/jenkins-infra/repository-permissions-updater/ do not allow for a package level metadata file. |
this is not an issue until we want to add new maven plugin. |
JENKINS-20679
Only tested manually, including with a patched plugins POM to see whether the compiler plugin configuration fallback works. Can rip that out and we only support property or HPI plugin configuration for this.
Downstream PRs are jenkins-infra/update-center2#164 and jenkinsci/jenkins#3016.
Upstream of: jenkinsci/plugin-pom#134
@jenkinsci/code-reviewers