Skip to content

[SPARK-6435] spark-shell --jars option does not add all jars to classpath #5227

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
tsudukim wants to merge 6 commits into from
Closed

[SPARK-6435] spark-shell --jars option does not add all jars to classpath #5227

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
04f4291
[SPARK-6435] spark-shell --jars option does not add all jars to class…
tsudukim Mar 27, 2015
2a332e5
Merge branch 'master' into feature/SPARK-6435-2
tsudukim Apr 2, 2015
0d4dc41
- escaped comman and semicolon in CommandBuilderUtils.java
tsudukim Apr 2, 2015
1fee420
fixed test code for escaping '='.
tsudukim Apr 2, 2015
60789a7
Merge branch 'master' of https://github.com/apache/spark into feature…
tsudukim Apr 23, 2015
ac55787
removed unnecessary argument.
tsudukim Apr 27, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion bin/spark-class2.cmd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,10 @@ if not "x%JAVA_HOME%"=="x" set RUNNER=%JAVA_HOME%\bin\java

rem The launcher library prints the command to be executed in a single line suitable for being
rem executed by the batch interpreter. So read all the output of the launcher into a variable.
for /f "tokens=*" %%i in ('cmd /C ""%RUNNER%" -cp %LAUNCH_CLASSPATH% org.apache.spark.launcher.Main %*"') do (
set LAUNCHER_OUTPUT=%temp%\spark-class-launcher-output-%RANDOM%.txt
"%RUNNER%" -cp %LAUNCH_CLASSPATH% org.apache.spark.launcher.Main %* > %LAUNCHER_OUTPUT%
for /f "tokens=*" %%i in (%LAUNCHER_OUTPUT%) do (
set SPARK_CMD=%%i
)
del %LAUNCHER_OUTPUT%
%SPARK_CMD%
9 changes: 4 additions & 5 deletions launcher/src/main/java/org/apache/spark/launcher/CommandBuilderUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ static String quoteForBatchScript(String arg) {
boolean needsQuotes = false;
for (int i = 0; i < arg.length(); i++) {
int c = arg.codePointAt(i);
if (Character.isWhitespace(c) || c == '"' || c == '=') {
if (Character.isWhitespace(c) || c == '"' || c == '=' || c == ',' || c == ';') {
needsQuotes = true;
break;
}
Expand All @@ -261,15 +261,14 @@ static String quoteForBatchScript(String arg) {
quoted.append('"');
break;

case '=':
quoted.append('^');
break;

default:
Copy link
Contributor

@vanzin vanzin Apr 3, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you run SparkLauncherSuite on Windows? I added this specifically to fix an issue with that path (see 92a9cfb).

Copy link
Contributor Author

@tsudukim tsudukim Apr 27, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've run SparkLauncherSuite on Windows and it's OK.
If double-quotation is parsed properly, = in double-quotation is not need to be escaped.

break;
}
quoted.appendCodePoint(cp);
}
if (arg.codePointAt(arg.length() - 1) == '\\') {
quoted.append("\\");
Copy link
Member

@srowen srowen Apr 2, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is indented too far. Why would a backslash need escaping only in the final position?

Copy link
Contributor Author

@tsudukim tsudukim Apr 2, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In batch, backslash \ followed by double-quotation " is parsed as a escape charactor.
For example, \" means " itself.
We face a problem in this case:
java.exe -cp "C:\path\to\directory\" ...
The closing " doesn't work properly, so we should escape only the last \.

Copy link
Member

@srowen srowen Apr 2, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I assumed you would escape all backslashes if they are the escape character. Is that not necessary?

Copy link
Contributor Author

@tsudukim tsudukim Apr 2, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Backslash is escape character only when followed by ". I assumed it's the only case when the string ends with backslash.

Copy link
Member

@srowen srowen Apr 2, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, so it's not a general escape character, got it.

}
quoted.append("\"");
return quoted.toString();
}
Expand Down
6 changes: 1 addition & 5 deletions launcher/src/main/java/org/apache/spark/launcher/Main.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,12 +101,9 @@ public static void main(String[] argsArray) throws Exception {
* The method quotes all arguments so that spaces are handled as expected. Quotes within arguments
* are "double quoted" (which is batch for escaping a quote). This page has more details about
* quoting and other batch script fun stuff: http://ss64.com/nt/syntax-esc.html
*
* The command is executed using "cmd /c" and formatted in single line, since that's the
* easiest way to consume this from a batch script (see spark-class2.cmd).
*/
private static String prepareWindowsCommand(List<String> cmd, Map<String, String> childEnv) {
StringBuilder cmdline = new StringBuilder("cmd /c \"");
StringBuilder cmdline = new StringBuilder();
for (Map.Entry<String, String> e : childEnv.entrySet()) {
cmdline.append(String.format("set %s=%s", e.getKey(), e.getValue()));
cmdline.append(" && ");
Expand All @@ -115,7 +112,6 @@ private static String prepareWindowsCommand(List<String> cmd, Map<String, String
cmdline.append(quoteForBatchScript(arg));
cmdline.append(" ");
}
cmdline.append("\"");
return cmdline.toString();
}

Expand Down
5 changes: 4 additions & 1 deletion launcher/src/test/java/org/apache/spark/launcher/CommandBuilderUtilsSuite.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,10 @@ public void testWindowsBatchQuoting() {
assertEquals("\"a b c\"", quoteForBatchScript("a b c"));
assertEquals("\"a \"\"b\"\" c\"", quoteForBatchScript("a \"b\" c"));
assertEquals("\"a\"\"b\"\"c\"", quoteForBatchScript("a\"b\"c"));
assertEquals("\"ab^=\"\"cd\"\"\"", quoteForBatchScript("ab=\"cd\""));
assertEquals("\"ab=\"\"cd\"\"\"", quoteForBatchScript("ab=\"cd\""));
assertEquals("\"a,b,c\"", quoteForBatchScript("a,b,c"));
assertEquals("\"a;b;c\"", quoteForBatchScript("a;b;c"));
assertEquals("\"a,b,c\\\\\"", quoteForBatchScript("a,b,c\\"));
}

@Test
Expand Down