8253761: Wrong URI syntax printed by jar --describe-module

[sormuras]

This commit replaces "/!" with "!/" in order to generate
valid URIs when printing a module description of a modular
JAR file. A valid URI is described here: [1]

https://bugs.openjdk.java.net/browse/JDK-8253761

[1] https://docs.oracle.com/en/java/javase/15/docs/api/java.base/java/net/JarURLConnection.html

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Issue

• JDK-8253761: Wrong URI syntax printed by jar --describe-module

Reviewers

• Alan Bateman (@AlanBateman - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jdk pull/393/head:pull/393
$ git checkout pull/393

[bridgekeeper]

👋 Welcome back cstein! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@sormuras The following labels will be automatically applied to this pull request:

• compiler
• core-libs

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.

[mlbridge]

Webrevs

• 03: Full - Incremental (49ee4d2)
• 02: Full (b8ead33a8af55cedaaa540d6be3734d34665e9b4)
• 01: Full - Incremental (acbde567c981e539586b7329535c0129828e48df)
• 00: Full (7df9f472db449806b11a4b769cd2a0582bb70824)

[AlanBateman]

/labels remove compiler

[openjdk]

@AlanBateman Unknown command labels - for a list of valid commands use /help.

[AlanBateman]

The change looks good.

test/jdk/tools/jar/modularJar/Basic.java has tests for jar --describe-module and maybe we could add a test to this to make sure that it outputs the expected JAR URL.

[sormuras]

/label remove compiler

[openjdk]

@sormuras
The compiler label was successfully removed.

[sormuras]

test/jdk/tools/jar/modularJar/Basic.java has tests for jar --describe-module and maybe we could add a test to this to make sure that it outputs the expected JAR URL.

Tests are always good to have.

Should I extend the existing @Test public void describeModuleFoo() ... method or should I add a new method that ensures the expected URL is emitted?

[AlanBateman]

A new assert in the describeModuleFoo test would be great, thank you!

[sormuras]

How do I trigger a "pre-submit testing using GitHub Actions workflow"[1] to be started for this PR?

[rwestberg]

The workflow definition file needs to be present in your branch, but it seems to be a bit too old (This branch is 2 commits ahead, 85 commits behind openjdk:master.). So simply merge jdk/master into your branch and they should be triggered.

[sormuras]

Rebased and squashed commits. Let's see...

They're running: https://github.com/sormuras/jdk/actions/runs/289171488

[AlanBateman]

I think the change exposes issues in a couple of tests. It looks like partialUpdateFooModuleInfo (modularJar/Basic.java) expects the wrong result, as does some of the tests in mmrjar/Basic.java.

[sormuras]

I can't find the modularJar/Basic.java test run in the logs. In which job/tier/part does it hide?

I think the change exposes issues in a couple of tests.

The one that fails the linked "pre-submit test" run above is already reported as https://bugs.openjdk.java.net/browse/JDK-8249095 ("Aux.java" is not a valid file name on Windows).

It looks like partialUpdateFooModuleInfo (modularJar/Basic.java) expects the wrong result, as does some of the tests in mmrjar/Basic.java.

Something for a follow-up issue/PR?

[AlanBateman]

The tests are test/jdk/tools/jar/modularJar/Basic.java and test/jdk/tools/jar/mmrjar/Basic.java. I've verified that they will fail with the update to the jar tool so we'll need to change this as part of this PR.

The tests are in the core_tools test group, which seems to be tier2. I suspect the pre-submit job is running tier1.

[sormuras]

I replaced all occurances of "/!" with the correct "!/" delimiter in both test files.

Hoping that I touched every failing assertion, as I am still struggling to set up a working JDK build environment locally. On Windows...

[openjdk]

@sormuras 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 more details.

After integration, the commit message for the final commit will be:

8253761: Wrong URI syntax printed by jar --describe-module

Reviewed-by: alanb

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 master branch:

• b29e108: 8253944: Certain method references to VarHandle methods should fail
• 88d75c9: 8156071: List.of: reduce array copying during creation
• ea27a54: 8224509: Incorrect alignment in CDS related allocation code on 32-bit platforms
• 4d29116: 8253433: Remove -XX:+Debugging product option
• 81dae70: 8253948: Memory leak in ImageFileReader
• 65cab55: 8253971: ZGC: Flush mark stacks after processing concurrent roots
• 19219a9: 8253960: Memory leak in Java_java_lang_ClassLoader_defineClass0()
• 5d4a135: 8253842: [JVMCI] Allow implicit exception to dispatch to other address in jvmci compilers.

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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 (@AlanBateman) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[sormuras]

/integrate

[openjdk]

@sormuras
Your change (at version 49ee4d2) is now ready to be sponsored by a Committer.

[AlanBateman]

/sponsor

[openjdk]

@AlanBateman @sormuras Since your change was applied there have been 8 commits pushed to the master branch:

• b29e108: 8253944: Certain method references to VarHandle methods should fail
• 88d75c9: 8156071: List.of: reduce array copying during creation
• ea27a54: 8224509: Incorrect alignment in CDS related allocation code on 32-bit platforms
• 4d29116: 8253433: Remove -XX:+Debugging product option
• 81dae70: 8253948: Memory leak in ImageFileReader
• 65cab55: 8253971: ZGC: Flush mark stacks after processing concurrent roots
• 19219a9: 8253960: Memory leak in Java_java_lang_ClassLoader_defineClass0()
• 5d4a135: 8253842: [JVMCI] Allow implicit exception to dispatch to other address in jvmci compilers.

Your commit was automatically rebased without conflicts.

Pushed as commit f2f77f7.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.