-
Notifications
You must be signed in to change notification settings - Fork 6.2k
JDK-8304291: [AIX] Broken build after JDK-8301998 #13529
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
Conversation
|
👋 Welcome back mbaesken! A progress list of the required criteria for merging this PR into |
TheRealMDoerr
left a comment
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. Thanks for fixing the AIX build! I think we will need this fix for 11u and 17u because we will probably stick with xlc 16 for these releases.
|
@MBaesken This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 8 new commits pushed to the
Please see this link for an up-to-date comparison between the source branch of this pull request and the ➡️ To integrate this PR with the above commit message to the |
backwaterred
left a comment
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 really appreciate your work on this one @MBaesken! This fix looks good.
I agree with Martin that we will want this to make its way to jdk17 and 11. Let me know if you would like me to take point on that task.
prrace
left a comment
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.
As a rule, we don't make changes to 3rd party code principally for the reason that it needs to be re-changed every time we upgrade. After some off-line discussion with the devs working on the AIX port of OpenJDK, we can make an exception here, and they will take care of re-applying it if it is still needed next time round.
And these changes only affect AIX of course ..
|
Hi Tyler, Phil, Martin, thanks for the reviews ! /integrate |
|
Going to push as commit 310aa93.
Your commit was automatically rebased without conflicts. |
From upstream: * 8304291: [AIX] Broken build after JDK-8301998 * openjdk/jdk#13529 Signed-off-by: Keith W. Campbell <keithc@ca.ibm.com>
This is from openjdk/jdk#13529 and may come from jdk17 upstream at some point. Signed-off-by: Peter Shipton <Peter_Shipton@ca.ibm.com>
|
/help |
|
@adamfarley Available commands:
|
|
/backport jdk11u-dev |
|
@adamfarley the backport was successfully created on the branch adamfarley-backport-310aa934 in my personal fork of openjdk/jdk11u-dev. To create a pull request with this backport targeting openjdk/jdk11u-dev:master, just click the following link: The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:
If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jdk11u-dev: |
|
Note: I asked the bot for help because the documentation here, along with examples here and here, seem to imply that backport is not a valid command for closed PRs, so I wanted to know what my options were. I mean, options aside from the somewhat lengthier process detailed here, which I'll do if I must. |
|
Hi Adam , I do my backports from a commit , not a PR. Not sure if this works from a PR too, maybe it does, maybe not - at least the 'create pull request' link showed up so I guess you will find out if it works too from PRs . |
|
Thanks Mat. Seems to work from PRs, and the PR it created seems to be valid. I'll update the associated issue now. |
|
Not sure why previous committers had issues. Maybe this is new functionality. |
|
The |
After the latest harfbuzz update, the AIX build is broken. The old clang compiler from xlc16 does not compile harfbuzz correctly.
First issue in hb-algs.hh is that xlc16 clang still sets some GNUC-related macros, so we do not run into the
__clang_major__ >= 8check that should prevent to try to compile__builtin_mul_overflowwith ancient clang.The other issue in
hb-subset.ccis a bit tricky and has been observed as well on macOS when very old clang versions were used.Probably we can get rid of those 2 workarounds in some months after switching to xlc17 which includes a rather new clang version.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/13529/head:pull/13529$ git checkout pull/13529Update a local copy of the PR:
$ git checkout pull/13529$ git pull https://git.openjdk.org/jdk.git pull/13529/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 13529View PR using the GUI difftool:
$ git pr show -t 13529Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/13529.diff
Webrev
Link to Webrev Comment