Re: RFR: 8281104: jar --create should create missing parent directories [v3]
On Fri, 4 Feb 2022 22:29:45 GMT, Christian Stein wrote: >> 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. > > Christian Stein has updated the pull request incrementally with one > additional commit since the last revision: > > Update help text of Compatibility Interface Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7327
Re: RFR: 8281104: jar --create should create missing parent directories [v3]
> 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. Christian Stein has updated the pull request incrementally with one additional commit since the last revision: Update help text of Compatibility Interface - Changes: - all: https://git.openjdk.java.net/jdk/pull/7327/files - new: https://git.openjdk.java.net/jdk/pull/7327/files/65105a36..f810c810 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7327=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7327=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7327.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7327/head:pull/7327 PR: https://git.openjdk.java.net/jdk/pull/7327
Re: RFR: 8281104: jar --create should create missing parent directories
On Fri, 4 Feb 2022 21:47:16 GMT, Christian Stein wrote: > Should I also extend this "usage.compat" help message block? When and where > is it displayed? 樂 > > https://github.com/openjdk/jdk/blob/48523b090886f7b24ed4009f0c150efaa6f7b056/src/jdk.jartool/share/classes/sun/tools/jar/resources/jar.properties#L167-L173 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 - PR: https://git.openjdk.java.net/jdk/pull/7327
Re: RFR: 8281104: jar --create should create missing parent directories
On Wed, 2 Feb 2022 20:21:54 GMT, Christian Stein wrote: > 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. Should I also extend this "usage.compat" help message block? When and where is it displayed? 樂 https://github.com/openjdk/jdk/blob/48523b090886f7b24ed4009f0c150efaa6f7b056/src/jdk.jartool/share/classes/sun/tools/jar/resources/jar.properties#L167-L173 - PR: https://git.openjdk.java.net/jdk/pull/7327
Re: RFR: 8281104: jar --create should create missing parent directories [v2]
> 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. Christian Stein has updated the pull request incrementally with one additional commit since the last revision: Extend help text for option `-c, --create` - Changes: - all: https://git.openjdk.java.net/jdk/pull/7327/files - new: https://git.openjdk.java.net/jdk/pull/7327/files/f34e767a..65105a36 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7327=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7327=00-01 Stats: 3 lines in 1 file changed: 2 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7327.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7327/head:pull/7327 PR: https://git.openjdk.java.net/jdk/pull/7327
Re: RFR: 8281104: jar --create should create missing parent directories [v2]
On Fri, 4 Feb 2022 21:55:28 GMT, Christian Stein wrote: >> 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. > > Christian Stein has updated the pull request incrementally with one > additional commit since the last revision: > > Extend help text for option `-c, --create` The current changes look good overall - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7327
Re: RFR: 8281104: jar --create should create missing parent directories
On Wed, 2 Feb 2022 20:21:54 GMT, Christian Stein wrote: > 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. > > 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=FILEThe 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! - PR: https://git.openjdk.java.net/jdk/pull/7327
Re: RFR: 8281104: jar --create should create missing parent directories
On Thu, 3 Feb 2022 21:43:30 GMT, Lance Andersen wrote: > 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=FILEThe 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. - PR: https://git.openjdk.java.net/jdk/pull/7327
Re: RFR: 8281104: jar --create should create missing parent directories
On Thu, 3 Feb 2022 19:42:25 GMT, Christian Stein wrote: > 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. - PR: https://git.openjdk.java.net/jdk/pull/7327
Re: RFR: 8281104: jar --create should create missing parent directories
On Wed, 2 Feb 2022 20:21:54 GMT, Christian Stein wrote: > 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. 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 - PR: https://git.openjdk.java.net/jdk/pull/7327
Re: RFR: 8281104: jar --create should create missing parent directories
On Wed, 2 Feb 2022 20:21:54 GMT, Christian Stein wrote:
> 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.
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
src/jdk.jartool/share/classes/sun/tools/jar/Main.java line 468:
> 466: if (parent != null) {
> 467: Files.createDirectories(parent);
> 468: }
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
test/jdk/tools/jar/CreateMissingParentDirectories.java line 38:
> 36: import java.util.stream.Stream;
> 37:
> 38: 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.
test/jdk/tools/jar/CreateMissingParentDirectories.java line 78:
> 76:
> 77: private static void doHappyPathTest(Path jar, Path entry) throws
> Throwable {
> 78: 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
-
PR: https://git.openjdk.java.net/jdk/pull/7327
RFR: 8281104: jar --create should create missing parent directories
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. - Commit messages: - Expect at least jar file's name in error message - Use provided error writer - Use LF as line separator - 8281104: Add happy-path and expected-to-fail tests - 8281104: jar --create should create missing parent directories Changes: https://git.openjdk.java.net/jdk/pull/7327/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7327=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8281104 Stats: 124 lines in 2 files changed: 119 ins; 0 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/7327.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7327/head:pull/7327 PR: https://git.openjdk.java.net/jdk/pull/7327
