8281104: jar --create should create missing parent directories #7327

sormuras · 2022-02-02T20:21:54Z

Calling jar --create --file a/b/foo.jar INPUT-FILES should create missing parent directories (here a/b) on the default file system before storing the JAR file (here foo.jar) in the destination directory.

Progress

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

Issue

JDK-8281104: jar --create should create missing parent directories

Reviewers

Lance Andersen (@LanceAndersen - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/7327/head:pull/7327
$ git checkout pull/7327

Update a local copy of the PR:
$ git checkout pull/7327
$ git pull https://git.openjdk.java.net/jdk pull/7327/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 7327

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/7327.diff

bridgekeeper · 2022-02-02T20:23:25Z

👋 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 · 2022-02-02T20:24:52Z

@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 · 2022-02-03T16:27:35Z

Webrevs

LanceAndersen

Thank you for taking this on. Overall looks good.

A few comments below.

Also, we should update jar.properties to indicate that the directory path will be created as needed in the help section for jar.

Best,
Lance

LanceAndersen · 2022-02-03T18:01:11Z

test/jdk/tools/jar/CreateMissingParentDirectories.java

+import java.util.spi.ToolProvider;
+import java.util.stream.Stream;
+
+public class CreateMissingParentDirectories {


Understand why you used went this route for the test given some of the older tests use this framework. I might have gone the route of using TestNG as the newer tests (such as in MultiRelease) use it.

LanceAndersen · 2022-02-03T18:01:17Z

src/jdk.jartool/share/classes/sun/tools/jar/Main.java

+                    Path parent = target.getParent();
+                    if (parent != null) {
+                        Files.createDirectories(parent);
+                    }


Would be good to move the creation after validating the arguments as this would fail after creating the temp zip file. Be good to do via another PR

LanceAndersen · 2022-02-03T18:06:59Z

test/jdk/tools/jar/CreateMissingParentDirectories.java

+    }
+
+    private static void doHappyPathTest(Path jar, Path entry) throws Throwable {
+        String[] jarArgs = new String[]{"cf", jar.toString(), entry.toString()};


I might consider also using --create --file in addition to "cf" in a run for an extra sanity check

sormuras · 2022-02-03T19:42:25Z

Thanks for the review, Lance.

I didn't change order of creation, validation, and movement of the temporary JAR file in order to keep existing behaviour consistent.

A reason to use the main-based testing approach was also the ability to run the test via JEP 330:
java CreateMissingParentDirectories.java

Also, we should update jar.properties to indicate that the directory path will be created as needed in the help section for jar.

That's a good point. Where would we put that information?

Extend the descriptions of option c/--create and/or option f/--file?
As a post scriptum note below the options?
https://github.com/openjdk/jdk/blob/master/src/jdk.jartool/share/classes/sun/tools/jar/resources/jar.properties#L309-L317

LanceAndersen · 2022-02-03T21:43:30Z

Thanks for the review, Lance.

I didn't change order of creation, validation, and movement of the temporary JAR file in order to keep existing behaviour consistent.

I do think we are better served with the validation check earlier on. In the case of a failure, we do not make the tmp file name known so it could be easy to miss.

I think it makes sense to report the issue before erring out after creating the jar.. If the behavior stays as is, it would be best to document that the tmp file would still be there.

Let's see what others think. Either way we can accomplish the additional change if we reach consensus via a separate PR.

A reason to use the main-based testing approach was also the ability to run the test via JEP 330: java CreateMissingParentDirectories.java

For me personally, I would not use that means to run the tests as typically would from an IDE. Plus there is a benefit of leveraging the test framework in TestNG. I totally understand everyone has a personal preference. A discussion for another day I guess ;-).

Also, we should update jar.properties to indicate that the directory path will be created as needed in the help section for jar.

That's a good point. Where would we put that information?

Extend the descriptions of option c/--create and/or option f/--file?

I think this would be the appropriate place for documenting the behavior.

As a post scriptum note below the options?
https://github.com/openjdk/jdk/blob/master/src/jdk.jartool/share/classes/sun/tools/jar/resources/jar.properties#L309-L317

Also we will need to update the MD file which represents the jar man page via a separate PR.

sormuras · 2022-02-04T21:30:59Z

I think this would be the appropriate place for documenting the behavior.

Like this?

-c, --create               Create the archive. When the archiche file name specified
                           by -f, --file contains a path, missing parent directories
                           will also be created
...
-f, --file=FILE            The archive file name. When omitted, either stdin or
                           stdout is used based on the operation. Missing parent
                           directories of the file name path will be created

Perhaps, only adding it to -c, --create suffices. Having it also on -f, --file may confuse users, as this option is used all operation modes.

Also we will need to update the MD file which represents the jar man page via a separate PR.

Yes. Will create PR for this.

LanceAndersen · 2022-02-04T21:36:38Z

I think this would be the appropriate place for documenting the behavior.

Like this?
-c, --create               Create the archive. When the path specified by -f, --file
                           contains a path, missing parent directories will also be created
...
-f, --file=FILE            The archive file name. When omitted, either stdin or
                           stdout is used based on the operation. Missing parent
                           directories of the file name path will be created
Perhaps, only adding it to -c, --create suffices. Having it also on -f, --file may confuse users, as this option is used all operation modes.

I think just having the verbiage when creating the jar should suffice as if we were updating it, the path would need to exist already.

Also we will need to update the MD file which represents the jar man page via a separate PR.

Yes. Will create PR for this.

Great, thank you. Have a good weekend!

sormuras · 2022-02-04T21:47:16Z

Should I also extend this "usage.compat" help message block? When and where is it displayed? 🤔

jdk/src/jdk.jartool/share/classes/sun/tools/jar/resources/jar.properties

Lines 167 to 173 in 48523b0

    
           usage.compat=\ 
        
           \Compatibility Interface:\ 
        
           \n\ 
        
           Usage: jar {ctxui}[vfmn0PMe] [jar-file] [manifest-file] [entry-point] [-C dir] files] ...\n\ 
        
           Options:\n\ 
        
           \ \   -c  create new archive\n\ 
        
           \ \   -t  list table of contents for archive\n\

LanceAndersen

The current changes look good overall

openjdk · 2022-02-04T21:55:17Z

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

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

8281104: jar --create should create missing parent directories

Reviewed-by: lancea

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

7f19c70: 8281061: [s390] JFR runs into assertions while validating interpreter frames
380378c: 8281400: Remove unused wcslen() function
83d6745: 8281450: Remove unnecessary operator new and delete from ObjectMonitor
f5d8ceb: 8281296: Create a regression test for JDK-4515999
861f279: 8280917: Simplify G1ConcurrentRefineThread activation
f2a9627: 8279329: Remove hardcoded IPv4 available policy on Windows
4eacacb: 8281314: Rename Stack{Red,Yellow,Reserved,Shadow}Pages multipliers
2f71a6b: 8279613: JFR: Snippify Javadoc
8a66210: 6779701: Wrong defect ID in the code of test LocalRMIServerSocketFactoryTest.java
1dfc94d: 8281377: Remove vmTestbase/nsk/monitoring/ThreadMXBean/ThreadInfo/Deadlock/JavaDeadlock001/TestDescription.java from problemlist.
... and 45 more: https://git.openjdk.java.net/jdk/compare/4ea6037ea57ce7bbad00ef172dfc3c122b2317fc...master

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 (@LanceAndersen) 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).

LanceAndersen · 2022-02-04T22:09:00Z

Should I also extend this "usage.compat" help message block? When and where is it displayed? 🤔

jdk/src/jdk.jartool/share/classes/sun/tools/jar/resources/jar.properties

Lines 167 to 173 in 48523b0

usage.compat=\

\Compatibility Interface:\

\n\

Usage: jar {ctxui}[vfmn0PMe] [jar-file] [manifest-file] [entry-point] [-C dir] files] ...\n\

Options:\n\

\ \ -c create new archive\n\

\ \ -t list table of contents for archive\n\

I guess that makes sense. Not sure how often anyone looks at this but good for consistency.
Thanks for the reminder of the help message for compat

sormuras · 2022-02-05T20:46:59Z

/integrate

openjdk · 2022-02-05T20:48:28Z

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

vicente-romero-oracle · 2022-02-06T00:17:07Z

/label remove compiler

openjdk · 2022-02-06T00:18:22Z

@vicente-romero-oracle
The compiler label was successfully removed.

LanceAndersen · 2022-02-08T17:51:18Z

/sponsor

openjdk · 2022-02-08T17:53:59Z

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

7f19c70: 8281061: [s390] JFR runs into assertions while validating interpreter frames
380378c: 8281400: Remove unused wcslen() function
83d6745: 8281450: Remove unnecessary operator new and delete from ObjectMonitor
f5d8ceb: 8281296: Create a regression test for JDK-4515999
861f279: 8280917: Simplify G1ConcurrentRefineThread activation
f2a9627: 8279329: Remove hardcoded IPv4 available policy on Windows
4eacacb: 8281314: Rename Stack{Red,Yellow,Reserved,Shadow}Pages multipliers
2f71a6b: 8279613: JFR: Snippify Javadoc
8a66210: 6779701: Wrong defect ID in the code of test LocalRMIServerSocketFactoryTest.java
1dfc94d: 8281377: Remove vmTestbase/nsk/monitoring/ThreadMXBean/ThreadInfo/Deadlock/JavaDeadlock001/TestDescription.java from problemlist.
... and 45 more: https://git.openjdk.java.net/jdk/compare/4ea6037ea57ce7bbad00ef172dfc3c122b2317fc...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-02-08T17:54:17Z

@LanceAndersen @sormuras Pushed as commit 92f4f40.

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

8281104: jar --create should create missing parent directories

077e899

openjdk bot added core-libs core-libs-dev@openjdk.org compiler compiler-dev@openjdk.org labels Feb 2, 2022

sormuras added 4 commits February 3, 2022 10:31

8281104: Add happy-path and expected-to-fail tests

f436e6f

Use LF as line separator

db53828

Use provided error writer

0dfa0c0

Expect at least jar file's name in error message

f34e767

sormuras marked this pull request as ready for review February 3, 2022 16:21

openjdk bot added the rfr Pull request is ready for review label Feb 3, 2022

LanceAndersen reviewed Feb 3, 2022

View reviewed changes

Extend help text for option -c, --create

65105a3

LanceAndersen approved these changes Feb 4, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Feb 4, 2022

Update help text of Compatibility Interface

f810c81

openjdk bot added the sponsor Pull request is ready to be sponsored label Feb 5, 2022

openjdk bot removed the compiler compiler-dev@openjdk.org label Feb 6, 2022

LanceAndersen approved these changes Feb 8, 2022

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Feb 8, 2022

openjdk bot closed this Feb 8, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Feb 8, 2022

sormuras deleted the JDK-8281104-jar-create-missing-dirs branch August 17, 2022 07:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8281104: jar --create should create missing parent directories #7327

8281104: jar --create should create missing parent directories #7327

sormuras commented Feb 2, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Feb 2, 2022

openjdk bot commented Feb 2, 2022

mlbridge bot commented Feb 3, 2022 •

edited

Loading

LanceAndersen left a comment

LanceAndersen Feb 3, 2022

LanceAndersen Feb 3, 2022

LanceAndersen Feb 3, 2022

sormuras commented Feb 3, 2022

LanceAndersen commented Feb 3, 2022

sormuras commented Feb 4, 2022 •

edited

Loading

LanceAndersen commented Feb 4, 2022

sormuras commented Feb 4, 2022 •

edited

Loading

LanceAndersen left a comment

openjdk bot commented Feb 4, 2022 •

edited

Loading

LanceAndersen commented Feb 4, 2022

sormuras commented Feb 5, 2022

openjdk bot commented Feb 5, 2022

vicente-romero-oracle commented Feb 6, 2022

openjdk bot commented Feb 6, 2022

LanceAndersen commented Feb 8, 2022

openjdk bot commented Feb 8, 2022

openjdk bot commented Feb 8, 2022

8281104: jar --create should create missing parent directories #7327

8281104: jar --create should create missing parent directories #7327

Conversation

sormuras commented Feb 2, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Feb 2, 2022

openjdk bot commented Feb 2, 2022

mlbridge bot commented Feb 3, 2022 • edited Loading

Webrevs

LanceAndersen left a comment

Choose a reason for hiding this comment

LanceAndersen Feb 3, 2022

Choose a reason for hiding this comment

LanceAndersen Feb 3, 2022

Choose a reason for hiding this comment

LanceAndersen Feb 3, 2022

Choose a reason for hiding this comment

sormuras commented Feb 3, 2022

LanceAndersen commented Feb 3, 2022

sormuras commented Feb 4, 2022 • edited Loading

LanceAndersen commented Feb 4, 2022

sormuras commented Feb 4, 2022 • edited Loading

LanceAndersen left a comment

Choose a reason for hiding this comment

openjdk bot commented Feb 4, 2022 • edited Loading

LanceAndersen commented Feb 4, 2022

sormuras commented Feb 5, 2022

openjdk bot commented Feb 5, 2022

vicente-romero-oracle commented Feb 6, 2022

openjdk bot commented Feb 6, 2022

LanceAndersen commented Feb 8, 2022

openjdk bot commented Feb 8, 2022

openjdk bot commented Feb 8, 2022

sormuras commented Feb 2, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Feb 3, 2022 •

edited

Loading

sormuras commented Feb 4, 2022 •

edited

Loading

sormuras commented Feb 4, 2022 •

edited

Loading

openjdk bot commented Feb 4, 2022 •

edited

Loading