Skip to content
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.

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

only consider R_LIBS_USER pkgs for remotes::install_deps #207

Closed
maxheld83 opened this issue May 21, 2019 · 6 comments
Closed

only consider R_LIBS_USER pkgs for remotes::install_deps #207

maxheld83 opened this issue May 21, 2019 · 6 comments
Labels
actions these are, well, actual actions (the building blocks) bug Something isn't working

Comments

@maxheld83
Copy link
Owner

currently there is a possible edge case were, say pkg a is used inside a docker container, and also installed via remotes::install_deps().
If said pkg is found in the system library of the comntainer, remotes::install_deps() will not install it again, though later actions, with different containers, won't have it.

@maxheld83 maxheld83 added bug Something isn't working actions these are, well, actual actions (the building blocks) labels May 21, 2019
@maxheld83 maxheld83 mentioned this issue May 21, 2019
maxheld83 added a commit that referenced this issue May 21, 2019
maxheld83 added a commit that referenced this issue May 21, 2019
@maxheld83 maxheld83 changed the title disable system library while user library is installed only consider R_LIBS_USER pkgs for remotes::install_deps May 22, 2019
@maxheld83
Copy link
Owner Author

I can think of three ways to address this:

  1. in remotes::update_packages() and upstream callers up to remotes::install_deps(), expose the lib.loc argument to installed.packages(), which currently defaults to NULL, i.e. the whole .libPaths() vector.
    Downside: This would require a PR to remotes, and may or may not be a suitable addition to remotes.
    Upside: This would be pretty clean and reuse all of the remotes logic.
  2. hack-fix this by just installing the pkgs missing in R_LIBS_USER manually in the install.R script.
  3. Modify the .libPaths() to only include R_LIBS_USER.
    Don't think this will work, because then of course we don't have the pkgs necessary for the action in the search tree anymore.
  4. Find a way to modify .libPaths() for some packages only, i.e. tell R to use some pkg from tree y and some pkg from tree x. That might also help solving prevent action scripts from calling pkg from user libs #211.

@jimhester
Copy link
Collaborator

jimhester commented May 22, 2019

You can use lib.loc, the ... are already passed to install.packages() in remotes.

maxheld83 added a commit that referenced this issue May 22, 2019
@maxheld83
Copy link
Owner Author

maxheld83 commented May 22, 2019

Uh, hope I don't have this completely backwards, but:

  • remotes::install_deps() passes ... on to install.packages(), including lib (the directory to install packages into)
  • but not lib.loc, which is an argument to installed.packages(), which is the directory where to find already installed packages. (I think remotes also uses installed.packages() to find whatever is already installed in .libPaths(), which is what installed.packages() defaults to).

Empirically, calling in my pkg root remotes::install_deps(lib.loc = "/Applications/") installs nothing, because, presumably, it's still looking for packages in my entire .libPaths() and not just in /Applications/, where it should find none.

@maxheld83
Copy link
Owner Author

Anyway, I hope I have a much cleaner solution to this and #211 now which should avoid any of these headaches in the future. Beginning is in c05048e, but I'll add more documentation to explain this.

@jimhester
Copy link
Collaborator

jimhester commented May 23, 2019

You can use withr::with_libpaths() to temporarily set your libpaths to whatever then, so in conjunction with the lib argument you can install into whatever library and get dependencies from any arbitrary location.

maxheld83 added a commit that referenced this issue May 23, 2019
@maxheld83
Copy link
Owner Author

thanks, I thought about that too.
withr is "just" syntactic sugar about setting/unsetting .libPaths() in this case, right?

I think

withr::with_libpaths(
  new = "/github/home/lib/R/library",
  code = {
    remotes::install_deps()
  },
  action = "prefix"
)

would still not solve the problem because then remotes and its dependencies would still be on the search tree, and therefore, say desc would not be installed to /github/home/lib/R/library – though it should be.

So what I did instead now (from c05048e) is:

  • install remotes and its dependencies to R_LIBS_ACTION (/usr/lib/R/action-library) (called action, because this dir persists only for the duration of the action)
  • install the pkg dependencies to R_LIBS_WORKFLOW (/github/home/lib/R/library as before).
  • "pick and choose" remotes from R_LIBS_ACTION so that it is loaded, but neither remotes nor its dependencies (desc, especially) are on the .libPaths() when remotes::install_deps() calls utils::installed_packages(), which defaults to the entire .libPaths().

I do:

message("Using 'remotes' from 'R_LIBS_ACTION'.")
unloadNamespace(ns = "remotes")  # just to be safe
requireNamespace(package = "remotes", lib.loc = Sys.getenv("R_LIBS_ACTION"))
message("Recording already installed dependencies ...")
deps_exp <- remotes::dev_package_deps(dependencies = TRUE)$package
message("Installing dependencies ...")
remotes::install_deps(dependencies = TRUE, verbose = TRUE)
message("Unload 'remotes' from 'R_LIBS_ACTION'...")
unloadNamespace(ns = "remotes")  # just to be safe
message("Checking installation success ...")
deps_present <- installed.packages(lib.loc = Sys.getenv("R_LIBS_WORKFLOW"))[, "Package"]
# this only compares pkgs, not version numbers or SHAs
deps_missing <- setdiff(deps_exp, deps_present)
if (length(deps_missing) == 0) {
  message("All package dependencies were successfully installed.")
} else {
  stop(
    "One or more package dependencies could not be installed: ",
    paste(deps_missing, collapse = ", ")
  )
}

My idea now is to have R_LIBS_WORKFLOW and R_LIBS_ACTION along with its mkdirs set as ARG in the base Dockerfile for all of our actions, and then we can set R_LIBS= to either one, both (in any order), or just pick+choose via withr() or even requireNamespace().
That seems to cut a lot of code duplication.

See docs draft: https://github.com/r-lib/ghactions/blob/safeinstall/vignettes/isolation.Rmd

Or maybe I'm really overthinking this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
actions these are, well, actual actions (the building blocks) bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants