-
-
Notifications
You must be signed in to change notification settings - Fork 251
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
Refactor how "vendor" configure arguments are generated #2073
Conversation
Signed-off-by: Austin Bailey <Austin.Bailey@ibm.com>
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.
Superficially looks like it'll be ok to me, but i'll let others make final judgements on the changes :-)
run tests |
What values do you get for an openj9 build? |
@lumpfish the only change from the existing code for jdk11u+ is that
After:
Somebody upstream decided that |
Thanks @austin0 , looks good. |
@austin0 I'm going to run tests again. Can you let me know if the results are what you expect? |
run tests |
🟢 PR TESTER RESULT 🟢✅ All pipelines passed! ✅ |
1 similar comment
🟢 PR TESTER RESULT 🟢✅ All pipelines passed! ✅ |
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 we should check the output carefully. Does PR Test Builder spit out Dragonwell or OpenJ9 builds?
@karianna I've manually checked OpenJ9 and it correctly matches other binaries, I haven't found a way to check dragonwell but I can't conceive any reason for it to break. It seems the dragonwell section will likely change anyway after a decision on what Vendor/Variant/Distribution means. |
The intention of this PR is to clean up and centralise how the
java.vendor.x
properties are set, and to do this in a way that is easily extensible for any future implementations. It also enables a number of properties for jdk8u that were previously only enabled for 9+. These include:I also intend for this to be a solution to #1387. The request made in the issue is to set
java.vendor.version
to the the source of the OpenJDK repo, this change has already been made to therelease
file (#2049) and is planned for the thejson
metadata file so seems redundant. I have instead set it to the name of the variant being built (Hotspot
,OpenJ9
,SAP
,Corretto
,Dragonwell
, etc). @brunoborges @lumpfish @sxa @gdams any comments?Java properties after PR:
Signed-off-by: Austin Bailey Austin.Bailey@ibm.com