8327460: Compile tests with the same visibility rules as product code #18135
Conversation
|
👋 Welcome back ihse! A progress list of the required criteria for merging this PR into |
|
@magicus The following labels will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command. |
openjdk
bot
added
build
Webrevs
|
|
Most of these changes is just putting the
|
|
@magicus 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 46 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. ➡️ To integrate this PR with the above commit message to the |
|
I have run the tests in tier1, tier2 and tier3 on the Oracle internal CI, which includes linux/x64, linux/aarch64, windows/x64, macosx/x64 and macosx/aarch64. I am currently running tier 4-10; this will take a while (and I won't integrate until this has finished running). When running multiple high-tier tests like this, the likelihood that I encounter intermittent problems increase. I will ignore test failures that seem likely to be intermittent and not caused by this patch. I might be overdoing the testing, but since this change affects all native tests, I want to be absolutely sure I don't break any tests that only execute as part of a high tier. |
This is now done. A handful of tests failed, but all are due to transient environmental problems, known errors, or seemingly intermittent product errors -- none are due to symbol visibility. So I'd argue that this is one of the most well-tested PRs posted ever. :-) |
|
@JornVernee Are you okay with this solution? No JNI in foreign tests. |
| @@ -21,9 +21,11 @@ | |||
| * questions. | |||
| */ | |||
|
|
|||
| #include "jni.h" | |||
| #include <stdio.h> | |||
There was a problem hiding this comment.
So what I did here was change:
#include "jni.h"
#include <stdio.h>
into
#include <stdio.h>
#include "export.h"
#include "jni.h"
The reordering was strictly not needed, but putting user includes in front of system ones looked like bad coding to me, and put me in a difficult spot in where to add the new #include "export.h" -- next to the current user include even if I thought that was wrong, or have the system includes "sandwitched" between two user includes.
Or do you mean that it seems unneeded to include jni.h at all? Yes, I agree, but it was there before, and I don't want to make any other changes to the tests. This change is scary enough as it is :-). If you want, I can file a follow-up to remove this include instead.
There was a problem hiding this comment.
Yes I meant the include is actually unnecessary, but fine to not make that change here.
| #include <stdio.h> | ||
|
|
||
| #include "export.h" | ||
| #include "jni.h" |
It's a Linux/macOS specific test so it wasn't needed. |
Yeah. I understand. I just wanted to give an explanation for why this particular test needed changes that were not present elsewhere (since other exported methods all were tested on Windows). |
Thanks! |
|
/integrate |
|
Going to push as commit cc9a8ab.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
Currently, our symbol visibility handling for tests are sloppy; we only handle it properly on Windows. We need to bring it up to the same levels as product code. This is a prerequisite for JDK-8327045, which in turn is a building block for Hermetic Java.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/18135/head:pull/18135$ git checkout pull/18135Update a local copy of the PR:
$ git checkout pull/18135$ git pull https://git.openjdk.org/jdk.git pull/18135/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 18135View PR using the GUI difftool:
$ git pr show -t 18135Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/18135.diff
Webrev
Link to Webrev Comment