[GitHub] spark pull request #19657: [SPARK-22344][SPARKR] clean up install dir if run...

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/spark/pull/19657


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #19657: [SPARK-22344][SPARKR] clean up install dir if run...

[felixcheung]

GitHub user felixcheung reopened a pull request:

https://github.com/apache/spark/pull/19657

[SPARK-22344][SPARKR] clean up install dir if running test as source package

## What changes were proposed in this pull request?

remove spark if spark downloaded & installed

## How was this patch tested?

manually by building package
Jenkins, AppVeyor

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/felixcheung/spark rinstalldir

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/spark/pull/19657.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #19657


commit d4433e13565e9e3d41928e1d2262696204476341
Author: Felix Cheung 
Date:   2017-11-04T08:14:33Z

add flag to cleanup

commit 0ea7c9b1c26c604296c35bc1588a6a5606a10cb2
Author: Felix Cheung 
Date:   2017-11-05T03:21:26Z

no get0

commit d0064ca24339143aeac9f1ef78b924361f908248
Author: Felix Cheung 
Date:   2017-11-07T10:27:13Z

make into function

commit 31f3bd06cc7d2b7bf482eddfe2f2738244cfbca7
Author: Felix Cheung 
Date:   2017-11-07T10:50:55Z

fix lint

commit ca5349bfc0dae03c2402b104e51c78a841541b09
Author: Felix Cheung 
Date:   2017-11-07T10:55:27Z

comment

commit f2aa5b7e12ed36e7b56610e695615260643f952f
Author: Felix Cheung 
Date:   2017-11-07T17:31:16Z

fix windows

commit 90d36c9ee3b0aed60ac9343e05b44366d1d2bf43
Author: Felix Cheung 
Date:   2017-11-07T17:38:12Z

more test

commit f21a90bef2a08c9d4cfdcc6588fb2da64679b4ec
Author: Felix Cheung 
Date:   2017-11-07T17:39:05Z

fix

commit 18e238a62d53de5a73283a741c1a9bb8230f4484
Author: Felix Cheung 
Date:   2017-11-08T04:54:53Z

fix 2




---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #19657: [SPARK-22344][SPARKR] clean up install dir if run...

[felixcheung]

Github user felixcheung closed the pull request at:

https://github.com/apache/spark/pull/19657


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #19657: [SPARK-22344][SPARKR] clean up install dir if run...

[felixcheung]

Github user felixcheung closed the pull request at:

https://github.com/apache/spark/pull/19657


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #19657: [SPARK-22344][SPARKR] clean up install dir if run...

[felixcheung]

GitHub user felixcheung reopened a pull request:

https://github.com/apache/spark/pull/19657

[SPARK-22344][SPARKR] clean up install dir if running test as source package

## What changes were proposed in this pull request?

remove spark if spark downloaded & installed

## How was this patch tested?

manually by building package
Jenkins, AppVeyor

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/felixcheung/spark rinstalldir

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/spark/pull/19657.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #19657


commit d4433e13565e9e3d41928e1d2262696204476341
Author: Felix Cheung 
Date:   2017-11-04T08:14:33Z

add flag to cleanup

commit 0ea7c9b1c26c604296c35bc1588a6a5606a10cb2
Author: Felix Cheung 
Date:   2017-11-05T03:21:26Z

no get0

commit d0064ca24339143aeac9f1ef78b924361f908248
Author: Felix Cheung 
Date:   2017-11-07T10:27:13Z

make into function

commit 31f3bd06cc7d2b7bf482eddfe2f2738244cfbca7
Author: Felix Cheung 
Date:   2017-11-07T10:50:55Z

fix lint

commit ca5349bfc0dae03c2402b104e51c78a841541b09
Author: Felix Cheung 
Date:   2017-11-07T10:55:27Z

comment

commit f2aa5b7e12ed36e7b56610e695615260643f952f
Author: Felix Cheung 
Date:   2017-11-07T17:31:16Z

fix windows

commit 90d36c9ee3b0aed60ac9343e05b44366d1d2bf43
Author: Felix Cheung 
Date:   2017-11-07T17:38:12Z

more test

commit f21a90bef2a08c9d4cfdcc6588fb2da64679b4ec
Author: Felix Cheung 
Date:   2017-11-07T17:39:05Z

fix

commit 18e238a62d53de5a73283a741c1a9bb8230f4484
Author: Felix Cheung 
Date:   2017-11-08T04:54:53Z

fix 2




---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #19657: [SPARK-22344][SPARKR] clean up install dir if run...

[felixcheung]

GitHub user felixcheung reopened a pull request:

https://github.com/apache/spark/pull/19657

[SPARK-22344][SPARKR] clean up install dir if running test as source package

## What changes were proposed in this pull request?

remove spark if spark downloaded & installed

## How was this patch tested?

manually by building package
Jenkins, AppVeyor

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/felixcheung/spark rinstalldir

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/spark/pull/19657.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #19657


commit d4433e13565e9e3d41928e1d2262696204476341
Author: Felix Cheung 
Date:   2017-11-04T08:14:33Z

add flag to cleanup

commit 0ea7c9b1c26c604296c35bc1588a6a5606a10cb2
Author: Felix Cheung 
Date:   2017-11-05T03:21:26Z

no get0

commit d0064ca24339143aeac9f1ef78b924361f908248
Author: Felix Cheung 
Date:   2017-11-07T10:27:13Z

make into function

commit 31f3bd06cc7d2b7bf482eddfe2f2738244cfbca7
Author: Felix Cheung 
Date:   2017-11-07T10:50:55Z

fix lint

commit ca5349bfc0dae03c2402b104e51c78a841541b09
Author: Felix Cheung 
Date:   2017-11-07T10:55:27Z

comment

commit f2aa5b7e12ed36e7b56610e695615260643f952f
Author: Felix Cheung 
Date:   2017-11-07T17:31:16Z

fix windows

commit 90d36c9ee3b0aed60ac9343e05b44366d1d2bf43
Author: Felix Cheung 
Date:   2017-11-07T17:38:12Z

more test

commit f21a90bef2a08c9d4cfdcc6588fb2da64679b4ec
Author: Felix Cheung 
Date:   2017-11-07T17:39:05Z

fix

commit 18e238a62d53de5a73283a741c1a9bb8230f4484
Author: Felix Cheung 
Date:   2017-11-08T04:54:53Z

fix 2




---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #19657: [SPARK-22344][SPARKR] clean up install dir if run...

[felixcheung]

Github user felixcheung closed the pull request at:

https://github.com/apache/spark/pull/19657


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #19657: [SPARK-22344][SPARKR] clean up install dir if run...

[felixcheung]

Github user felixcheung commented on a diff in the pull request:

https://github.com/apache/spark/pull/19657#discussion_r149448556
  
--- Diff: R/pkg/tests/fulltests/test_utils.R ---
@@ -236,4 +236,23 @@ test_that("basenameSansExtFromUrl", {
   expect_equal(basenameSansExtFromUrl(z), "spark-2.1.0--hive")
 })
 
+test_that("getOne", {
+  dummy <- getOne(".dummyValue", envir = new.env(), ifnotfound = FALSE)
+  expect_equal(dummy, FALSE)
+})
+
+test_that("traverseParentDirs", {
+  if (is_windows()) {
+dirs <- 
traverseParentDirs("c:\\Users\\user\\AppData\\Local\\Apache\\Spark\\Cache\\spark2.2",
 3)
+expect <- 
c("c:\\Users\\user\\AppData\\Local\\Apache\\Spark\\Cache\\spark2.2",
+"c:\\Users\\user\\AppData\\Local\\Apache\\Spark\\Cache",
+"c:\\Users\\user\\AppData\\Local\\Apache\\Spark",
+"c:\\Users\\user\\AppData\\Local\\Apache")
+  } else {
+dirs <- 
traverseParentDirs("/Users/user/Library/Caches/spark/spark2.2", 1)
--- End diff --

sure, but well hopefully the implementation is not platform dependent, 
otherwise we will need to test linux as well as osx
 (and it doesn't check if the path is valid/present) 


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #19657: [SPARK-22344][SPARKR] clean up install dir if run...

[shivaram]

Github user shivaram commented on a diff in the pull request:

https://github.com/apache/spark/pull/19657#discussion_r149436164
  
--- Diff: R/pkg/tests/fulltests/test_utils.R ---
@@ -236,4 +236,23 @@ test_that("basenameSansExtFromUrl", {
   expect_equal(basenameSansExtFromUrl(z), "spark-2.1.0--hive")
 })
 
+test_that("getOne", {
+  dummy <- getOne(".dummyValue", envir = new.env(), ifnotfound = FALSE)
+  expect_equal(dummy, FALSE)
+})
+
+test_that("traverseParentDirs", {
+  if (is_windows()) {
+dirs <- 
traverseParentDirs("c:\\Users\\user\\AppData\\Local\\Apache\\Spark\\Cache\\spark2.2",
 3)
+expect <- 
c("c:\\Users\\user\\AppData\\Local\\Apache\\Spark\\Cache\\spark2.2",
+"c:\\Users\\user\\AppData\\Local\\Apache\\Spark\\Cache",
+"c:\\Users\\user\\AppData\\Local\\Apache\\Spark",
+"c:\\Users\\user\\AppData\\Local\\Apache")
+  } else {
+dirs <- 
traverseParentDirs("/Users/user/Library/Caches/spark/spark2.2", 1)
--- End diff --

can we also test the linux one (`/home/user/.cache`) - Just want to make 
sure we will not miss hidden files / directories.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #19657: [SPARK-22344][SPARKR] clean up install dir if run...

[felixcheung]

Github user felixcheung commented on a diff in the pull request:

https://github.com/apache/spark/pull/19657#discussion_r149331874
  
--- Diff: R/pkg/vignettes/sparkr-vignettes.Rmd ---
@@ -1183,3 +1183,24 @@ env | map
 ```{r, echo=FALSE}
 sparkR.session.stop()
 ```
+
+```{r cleanup, include=FALSE}
+# clean up if Spark was downloaded
+# get0 not supported before R 3.2.0
+sparkDownloaded <- mget(".sparkDownloaded"[1L],
--- End diff --

since this needs to go into 2.2, let's not add a public method for now, we 
could revisit this for 2.3


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #19657: [SPARK-22344][SPARKR] clean up install dir if run...

[shivaram]

Github user shivaram commented on a diff in the pull request:

https://github.com/apache/spark/pull/19657#discussion_r149174223
  
--- Diff: R/pkg/R/install.R ---
@@ -152,6 +152,9 @@ install.spark <- function(hadoopVersion = "2.7", 
mirrorUrl = NULL,
  })
   if (!tarExists || overwrite || !success) {
 unlink(packageLocalPath)
+if (success) {
--- End diff --

Hmm ok that still looks weird in the code. Maybe add a comment before this 
of the form `If we downloaded a tarfile or overwrote it, set sparkDownloaded 
flag` ?


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #19657: [SPARK-22344][SPARKR] clean up install dir if run...

[shivaram]

Github user shivaram commented on a diff in the pull request:

https://github.com/apache/spark/pull/19657#discussion_r149140297
  
--- Diff: R/pkg/vignettes/sparkr-vignettes.Rmd ---
@@ -1183,3 +1183,24 @@ env | map
 ```{r, echo=FALSE}
 sparkR.session.stop()
 ```
+
+```{r cleanup, include=FALSE}
+# clean up if Spark was downloaded
+# get0 not supported before R 3.2.0
+sparkDownloaded <- mget(".sparkDownloaded"[1L],
--- End diff --

Yeah - lets do that. I might even be fine with exposing an external 
function 'uninstallSpark' or 'uninstallDownloadedSpark' ?


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #19657: [SPARK-22344][SPARKR] clean up install dir if run...

[felixcheung]

Github user felixcheung commented on a diff in the pull request:

https://github.com/apache/spark/pull/19657#discussion_r149017035
  
--- Diff: R/pkg/R/install.R ---
@@ -152,6 +152,9 @@ install.spark <- function(hadoopVersion = "2.7", 
mirrorUrl = NULL,
  })
   if (!tarExists || overwrite || !success) {
 unlink(packageLocalPath)
+if (success) {
--- End diff --

basically if `success` is TRUE then only the other cases matter: 
`!tarExists || overwrite` - that's the exact condition where the download would 
have occurred in L126 which is the else case of `tarExists && !overwrite` on 
L123


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #19657: [SPARK-22344][SPARKR] clean up install dir if run...

[felixcheung]

Github user felixcheung commented on a diff in the pull request:

https://github.com/apache/spark/pull/19657#discussion_r149015420
  
--- Diff: R/pkg/vignettes/sparkr-vignettes.Rmd ---
@@ -1183,3 +1183,24 @@ env | map
 ```{r, echo=FALSE}
 sparkR.session.stop()
 ```
+
+```{r cleanup, include=FALSE}
+# clean up if Spark was downloaded
+# get0 not supported before R 3.2.0
+sparkDownloaded <- mget(".sparkDownloaded"[1L],
--- End diff --

yes! but both call sites are outside of the package technically and we 
would need to call the private function with `SparkR:::`, which is kinda ugly...

I guess we could wrap the entire cleanup thing into a private/internal 
function (since it has to access a private flag anyway..)


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #19657: [SPARK-22344][SPARKR] clean up install dir if run...

[felixcheung]

Github user felixcheung commented on a diff in the pull request:

https://github.com/apache/spark/pull/19657#discussion_r149014391
  
--- Diff: R/pkg/tests/run-all.R ---
@@ -60,3 +60,22 @@ if (identical(Sys.getenv("NOT_CRAN"), "true")) {
NULL,
"summary")
 }
+
+# clean up if Spark was downloaded for the test run
+# get0 not supported before R 3.2.0
+sparkDownloaded <- mget(".sparkDownloaded"[1L],
+envir = SparkR:::.sparkREnv,
+inherits = TRUE,
+ifnotfound = list(FALSE))[[1L]]
+if (sparkDownloaded) {
+  unlink(sparkDownloadedDir, recursive = TRUE, force = TRUE)
+
+  # .cache/spark, or on Windows, LOCALAPPDATA\Apache\Spark\Cache (there 
are 3 levels)
+  parentDir <- SparkR:::sparkCachePath()
+  dirs <- list(parentDir, dirname(parentDir), dirname(dirname(parentDir)))
+  lapply(dirs, function(d) {
+if (length(list.files(d, all.files = TRUE, include.dirs = TRUE, no.. = 
TRUE)) == 0) {
--- End diff --

yes, it would, as commented above 
https://github.com/apache/spark/pull/19657#issuecomment-341880175
problem is we have no idea whether the vignettes build is going to happen 
or not (it could easily be disabled via commandline)


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #19657: [SPARK-22344][SPARKR] clean up install dir if run...

[shivaram]

Github user shivaram commented on a diff in the pull request:

https://github.com/apache/spark/pull/19657#discussion_r148979368
  
--- Diff: R/pkg/R/install.R ---
@@ -152,6 +152,9 @@ install.spark <- function(hadoopVersion = "2.7", 
mirrorUrl = NULL,
  })
   if (!tarExists || overwrite || !success) {
 unlink(packageLocalPath)
+if (success) {
--- End diff --

why should this be in inside this `if` block for `overwrite || !success` -- 
Can't we just have it outside this if as an else below ?


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #19657: [SPARK-22344][SPARKR] clean up install dir if run...

[shivaram]

Github user shivaram commented on a diff in the pull request:

https://github.com/apache/spark/pull/19657#discussion_r148979827
  
--- Diff: R/pkg/vignettes/sparkr-vignettes.Rmd ---
@@ -1183,3 +1183,24 @@ env | map
 ```{r, echo=FALSE}
 sparkR.session.stop()
 ```
+
+```{r cleanup, include=FALSE}
+# clean up if Spark was downloaded
+# get0 not supported before R 3.2.0
+sparkDownloaded <- mget(".sparkDownloaded"[1L],
--- End diff --

Can we make this an internal util function and call it from here ?


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #19657: [SPARK-22344][SPARKR] clean up install dir if run...

[shivaram]

Github user shivaram commented on a diff in the pull request:

https://github.com/apache/spark/pull/19657#discussion_r148979924
  
--- Diff: R/pkg/tests/run-all.R ---
@@ -60,3 +60,22 @@ if (identical(Sys.getenv("NOT_CRAN"), "true")) {
NULL,
"summary")
 }
+
+# clean up if Spark was downloaded for the test run
+# get0 not supported before R 3.2.0
+sparkDownloaded <- mget(".sparkDownloaded"[1L],
+envir = SparkR:::.sparkREnv,
+inherits = TRUE,
+ifnotfound = list(FALSE))[[1L]]
+if (sparkDownloaded) {
+  unlink(sparkDownloadedDir, recursive = TRUE, force = TRUE)
+
+  # .cache/spark, or on Windows, LOCALAPPDATA\Apache\Spark\Cache (there 
are 3 levels)
+  parentDir <- SparkR:::sparkCachePath()
+  dirs <- list(parentDir, dirname(parentDir), dirname(dirname(parentDir)))
+  lapply(dirs, function(d) {
+if (length(list.files(d, all.files = TRUE, include.dirs = TRUE, no.. = 
TRUE)) == 0) {
--- End diff --

One consequence of this is that if we run `R CMD check --as-cran` we will 
do the download twice -- once for the unit tests and once for the vignettes


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #19657: [SPARK-22344][SPARKR] clean up install dir if run...

[felixcheung]

GitHub user felixcheung opened a pull request:

https://github.com/apache/spark/pull/19657

[SPARK-22344][SPARKR] clean up install dir if running test as source package

## What changes were proposed in this pull request?

remove spark if spark downloaded & installed

## How was this patch tested?

manually by building package

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/felixcheung/spark rinstalldir

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/spark/pull/19657.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #19657


commit d4433e13565e9e3d41928e1d2262696204476341
Author: Felix Cheung 
Date:   2017-11-04T08:14:33Z

add flag to cleanup




---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org