-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8349099: java/awt/Headless/HeadlessMalfunctionTest.java fails on CI with Compilation error #23852
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 Karm! A progress list of the required criteria for merging this PR into |
|
@Karm 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 52 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@jerboaa, @prrace, @aivanov-jdk) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
|
@jerboaa FYI |
Webrevs
|
jerboaa
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 can confirm the test compiles and runs fine. I've reverted JDK-8336382 locally - product code only - and verified the test fails as expected. So +1 on the fix.
There is pre-existing code in the test harness, which can get removed now --add-opens java.base/jdk.internal.org.objectweb.asm=ALL-UNNAMED.
|
Once bitten and all that .. I'd like to run this through our internal test system before it is pushed. |
|
Thanks. |
|
The test fails on Windows and macOS. In other words it passes only on Linux. stdout: [Transforming java.awt.GraphicsEnvironment. java.lang.RuntimeException: 'FATAL ERROR in native method: GetStaticMethodID isHeadless failed' missing from stdout/stderr JavaTest Message: Test threw exception: java.lang.RuntimeException: 'FATAL ERROR in native method: GetStaticMethodID isHeadless failed' missing from stdout/stderr STATUS:Failed.`main' threw exception: java.lang.RuntimeException: 'FATAL ERROR in native method: GetStaticMethodID isHeadless failed' missing from stdout/stderr I see you did not remove the add-opens for "java.base/jdk.internal.org.objectweb.asm=ALL-UNNAMED" |
|
I've got my aarch64 MacMini building it and I can see that too. Gonna wire it to ci before future contributions. I realxed the output matching in the test as the log comes from a different code path on MacOS. Gonna take a look at Windows too... |
|
@Karm Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information. |
|
I am running the Windows, Mac and Linux CI now, will post links to test results when it completes... |
|
I ran your tests, and now windows and mac pass, but Linux now fails |
Oh. I see you just updated to fix this a few hours ago. I'll re-run. |
|
Given the different code path MacOS and Windows take to initialize the affected isHeadless part, I propose excluding Windows and MacOS from the test. Mere relaxing the test, parsing only for "isHeadless" string in the error message, makes little sense, because such test passes both on Windows and MacOS regardless of the JDK-8336382 #20169 patch. The current state of the PR passes
@prrace I will keep using this CI I established for my future client libs contributions, to aviod platform surprises... |
|
Excluding on windows and mac should be OK |
|
I'll hold off re-running the tests until you have updated the PR with the exclusions. |
| * @summary Test that in absence of isHeadless method, the JDK throws a meaningful error message. | ||
| * @library /test/lib | ||
| * @modules java.base/jdk.internal.org.objectweb.asm | ||
| * @requires os.family == "linux" |
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'll hold off re-running the tests until you have updated the PR with the exclusions.
Doesn't this bit do the exclusion? I found out about it looking at other tests...
My Windows says:
Test results: passed: 14; did not meet platform requirements: 1
Report written to C:\workspace\workspace\mandrel-jdk-build-matrix\LABEL\w2k19\jdk\jtreg_results\jdk\JTreport\html\report.html
Results written to C:\workspace\workspace\mandrel-jdk-build-matrix\LABEL\w2k19\jdk\jtreg_results\jdk\JTwork
and MacOS also:
Test results: passed: 14; did not meet platform requirements: 1
Report written to /Users/tester/jenkins/workspace/mandrel-jdk-build-matrix/LABEL/macos_aarch64/jdk/jtreg_results/jdk/JTreport/html/report.html
Results written to /Users/tester/jenkins/workspace/mandrel-jdk-build-matrix/LABEL/macos_aarch64/jdk/jtreg_results/jdk/JTwork
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.
yes it does, I thought you had not done that yet.
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.
testing looks OK
aivanov-jdk
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.
The initial year in the copyright should be preserved; and a second one is to be added if there's no second year, or the second year needs to be bumped to the current year.
Other than that, it looks good.
|
Thanks for the review jerboaa, prrace. @aivanov-jdk thx, amended the headers |
|
/integrate |
|
/sponsor |
|
Going to push as commit 1dd9cf1.
Your commit was automatically rebased without conflicts. |
|
@aivanov-jdk @Karm Pushed as commit 1dd9cf1. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Removed objectweb.asm for bytecode manipulation and uses JEP 484 classfile API.
Test passes on Linux amd64 so far:
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/23852/head:pull/23852$ git checkout pull/23852Update a local copy of the PR:
$ git checkout pull/23852$ git pull https://git.openjdk.org/jdk.git pull/23852/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 23852View PR using the GUI difftool:
$ git pr show -t 23852Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/23852.diff
Using Webrev
Link to Webrev Comment