-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
JDK-8304163: Move jdk.internal.module.ModuleInfoWriter to the test library #13085
Conversation
👋 Welcome back mchung! A progress list of the required criteria for merging this PR into |
@mlchung 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. |
Webrevs
|
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 changes look good to me
@mlchung 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 7 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 |
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.
Thanks for moving this to the test lib.
On the tag ordering, the reason some of these tests had @modules before @library was just to recommended ordering in the tag tag spec (https://openjdk.org/jtreg/tag-spec.html#ORDER).
Thanks for the pointer to the recommended ordering. Tests updated. |
* java.base/jdk.internal.module | ||
* @library /test/lib | ||
* @build jdk.test.lib.util.ModuleInfoWriter |
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.
You don't need to build library classes explicitly. I think @library /test/lib it enough.
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.
Hello @lmesnik, on the contrary, these build directives are recommended (and based on some of the issues we have encountered, are in fact necessary). The jtreg documentation has this to say https://openjdk.org/jtreg/tag-spec.html:
In general, classes in library directories are not automatically compiled as part of a compilation command explicitly naming the source files containing those classes. A test that relies upon library classes should contain appropriate @build directives to ensure that the classes will be compiled. It is strongly recommended that tests do not rely on the use of implicit compilation by the Java compiler. Such an approach is generally fragile, and may lead to incomplete recompilation when a test or library code has been modified.
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.
Explicit compilation is exactly the reason of adding @build
/integrate |
Going to push as commit 622f239.
Your commit was automatically rebased without conflicts. |
Sigh... And again we have the situation where some folks are adding Another recent PR removed library build directives: and that made the related tests stop failing with NoClassDefFoundErrors. This mess is related to: and these problems show up when doing parallel execution of tests We really, really need @jonathan-gibbons to chime in on review threads like these. |
ModuleInfoWriter
is not used by the runtime. Move it to the test library asjdk.test.lib.util.ModuleInfoWriter
. The tests are updated to use the test library instead.ModuleInfoWriter
depends onjdk.internal.module
types and the Classfile API. Hence@modules java.base/jdk.internal.classfile
and other classfile subpackages are added.Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/13085/head:pull/13085
$ git checkout pull/13085
Update a local copy of the PR:
$ git checkout pull/13085
$ git pull https://git.openjdk.org/jdk pull/13085/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 13085
View PR using the GUI difftool:
$ git pr show -t 13085
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/13085.diff