8173970: jar tool should have a way to extract to a directory #2752
Conversation
|
👋 Welcome back jpai! A progress list of the required criteria for merging this PR into |
openjdk
bot
added
core-libs
Webrevs
|
|
/csr |
|
@jonathan-gibbons has indicated that a compatibility and specification (CSR) request is needed for this pull request. |
|
Mailing list message from Lance Andersen on compiler-dev: Hi Jaikiran, Thank you for the proposed patch. Assuming there is consensus to add support for this enhancement, I think we need to discuss what is the correct option. The jar tool borrows -C from tar for creating/updating a jar and the -C option is also a valid option when extracting files from a tar file. Perhaps keeping symmetry with tar and extend support for -C when extracting a jar file would be a better way forward. Let?s give time for additional input. I believe this would also warrant a CSR to be created as well as updates to the jar man page. Best p.s. I think it would be useful in the future to start the discussion on core-libs-dev prior to creating a PR (or leave it as a draft PR) for a feature request. On Feb 26, 2021, at 12:08 PM, Jaikiran Pai <jpai at openjdk.java.net<mailto:jpai at openjdk.java.net>> wrote: Can I please get a review for this patch which proposes to implement the enhancement request noted in https://bugs.openjdk.java.net/browse/JDK-8173970? The commit in this PR introduces the `-o` and `--output-dir` option to the `jar` command. The option takes a path to a destination directory as a value and extracts the contents of the jar into that directory. This is an optional option and the changes in the commit continue to maintain backward compatibility where the jar is extracted into current directory, if no `-o` or `--output-dir` option has been specified. As far as I know, there hasn't been any discussion on what the name of this new option should be. I was initially thinking of using `-d` but that is currently used by the `jar` command for a different purpose. So I decided to use `-o` and `--output-dir`. This is of course open for change depending on any suggestions in this PR. The commit in this PR also updates the `jar.properties` file which contains the English version of the jar command's `--help` output. However, no changes have been done to the internationalization files corresponding to this one (for example: `jar_de.properties`), because I don't know what process needs to be followed to have those files updated (if at all they need to be updated). The commit also includes a jtreg testcase which verifies the usage of this new option. ------------- Commit messages: Changes: https://git.openjdk.java.net/jdk/pull/2752/files PR: https://git.openjdk.java.net/jdk/pull/2752 [cid:E1C4E2F0-ECD0-4C9D-ADB4-B16CA7BCB7FC at home] Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 -------------- next part -------------- |
|
Mailing list message from Jaikiran Pai on compiler-dev: Hello Lance, On 27/02/21 1:17 am, Lance Andersen wrote:
Thank you for that input, I'll keep that in mind for any similar work in -Jaikiran |
|
Mailing list message from Jaikiran Pai on compiler-dev: On 27/02/21 1:17 am, Lance Andersen wrote:
I haven't created a CSR before, so I will need some guidance on that -Jaikiran |
|
Mailing list message from Alan Bateman on compiler-dev: On 26/02/2021 19:47, Lance Andersen wrote:
I created JDK-8173970 a few years ago so happy it it getting some attention. Yes, the option name will need to be agreed. It would be useful to There are other discussion points around the behavior when the target Yes, a CSR will be needed. -Alan |
|
Mailing list message from Lance Andersen on compiler-dev: HI Jaikiran, I am more than happy to work with you through the CSR process Lets get the discussion going regarding possible option names on core-libs-dev and then we can move forward :-) Have a great weekend On Feb 26, 2021, at 9:03 PM, Jaikiran Pai <jai.forums2013 at gmail.com<mailto:jai.forums2013 at gmail.com>> wrote: On 27/02/21 1:17 am, Lance Andersen wrote: I believe this would also warrant a CSR to be created as well as updates to the jar man page. I haven't created a CSR before, so I will need some guidance on that part. Is it usually created after all the implementation details have been decided upon or should it be created now? -Jaikiran [cid:E1C4E2F0-ECD0-4C9D-ADB4-B16CA7BCB7FC at home] Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 -------------- next part -------------- |
|
Mailing list message from Jaikiran Pai on compiler-dev: Hello Alan, On 27/02/21 2:23 pm, Alan Bateman wrote:
I had a look at both tar and unzip commands on MacOS and Linux (CentOS) -------------- tar --version The version of this tool has: -C directory, --cd directory, --directory directory A command like "tar -xzf foo.tar.gz -C /tmp/bar/" works fine and -------------- tar --version This version of the tool has: Common options: Although the wording isn't clear that, when used with -x, it extracts to Specifically, a command like "tar -xzf foo.tar.gz -C /tmp/bar/" works ------------------------------- unzip -v This version of the tool has: [-d exdir] unzip foo.zip -d /tmp/bar/ works fine and extracts the foo.zip from --------------- The jimage and jmod as you note use the --dir option for extracting to Those were the tools I looked at. I think using the -d option with -x As for using --dir for this new feature, I don't think it alone will be I think reusing the -C option, for this new feature, perhaps is a good -C dir ????????????? jar uf my.jar -C classes Bar.class Using the -C option would indeed align it with the tar command. For the So I think the combination of -C (short form) and --dir (long form)
I'm guessing you mean the behaviour of creating a directory (or a unzip foo.zip -d /tmp/blah/ and if "blah/" isn't a directory inside /tmp/ directory, then it creates However, unzip foo.zip -d /tmp/blah/hello/ and if "blah/" isn't a directory inside /tmp/ directory, then this Coming to the jimage and the jmod commands, both these commands create jimage extract --dir /tmp/blah/foo/bar/ jdkmodules will create the blah/foo/bar/ directory hierarchy if blah doesn't exist From the user point of view, I think this behaviour of creating the One another minor detail, while we are at this, is that, IMO we should -Jaikiran |
|
Mailing list message from Lance Andersen on compiler-dev: Hi Jaikiran, Thank you for this. I went through this myself yesterday in addition to what you have below here are a few more: 7zip: -o Thinking about this some more, I might suggest supporting -C Best On Feb 27, 2021, at 11:19 PM, Jaikiran Pai <jai.forums2013 at gmail.com<mailto:jai.forums2013 at gmail.com>> wrote: Hello Alan, On 27/02/21 2:23 pm, Alan Bateman wrote: Yes, the option name will need to be agreed. It would be useful to enumerate the options that the other tools are using to specify the location where to extract. If you see JBS issues mentioning tar -C not supporting chdir when extracting then it might be Solaris tar, which isn't the same as GNU tar which has different options. It might be better to look at more main stream tools, like unzip although jar -d is already taken. It would be nice if there were some consistency with other tools in the JDK that doing extracting (The jmod and jimage extract commands use use --dir for example). I had a look at both tar and unzip commands on MacOS and Linux (CentOS) setup that I had access to. -------------- tar --version The version of this tool has: -C directory, --cd directory, --directory directory A command like "tar -xzf foo.tar.gz -C /tmp/bar/" works fine and extracts the foo.tar.gz from current directory to a target directory /tmp/bar/ -------------- tar --version This version of the tool has: Common options: Although the wording isn't clear that, when used with -x, it extracts to the directory specified in -C, it does indeed behave that way. Specifically, a command like "tar -xzf foo.tar.gz -C /tmp/bar/" works fine and extracts the foo.tar.gz from current directory to a target directory /tmp/bar/ ------------------------------- unzip -v This version of the tool has: [-d exdir] unzip foo.zip -d /tmp/bar/ works fine and extracts the foo.zip from current directory to /tmp/bar/ --------------- The jimage and jmod as you note use the --dir option for extracting to that specified directory. Those were the tools I looked at. I think using the -d option with -x for the jar command is ruled out since it already is used for a different purpose, although for a different "main" operation of the jar command. As for using --dir for this new feature, I don't think it alone will be enough. Specifically, I couldn't find a "short form" option for the --dir option in the jimage or jmod commands. For the jar extract feature that we are discussing here, I think having a short form option (in addition to the longer form) is necessary to have it match the usage expectations of similar other options that the jar command exposes. So even if we do choose --dir as the long form option, we would still need a short form for it and since -d is already taken for something else, we would still need to come up with a different one. The short form of this option could be -C (see below). I think reusing the -C option, for this new feature, perhaps is a good thing. The -C is currently used by the update and create "main" operation of the jar command and the man page for this option states: -C dir jar uf my.jar -C classes Bar.class Using the -C option would indeed align it with the tar command. For the "long form" of this option, the tar command (both on MacOS and CentOS) uses --directory. For this jar extract feature though, we could perhaps just use --dir to have it align with the jimage and the jmod tools. So I think the combination of -C (short form) and --dir (long form) would perhaps be suitable for this feature. There are other discussion points around the behavior when the target directory exists or does not exist, to ensure there is some consistency with main stream tools. I'm guessing you mean the behaviour of creating a directory (or a hierarchy of directories) if the target directory is not present? My testing with the tar tool (both on MacOS and CentOS) shows that if the specified target directory doesn't exist, then the extract fails. The tar extract command doesn't create the target directory during extract. On the other hand, the unzip tool, does create the directory if it doesn't exist. However, interestingly, the unzip tool creates only one level of that directory if it doesn't exist. Specifically, if you specify: unzip foo.zip -d /tmp/blah/ and if "blah/" isn't a directory inside /tmp/ directory, then it creates the "blah/" directory inside /tmp/ and then extracts the contents of the zip into it. However, unzip foo.zip -d /tmp/blah/hello/ and if "blah/" isn't a directory inside /tmp/ directory, then this command fails with an error and it doesn't create the hierarchy of the target directories. Coming to the jimage and the jmod commands, both these commands create the entire directory hierarchy if the target directory specified during extract, using --dir, doesn't exist. So a command like: jimage extract --dir /tmp/blah/foo/bar/ jdkmodules will create the blah/foo/bar/ directory hierarchy if blah doesn't exist in /tmp/, while extracting the "jdkmodules" image.From the user point of view, I think this behaviour of creating the directories if the target directory doesn't exist, is probably the most intuitive and useful and if we did decide to use this approach for this new option for jar extract command, then it would align with what we already do in jimage and jmod commands. One another minor detail, while we are at this, is that, IMO we should let the jar extract command to continue to behave the way it currently does when it comes to overwriting existing files. If the jar being extracted contains a file by the same name, in the target directory (hierarchy) then it should continue to overwrite that file. In other words, I don't think we should change the way the jar extract command currently behaves where it overwrites existing files when extracting. -Jaikiran [cid:E1C4E2F0-ECD0-4C9D-ADB4-B16CA7BCB7FC at home] Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 -------------- next part -------------- |
|
Mailing list message from Jaikiran Pai on compiler-dev: Thank you Lance. So I think there's some level of agreement on using -C Anymore opinion on the directory creation semantics and any other -Jaikiran On 28/02/21 5:38 pm, Lance Andersen wrote:
-------------- next part -------------- |
|
Mailing list message from Lance Andersen on compiler-dev: Hi Jaikiran, It should not be too difficult to support the options listed below via GNUStyleOptions. Some other things needed to be defined and agreed upon in order to move forward * The behavior if the path does not exist Once you have a chance to consider the above, please send your proposal back to the alias to continue the discussion Best On Mar 3, 2021, at 5:14 AM, Jaikiran Pai <jai.forums2013 at gmail.com<mailto:jai.forums2013 at gmail.com>> wrote: Thank you Lance. So I think there's some level of agreement on using -C and --dir (or --directory) for the option names. Anymore opinion on the directory creation semantics and any other aspects to consider? -Jaikiran On 28/02/21 5:38 pm, Lance Andersen wrote: Thank you for this. I went through this myself yesterday in addition to what you have below here are a few more: 7zip: -o Thinking about this some more, I might suggest supporting -C Best On Feb 27, 2021, at 11:19 PM, Jaikiran Pai <jai.forums2013 at gmail.com<mailto:jai.forums2013 at gmail.com>> wrote: Hello Alan, On 27/02/21 2:23 pm, Alan Bateman wrote: Yes, the option name will need to be agreed. It would be useful to enumerate the options that the other tools are using to specify the location where to extract. If you see JBS issues mentioning tar -C not supporting chdir when extracting then it might be Solaris tar, which isn't the same as GNU tar which has different options. It might be better to look at more main stream tools, like unzip although jar -d is already taken. It would be nice if there were some consistency with other tools in the JDK that doing extracting (The jmod and jimage extract commands use use --dir for example). I had a look at both tar and unzip commands on MacOS and Linux (CentOS) setup that I had access to. -------------- tar --version The version of this tool has: -C directory, --cd directory, --directory directory A command like "tar -xzf foo.tar.gz -C /tmp/bar/" works fine and extracts the foo.tar.gz from current directory to a target directory /tmp/bar/ -------------- tar --version This version of the tool has: Common options: Although the wording isn't clear that, when used with -x, it extracts to the directory specified in -C, it does indeed behave that way. Specifically, a command like "tar -xzf foo.tar.gz -C /tmp/bar/" works fine and extracts the foo.tar.gz from current directory to a target directory /tmp/bar/ ------------------------------- unzip -v This version of the tool has: [-d exdir] unzip foo.zip -d /tmp/bar/ works fine and extracts the foo.zip from current directory to /tmp/bar/ --------------- The jimage and jmod as you note use the --dir option for extracting to that specified directory. Those were the tools I looked at. I think using the -d option with -x for the jar command is ruled out since it already is used for a different purpose, although for a different "main" operation of the jar command. As for using --dir for this new feature, I don't think it alone will be enough. Specifically, I couldn't find a "short form" option for the --dir option in the jimage or jmod commands. For the jar extract feature that we are discussing here, I think having a short form option (in addition to the longer form) is necessary to have it match the usage expectations of similar other options that the jar command exposes. So even if we do choose --dir as the long form option, we would still need a short form for it and since -d is already taken for something else, we would still need to come up with a different one. The short form of this option could be -C (see below). I think reusing the -C option, for this new feature, perhaps is a good thing. The -C is currently used by the update and create "main" operation of the jar command and the man page for this option states: -C dir jar uf my.jar -C classes Bar.class Using the -C option would indeed align it with the tar command. For the "long form" of this option, the tar command (both on MacOS and CentOS) uses --directory. For this jar extract feature though, we could perhaps just use --dir to have it align with the jimage and the jmod tools. So I think the combination of -C (short form) and --dir (long form) would perhaps be suitable for this feature. There are other discussion points around the behavior when the target directory exists or does not exist, to ensure there is some consistency with main stream tools. I'm guessing you mean the behaviour of creating a directory (or a hierarchy of directories) if the target directory is not present? My testing with the tar tool (both on MacOS and CentOS) shows that if the specified target directory doesn't exist, then the extract fails. The tar extract command doesn't create the target directory during extract. On the other hand, the unzip tool, does create the directory if it doesn't exist. However, interestingly, the unzip tool creates only one level of that directory if it doesn't exist. Specifically, if you specify: unzip foo.zip -d /tmp/blah/ and if "blah/" isn't a directory inside /tmp/ directory, then it creates the "blah/" directory inside /tmp/ and then extracts the contents of the zip into it. However, unzip foo.zip -d /tmp/blah/hello/ and if "blah/" isn't a directory inside /tmp/ directory, then this command fails with an error and it doesn't create the hierarchy of the target directories. Coming to the jimage and the jmod commands, both these commands create the entire directory hierarchy if the target directory specified during extract, using --dir, doesn't exist. So a command like: jimage extract --dir /tmp/blah/foo/bar/ jdkmodules will create the blah/foo/bar/ directory hierarchy if blah doesn't exist in /tmp/, while extracting the "jdkmodules" image.From the user point of view, I think this behaviour of creating the directories if the target directory doesn't exist, is probably the most intuitive and useful and if we did decide to use this approach for this new option for jar extract command, then it would align with what we already do in jimage and jmod commands. One another minor detail, while we are at this, is that, IMO we should let the jar extract command to continue to behave the way it currently does when it comes to overwriting existing files. If the jar being extracted contains a file by the same name, in the target directory (hierarchy) then it should continue to overwrite that file. In other words, I don't think we should change the way the jar extract command currently behaves where it overwrites existing files when extracting. -Jaikiran <oracle_sig_logo.gif> Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 [cid:E1C4E2F0-ECD0-4C9D-ADB4-B16CA7BCB7FC at home] Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 -------------- next part -------------- |
|
Mailing list message from Jaikiran Pai on compiler-dev: Hello Lance, On 03/03/21 9:14 pm, Lance Andersen wrote:
One of my previous reply included the details of how I think it should
I'm guessing you mean the behaviour of creating a directory (or a unzip foo.zip -d /tmp/blah/ and if "blah/" isn't a directory inside /tmp/ directory, then it creates However, unzip foo.zip -d /tmp/blah/hello/ and if "blah/" isn't a directory inside /tmp/ directory, then this Coming to the jimage and the jmod commands, both these commands create jimage extract --dir /tmp/blah/foo/bar/ jdkmodules will create the blah/foo/bar/ directory hierarchy if blah doesn't exist From the user point of view, I think this behaviour of creating the One another minor detail, while we are at this, is that, IMO we should -Jaikiran -------------- next part -------------- |
|
Mailing list message from Lance Andersen on compiler-dev: Hi JaikiranFrom https://docs.oracle.com/en/java/javase/15/docs/specs/man/jar.html The jar man page includes the following : Because of the above, I feel that we should support: ??? The addition of ?-dir? adds support for an option used by some of the other java command line tools. I would suggest for the next step is flush/write out your proposed syntax as it would be included in an updated version of the jar man page. Given the other java command line tools create the directory if it does not exist, I think that is reasonable to propose that. Your change should not modify the longstanding overwrite behavior of jar. Another thing to think about is whether there should be any additional output when the -v option is specified In summary, I think you are moving in the right direction. The next step is to make a pass at creating the man page updates so that we can reach consensus on the syntax and behavior. Thank you again for your efforts on this Best On Mar 3, 2021, at 9:40 PM, Jaikiran Pai <jai.forums2013 at gmail.com<mailto:jai.forums2013 at gmail.com>> wrote: Hello Lance, On 03/03/21 9:14 pm, Lance Andersen wrote: Some other things needed to be defined and agreed upon in order to move forward * The behavior if the path does not exist One of my previous reply included the details of how I think it should behave for 2 of the above cases. I'll paste that here again for easier visibility. As for how it should behave if the option is specified more than once, I'll spend some time today to see how the jar tool currently behaves for some of the other options in this aspect and send back my response. Thank you for your help so far. Pasting below my proposal from a previous reply for the other 2 cases: There are other discussion points around the behavior when the target directory exists or does not exist, to ensure there is some consistency with main stream tools. I'm guessing you mean the behaviour of creating a directory (or a hierarchy of directories) if the target directory is not present? My testing with the tar tool (both on MacOS and CentOS) shows that if the specified target directory doesn't exist, then the extract fails. The tar extract command doesn't create the target directory during extract. On the other hand, the unzip tool, does create the directory if it doesn't exist. However, interestingly, the unzip tool creates only one level of that directory if it doesn't exist. Specifically, if you specify: unzip foo.zip -d /tmp/blah/ and if "blah/" isn't a directory inside /tmp/ directory, then it creates the "blah/" directory inside /tmp/ and then extracts the contents of the zip into it. However, unzip foo.zip -d /tmp/blah/hello/ and if "blah/" isn't a directory inside /tmp/ directory, then this command fails with an error and it doesn't create the hierarchy of the target directories. Coming to the jimage and the jmod commands, both these commands create the entire directory hierarchy if the target directory specified during extract, using --dir, doesn't exist. So a command like: jimage extract --dir /tmp/blah/foo/bar/ jdkmodules will create the blah/foo/bar/ directory hierarchy if blah doesn't exist in /tmp/, while extracting the "jdkmodules" image.From the user point of view, I think this behaviour of creating the directories if the target directory doesn't exist, is probably the most intuitive and useful and if we did decide to use this approach for this new option for jar extract command, then it would align with what we already do in jimage and jmod commands. One another minor detail, while we are at this, is that, IMO we should let the jar extract command to continue to behave the way it currently does when it comes to overwriting existing files. If the jar being extracted contains a file by the same name, in the target directory (hierarchy) then it should continue to overwrite that file. In other words, I don't think we should change the way the jar extract command currently behaves where it overwrites existing files when extracting. -Jaikiran [cid:E1C4E2F0-ECD0-4C9D-ADB4-B16CA7BCB7FC at home] Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 -------------- next part -------------- |
|
Mailing list message from Alan Bateman on core-libs-dev: On 04/03/2021 12:01, Lance Andersen wrote:
Is --directory needed with this proposal? I agree with the suggest to create the directory so that it's consistent -Alan. |
|
Mailing list message from Lance Andersen on core-libs-dev: On Mar 11, 2021, at 9:29 AM, Alan Bateman <alan.bateman at oracle.com<mailto:alan.bateman at oracle.com>> wrote: On 04/03/2021 12:01, Lance Andersen wrote: The jar man page includes the following : Because of the above, I feel that we should support: ??? The addition of ?-dir? adds support for an option used by some of the other java command line tools. Is --directory needed with this proposal? I agree with the suggest to create the directory so that it's consistent with other tools. The only reason I included it is for completeness with tar which the jar options were originally derived from. Could go either way though Best -Alan. [cid:E1C4E2F0-ECD0-4C9D-ADB4-B16CA7BCB7FC at home] Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 |
|
Mailing list message from Alan Bateman on core-libs-dev: On 11/03/2021 15:25, Lance Andersen wrote:
I think it would be surprising to have two GNU-style long options so I -Alan |
|
Mailing list message from Lance Andersen on core-libs-dev: On Mar 12, 2021, at 6:32 AM, Alan Bateman <alan.bateman at oracle.com<mailto:alan.bateman at oracle.com>> wrote: On 11/03/2021 15:25, Lance Andersen wrote: The only reason I included it is for completeness with tar which the jar options were originally derived from. I think it would be surprising to have two GNU-style long options so I think we should pick one. "--dir" seems okay to me and is consistent with the other the other JDK tools that extract to a directory (although those tools might not be well known). If you prefer "--directory" then I think we'll should add it to the other tools and hide "--dir" from the usage output. I don?t have a strong preference but lean slightly towards ?-directory? as it is more descriptive, similar to the other GNU-style commands jar currently supports . Tar supports ??cd?, ??directory? in addition to ?-C? which is why I suggested supporting both GNU-style long options. Perhaps jpackage should also support ?dir/directory in addition to ??dest' if we are looking at consistency between java tools. I do agree that it would be nice to be consistent across the java tools for options so if we go the ?-directory?, we should follow your suggestion and make it the primary and remove ??dir? from the usage output. -Alan [cid:E1C4E2F0-ECD0-4C9D-ADB4-B16CA7BCB7FC at home] Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 |
|
Mailing list message from Alan Bateman on core-libs-dev: On 12/03/2021 12:18, Lance Andersen wrote:
My comment on consistency was limited to the long option to specify the -Alan |
|
Mailing list message from Jaikiran Pai on core-libs-dev: On 14/03/21 6:21 pm, Alan Bateman wrote:
I'll sending the man page content that I have been working on, shortly -Jaikiran |
|
Mailing list message from Jaikiran Pai on core-libs-dev: Based on the inputs so far, I've updated the PR to include the provided Command option: In this current discussion, we seem to have an agreement for using -C Directories creation: There was an agreement in our discussion that if the destination Verbose logging: During the discussion, there was a question whether this feature should Repeatability of the newly introduce options: Unlike in the other main operations of the jar command, the -C option An alternate approach would have been to allow the -C and/or --dir Overwriting of contents in existing target directory: No specific change has been done when it comes to dealing with the Compatibility mode: The code in this PR also supports the compatibility mode for this jar -xvf somejar.jar -C /tmp/foo/ will work fine and the jar will be extracted into /tmp/foo directory. Message/error internationalization: I have only updated the jar.properties for the English version of the jar --help output: Currently the jar --help output only talks about creation and updation "jar creates an archive for classes and resources, and can manipulate or It does mention "manipulate" but doesn't specifically say extraction. Testing: A new jtreg test has been introduced which tests various aspects of this A couple of tests in the new introduced test case, check for the Man page: This one I need input on. I have tried to see how these man pages are Example usage: Here are some example usages: jar -x -f somejar.jar -C /tmp/foo/bar/ This command extracts the somejar.jar file to the /tmp/foo/bar/ jar -x -f somejar.jar --dir /tmp/foo/bar/ Same as above, except uses the long form --dir option jar -x -f somejar.jar -C /tmp/foo/bar/ f1.txt d1/f2.txt Assuming somejar.jar contains "f1.txt" (at root), "d1/f2.txt" and other -Jaikiran On 14/03/21 6:21 pm, Alan Bateman wrote:
|
|
I think the summary is that we've converged on -C/--dir. We might have to tweak the usage message for -C so that it starts with the existing "Change to the specified directory ..." rather than changing it to start with the extract case. |
|
A CSR for this enhancement has now been created https://bugs.openjdk.java.net/browse/JDK-8264510 |
| @@ -1391,7 +1424,7 @@ ZipEntry extractFile(InputStream is, ZipEntry e) throws IOException { | |||
| if (name.length() == 0) { | |||
| return rc; // leading '/' or 'dot-dot' only path | |||
| } | |||
| File f = new File(name.replace('/', File.separatorChar)); | |||
| File f = new File(xdestDir, name.replace('/', File.separatorChar)); | |||
There was a problem hiding this comment.
Could you please add a comment regarding xdestDir and also correct the typo on line 1418 'requres' -> 'requires'
There was a problem hiding this comment.
Fixed the typo and also added code comment about the xdestDir usage and semantics. If the new comment needs clarification/changes, do let me know.
| @@ -159,6 +163,8 @@ out.inflated=\ | |||
| \ inflated: {0} | |||
| out.size=\ | |||
| (in = {0}) (out= {1}) | |||
| out.extract.dir=\ | |||
| extracting to {0} | |||
There was a problem hiding this comment.
Perhaps 'Extracting to directory: {0}'
There was a problem hiding this comment.
Updated the message to say extracting to directory: {0}. I decided to use lower case for extracting to keep it consistent with other similar messages that get logged here.
| } | ||
| return abs; | ||
| } | ||
|
|
There was a problem hiding this comment.
Please add a comment to each test giving a high level overview of what it does as it will help future maintainers
There was a problem hiding this comment.
Done in latest update to this PR
| Assert.assertEquals(exitCode, 0, "Failed to extract " + testJarPath); | ||
| // make sure only the specific files were extracted | ||
| final Stream<Path> paths = Files.walk(Path.of(tmpDir)); | ||
| Assert.assertEquals(paths.count(), 6, "Unexpected number of files/dirs in " + tmpDir); |
There was a problem hiding this comment.
Should '6' be in a local variable to make it clearer or at a minimum a comment
There was a problem hiding this comment.
The latest update to the PR includes a comment explaining what this number represents.
| Assert.assertEquals(Files.readAllBytes(f2), FILE_CONTENT, "Unexpected content in file " + f2); | ||
| } | ||
|
|
||
| private static Path createJarWithPFlagSemantics() throws IOException { |
There was a problem hiding this comment.
Perhaps add a comment as to what the method does
| @@ -239,7 +245,8 @@ main.help.opt.any=\ | |||
| \ Operation modifiers valid in any mode:\n\ | |||
| \n\ | |||
| \ -C DIR Change to the specified directory and include the\n\ | |||
| \ following file | |||
| \ following file. When used in extract mode, extracts\n\ | |||
| \ the jar to the specified directory | |||
There was a problem hiding this comment.
Should this be updated on line 187 as well in the compatibility mode section?
There was a problem hiding this comment.
Updated in latest version of the PR.
| error.extract.multiple.dest.dir=\ | ||
| You may not specify more than one directory for extracting the jar | ||
| error.extract.pflag.not.allowed=\ | ||
| -P option cannot be used when extracting a jar to a specific location |
There was a problem hiding this comment.
Perhaps something similar to
You may not specify '-Px' with the '-C' or '--dir' options
There was a problem hiding this comment.
Makes sense. I've updated the PR with this change.
| @@ -58,6 +58,10 @@ error.incorrect.length=\ | |||
| incorrect length while processing: {0} | |||
| error.create.tempfile=\ | |||
| Could not create a temporary file | |||
| error.extract.multiple.dest.dir=\ | |||
| You may not specify more than one directory for extracting the jar | |||
There was a problem hiding this comment.
Perhaps something similar to:
You may not specify the '-C' or '--dir' option more than once with the '-x' option
| */ | ||
| @Test | ||
| public void testExtractWithDirPFlagNotAllowed() throws Exception { | ||
| final String expectedErrMsg = "-P option cannot be used when extracting a jar to a specific location"; |
There was a problem hiding this comment.
Probably need to add a comment that this must match the entry in jar.properties
There was a problem hiding this comment.
Added a comment mentioning where this message is sourced from.
| * {@code --dir} option is used | ||
| */ | ||
| @Test | ||
| public void testExtractWithDirPFlagNotAllowed() throws Exception { |
There was a problem hiding this comment.
I would include --extract in your testing options
There was a problem hiding this comment.
Done in latest version of the PR
| * Tests that {@code jar -xvf <jarname> -C <dir>} works fine too | ||
| */ | ||
| @Test | ||
| public void testLegacyCompatibilityMode() throws Exception { |
There was a problem hiding this comment.
Perhaps add a DataProvider so you can test --extract as well?
There was a problem hiding this comment.
So from what I understand of the code in the jar tool the "legacy compatibility" mode stands for using the single hypen option and clubbing multiple options into one. Something like -xvfP instead of -x -v -f -P and this legacy compatibility mode isn't applicable for the long form options like --extract. So IMO using --extract here won't work. Let me know if I have misunderstood this review comment or the semantics of the legacy compatibility mode.
| final List<String[]> cmdArgs = new ArrayList<>(); | ||
| cmdArgs.add(new String[]{"-x", "-f", testJarPath.toString(), "-C", tmpDir, "-C", tmpDir}); | ||
| cmdArgs.add(new String[]{"-x", "-f", testJarPath.toString(), "--dir", tmpDir, "--dir", tmpDir}); | ||
| cmdArgs.add(new String[]{"-x", "-f", testJarPath.toString(), "--dir", tmpDir, "-C", tmpDir}); |
There was a problem hiding this comment.
Perhaps use a DataProvider so you can test --extract as well?
| @Test | ||
| public void testExtractPartialContent() throws Exception { | ||
| final String tmpDir = Files.createTempDirectory(Path.of("."), "8173970-").toString(); | ||
| final String[] cmdArgs = new String[]{"-x", "-f", testJarPath.toString(), "--dir", tmpDir, |
There was a problem hiding this comment.
Perhaps add a DataProvider so you can test --extract as well?
There was a problem hiding this comment.
Didn't use a data provider, but did add tests for --extract as well, in the latest version of this PR.
| * Extracts the jar file using {@code jar -x -f <jarfile> -C <dest>} | ||
| */ | ||
| private void testExtract(final String dest) throws Exception { | ||
| final String[] args = new String[]{"-x", "-f", testJarPath.toString(), "-C", dest}; |
There was a problem hiding this comment.
Perhaps add a DataProvider so you can test --extract as well?
There was a problem hiding this comment.
The latest version of the PR tests the --extract as well.
| cmdArgs.add(new String[]{"-x", "-f", testJarPath.toString(), "-P", "--dir", tmpDir}); | ||
| cmdArgs.add(new String[]{"-x", "-f", testJarPath.toString(), "-P", "-C", "."}); | ||
| cmdArgs.add(new String[]{"-x", "-f", testJarPath.toString(), "-P", "--dir", "."}); | ||
| cmdArgs.add(new String[]{"-xvfP", testJarPath.toString(), "-C", tmpDir}); |
There was a problem hiding this comment.
Perhaps add a DataProvider so you can test --extract as well?
There was a problem hiding this comment.
Not a data provider but the latest version of PR tests --extract as well.
| // create a jar which has leading slash (/) and dot-dot (..) preserved in entry names | ||
| final Path jarPath = createJarWithPFlagSemantics(); | ||
| // extract with -P flag without any explicit destination directory (expect the extraction to work fine) | ||
| final String[] args = new String[]{"-xvfP", jarPath.toString()}; |
There was a problem hiding this comment.
Perhaps add a DataProvider so you can test --extract as well?
There was a problem hiding this comment.
Not a data provider but the latest version of PR tests --extract as well
| public void testExtractLongForm() throws Exception { | ||
| final String dest = "foo-bar"; | ||
| System.out.println("Extracting " + testJarPath + " to " + dest); | ||
| final int exitCode = JAR_TOOL.run(System.out, System.err, "-x", "-f", testJarPath.toString(), |
There was a problem hiding this comment.
Perhaps add a DataProvider so you can test --extract as well?
There was a problem hiding this comment.
Not a data provider but the latest version of PR tests --extract as well
|
Thank you for the reviews, Lance. The latest version of this PR has taken into account most of these comments. There's one review comment which hasn't resulted in a change and I've added a reply to that review comment explaining my thoughts. |
|
@jaikiran This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
|
Still active. Been busy with a few other things and I just need to get back and implement some inputs from Lance. |
|
@jaikiran This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
|
@jaikiran This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the |
|
Mailing list message from Peter Levart on core-libs-dev: Hi, On 29/03/2021 12:46, Jaikiran Pai wrote:
If we are trying to follow the semantics of tar -C option (gnutar and ?????? -C, --directory=DIR ...the meaning is that relative paths from archive get extracted dir1/subdir1/a.txt The command: tar -cf archive.tar -C dir1 . -C ../dir2 . ...will create archive.tar with the following content (notice the 2nd -C ./ When extracting, -C may be used to scatter different relative archive tar -xf archive.tar -C dir1 ./file1.txt ./subdir1 -C ../dir2 ./file2.txt So here's a hint about the behavior of multiple -C options: if the Regards, Peter |
|
Mailing list message from Peter Levart on core-libs-dev: A word about "compatibility" with other jdk tools... jar is a specific tool. It was initially conceived to be user-friendly "The syntax for the jar command resembles the syntax for the tar command." Mixing-in behaviors not consistent with tar will make it a strange Regards, Peter On 29/06/2021 09:05, Peter Levart wrote:
|
|
/open |
|
@jaikiran This pull request is now open |
|
❗ This change is not yet ready to be integrated. |
|
@jaikiran Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information. |
Can I please get a review for this patch which proposes to implement the enhancement request noted in https://bugs.openjdk.java.net/browse/JDK-8173970?
The commit in this PR introduces the
-oand--output-diroption to thejarcommand. The option takes a path to a destination directory as a value and extracts the contents of the jar into that directory. This is an optional option and the changes in the commit continue to maintain backward compatibility where the jar is extracted into current directory, if no-oor--output-diroption has been specified.As far as I know, there hasn't been any discussion on what the name of this new option should be. I was initially thinking of using
-dbut that is currently used by thejarcommand for a different purpose. So I decided to use-oand--output-dir. This is of course open for change depending on any suggestions in this PR.The commit in this PR also updates the
jar.propertiesfile which contains the English version of the jar command's--helpoutput. However, no changes have been done to the internationalization files corresponding to this one (for example:jar_de.properties), because I don't know what process needs to be followed to have those files updated (if at all they need to be updated).The commit also includes a jtreg testcase which verifies the usage of this new option.
Progress
Warning
8173970: jar tool should have a way to extract to a directoryIssues
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/2752/head:pull/2752$ git checkout pull/2752Update a local copy of the PR:
$ git checkout pull/2752$ git pull https://git.openjdk.org/jdk.git pull/2752/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 2752View PR using the GUI difftool:
$ git pr show -t 2752Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/2752.diff
Webrev
Link to Webrev Comment