8335912: Add an operation mode to the jar command when extracting to not overwriting existing files

[alexeybakhtin]

Not clean backport from JDK11 patch:

• No GNUStyle options in JDK8
• No support for long options
• Tests updated to use JDK8 API
• Removed subtest for long option

JTREG tests passed

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change requires CSR request JDK-8344651 to be approved
• JDK-8335912 needs maintainer approval

Issues

• JDK-8335912: Add an operation mode to the jar command when extracting to not overwriting existing files (Enhancement - P5 - Approved)
• JDK-8344651: Add an operation mode to the jar command when extracting to not overwriting existing files (CSR)

Reviewers

• Martin Balao (@martinuy - Reviewer)
• Andrew John Hughes (@gnu-andrew - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk8u-dev.git pull/608/head:pull/608
$ git checkout pull/608

Update a local copy of the PR:
$ git checkout pull/608
$ git pull https://git.openjdk.org/jdk8u-dev.git pull/608/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 608

View PR using the GUI difftool:
$ git pr show -t 608

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk8u-dev/pull/608.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back abakhtin! 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]

@alexeybakhtin This change now passes all automated pre-integration checks.

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

8335912: Add an operation mode to the jar command when extracting to not overwriting existing files

Reviewed-by: mbalao, andrew

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 5 new commits pushed to the master branch:

• d407d63: 8345504: Bump update version of OpenJDK: 8u452
• 2931b1d: 8068305: [TEST_BUG] Test java/awt/Mixing/HWDisappear.java fails with GTKL&F
• 10122b5: 8268457: XML Transformer outputs Unicode supplementary character incorrectly to HTML
• 00d2542: 8326110: [8u] The Marlin tests should be updated after JDK-8241307
• 9d330da: 8037013: [TESTBUG] Fix test/java/lang/ClassLoader/Assert.sh on AIX

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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[openjdk]

This backport pull request has now been updated with issue from the original commit.

[mlbridge]

Webrevs

• 03: Full - Incremental (a99b5b5e)
• 02: Full - Incremental (bdb6cf46)
• 01: Full - Incremental (538f97fe)
• 00: Full (c644d77a)

[martinuy]

Thanks for this contribution.

One minor comment regarding the text "-k/k" in the warning message. Passing the argument as a standalone "-k" seems to work only with GNUStyleOptions. We can pass "-k..." (along with other options), but the most typical invocation would be "-xk...". I'm okay with the suggested "-k/k" text but would have been okay with with "k" too.

I have the impression that the addition of "main.help.opt.extract.keep-old-files" is related to GNUStyleOptions. The place where "-k Do not overwrite existing files" was added is not the exact equivalent, but we should be able to live with that. Just wonder if we should add "k" to "[vfmn0PMe]". What do you think?

The test "testGnuOptionsKeptOldFile" does not apply, does it? The leading "-" is optional for the non-Gnu interface.

[alexeybakhtin]

Hello @martinuy
Thank you for review

One minor comment regarding the text "-k/k" in the warning message. Passing the argument as a standalone "-k" seems to work only with GNUStyleOptions. We can pass "-k..." (along with other options), but the most typical invocation would be "-xk...". I'm okay with the suggested "-k/k" text but would have been okay with with "k" too.

You are right. "-k" is not used separately, so I have updated the warning message

I have the impression that the addition of "main.help.opt.extract.keep-old-files" is related to GNUStyleOptions. The place where "-k Do not overwrite existing files" was added is not the exact equivalent, but we should be able to live with that. Just wonder if we should add "k" to "[vfmn0PMe]". What do you think?

Good catch. Thank you. It should be added as soon as it is main usage string for JDK8

The test "testGnuOptionsKeptOldFile" does not apply, does it? The leading "-" is optional for the non-Gnu interface.

Right. "testGnuOptionsKeptOldFile" does not apply for JDK8

[martinuy]

@alexeybakhtin , thanks for considering the suggested changes.

Looks like jar_pt_BR.properties was missed, if we really want to update all jar...properties. Changes to jar...properties make me think that the new option will not be documented for languages other than English. Would be very tricky to do the translation for all languages and perhaps is acceptable not to do it, as it has not been done for JDK main line either. While the spirit in 8u was different, it's not unreasonable for developer tools to be in English only. I'd suggest to update jar_pt_BR.properties and leave it there. The reader will probably fall back to English for an explanation of the (undocumented) k. If people really want to do the translation, I can contribute Spanish and Portuguese.

What do you think?

[alexeybakhtin]

@alexeybakhtin , thanks for considering the suggested changes.

Looks like jar_pt_BR.properties was missed, if we really want to update all jar...properties. Changes to jar...properties make me think that the new option will not be documented for languages other than English. Would be very tricky to do the translation for all languages and perhaps is acceptable not to do it, as it has not been done for JDK main line either. While the spirit in 8u was different, it's not unreasonable for developer tools to be in English only. I'd suggest to update jar_pt_BR.properties and leave it there. The reader will probably fall back to English for an explanation of the (undocumented) k. If people really want to do the translation, I can contribute Spanish and Portuguese.

What do you think?

Thank you @martinuy
Added the 'k' option to jar_pt_BR.properties. Translation for new and missed options can be done separately.

[openjdk]

⚠️ @alexeybakhtin This change is now ready for you to apply for maintainer approval. This can be done directly in each associated issue or by using the /approval command.

[gnu-andrew]

The change looks good to me. Martin has already covered the same comments I would have had about the initial version. I note that the simple warn method is pulled in from JDK-8164805, which is fine because we hardly want to backport a change to do with modular JARs to 8u.

I appreciate you considering the translated property files, as I think there's a building problem there in update releases. I'll say more on this in reply to Martin's comment.

[gnu-andrew]

@alexeybakhtin , thanks for considering the suggested changes.

Looks like jar_pt_BR.properties was missed, if we really want to update all jar...properties. Changes to jar...properties make me think that the new option will not be documented for languages other than English. Would be very tricky to do the translation for all languages and perhaps is acceptable not to do it, as it has not been done for JDK main line either. While the spirit in 8u was different, it's not unreasonable for developer tools to be in English only. I'd suggest to update jar_pt_BR.properties and leave it there. The reader will probably fall back to English for an explanation of the (undocumented) k. If people really want to do the translation, I can contribute Spanish and Portuguese.

What do you think?

They do tend to get documented, but in separate updates from the i18n team. The last one is JDK-8337054 for JDK 23. I suspect we'll get another mass update for 24 when it is released, which will include these.

There is an issue with backport releases that we don't get these updates when Oracle cede ownership of them. The last one for 8u is 8214357. We likely need to do a check for where the base properties file has keys that the translations do not, but content changes to existing keys will be harder to spot.

Contributing translations for this to trunk might be a good way to open conversation on this, if you have the time.

[gnu-andrew]

/approve yes

[openjdk]

@gnu-andrew
8335912: The approval request has been approved.

[alexeybakhtin]

/integrate

[openjdk]

Going to push as commit 5a4b440.
Since your change was applied there have been 5 commits pushed to the master branch:

• d407d63: 8345504: Bump update version of OpenJDK: 8u452
• 2931b1d: 8068305: [TEST_BUG] Test java/awt/Mixing/HWDisappear.java fails with GTKL&F
• 10122b5: 8268457: XML Transformer outputs Unicode supplementary character incorrectly to HTML
• 00d2542: 8326110: [8u] The Marlin tests should be updated after JDK-8241307
• 9d330da: 8037013: [TESTBUG] Fix test/java/lang/ClassLoader/Assert.sh on AIX

Your commit was automatically rebased without conflicts.

[openjdk]

@alexeybakhtin Pushed as commit 5a4b440.

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

[sendaoYan]

The two newly added tests fails.

Tests:

tools/jar/ExtractFilesTest.java
tools/jar/MultipleManifestTest.java

Test log snippet:

/tmp/tone/run/jtreg/jdk-repo/jdk/test/tools/jar/ExtractFilesTest.java:38: error: package org.junit.jupiter.api does not exist
import org.junit.jupiter.api.TestInstance;
                            ^
/tmp/tone/run/jtreg/jdk-repo/jdk/test/tools/jar/ExtractFilesTest.java:39: error: package org.junit.jupiter.api.TestInstance does not exist
import org.junit.jupiter.api.TestInstance.Lifecycle;
                                         ^
/tmp/tone/run/jtreg/jdk-repo/jdk/test/tools/jar/ExtractFilesTest.java:54: error: cannot find symbol
@TestInstance(Lifecycle.PER_CLASS)
 ^
  symbol: class TestInstance

[sendaoYan]

The minimum jtreg version of jdk11u-dev is 7.3.1+1, the minimum jtreg version of jdk8u-dev seems to be 5.1. So these two tests can't backport from jdk11u-dev to jdk8u-dev directively. The org.junit.jupiter.api does not exist in jtreg5.1

[alexeybakhtin]

The minimum jtreg version of jdk11u-dev is 7.3.1+1, the minimum jtreg version of jdk8u-dev seems to be 5.1. So these two tests can't backport from jdk11u-dev to jdk8u-dev directively. The org.junit.jupiter.api does not exist in jtreg5.1

Sorry for breaking this compatibility, but is there any document that defines the minimum jtreg version for jdk8u-dev?

[sendaoYan]

is there any document that defines the minimum jtreg version for jdk8u-dev?

jdk8u-dev:

https://github.com/openjdk/jdk8u-dev/blob/master/make/conf/test-dependencies#L29

jdk11u-dev:

https://github.com/openjdk/jdk11u-dev/blob/master/make/conf/github-actions.conf#L29

or:

https://github.com/openjdk/jdk11u-dev/blob/master/test/jdk/TEST.ROOT#L65

[alexeybakhtin]

Thank you @sendaoYan,

Could you suggest how we should deal with these tests? Should I submit an issue to remove these tests OR add them to the ProblemList OR update jtreg requirements?

[sendaoYan]

Could you suggest how we should deal with these tests? Should I submit an issue to remove these tests OR add them to the ProblemList OR update jtreg requirements?

Maybe there are some tests in jdk8u-dev depends jtreg5.1, so I think update jtreg version requirements maybe not a good idea.
I think the better way is resolve the dependencies of jtreg7.3.1 for these tests, similar to openjdk/jdk11u-dev#2553

[alexeybakhtin]

@sendaoYan ,

Thank you for the finding one more time. Please have a look at the proposed fix at #612