diff --git a/DESCRIPTION b/DESCRIPTION index 1ccee772..93683649 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -5,7 +5,7 @@ Description: Simplifies regression tests by comparing objects produced by test the tests pass, otherwise execution stops with error details. If in interactive mode, tests can be reviewed through the provided interactive environment. -Version: 1.4.17.9002 +Version: 1.4.17.9003 Authors@R: c( person( "Brodie", "Gaslam", email="brodie.gaslam@yahoo.com", diff --git a/NEWS.md b/NEWS.md index 55454349..95adefd9 100644 --- a/NEWS.md +++ b/NEWS.md @@ -5,6 +5,12 @@ * [#288](https://github.com/brodieG/unitizer/issues/288): Prevent upgrade prompt in non-interactive mode interrupting result display (regression introduced in 1.4.15). +* [#252](https://github.com/brodieG/unitizer/issues/252), + [#253](https://github.com/brodieG/unitizer/issues/252): Better documentation + of feature incompatibility with `devtools::load_all`, and more graceful + recovery from failures caused by the incompatibility. This only affects + `unitizer` sessions run with package search path management enabled (h/t + @blset). ## v1.4.16-17 diff --git a/R/global.R b/R/global.R index 2ea32a62..ae5728b1 100644 --- a/R/global.R +++ b/R/global.R @@ -1,17 +1,17 @@ # Copyright (C) 2022 Brodie Gaslam -# +# # This file is part of "unitizer" -# +# # This program is free software: you can redistribute it and/or modify # it under the terms of the GNU General Public License as published by # the Free Software Foundation, either version 2 of the License, or # (at your option) any later version. -# +# # This program is distributed in the hope that it will be useful, # but WITHOUT ANY WARRANTY; without even the implied warranty of # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the # GNU General Public License for more details. -# +# # Go to for a copy of the license. #' @include is.R @@ -485,17 +485,30 @@ unitizerGlobal <- setRefClass( ' stopifnot(is(to, "unitizerGlobalIndices")) + success <- TRUE if(status@search.path && to@search.path){ - search_path_update(to@search.path, .self) + fail <- + inherits(try(search_path_update(to@search.path, .self)), "try-error") + success <- success && !fail } - if(status@namespaces && to@namespaces) - namespace_update(to@namespaces, .self) - if(status@options && to@options) - options_update(tracking@options[[to@options]]) - if(status@working.directory && to@working.directory) - setwd( - tracking@working.directory[[to@working.directory]] + if(status@namespaces && to@namespaces) { + fail <- + inherits(try(namespace_update(to@namespaces, .self)), "try-error") + success <- success && !fail + } + if(status@options && to@options) { + fail <- inherits( + try(options_update(tracking@options[[to@options]])), "try-error" + ) + success <- success && !fail + } + if(status@working.directory && to@working.directory) { + fail <- inherits( + try(setwd(tracking@working.directory[[to@working.directory]])), + "try-error" ) + success <- success && !fail + } if(status@random.seed && to@random.seed) { if(is.null(tracking@random.seed[[to@random.seed]])) { if(exists(".Random.seed", .GlobalEnv, inherits=FALSE)) @@ -504,6 +517,7 @@ unitizerGlobal <- setRefClass( assign( ".Random.seed", tracking@random.seed[[to@random.seed]], .GlobalEnv ) } } + if(!success) stop("Global state reset failed, see prior error.") indices.last <<- to indices.last }, diff --git a/R/search.R b/R/search.R index 5aaaa054..135c4d79 100644 --- a/R/search.R +++ b/R/search.R @@ -599,8 +599,9 @@ reattach <- function(pos, name, type, data, extra=NULL, global) { if(inherits(lib.try, "try-error")) { # nocov start warning( - "Internal Warning: unable to fully restore search path; see prior ", - "error. Contact maintainer if this warning persists.", + "Warning: unable to fully restore search path; see prior ", + "error, and consult `?unitizerState`, searching for \"Caveats\". ", + "Contact maintainer if your problem is not covered by documentation.", immediate.=TRUE ) global$state() diff --git a/R/state.R b/R/state.R index 5ec31c38..ea1d03f4 100644 --- a/R/state.R +++ b/R/state.R @@ -35,7 +35,7 @@ NULL #' \code{.Rprofile}, although if you intend to do this be sure to read the #' \dQuote{CRAN non-compliance} section. #' -#' @section CRAN non-compliance: +#' @section CRAN Non-Compliance and Other Caveats: #' #' In the default state management mode, this package fully complies with CRAN #' policies. In order to implement advanced state management features we must @@ -47,8 +47,21 @@ NULL #' Unfortunately this tracing is against CRAN policies, which is why it is #' disabled by default. #' +#' Arguably other aspects of state management employed outside of +#' \code{state="default"} _could_ be considered CRAN non-compliant, but none of +#' these are deployed unless you explicitly chose to do so. Additionally, +#' \code{unitizer} limits state manipulation to the evaluation of its processes +#' and restores state on exit. Some exceptional failures may prevent restoring +#' state fully. +#' +#' If state management were to fail fail in an unhandled form, the simplest +#' work-around is to turn off state management altogether with +#' \code{state="default"}. If it is a particular aspect of state management +#' (e.g. search paths with packages attached with \code{devtools::load_all}), +#' you can disable just that aspect of state (see "Custom Control" section). +#' #' For more details see the reproducible tests vignette with: -#' \code{vignette(package='unitizer', 'unitizer_reproducible_tests')} +#' \code{vignette(package='unitizer', 'u4_reproducible-tests')} #' #' @section Overview: #' @@ -115,7 +128,9 @@ NULL #' and re-attaches packages. This is not always the same as loading a package #' into a fresh R session as detaching a package does not necessarily undo every #' action that a package takes when it is loaded. See \code{\link{detach}} for -#' potential pitfalls of enabling this setting. +#' potential pitfalls of enabling this setting. Additionally, packages attached +#' in non-standard ways (e.g. \code{devtools::load_all}) may not re-attach +#' properly. #' #' You can modify what aspects of state are managed by using the \code{state} #' parameter to \code{\link{unitize}}. If you are satisfied with basic default diff --git a/R/unitize.core.R b/R/unitize.core.R index f9db67f2..00a77f49 100644 --- a/R/unitize.core.R +++ b/R/unitize.core.R @@ -1089,14 +1089,14 @@ reset_and_unshim <- function(global) { meta_word_msg( "Failed unshimming library/detach/attach; you may want to restart", "your R session to reset them to their original values (or you", - "can `untrace` them manually)" + "can `untrace` them manually)." ) if(!success.release) meta_word_msg( "Failed releasing global tracking object; you will not be able to", "instantiate another `unitizer` session. This should not happen, ", "please contact the maintainer. In the meantime, restarting your R", - "session should restore functionality" + "session should restore functionality." ) success.clear && success.unshim } diff --git a/man/unitizerState.Rd b/man/unitizerState.Rd index efaa8e5d..9d6168a8 100644 --- a/man/unitizerState.Rd +++ b/man/unitizerState.Rd @@ -64,7 +64,7 @@ level by adding \code{options(unitizer.state='suggested')} in your \code{.Rprofile}, although if you intend to do this be sure to read the \dQuote{CRAN non-compliance} section. } -\section{CRAN non-compliance}{ +\section{CRAN Non-Compliance and Other Caveats}{ In the default state management mode, this package fully complies with CRAN @@ -77,8 +77,21 @@ only traced during \code{unitize} evaluation and are untraced on exit. Unfortunately this tracing is against CRAN policies, which is why it is disabled by default. +Arguably other aspects of state management employed outside of +\code{state="default"} _could_ be considered CRAN non-compliant, but none of +these are deployed unless you explicitly chose to do so. Additionally, +\code{unitizer} limits state manipulation to the evaluation of its processes +and restores state on exit. Some exceptional failures may prevent restoring +state fully. + +If state management were to fail fail in an unhandled form, the simplest +work-around is to turn off state management altogether with +\code{state="default"}. If it is a particular aspect of state management +(e.g. search paths with packages attached with \code{devtools::load_all}), +you can disable just that aspect of state (see "Custom Control" section). + For more details see the reproducible tests vignette with: -\code{vignette(package='unitizer', 'unitizer_reproducible_tests')} +\code{vignette(package='unitizer', 'u4_reproducible-tests')} } \section{Overview}{ @@ -147,7 +160,9 @@ To manage the search path \code{unitizer} detaches and re-attaches packages. This is not always the same as loading a package into a fresh R session as detaching a package does not necessarily undo every action that a package takes when it is loaded. See \code{\link{detach}} for -potential pitfalls of enabling this setting. +potential pitfalls of enabling this setting. Additionally, packages attached +in non-standard ways (e.g. \code{devtools::load_all}) may not re-attach +properly. You can modify what aspects of state are managed by using the \code{state} parameter to \code{\link{unitize}}. If you are satisfied with basic default diff --git a/tests/t-search.R b/tests/t-search.R index 996518b1..e09edd4f 100644 --- a/tests/t-search.R +++ b/tests/t-search.R @@ -242,3 +242,24 @@ glb$release() unitizer:::unitizerUniqueNames(list(search.path = c(goodbye = "0", hello = "1", goodbye = "2", goodbye = "3"))) +# - "Fake Package Re-attach" --------------------------------------------------- + +# Make sure that aspects of search path management other than search path +# survive a failure caused by bad search path env (#252, #253). + +owd <- getwd() +test.f <- paste0(tempfile(), ".R") +writeLines(" +f <- tempfile() +dir.create(f) +setwd(f) +# Package assumed non-existing; R could disallow this in the future +# which could change the test. +attach(list(x=42), name='package:adfaadcxuqyojfnkfadsf') +1 + 1", test.f) +out <- unitizer:::capture_output( + try(unitize(test.f, state='recommended', interactive.mode=FALSE)) +) +any(grepl("mismatch between actual search path and tracked", out$message)) +identical(owd, getwd()) # confirm working directory restored + diff --git a/tests/t-search.Rout.save b/tests/t-search.Rout.save index fc7d0c6a..62fdb011 100644 --- a/tests/t-search.Rout.save +++ b/tests/t-search.Rout.save @@ -1,12 +1,14 @@ -R Under development (unstable) (2021-07-17 r80639) -- "Unsuffered Consequences" -Copyright (C) 2021 The R Foundation for Statistical Computing +R Under development (unstable) (2022-02-01 r81609) -- "Unsuffered Consequences" +Copyright (C) 2022 The R Foundation for Statistical Computing Platform: x86_64-apple-darwin17.0 (64-bit) R is free software and comes with ABSOLUTELY NO WARRANTY. You are welcome to redistribute it under certain conditions. Type 'license()' or 'licence()' for distribution details. + Natural language support but running in an English locale + R is a collaborative project with many contributors. Type 'contributors()' for more information and 'citation()' on how to cite R or R packages in publications. @@ -479,7 +481,30 @@ NULL + hello = "1", goodbye = "2", goodbye = "3"))) [1] "goodbye" "hello" "goodbye.1" "goodbye.2" > +> # - "Fake Package Re-attach" --------------------------------------------------- +> +> # Make sure that aspects of search path management other than search path +> # survive a failure caused by bad search path env (#252, #253). +> +> owd <- getwd() +> test.f <- paste0(tempfile(), ".R") +> writeLines(" ++ f <- tempfile() ++ dir.create(f) ++ setwd(f) ++ # Package assumed non-existing; R could disallow this in the future ++ # which could change the test. ++ attach(list(x=42), name='package:adfaadcxuqyojfnkfadsf') ++ 1 + 1", test.f) +> out <- unitizer:::capture_output( ++ try(unitize(test.f, state='recommended', interactive.mode=FALSE)) ++ ) +> any(grepl("mismatch between actual search path and tracked", out$message)) +[1] TRUE +> identical(owd, getwd()) # confirm working directory restored +[1] TRUE +> > > proc.time() user system elapsed - 5.52 1.67 9.49 + 3.534 0.721 4.428 diff --git a/vignettes/u1_intro.Rmd b/vignettes/u1_intro.Rmd index 508f9fa2..0fd6f9b1 100644 --- a/vignettes/u1_intro.Rmd +++ b/vignettes/u1_intro.Rmd @@ -166,9 +166,10 @@ tracked and managed: * Loaded namespaces State management is turned off by default because it requires tracing some base -functions which is against CRAN policy. If you wish to enable this feature use -`unitize(..., state='suggested')` or `options(unitizer.state='suggested')`. -For more details see `?unitizerState` and the [reproducible tests +functions which is against CRAN policy, and generally affects session state in +uncommon ways. If you wish to enable this feature use `unitize(..., +state='suggested')` or `options(unitizer.state='suggested')`. For more details +including potential pitfalls see `?unitizerState` and the [reproducible tests vignette](u4_reproducible-tests.html). ### Beware of Force Quitting from `unitizer` diff --git a/vignettes/u4_reproducible-tests.Rmd b/vignettes/u4_reproducible-tests.Rmd index ed1e7fd1..8434b7fc 100644 --- a/vignettes/u4_reproducible-tests.Rmd +++ b/vignettes/u4_reproducible-tests.Rmd @@ -274,33 +274,32 @@ For example, S3 method registration is not undone when detaching a package, or even unloading its namespace. See discussion in `?detach` and in `?unitizerState`. -Another good example of problematic behavior are attaching environments that -contain references to themselves, as the `tools:rstudio` environment attached -by `Rstudio` does. It contains functions that have for environment the -`tools:rstudio` environment. The problem is that once that environment is -detached from the search path, those functions no longer have access to the -search path. Re-attaching the environment to the search path does not solve the -problem because `attach` attaches a _copy_ of the environment, not the -environment itself. This new environment will contain the same objects as the -original environment, but all the functions therein will have for environment -the original detached environment, not the copy that is attached to -the search path. - -There are a few possible solutions to this problem. For now we have adopted the -simplest which is to keep the `tools:rstudio` environment on the search path -even search path tracking is enabled (you can over-ride this by changing -`search.path.keep`, or, if you have environments on your search path with -similar properties, add their names to `search.path.keep`). Other options +One known problem is the use of `devtools::load_all` and similar which place a +pretend package environment on the search path. Such packages cannot be +re-loaded with `library` so the re-attach process will fail (see +[#252](https://github.com/brodieG/unitizer/issues/252)). + +Another issue is attached environments that contain references to themselves, as +the `tools:rstudio` environment attached by `Rstudio` does. It contains +functions that have for environment the `tools:rstudio` environment. The +problem is that once that environment is detached from the search path, those +functions no longer have access to the search path. Re-attaching the +environment to the search path does not solve the problem because `attach` +attaches a _copy_ of the environment, not the environment itself. This new +environment will contain the same objects as the original environment, but all +the functions therein will have for environment the original detached +environment, not the copy that is attached to the search path. + +For the specific `tools::rstudio` problem we work around the issue by keeping it +on the search path even search path tracking is enabled (you can over-ride this +by changing `search.path.keep`, or, if you have environments on your search path +with similar properties, add their names to `search.path.keep`). Other options include re-attaching with `parent.env<-` instead of `attach`, but messing with the search path in that way seems to be exactly what R core warns about in `?parent.env`: > The replacement function parent.env<- is extremely dangerous as it can be used to destructively change environments in ways that violate assumptions made by the internal C code. It may be removed in the near future. -Another possibility would be to re-set the environments of functions inside -detached environments that have for parent the detached environment, but we do -not do this currently. - ## Global Options `unitizer` can track and reset global options. Because many packages set