Fixes #2114 - Add Shenandoah for Java 11 builds. #2125
Conversation
karianna
added
the
bug
There was a problem hiding this comment.
can you make these changes in build.sh? E.g https://github.com/AdoptOpenJDK/openjdk-build/blob/master/sbin/build.sh#L211
There was a problem hiding this comment.
The location for changes such as this is something we need to discuss really. I came across it when putting this section of the FAQ together as things are currently in one of three places. Which to use depends on who we want to be affected and to me knowledge we've avoided being clear on where changes should be made which has already lead to confusion in the past. Quick summary:
| Location | Impact |
|---|---|
| groovy files (as per this PR) | Only when run through our jenkins pipelines |
| platform-specific-configurations scripts | Those using build-farm/make-adopt-build-farm.sh (inc. our pipelines) - should be stuff specific to our machines |
| build.sh | Anyone (including end users) who are running makejdk-any-platform.sh |
So it depends what we want the last line of those to be. If it's to allow users to replicate the adopt builds with the same configuration options as us as far as possible, then I think it ought to be in build.sh but if we want it to be optional for users building it themselves then the jenkins pipelines aren't a bad choice. But we should really make it clear for new people to the project where changes should be made e.g. via an update to the FAQ.
To be clear, I'm ok with this going in as-is for now, or in build.sh (which is why this is a comment rather than an explicit approve/reject for now) but let's make sure we clarify for the future (I'll spin off another issue for it to ensure we get consensus - as any outcome from that should include tidying up where things are currently defined)
|
(Side note: Is this really a bug as opposed to an enhancement?) |
🟠 PR TESTER RESULT 🟠❎ Some pipelines failed or the job was aborted! ❎ |
karianna
added
enhancement
|
@gdams I've given that a go, I'm not sure if the whitespace changes my IDE enforced will mean I need to redo this PR, let me know. |
|
@sxa fixes in place. |
🟠 PR TESTER RESULT 🟠❎ Some pipelines failed or the job was aborted! ❎ |
|
run tests |
🟠 PR TESTER RESULT 🟠❎ Some pipelines failed or the job was aborted! ❎ |
|
run tests |
|
Can we do something to prevent accidental whitespace changes in the future? Terrible to review. |
🟢 PR TESTER RESULT 🟢✅ All pipelines passed! ✅ |
Yeah I need to change my IDE to 'not do that', apologies! |
karianna
dismissed
gdams’s stale review
Concerns addressed (changes now in build.sh)
🟠 PR TESTER RESULT 🟠❎ Some pipelines failed or the job was aborted! ❎ |
As Shenandoah is ready for production in 16, I'm assuming the flag is not required there.