RFR - 8027634: Support @argfiles for java command-line tool
Hi, Please review proposed patch for JDK-8027634[1]. This patch is to enable java support command line argument file like javac does. The implementation use the same syntax rule, which is implemented in CommandLine.java[3] with java.io.StreamTokenizer. Some early comment is that we probably don’t need such complexity to support same syntax, also require to quote whole token is a little inconvenient. For example, must be -cp “c:\\foo bar\\lib;c:\\lib” instead of -cp c:\”foo bar”\lib;c:\lib. I am debating if such compatibility is necessary useful, after all, easy and intuitive is more important, and with simpler rule, the implementation will be cleaner as well. Anyhow, with the patch out, at least developer can build idk and have something to test with to see if this can fulfill their use cases. Also, for tools other than java that use launcher, it’s possible to use -J@argfile to pass arguments. For example, if want to pass -J options to javac, it’s now possible to do so with javac -J@argfile, and put -J options in the argfile. If the consensus is that such syntax compatibility is not important, we will go ahead remove the escaping support(except probably enable escape for quote itself) in quote, and maybe add support of quote within a token. CCing build-dev for build changes, jdk9-dev for wider audience for tools. Cheers, Henry [1] https://bugs.openjdk.java.net/browse/JDK-8027634 [2] http://docs.oracle.com/javase/7/docs/technotes/tools/windows/javac.html#comm... [3] http://hg.openjdk.java.net/jdk9/jdk9/langtools/file/03e083639ee9/src/jdk.com...
Sigh, forgot the link to the webrev again. http://cr.openjdk.java.net/~henryjen/jdk9/8027634/webrev/ Cheers, Henry
On Jul 9, 2015, at 8:48 PM, Henry Jen <henry.jen@oracle.com> wrote:
Hi,
Please review proposed patch for JDK-8027634[1]. This patch is to enable java support command line argument file like javac does. The implementation use the same syntax rule, which is implemented in CommandLine.java[3] with java.io.StreamTokenizer.
Some early comment is that we probably don’t need such complexity to support same syntax, also require to quote whole token is a little inconvenient. For example, must be -cp “c:\\foo bar\\lib;c:\\lib” instead of -cp c:\”foo bar”\lib;c:\lib.
I am debating if such compatibility is necessary useful, after all, easy and intuitive is more important, and with simpler rule, the implementation will be cleaner as well.
Anyhow, with the patch out, at least developer can build idk and have something to test with to see if this can fulfill their use cases.
Also, for tools other than java that use launcher, it’s possible to use -J@argfile to pass arguments. For example, if want to pass -J options to javac, it’s now possible to do so with javac -J@argfile, and put -J options in the argfile.
If the consensus is that such syntax compatibility is not important, we will go ahead remove the escaping support(except probably enable escape for quote itself) in quote, and maybe add support of quote within a token.
CCing build-dev for build changes, jdk9-dev for wider audience for tools.
Cheers, Henry
[1] https://bugs.openjdk.java.net/browse/JDK-8027634 [2] http://docs.oracle.com/javase/7/docs/technotes/tools/windows/javac.html#comm... [3] http://hg.openjdk.java.net/jdk9/jdk9/langtools/file/03e083639ee9/src/jdk.com...
Build changes look fine. /Magnus
10 jul 2015 kl. 05:50 skrev Henry Jen <henry.jen@oracle.com>:
Sigh, forgot the link to the webrev again.
http://cr.openjdk.java.net/~henryjen/jdk9/8027634/webrev/
Cheers, Henry
On Jul 9, 2015, at 8:48 PM, Henry Jen <henry.jen@oracle.com> wrote:
Hi,
Please review proposed patch for JDK-8027634[1]. This patch is to enable java support command line argument file like javac does. The implementation use the same syntax rule, which is implemented in CommandLine.java[3] with java.io.StreamTokenizer.
Some early comment is that we probably don’t need such complexity to support same syntax, also require to quote whole token is a little inconvenient. For example, must be -cp “c:\\foo bar\\lib;c:\\lib” instead of -cp c:\”foo bar”\lib;c:\lib.
I am debating if such compatibility is necessary useful, after all, easy and intuitive is more important, and with simpler rule, the implementation will be cleaner as well.
Anyhow, with the patch out, at least developer can build idk and have something to test with to see if this can fulfill their use cases.
Also, for tools other than java that use launcher, it’s possible to use -J@argfile to pass arguments. For example, if want to pass -J options to javac, it’s now possible to do so with javac -J@argfile, and put -J options in the argfile.
If the consensus is that such syntax compatibility is not important, we will go ahead remove the escaping support(except probably enable escape for quote itself) in quote, and maybe add support of quote within a token.
CCing build-dev for build changes, jdk9-dev for wider audience for tools.
Cheers, Henry
[1] https://bugs.openjdk.java.net/browse/JDK-8027634 [2] http://docs.oracle.com/javase/7/docs/technotes/tools/windows/javac.html#comm... [3] http://hg.openjdk.java.net/jdk9/jdk9/langtools/file/03e083639ee9/src/jdk.com...
On Jul 10, 2015, at 11:48 AM, Henry Jen <henry.jen@oracle.com> wrote:
Hi,
Please review proposed patch for JDK-8027634[1]. This patch is to enable java support command line argument file like javac does. The implementation use the same syntax rule, which is implemented in CommandLine.java[3] with java.io.StreamTokenizer.
Some early comment is that we probably don’t need such complexity to support same syntax, also require to quote whole token is a little inconvenient. For example, must be -cp “c:\\foo bar\\lib;c:\\lib” instead of -cp c:\”foo bar”\lib;c:\lib.
I am debating if such compatibility is necessary useful, after all, easy and intuitive is more important, and with simpler rule, the implementation will be cleaner as well.
I have a slight preference to maintain consistent syntax as javac @argfile support in terms of the quotation. A user could use the same path specified in -cp for both javac @argfile an java @argfile use. I skimmed on the webrev and looks okay to me. I’ll leave it for Kumar to do detailed review. One minor comment: args.c Are you planning to remove the test within #ifdef DEBUG_ARGFILE block? thanks Mandy
Thanks Mandy and Magnus for looking into the patch, and feedbacks from Kumar Srinivasan and Michel Trudeau. We have an updated version to be reviewed at, http://cr.openjdk.java.net/~henryjen/jdk9/8027634/v2/webrev/ This version changes the syntax a little bit as following, but pretty much still javac compatible, 1) Whitespace characters are ‘ ‘, ‘\t’. ‘\n’ and ‘\r’, not everything from ‘\0\ to ‘ ‘. 2) Open quote still stop at EOL unless ‘\’ is the last character, which will join the next line by removing leading white spaces. This allows multiple line supports in quote. If leading space is significant, add ‘\’ to escape the white space character. 3) Escape sequences is now only limited to ‘\n’ and ‘\r’. No octal character support. In terms of javac compatibility, unless octal or \a\b\f\t\v escaping sequence are used, they should be completely compatible. I don’t have plan to remove the test as it would be convenient to have it around. Move it into a separate test is probably desirable, though. I am not sure about what is a better place, seems to me test inside implementation file is quite common as in wildcard.c and cmdtoargs.c. Cheers, Henry
On Jul 15, 2015, at 7:51 AM, Mandy Chung <mandy.chung@oracle.com> wrote:
On Jul 10, 2015, at 11:48 AM, Henry Jen <henry.jen@oracle.com> wrote:
Hi,
Please review proposed patch for JDK-8027634[1]. This patch is to enable java support command line argument file like javac does. The implementation use the same syntax rule, which is implemented in CommandLine.java[3] with java.io.StreamTokenizer.
Some early comment is that we probably don’t need such complexity to support same syntax, also require to quote whole token is a little inconvenient. For example, must be -cp “c:\\foo bar\\lib;c:\\lib” instead of -cp c:\”foo bar”\lib;c:\lib.
I am debating if such compatibility is necessary useful, after all, easy and intuitive is more important, and with simpler rule, the implementation will be cleaner as well.
I have a slight preference to maintain consistent syntax as javac @argfile support in terms of the quotation. A user could use the same path specified in -cp for both javac @argfile an java @argfile use.
I skimmed on the webrev and looks okay to me. I’ll leave it for Kumar to do detailed review. One minor comment:
args.c Are you planning to remove the test within #ifdef DEBUG_ARGFILE block?
thanks Mandy
Hi, Another update for argument file support, this version we added a couple measures to ensure compatibility, 1) Compile time directive ENABLE_ARG_FILES is needed to enable the feature, only java is enabled with this webrev. 2) Escape @argument if so desired: Additional prefixes of @ to a @ prefixed option will act as an escape, ie. the first @ will be removed and the rest of the arguments presented to the launcher literally. For example: java @ foo gets arguments of @ foo java @filename, filename expanded java @@filename will have the leading @ removed and reduced to literal @filename similarly, java @@@filenamewill have the leading @ removed and reduced to literal @@filename 3) The option '-Xdisable-@files' can be used on the command line anywhere to prevent further @filename expansion. 4) Limit the file size must not exceed MAXINT (2, 147, 483, 647) bytes. Webrev at http://cr.openjdk.java.net/~henryjen/jdk9/8027634/v4/webrev/ Cheers, Henry
New build changes look fine to me. /Erik On 2015-08-08 09:03, Henry Jen wrote:
Hi,
Another update for argument file support, this version we added a couple measures to ensure compatibility,
1) Compile time directive ENABLE_ARG_FILES is needed to enable the feature, only java is enabled with this webrev. 2) Escape @argument if so desired: Additional prefixes of @ to a @ prefixed option will act as an escape, ie. the first @ will be removed and the rest of the arguments presented to the launcher literally.
For example:
java @ foo gets arguments of @ foo java @filename, filename expanded java @@filename will have the leading @ removed and reduced to literal @filename similarly, java @@@filenamewill have the leading @ removed and reduced to literal @@filename
3) The option '-Xdisable-@files' can be used on the command line anywhere to prevent further @filename expansion. 4) Limit the file size must not exceed MAXINT (2, 147, 483, 647) bytes.
Webrev at http://cr.openjdk.java.net/~henryjen/jdk9/8027634/v4/webrev/
Cheers, Henry
Henry, Generally looks good here are some comments, on my initial pass, I am not fully done with args.c I will look at this some more later today or tomorrow. args.c: 1.) Can be folded 45 char *rv; 46 47 rv = (char *) JLI_MemAlloc(len + 1); char *rv = (char *) JLI_MemAlloc(len + 1); 2.) replace the defines with enum ? 53 #define FIND_NEXT 0 54 #define IN_COMMENT 1 55 #define IN_QUOTE 2 56 #define IN_ESCAPE 3 57 #define SKIP_LEAD_WS 4 58 #define IN_TOKEN 5 3.) I would rename the following, this is maintaining the state of the state machine right ? So rename stage -> state 4.) expectingNoDash is expectingMain right ? if so I would rename it so. 5.) 84 if (isJava) { 85 firstAppArgIndex = -1; 86 } else { 87 // tools, this value remains 0 all the time. 88 firstAppArgIndex = 0; 89 } 90 } can be written with unary operator firstAppArgIndex = isJava ? -1 : 0; TestHelper.java: Why add findInOutput method ? this seems to be identical to "matches" at line 606, which does exactly that. ArgFileSyntax.java Nice, but I think the creation and storage of test case can be simplified, basically loadCases(), also please avoid Xbootclasspath. The trouble with these tests which operate loops, are very painful to debug and isolate a failing case, on the night before GA. Can we do something like this..... Map<String, List<String>> tests = new ArrayList<>(); tests.put("testing quotes", Array.asList({{...}, {....}}); tests.add("no recurse expansion", Array.asList({{...}, {....}}); The execute each test, with a description of what the test is doing. This way if a test fails its easy to zero in on the failing test. Kumar On 8/8/2015 12:03 AM, Henry Jen wrote:
Hi,
Another update for argument file support, this version we added a couple measures to ensure compatibility,
1) Compile time directive ENABLE_ARG_FILES is needed to enable the feature, only java is enabled with this webrev. 2) Escape @argument if so desired: Additional prefixes of @ to a @ prefixed option will act as an escape, ie. the first @ will be removed and the rest of the arguments presented to the launcher literally.
For example:
java @ foo gets arguments of @ foo java @filename, filename expanded java @@filename will have the leading @ removed and reduced to literal @filename similarly, java @@@filenamewill have the leading @ removed and reduced to literal @@filename
3) The option '-Xdisable-@files' can be used on the command line anywhere to prevent further @filename expansion. 4) Limit the file size must not exceed MAXINT (2, 147, 483, 647) bytes.
Webrev athttp://cr.openjdk.java.net/~henryjen/jdk9/8027634/v4/webrev/
Cheers, Henry
On Aug 12, 2015, at 7:55 AM, Kumar Srinivasan <kumar.x.srinivasan@oracle.com> wrote:
Henry,
Generally looks good here are some comments, on my initial pass, I am not fully done with args.c I will look at this some more later today or tomorrow.
4.) expectingNoDash is expectingMain right ? if so I would rename it so.
Not really, it is expecting a argument without dash, such as -cp or -classpath.
TestHelper.java:
Why add findInOutput method ? this seems to be identical to "matches" at line 606, which does exactly that.
To return a string, this is used when trying to match a pattern and get information from that line, I used to get the starting index for an argument in verifyOptions. Guess I can remove that now as each use of verifyOptions all starts from 1.
ArgFileSyntax.java Nice, but I think the creation and storage of test case can be simplified, basically loadCases(), also please avoid Xbootclasspath. The trouble with these tests which operate loops, are very painful to debug and isolate a failing case, on the night before GA.
I wrote that because I start with testng as a DataProvider, we can change that.
Can we do something like this.....
Map<String, List<String>> tests = new ArrayList<>(); tests.put("testing quotes", Array.asList({{...}, {....}}); tests.add("no recurse expansion", Array.asList({{...}, {....}});
The execute each test, with a description of what the test is doing. This way if a test fails its easy to zero in on the failing test.
verifyOutput will print out what line is not matching, so it’s pretty easy to identify which case failed. Cheers, Henry
Henry, <trimmed the cc: list> I am still not complete, but here are some more comments and specifics. args.c 1. -static int firstAppArgIndex = -1; +#define NOT_FOUND -1; +static int firstAppArgIndex = NOT_FOUND; 2. // Any @argument comes in here is an argument as is. // Expansion should had done before checkArg called. I think you are trying to say // All arguments arrive here must be a launcher argument, // ie. by now, all argfile expansions must have been performed. 3. void JLI_InitArgProcessing(jboolean isJava, jboolean disableArgFile) { // No expansion for relaunch The launcher might re-exec itself, this might happen under certain circumstances on *nix, is the above logic to prevent subsequent expansion on relaunch ? However when a re-exec occurs, the expanded args will be sent to the second invocation of the launcher. So what does "relaunch" mean ? in your comment ? ArgFileSyntax.java: usage of ArrayList vs. Arrays will simplify things. // arg file content, expected options static String[] testCaseArrays[][] = { { // empty file {}, {} }, ... public List<List<List<String>>> loadCases() { List<List<List<String>>> rv = new ArrayList<>(); for (String[][] testCaseArray : testCaseArrays) { List<List<String>> testCase = new ArrayList<>(2); testCase.add(Arrays.asList(testCaseArray[0])); testCase.add(Arrays.asList(testCaseArray[1])); rv.add(testCase); } // long lines ..... @Test public void testSyntax() throws IOException { List<List<List<String>>> allCases = loadCases(); for (List<List<String>> testCase : allCases) { verifyParsing(testCase.get(0), testCase.get(1)); } } ArgsFileTest.java: use case coverage looks great, still haven't finished with this yet. Kumar
On Aug 12, 2015, at 7:55 AM, Kumar Srinivasan <kumar.x.srinivasan@oracle.com> wrote:
Henry,
Generally looks good here are some comments, on my initial pass, I am not fully done with args.c I will look at this some more later today or tomorrow.
4.) expectingNoDash is expectingMain right ? if so I would rename it so.
Not really, it is expecting a argument without dash, such as -cp or -classpath.
TestHelper.java:
Why add findInOutput method ? this seems to be identical to "matches" at line 606, which does exactly that.
To return a string, this is used when trying to match a pattern and get information from that line, I used to get the starting index for an argument in verifyOptions.
Guess I can remove that now as each use of verifyOptions all starts from 1.
ArgFileSyntax.java Nice, but I think the creation and storage of test case can be simplified, basically loadCases(), also please avoid Xbootclasspath. The trouble with these tests which operate loops, are very painful to debug and isolate a failing case, on the night before GA.
I wrote that because I start with testng as a DataProvider, we can change that.
Can we do something like this.....
Map<String, List<String>> tests = new ArrayList<>(); tests.put("testing quotes", Array.asList({{...}, {....}}); tests.add("no recurse expansion", Array.asList({{...}, {....}});
The execute each test, with a description of what the test is doing. This way if a test fails its easy to zero in on the failing test.
verifyOutput will print out what line is not matching, so it’s pretty easy to identify which case failed.
Cheers, Henry
On Aug 13, 2015, at 9:38 AM, Kumar Srinivasan <kumar.x.srinivasan@oracle.com> wrote:
3. void JLI_InitArgProcessing(jboolean isJava, jboolean disableArgFile) { // No expansion for relaunch
The launcher might re-exec itself, this might happen under certain circumstances on *nix, is the above logic to prevent subsequent expansion on relaunch ? However when a re-exec occurs, the expanded args will be sent to the second invocation of the launcher.
So what does "relaunch" mean ? in your comment ?
Right, relaunch means that re-exec. As the first time, the arguments has expanded, we don’t want to do that again for succeeding runs. For example an argument @@arg, it will become @arg at the second execution because of escape. That should not be expanded again. Cheers, Henry
Hi, Another minor revision address comments, no real behavior changes except use JLI_StrCmp instead of JLI_StrCCmp in checkArg(). http://cr.openjdk.java.net/~henryjen/jdk9/8027634/v5/webrev/ Cheers, Henry
On Aug 14, 2015, at 1:10 PM, Henry Jen <henry.jen@oracle.com> wrote:
Hi,
Another minor revision address comments, no real behavior changes except use JLI_StrCmp instead of JLI_StrCCmp in checkArg().
http://cr.openjdk.java.net/~henryjen/jdk9/8027634/v5/webrev/
main.c JLI_PreprocessingArg returns NULL if not @argfile Would it be better to return JLI_List containing one element as argv[i]? We want to avoid new/free JLI_List for every argument and maybe a preallocated reusable copy for single element list to use (non-growable)? 140 // Shallow free, we reuse the string to avoid copy 141 JLI_MemFree(argsInFile->elements); 142 JLI_MemFree(argsInFile); What about adding JLI_List_free(JLI_List sl, boolean shadow)? This would be useful for the reusable single element list. Same comment applied to cmdtoargs.c args.c You have #define NOT_FOUND -1 but NOT_FOUND is not used else where. void JLI_InitArgProcessing(jboolean isJava, jboolean disableArgFile) { // for tools, this value remains 0 all the time. firstAppArgIndex = isJava ? -1 : 0; If I understand correctly, firstAppArgIndex set to 0 means that @argfile is disabled. On the other hand, ENABLE_ARG_FILES is also the flag to enable JDK tools to enable @argfile (disabled by default). It seems to me that you no longer needs isJava parameter here. Might be killSwitchOn can be replaced with firstAppArgIndex == 0 case (not sure - will let you think about that.) killSwitchOn is unclear what it means; maybe renaming it to disableArgFile? You may have to try building one tool with ENABLE_ARG_FILES to verify the change. JLI_PreprocessArg - as mentioned in the comment above, I suggest to have JLI_PreprocessArg to always return a non-null JLI_List and perhaps renaming the method to JLI_ExpandArgumentList // We can not just update the idx here because if -jar @file // still need expansion of @file to get the argument for -jar I only skimmed on the tests. ArgFileSyntax.java line 167-170: nit: indentation should be 4-space It might be useful to add a few negative tests to sanity check especially on the quotation. TestHelper.java has no change - should be reverted. Mandy
Thanks for reviewing, comment inline below,
On Aug 14, 2015, at 4:07 PM, Mandy Chung <mandy.chung@oracle.com> wrote:
On Aug 14, 2015, at 1:10 PM, Henry Jen <henry.jen@oracle.com> wrote:
Hi,
Another minor revision address comments, no real behavior changes except use JLI_StrCmp instead of JLI_StrCCmp in checkArg().
http://cr.openjdk.java.net/~henryjen/jdk9/8027634/v5/webrev/
main.c JLI_PreprocessingArg returns NULL if not @argfile
Would it be better to return JLI_List containing one element as argv[i]? We want to avoid new/free JLI_List for every argument and maybe a preallocated reusable copy for single element list to use (non-growable)?
Only argument with @prefix will be expanded, for any other cases, the function return NULL. This avoid any copy when not necessary. Regular argument is left alone, so that caller will just use the original value.
140 // Shallow free, we reuse the string to avoid copy 141 JLI_MemFree(argsInFile->elements); 142 JLI_MemFree(argsInFile);
What about adding JLI_List_free(JLI_List sl, boolean shadow)? This would be useful for the reusable single element list. Same comment applied to cmdtoargs.c
Thought of that, but decided to keep in place for clarity. When move into a function, then we want to check for NULL, and ask how shallow the free is, do we keep the array but free the list or both?
args.c You have #define NOT_FOUND -1 but NOT_FOUND is not used else where.
Used right under to initialize firstAppArgIndex, and should be used in following statement you shown.
void JLI_InitArgProcessing(jboolean isJava, jboolean disableArgFile) { // for tools, this value remains 0 all the time. firstAppArgIndex = isJava ? -1 : 0;
If I understand correctly, firstAppArgIndex set to 0 means that @argfile is disabled. On the other hand, ENABLE_ARG_FILES is also the flag to enable JDK tools to enable @argfile (disabled by default). It seems to me that you no longer needs isJava parameter here.
firstAppArgIndex is the first index of user’s application argument, has nothing to do with disable @argfile. This value remains 0 for launcher-based tools, and returned by JLI_GetAppArgIndex(). A tools can have ENABLE_ARG_FILES to enable argument expansion, but we still need to know it’s a launcher-based tool.
Might be killSwitchOn can be replaced with firstAppArgIndex == 0 case (not sure - will let you think about that.) killSwitchOn is unclear what it means; maybe renaming it to disableArgFile?
As explained earlier, firstAppArgIndex is different. We can rename killSwitchOn, it was clear as we discussed kill switch, the -X:disable-@files option. disableArgFile is used as the function argument, thus I left them as is.
You may have to try building one tool with ENABLE_ARG_FILES to verify the change.
java is built with the flag. Others are not. I tested with javac before the flag is reversed, so we know the flag is working.
JLI_PreprocessArg - as mentioned in the comment above, I suggest to have JLI_PreprocessArg to always return a non-null JLI_List and perhaps renaming the method to JLI_ExpandArgumentList
NULL is better than non-NULL copy so that we don’t need to copy the argument or check content when it’s not @ prefixed. The function was named JLI_ExpandArgumentList, later renamed to current form as it also check each argument to determine the location of first user argument for main class.
// We can not just update the idx here because if -jar @file // still need expansion of @file to get the argument for -jar
I only skimmed on the tests. ArgFileSyntax.java line 167-170: nit: indentation should be 4-space
You are right, thanks. When I was making those changes, I noted the editor changed something, but didn’t realize it’s the indentation.
It might be useful to add a few negative tests to sanity check especially on the quotation.
Make sense. Do you mean test cases that will fail launch of java?
TestHelper.java has no change - should be reverted.
It has two minor indentation clean up, I can revert them, but think since we were here, perhaps just fix it. Cheers, Henry
On Aug 16, 2015, at 4:51 PM, Henry Jen <henry.jen@oracle.com> wrote:
Thanks for reviewing, comment inline below,
On Aug 14, 2015, at 4:07 PM, Mandy Chung <mandy.chung@oracle.com> wrote:
On Aug 14, 2015, at 1:10 PM, Henry Jen <henry.jen@oracle.com> wrote:
Hi,
Another minor revision address comments, no real behavior changes except use JLI_StrCmp instead of JLI_StrCCmp in checkArg().
http://cr.openjdk.java.net/~henryjen/jdk9/8027634/v5/webrev/
main.c JLI_PreprocessingArg returns NULL if not @argfile
Would it be better to return JLI_List containing one element as argv[i]? We want to avoid new/free JLI_List for every argument and maybe a preallocated reusable copy for single element list to use (non-growable)?
Only argument with @prefix will be expanded, for any other cases, the function return NULL. This avoid any copy when not necessary. Regular argument is left alone, so that caller will just use the original value.
I think I may be talking at a different thing, guess you meant the make JLI_List JLI_PreprocessArg(const char*) to void JLI_PreprocessArg(JLI_List, const char*) and pass in a list to hold all the arguments. That is reasonable. Considered that but didn’t do it eventually because not much benefits and hide the “expansion” fact. Since now you mentioned this, it probably worth to do it that way. Cheers, Henry
On Aug 17, 2015, at 7:10 AM, Henry Jen <henry.jen@oracle.com> wrote:
On Aug 16, 2015, at 4:51 PM, Henry Jen <henry.jen@oracle.com> wrote:
Thanks for reviewing, comment inline below,
On Aug 14, 2015, at 4:07 PM, Mandy Chung <mandy.chung@oracle.com> wrote:
On Aug 14, 2015, at 1:10 PM, Henry Jen <henry.jen@oracle.com> wrote:
Hi,
Another minor revision address comments, no real behavior changes except use JLI_StrCmp instead of JLI_StrCCmp in checkArg().
http://cr.openjdk.java.net/~henryjen/jdk9/8027634/v5/webrev/
main.c JLI_PreprocessingArg returns NULL if not @argfile
Would it be better to return JLI_List containing one element as argv[i]? We want to avoid new/free JLI_List for every argument and maybe a preallocated reusable copy for single element list to use (non-growable)?
Only argument with @prefix will be expanded, for any other cases, the function return NULL. This avoid any copy when not necessary. Regular argument is left alone, so that caller will just use the original value.
I think I may be talking at a different thing, guess you meant the make JLI_List JLI_PreprocessArg(const char*) to void JLI_PreprocessArg(JLI_List, const char*) and pass in a list to hold all the arguments.
That is reasonable. Considered that but didn’t do it eventually because not much benefits and hide the “expansion” fact. Since now you mentioned this, it probably worth to do it that way.
Now I remember another reason I didn’t do it, because of the wildcard processing in cmdtoargs.c. I don’t want to convert every platform to use StdArg or maintain different version of PreprocesArgs. I am going to leave it as it. Cheers, Henry
On 08/17/2015 08:00 AM, Henry Jen wrote:
On Aug 17, 2015, at 7:10 AM, Henry Jen <henry.jen@oracle.com> wrote:
I think I may be talking at a different thing, guess you meant the make JLI_List JLI_PreprocessArg(const char*) to void JLI_PreprocessArg(JLI_List, const char*) and pass in a list to hold all the arguments.
That is reasonable. Considered that but didn’t do it eventually because not much benefits and hide the “expansion” fact. Since now you mentioned this, it probably worth to do it that way.
Now I remember another reason I didn’t do it, because of the wildcard processing in cmdtoargs.c. I don’t want to convert every platform to use StdArg or maintain different version of PreprocesArgs.
My suggestion is to keep JLI_PreprocessArg(const char* arg) is to return a non-null JLI_List. If the given arg is not @argfile, you return a static preallocated JLI_List with one single element and element[0] = JLI_StringDup(arg). The change in main.c and cmdtoargs.c add the arguments in either a JLI_List or StdArg. For the non-@argfile case, you need to do JLI_StringDup explicitly. For @argfile case, JLI_StringDup is done before it returns JLI_List. I also suggest to add JLI_FreeList(JLI_List*, boolean shallow_free) that should special case the static preallocated JLI_List to clear the count and element and not to free that list. I think that will simplify the code. Mandy
On Aug 16, 2015, at 4:51 PM, Henry Jen <henry.jen@oracle.com> wrote:
args.c You have #define NOT_FOUND -1 but NOT_FOUND is not used else where.
Used right under to initialize firstAppArgIndex, and should be used in following statement you shown.
Please change that.
void JLI_InitArgProcessing(jboolean isJava, jboolean disableArgFile) { // for tools, this value remains 0 all the time. firstAppArgIndex = isJava ? -1 : 0;
If I understand correctly, firstAppArgIndex set to 0 means that @argfile is disabled. On the other hand, ENABLE_ARG_FILES is also the flag to enable JDK tools to enable @argfile (disabled by default). It seems to me that you no longer needs isJava parameter here.
firstAppArgIndex is the first index of user’s application argument, has nothing to do with disable @argfile. This value remains 0 for launcher-based tools, and returned by JLI_GetAppArgIndex().
A tools can have ENABLE_ARG_FILES to enable argument expansion, but we still need to know it’s a launcher-based tool.
OK. firstAppArgIndex is not used in the parsing.
Might be killSwitchOn can be replaced with firstAppArgIndex == 0 case (not sure - will let you think about that.) killSwitchOn is unclear what it means; maybe renaming it to disableArgFile?
As explained earlier, firstAppArgIndex is different. We can rename killSwitchOn, it was clear as we discussed kill switch, the -X:disable-@files option. disableArgFile is used as the function argument, thus I left them as is.
It’s better to rename killSwitchOn to match what it means.
You may have to try building one tool with ENABLE_ARG_FILES to verify the change.
java is built with the flag. Others are not. I tested with javac before the flag is reversed, so we know the flag is working.
That’s good. I re-read that piece of code and that looks fine.
It might be useful to add a few negative tests to sanity check especially on the quotation.
Make sense. Do you mean test cases that will fail launch of java?
Yes. Mandy
On Aug 17, 2015, at 8:45 PM, Mandy Chung <mandy.chung@oracle.com> wrote:
OK. firstAppArgIndex is not used in the parsing.
It is set in args.c:127, and stop expansion on line 369. v6 is available at http://cr.openjdk.java.net/~henryjen/jdk9/8027634/v6/webrev/ Changes based on Mandy’s comment, - add a new test, basCases, which not escaping backslash properly and cause an open quote and failed java in ArgFileSyntac.java - rename killSwitchOn to stopExpansion in args.c - change to use NOT_FOUND at args.c:87 - fix indentation at ArgFileSyntax.java line 167-170 Cheers, Henry
I think using StdArg across all platforms will make the JLI_List management neater, and solve other issues, but that is an orthogonal fix. Kumar
On Aug 17, 2015, at 8:45 PM, Mandy Chung <mandy.chung@oracle.com> wrote:
OK. firstAppArgIndex is not used in the parsing. It is set in args.c:127, and stop expansion on line 369.
v6 is available at
http://cr.openjdk.java.net/~henryjen/jdk9/8027634/v6/webrev/
Changes based on Mandy’s comment,
- add a new test, basCases, which not escaping backslash properly and cause an open quote and failed java in ArgFileSyntac.java - rename killSwitchOn to stopExpansion in args.c - change to use NOT_FOUND at args.c:87 - fix indentation at ArgFileSyntax.java line 167-170
Cheers, Henry
On 08/17/2015 09:15 PM, Henry Jen wrote:
v6 is available at
http://cr.openjdk.java.net/~henryjen/jdk9/8027634/v6/webrev/
Thanks for the update. Looks fine. Mandy
v7 is up, changes are - Add formfeed character(\u000c) as while space character - Support escape \f for formfeed character in quote - Update java help output to include @<filepath> and -Xdisable-@files http://cr.openjdk.java.net/~henryjen/jdk9/8027634/v7/webrev/ Cheers, Henry
On Aug 18, 2015, at 11:00 AM, Mandy Chung <mandy.chung@oracle.com> wrote:
On 08/17/2015 09:15 PM, Henry Jen wrote:
v6 is available at
http://cr.openjdk.java.net/~henryjen/jdk9/8027634/v6/webrev/
Thanks for the update. Looks fine.
Mandy
On Aug 21, 2015, at 3:46 PM, Henry Jen <henry.jen@oracle.com> wrote:
v7 is up, changes are
- Add formfeed character(\u000c) as while space character - Support escape \f for formfeed character in quote - Update java help output to include @<filepath> and -Xdisable-@files
http://cr.openjdk.java.net/~henryjen/jdk9/8027634/v7/webrev/
The diff you sent in another mail is useful for just reviewing the delta change. That looks fine. launcher.properties @<filepath> following -splash seems a little awk but it's consistent with javac -help and that’s okay. Thanks Mandy
Henry, Looks good, and thank you for taking this on. Kumar On 8/21/2015 3:46 PM, Henry Jen wrote:
v7 is up, changes are
- Add formfeed character(\u000c) as while space character - Support escape \f for formfeed character in quote - Update java help output to include @<filepath> and -Xdisable-@files
http://cr.openjdk.java.net/~henryjen/jdk9/8027634/v7/webrev/
Cheers, Henry
On Aug 18, 2015, at 11:00 AM, Mandy Chung <mandy.chung@oracle.com> wrote:
On 08/17/2015 09:15 PM, Henry Jen wrote:
v6 is available at
http://cr.openjdk.java.net/~henryjen/jdk9/8027634/v6/webrev/ Thanks for the update. Looks fine.
Mandy
Kumar, Mandy, Thanks for reviewing. Cheers, Henry
On Aug 22, 2015, at 12:42 PM, Kumar Srinivasan <kumar.x.srinivasan@oracle.com> wrote:
Henry,
Looks good, and thank you for taking this on.
Kumar
On 8/21/2015 3:46 PM, Henry Jen wrote:
v7 is up, changes are
- Add formfeed character(\u000c) as while space character - Support escape \f for formfeed character in quote - Update java help output to include @<filepath> and -Xdisable-@files
http://cr.openjdk.java.net/~henryjen/jdk9/8027634/v7/webrev/
Cheers, Henry
On Aug 18, 2015, at 11:00 AM, Mandy Chung <mandy.chung@oracle.com> wrote:
On 08/17/2015 09:15 PM, Henry Jen wrote:
v6 is available at
http://cr.openjdk.java.net/~henryjen/jdk9/8027634/v6/webrev/ Thanks for the update. Looks fine.
Mandy
Hello Henry, Build changes look good to me. /Erik On 2015-07-10 05:48, Henry Jen wrote:
Hi,
Please review proposed patch for JDK-8027634[1]. This patch is to enable java support command line argument file like javac does. The implementation use the same syntax rule, which is implemented in CommandLine.java[3] with java.io.StreamTokenizer.
Some early comment is that we probably don’t need such complexity to support same syntax, also require to quote whole token is a little inconvenient. For example, must be -cp “c:\\foo bar\\lib;c:\\lib” instead of -cp c:\”foo bar”\lib;c:\lib.
I am debating if such compatibility is necessary useful, after all, easy and intuitive is more important, and with simpler rule, the implementation will be cleaner as well.
Anyhow, with the patch out, at least developer can build idk and have something to test with to see if this can fulfill their use cases.
Also, for tools other than java that use launcher, it’s possible to use -J@argfile to pass arguments. For example, if want to pass -J options to javac, it’s now possible to do so with javac -J@argfile, and put -J options in the argfile.
If the consensus is that such syntax compatibility is not important, we will go ahead remove the escaping support(except probably enable escape for quote itself) in quote, and maybe add support of quote within a token.
CCing build-dev for build changes, jdk9-dev for wider audience for tools.
Cheers, Henry
[1] https://bugs.openjdk.java.net/browse/JDK-8027634 [2] http://docs.oracle.com/javase/7/docs/technotes/tools/windows/javac.html#comm... [3] http://hg.openjdk.java.net/jdk9/jdk9/langtools/file/03e083639ee9/src/jdk.com...
participants (5)
-
Erik Joelsson
-
Henry Jen
-
Kumar Srinivasan
-
Magnus Ihse Bursie
-
Mandy Chung