Skip to content

Commit 4f66bf5

Browse files
HyukjinKwonshivaram
authored andcommitted
[SPARK-8603][SPARKR] Use shell() instead of system2() for SparkR on Windows
## What changes were proposed in this pull request? This PR corrects SparkR to use `shell()` instead of `system2()` on Windows. Using `system2(...)` on Windows does not process windows file separator `\`. `shell(tralsate = TRUE, ...)` can treat this problem. So, this was changed to be chosen according to OS. Existing tests were failed on Windows due to this problem. For example, those were failed. ``` 8. Failure: sparkJars tag in SparkContext (test_includeJAR.R#34) 9. Failure: sparkJars tag in SparkContext (test_includeJAR.R#36) ``` The cases above were due to using of `system2`. In addition, this PR also fixes some tests failed on Windows. ``` 5. Failure: sparkJars sparkPackages as comma-separated strings (test_context.R#128) 6. Failure: sparkJars sparkPackages as comma-separated strings (test_context.R#131) 7. Failure: sparkJars sparkPackages as comma-separated strings (test_context.R#134) ``` The cases above were due to a weird behaviour of `normalizePath()`. On Linux, if the path does not exist, it just prints out the input but it prints out including the current path on Windows. ```r # On Linus path <- normalizePath("aa") print(path) [1] "aa" # On Windows path <- normalizePath("aa") print(path) [1] "C:\\Users\\aa" ``` ## How was this patch tested? Jenkins tests and manually tested in a Window machine as below: Here is the [stdout](https://gist.github.com/HyukjinKwon/4bf35184f3a30f3bce987a58ec2bbbab) of testing. Closes #7025 Author: hyukjinkwon <gurwls223@gmail.com> Author: Hyukjin Kwon <gurwls223@gmail.com> Author: Prakash PC <prakash.chinnu@gmail.com> Closes #13165 from HyukjinKwon/pr/7025. (cherry picked from commit 1c40373) Signed-off-by: Shivaram Venkataraman <shivaram@cs.berkeley.edu>
1 parent 8e26b74 commit 4f66bf5

6 files changed

Lines changed: 44 additions & 10 deletions

File tree

R/WINDOWS.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,6 @@ To run the SparkR unit tests on Windows, the following steps are required —ass
2828

2929
```
3030
R -e "install.packages('testthat', repos='http://cran.us.r-project.org')"
31-
.\bin\spark-submit2.cmd --conf spark.hadoop.fs.defualt.name="file:///" R\pkg\tests\run-all.R
31+
.\bin\spark-submit2.cmd --conf spark.hadoop.fs.default.name="file:///" R\pkg\tests\run-all.R
3232
```
3333

R/pkg/R/client.R

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ determineSparkSubmitBin <- function() {
3838
if (.Platform$OS.type == "unix") {
3939
sparkSubmitBinName <- "spark-submit"
4040
} else {
41-
sparkSubmitBinName <- "spark-submit.cmd"
41+
sparkSubmitBinName <- "spark-submit2.cmd"
4242
}
4343
sparkSubmitBinName
4444
}
@@ -69,5 +69,5 @@ launchBackend <- function(args, sparkHome, jars, sparkSubmitOpts, packages) {
6969
}
7070
combinedArgs <- generateSparkSubmitArgs(args, sparkHome, jars, sparkSubmitOpts, packages)
7171
cat("Launching java with spark-submit command", sparkSubmitBin, combinedArgs, "\n")
72-
invisible(system2(sparkSubmitBin, combinedArgs, wait = F))
72+
invisible(launchScript(sparkSubmitBin, combinedArgs))
7373
}

R/pkg/R/utils.R

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -664,3 +664,12 @@ varargsToJProperties <- function(...) {
664664
}
665665
props
666666
}
667+
668+
launchScript <- function(script, combinedArgs, capture = FALSE) {
669+
if (.Platform$OS.type == "windows") {
670+
scriptWithArgs <- paste(script, combinedArgs, sep = " ")
671+
shell(scriptWithArgs, translate = TRUE, wait = capture, intern = capture) # nolint
672+
} else {
673+
system2(script, combinedArgs, wait = capture, stdout = capture)
674+
}
675+
}
Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
#
2+
# Licensed to the Apache Software Foundation (ASF) under one or more
3+
# contributor license agreements. See the NOTICE file distributed with
4+
# this work for additional information regarding copyright ownership.
5+
# The ASF licenses this file to You under the Apache License, Version 2.0
6+
# (the "License"); you may not use this file except in compliance with
7+
# the License. You may obtain a copy of the License at
8+
#
9+
# http://www.apache.org/licenses/LICENSE-2.0
10+
#
11+
# Unless required by applicable law or agreed to in writing, software
12+
# distributed under the License is distributed on an "AS IS" BASIS,
13+
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
14+
# See the License for the specific language governing permissions and
15+
# limitations under the License.
16+
#
17+
context("Windows-specific tests")
18+
19+
test_that("sparkJars tag in SparkContext", {
20+
if (.Platform$OS.type != "windows") {
21+
skip("This test is only for Windows, skipped")
22+
}
23+
testOutput <- launchScript("ECHO", "a/b/c", capture = TRUE)
24+
abcPath <- testOutput[1]
25+
expect_equal(abcPath, "a\\b\\c")
26+
})

R/pkg/inst/tests/testthat/test_context.R

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -129,13 +129,13 @@ test_that("getClientModeSparkSubmitOpts() returns spark-submit args from whiteli
129129
test_that("sparkJars sparkPackages as comma-separated strings", {
130130
expect_warning(processSparkJars(" a, b "))
131131
jars <- suppressWarnings(processSparkJars(" a, b "))
132-
expect_equal(jars, c("a", "b"))
132+
expect_equal(lapply(jars, basename), list("a", "b"))
133133

134134
jars <- suppressWarnings(processSparkJars(" abc ,, def "))
135-
expect_equal(jars, c("abc", "def"))
135+
expect_equal(lapply(jars, basename), list("abc", "def"))
136136

137137
jars <- suppressWarnings(processSparkJars(c(" abc ,, def ", "", "xyz", " ", "a,b")))
138-
expect_equal(jars, c("abc", "def", "xyz", "a", "b"))
138+
expect_equal(lapply(jars, basename), list("abc", "def", "xyz", "a", "b"))
139139

140140
p <- processSparkPackages(c("ghi", "lmn"))
141141
expect_equal(p, c("ghi", "lmn"))

R/pkg/inst/tests/testthat/test_includeJAR.R

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -21,10 +21,9 @@ runScript <- function() {
2121
sparkTestJarPath <- "R/lib/SparkR/test_support/sparktestjar_2.10-1.0.jar"
2222
jarPath <- paste("--jars", shQuote(file.path(sparkHome, sparkTestJarPath)))
2323
scriptPath <- file.path(sparkHome, "R/lib/SparkR/tests/testthat/jarTest.R")
24-
submitPath <- file.path(sparkHome, "bin/spark-submit")
25-
res <- system2(command = submitPath,
26-
args = c(jarPath, scriptPath),
27-
stdout = TRUE)
24+
submitPath <- file.path(sparkHome, paste("bin/", determineSparkSubmitBin(), sep = ""))
25+
combinedArgs <- paste(jarPath, scriptPath, sep = " ")
26+
res <- launchScript(submitPath, combinedArgs, capture = TRUE)
2827
tail(res, 2)
2928
}
3029

0 commit comments

Comments
 (0)