Skip to content

[SPARK-18895][TESTS] Fix resource-closing-related and path-related test failures in identified ones on Windows #16305

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
HyukjinKwon wants to merge 3 commits into from
Closed

[SPARK-18895][TESTS] Fix resource-closing-related and path-related test failures in identified ones on Windows #16305

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
22cb8fc
Fix resource-closing-related and path-related test failures in identi…
HyukjinKwon Dec 16, 2016
00d5322
Fix EventLoggingListenerSuite too
HyukjinKwon Dec 16, 2016
0475ac5
Update comment for sure
HyukjinKwon Dec 17, 2016
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
Prev Previous commit
Update comment for sure
  • Loading branch information
@HyukjinKwon
HyukjinKwon committed Dec 17, 2016
commit 0475ac5448a8e8111a9e7beed1e8fd0d071b778e
8 changes: 5 additions & 3 deletions core/src/main/scala/org/apache/spark/deploy/RPackageUtils.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,9 +236,11 @@ private[deploy] object RPackageUtils extends Logging {
val zipOutputStream = new ZipOutputStream(new FileOutputStream(zipFile, false))
try {
filesToBundle.foreach { file =>
// Get the relative paths for proper naming in the zip file. Note that
// the separator should always be / for according to ZIP specification.
// `relPath` here should be, for example, "/packageTest/def.R" or "/test.R".
// Get the relative paths for proper naming in the ZIP file. Note that
// we convert dir to URI to force / and then remove trailing / that show up for
// directories because the separator should always be / for according to ZIP
// specification and therefore `relPath` here should be, for example,
// "/packageTest/def.R" or "/test.R".
val relPath = file.toURI.toString.replaceFirst(dir.toURI.toString.stripSuffix("/"), "")
Copy link
Member Author

@HyukjinKwon HyukjinKwon Dec 16, 2016

Choose a reason for hiding this comment

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

It should always be / according to ZIP specification (See 4.4.17 file name: (Variable) in https://pkware.cachefly.net/webdocs/casestudies/APPNOTE.TXT)

Copy link
Member Author

@HyukjinKwon HyukjinKwon Dec 16, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

cc @shivaram, could I please ask to take a look for this one? This fixes the test, SparkR zipping works properly on Windows in RPackageUtilsSuite.

Copy link
Contributor

@shivaram shivaram Dec 16, 2016

Choose a reason for hiding this comment

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

Yeah I think using / always is good. Could you write a small comment on what the toURI is accomplishing here (as opposed to the the getAbsolutePath we were using before)

Copy link
Member Author

@HyukjinKwon HyukjinKwon Dec 17, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Oh, I thought you meant writing a comment in the codes.. :).

it just replaces the \ on Windows to /. The reason for stripSuffix is, it seems it has a trailing / when the uri is known as a directory.

Copy link
Contributor

@shivaram shivaram Dec 17, 2016

Choose a reason for hiding this comment

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

You can put it in the code as well :) Something like We convert dir to URI to force / and then remove trailing / that show up for directories

Copy link
Member Author

@HyukjinKwon HyukjinKwon Dec 17, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

For example,

Before

  • Windows

    val a = file.getAbsolutePath // "C:\...\tmp\1481863447985-0"
val b = dir.getAbsolutePath  // "C:\...\tmp\1481863447985-0\test.R"
a.replaceFirst(b, "")        // java.util.regex.PatternSyntaxException: Unknown character property name {r} near index 4

    Full exception message:

    [info]   java.util.regex.PatternSyntaxException: Unknown character property name {r} near index 4
[info] C:\projects\spark\target\tmp\1481863447985-0
[info]     ^
[info]   at java.util.regex.Pattern.error(Pattern.java:1955)
[info]   at java.util.regex.Pattern.charPropertyNodeFor(Pattern.java:2781)
[info]   at java.util.regex.Pattern.family(Pattern.java:2736)
[info]   at java.util.regex.Pattern.sequence(Pattern.java:2076)
[info]   at java.util.regex.Pattern.expr(Pattern.java:1996)
[info]   at java.util.regex.Pattern.compile(Pattern.java:1696)
[info]   at java.util.regex.Pattern.<init>(Pattern.java:1351)
[info]   at java.util.regex.Pattern.compile(Pattern.java:1028)
[info]   at java.lang.String.replaceFirst(String.java:2178)
[info]   at org.apache.spark.deploy.RPackageUtils$$anonfun$zipRLibraries$2.apply(RPackageUtils.scala:235)

  • Linux/Mac

    val a = file.getAbsolutePath // "/var/.../T/1481938681657-0/test.R"
val b = dir.getAbsolutePath  // "/var/.../T/1481938681657-0"
a.replaceFirst(b, "")        // "/test.R"

After

  • Windows

    val a = file.toURI.toString              // "file:/C:/.../tmp/1481863447985-0/test.R"
val b = dir.toURI.toString               // "file:/C:/.../tmp/1481863447985-0/"
a.replaceFirst(b.stripSuffix("/"),  "")  // "/test.R"

  • Linux/Mac

    val a = file.toURI.toString              // "file:/var/.../T/1481938681657-0/test.R"
val b = dir.toURI.toString               // "file:/var/.../T/1481938681657-0/"
a.replaceFirst(b.stripSuffix("/"),  "")  // "/test.R"

Copy link
Member Author

@HyukjinKwon HyukjinKwon Dec 17, 2016

Choose a reason for hiding this comment

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

Sure, thanks!

val fis = new FileInputStream(file)
val zipEntry = new ZipEntry(relPath)
Expand Down