[GitHub] spark pull request #19657: [SPARK-22344][SPARKR] clean up install dir if run...
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...
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 CheungDate: 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...
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...
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...
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 CheungDate: 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...
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 CheungDate: 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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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 CheungDate: 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