feat: experimental uvr backend (shiny2docker_uvr)#11
Open
VincentGuyader wants to merge 11 commits into
Open
Conversation
Adds shiny2docker_uvr() as an experimental sister to shiny2docker(): generates a Dockerfile that uses nbafrank/uvr to install R inside the container and restore packages from uvr.lock, instead of an rocker/r-ver image + renv::restore(). Decouples the base image from the R version so any minimal Linux base can be used (defaults to debian:stable-slim). Pre-conditions are asserted in R (uvr.toml, uvr.lock, .r-version present and consistent), the launch step writes a small _uvr_start.R inside the image since `uvr run` only accepts a script. Workarounds embedded in the template for current uvr 0.2.15 issues: - pre-create <r-versions>/<ver>/etc before `uvr r install` (otherwise fails writing Renviron.site) - include `binutils` so uvr can call `ar x` on Posit's R .deb - ship the R-build sysreqs and a few common shiny-stack ones, since P3M binary fallback is not effective on debian targets and uvr ends up compiling all packages from source - run `uvr sync` without --frozen for now (--frozen rejects locks generated on a different host than the container's target OS) Tested via tests/testthat/test-shiny2docker_uvr.R (23 passing) and an env-gated end-to-end test that builds the image and probes the running shiny app over HTTP (5 passing). On the included fixture: cold build ~580s, image 985MB, HTTP 200 served in ~12s post-start.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds an experimental shiny2docker_uvr() backend to generate a Dockerfile that installs R via uvr and restores packages from uvr.lock, plus accompanying fixtures and tests.
Changes:
- Introduces
shiny2docker_uvr()and uvr-specific helpers for state checks, release URL construction, Dockerfile generation, and.dockerignorecreation. - Adds unit tests plus an env-gated Docker integration test.
- Adds a minimal uvr-managed Shiny app fixture for testing.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| R/shiny2docker_uvr.R | Public entrypoint for the experimental uvr backend; writes Dockerfile and .dockerignore. |
| R/utils_uvr.R | uvr helper utilities: project state assertion, GitHub release URL builder, Dockerfile builder, uvr-aware .dockerignore. |
| man/shiny2docker_uvr.Rd | Generated documentation for shiny2docker_uvr(). |
| NAMESPACE | Exports shiny2docker_uvr() and imports dockerfiler::Dockerfile. |
| tests/testthat/test-shiny2docker_uvr.R | Unit tests for URL resolver, state assertion, and generated Dockerfile content. |
| tests/testthat/test-shiny2docker_uvr-integration.R | Env-gated end-to-end test building/running the image and probing HTTP. |
| tests/testthat/fixtures/uvr-app/uvr.toml | Fixture uvr manifest for the integration test. |
| tests/testthat/fixtures/uvr-app/uvr.lock | Fixture lockfile for restoring dependencies in-container. |
| tests/testthat/fixtures/uvr-app/app.R | Minimal Shiny app used by the integration test. |
| tests/testthat/fixtures/uvr-app/.r-version | R version pin used by uvr r install. |
| tests/testthat/fixtures/uvr-app/.gitignore | Ignores .uvr/library/ created by uvr. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Doc: drop the `--frozen` mention (we run plain `uvr sync`); document the Debian/Ubuntu + amd64/glibc base-image constraint and the `[1, 65535]` port range; clarify that `.dockerignore` is only written when `write = TRUE`. - Behaviour: gate `.dockerignore` creation behind `write` so `write = FALSE` is a true no-op on the filesystem. - Validation: add `uvr_validate_host_port()` and `uvr_validate_sysreqs()` to reject shell-unsafe inputs (host/port interpolated into Dockerfile commands; extra_sysreqs concatenated into apt-get install). Use `encodeString()` + `shQuote()` when building `_uvr_start.R` so quoted / multiline values can't escape the printf or the resulting R source. - Comment alignment: `.Rprofile` is now part of the generated .dockerignore (matches the helper's docstring). - Anchor the semver regex in `uvr_release_url()` so `v0.3.1junk` is rejected. - Tests: drop unused `free_port()` helper from the integration test and rewrite `http_probe()` to actually parse the HTTP status line over a socket connection (previously any TCP-connect succeeded was treated as HTTP 200, hiding 4xx/5xx). - Add unit tests for the new validators and for the regex anchoring.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Follow-up to PR #11 self-review (orange-tier items): - Add `frozen` argument to `shiny2docker_uvr()`. Default FALSE keeps the current behaviour green on hosts where uvr 0.2.15 cannot honour `uvr sync --frozen` cross-host; opt in with `frozen = TRUE` for strict CI builds. When FALSE, emit a `cli` warning so the relaxed strictness is explicit at Dockerfile-generation time. - Create `dirname(output)` recursively when missing. Previously `output = "deploy/sub/Dockerfile"` would crash on `.dockerignore` creation with a cryptic "cannot open file" error. - Validate the content of `.r-version` in `uvr_assert_state()`: must be a single MAJOR.MINOR.PATCH line. Empty / multi-line / garbage values now fail fast in R rather than producing an opaque Docker build error later (`uvr r install ""`). Tests: +13 assertions covering the new validation branches and the two new behaviours; existing tests wrapped with suppressMessages where they previously did not assert on the new warning. Total now 56 unit + 5 integration, all green.
Copilot pointed out that `uvr_toml`, `uvr_lock`, and `r_version_file` were public arguments but the generated Dockerfile still hard-codes `COPY uvr.toml`, `COPY uvr.lock`, `COPY .r-version` from the build context root. Any caller passing non-default paths would have a Dockerfile that does not match the validated inputs, and `docker build` would fail. Resolution: drop the three arguments. uvr itself enforces these exact filenames at the project root, so the configurability was illusory. The helper `uvr_assert_state()` keeps its path arguments (it's an internal helper) — paths are now computed inside the public function. Doc clarifications: - Make explicit that uvr.toml/uvr.lock/.r-version must live at `<path>/` under those exact names. - Spell out exactly what `uvr_assert_state()` checks (existence, lock mtime >= toml mtime, .r-version content shape) and that we do *not* parse uvr.toml to cross-validate the [r] version constraint — uvr owns that contract (Copilot review #1). Re Copilot review #3 ("internal helpers won't resolve under R CMD check, use shiny2docker:::"): verified empirically — the package installs and `library(testthat); library(shiny2docker); test_check()` runs all 56 unit tests successfully without `:::`. testthat under `R CMD check` makes the package's namespace available to tests, so internal helpers resolve normally. No code change needed.
CI on the PR surfaced: - WARNING: non-ASCII character in R/utils_uvr.R — an em dash in the "uvr.lock is older than uvr.toml" stop() message. Replaced with `--`. - NOTE: hidden file `tests/testthat/fixtures/uvr-app/.r-version`. Added `^tests/testthat/fixtures$` to .Rbuildignore. The fixture is only needed by the env-gated integration test, which already skip_on_cran's, so excluding the dir from the package tarball is safe.
…as a markdown link The doc was rendered with markdown mode, so `[1, 65535]` looked like a [topic] link with target "1, 65535" and devtools::document() warned: utils_uvr.R:31: @param Could not resolve link to topic "1, 65535" in the dependencies or base packages. Backticks turn the range into inline code, removing the ambiguity. The block is @nord so no Rd file changes — pure document()-time warning cleanup. RoxygenNote bumped to 7.3.3 incidentally.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…env) Previously a fresh `shiny2docker_uvr()` call in a project without uvr files errored out with "uvr.toml not found ...". The renv counterpart auto-creates `renv.lock` via `attachment::create_renv_for_prod()` when missing — this commit brings the same UX to the uvr backend. New `uvr_bootstrap()` helper: - if `uvr.toml` is missing: generate `renv.lock` via `attachment` (or reuse one if present), then `uvr init` + `uvr import` - if `.r-version` is missing: pin to the local R version (with a 4.4.2 fallback if uvr's catalog rejects the local one) - if `uvr.lock` is missing or stale: `uvr lock` All `uvr` calls run with `path` as cwd. Errors out cleanly with install instructions if the `uvr` CLI is not on `PATH`. `shiny2docker_uvr()` gains `bootstrap = TRUE` (default). When `FALSE`, missing files trigger an immediate error (CI-friendly). Tests: +2 unit tests for the no-bootstrap and no-CLI error paths (58 passing). End-to-end exercised manually on a `app.R`-only project: Dockerfile generated, all four artefacts (`renv.lock`, `uvr.toml`, `uvr.lock`, `.r-version`) created from scratch.
Fresh shiny2docker users hit "uvr CLI not found" with a misleading
message that mentioned `install.packages('uvr')` (which does not exist
on CRAN — uvr is on GitHub). Replace it with a real, self-contained
solution:
- New exported `install_uvr(dest = '~/.local/bin', version, force)`:
detects the host platform (Linux/macOS/Windows × x86_64/arm64),
downloads the matching release asset from
github.com/nbafrank/uvr/releases, extracts the binary into `dest`,
chmods +x. Returns the absolute path. Warns if `dest` is not on
PATH for future sessions.
- New helper `uvr_locate_or_install()`: looks for `uvr` on PATH, then
in `~/.local/bin`, and in interactive sessions prompts the user to
run `install_uvr()` automatically. In non-interactive sessions,
raises an actionable error that now points at
`shiny2docker::install_uvr()` (no more bogus install.packages line).
- `uvr_bootstrap()` now uses the located absolute path, so it works
even when `~/.local/bin` is not yet on the running R session's PATH
(typical first-install scenario).
Tested manually end-to-end on a sandbox with no `uvr` on PATH:
install_uvr() downloads + extracts + writes uvr v0.2.15 to a custom
HOME, and `system2(<path>, '--version')` reports `uvr 0.2.15`.
+2 unit tests (60 passing total).
`~/.local/bin` only makes sense on Unix; the previous version used it unconditionally, and printed an `export PATH=...` shell hint that is wrong on Windows. Both are now platform-aware: - Default `dest`: - Unix: `~/.local/bin/` (matches uvr's install.sh) - Windows: `%LOCALAPPDATA%/shiny2docker/bin/` - PATH hint after install: - Unix: `export PATH=...` (with bashrc/zshrc note) - Windows: `setx PATH ...` (with restart-shell note) Both `install_uvr()` and the interactive prompt in `uvr_locate_or_install()` use the new helper, so Windows users hitting "uvr CLI not installed yet" get a sensible default location and a correct hint.
Documents: - shiny2docker_uvr() generates a Dockerfile that installs R via uvr inside the container, restores packages from uvr.lock at build time, and supports any Debian/Ubuntu amd64 base image. - install_uvr() downloads the uvr CLI binary for the current platform (Linux/macOS/Windows × x86_64/aarch64) into a sensible default path and prints a platform-correct PATH hint. - Bootstrapping: missing uvr.toml/uvr.lock/.r-version are auto-created from a renv.lock generated via attachment, mirroring the renv backend's UX. Interactive prompt to install uvr if absent. `bootstrap = FALSE` for fail-fast CI usage. - The `frozen` argument: explains the manifest-vs-lockfile distinction, why frozen = FALSE is the current default (uvr 0.2.15 has a known cross-host false positive on `uvr sync --frozen`), and when to flip it to TRUE for strict reproducibility. - Lifecycle disclaimer + pointer to upstream nbafrank/uvr. README.md regenerated from README.Rmd.
CI on PR #11 was red because the unit tests use `withr::local_envvar()` but `withr` was not declared in DESCRIPTION. Add it under Suggests so `R CMD check` stops complaining about the unstated dependency. Also pick up the four most recent Copilot comments: - `R/utils_uvr.R` -- drop the gratuitous `apt-get update && ... && rm -rf /var/lib/apt/lists/*` around `uvr sync`. uvr sync does not invoke apt at this point, so the prefix added latency and a network dependency for no benefit. The RUN is now just `RUN uvr sync` (or `uvr sync --frozen` when `frozen = TRUE`). Test updated to assert the new shape via an anchored regex. - `R/shiny2docker_uvr.R` -- grammar: "an `cli` warning" -> "a `cli` warning" in the `frozen` argument doc. - `tests/testthat/test-shiny2docker_uvr-integration.R` -- replace the convoluted `!is.null(attr(build_rc, "status")) == FALSE` with the equivalent `is.null(attr(build_rc, "status"))`. Also skip cleanly when the fixture dir is absent (e.g. running on an installed package after `R CMD check` -- the dir is excluded via .Rbuildignore) rather than crashing on `file.copy()`. `.Rbuildignore` is intentionally left as is: the fixture is only used by the env-gated integration test, which now skips with a clear reason when run outside the source repo.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| run_uvr <- function(args, label) { | ||
| owd <- setwd(path); on.exit(setwd(owd), add = TRUE) | ||
| rc <- suppressWarnings(system2(uvr_bin, args, stdout = TRUE, stderr = TRUE)) | ||
| setwd(owd); on.exit() |
There was a problem hiding this comment.
run_uvr() temporarily changes the process working directory and then clears the on.exit() handler manually (setwd(owd); on.exit()). This is hard to reason about and easy to break if additional cleanup is added later. Prefer a single on.exit(setwd(owd), add = TRUE) and let it run naturally (or avoid changing the global wd entirely).
Suggested change
| setwd(owd); on.exit() |
Comment on lines
+483
to
+486
| if (!is.na(release$arg_value)) { | ||
| dock$ARG(paste0("UVR_VERSION=", release$arg_value)) | ||
| } | ||
|
|
There was a problem hiding this comment.
ARG UVR_VERSION=... is injected but the download RUN uses a fully resolved URL and never references $UVR_VERSION, so the build arg is effectively unused and can't be overridden via --build-arg. Either remove the ARG line, or use it in the download URL to make it meaningful.
Suggested change
| if (!is.na(release$arg_value)) { | |
| dock$ARG(paste0("UVR_VERSION=", release$arg_value)) | |
| } |
| if (!nzchar(Sys.which("docker"))) { | ||
| testthat::skip("docker not on PATH") | ||
| } | ||
| if (system("docker info > /dev/null 2>&1") != 0) { |
There was a problem hiding this comment.
The docker info > /dev/null 2>&1 reachability check relies on POSIX shell redirection and may not behave consistently on Windows. Using system2("docker", "info", stdout = FALSE/NULL, stderr = FALSE/NULL) (or capturing output) would make the skip logic more portable.
Suggested change
| if (system("docker info > /dev/null 2>&1") != 0) { | |
| if (system2("docker", "info", stdout = FALSE, stderr = FALSE) != 0) { |
Comment on lines
+153
to
+155
| You don’t have to leave R to install the binary — `install_uvr()` | ||
| downloads the right release for your platform and drops the executable | ||
| in a sensible place: |
There was a problem hiding this comment.
This README section uses non-ASCII typographic characters (e.g., curly apostrophes in “don’t”). In some CRAN/CI setups this can trigger a non-ASCII NOTE; consider replacing with plain ASCII (don't, etc.) if you want to avoid that noise.
Comment on lines
+196
to
+200
| asset <- uvr_detect_asset() | ||
| url <- if (identical(version, "latest")) { | ||
| sprintf("https://github.com/nbafrank/uvr/releases/latest/download/%s", | ||
| asset$name) | ||
| } else { |
There was a problem hiding this comment.
install_uvr() accepts any version string and will build a download URL even for invalid tags, which then fails later with a generic download.file error/404. Consider validating version up front (e.g., require latest or vMAJOR.MINOR.PATCH / MAJOR.MINOR.PATCH) and erroring with an actionable message.
Suggested change
| asset <- uvr_detect_asset() | |
| url <- if (identical(version, "latest")) { | |
| sprintf("https://github.com/nbafrank/uvr/releases/latest/download/%s", | |
| asset$name) | |
| } else { | |
| asset <- uvr_detect_asset() | |
| if (!is.character(version) || length(version) != 1L || is.na(version)) { | |
| stop( | |
| "`version` must be a single character string: \"latest\", ", | |
| "\"vMAJOR.MINOR.PATCH\", or \"MAJOR.MINOR.PATCH\".", | |
| call. = FALSE | |
| ) | |
| } | |
| url <- if (identical(version, "latest")) { | |
| sprintf("https://github.com/nbafrank/uvr/releases/latest/download/%s", | |
| asset$name) | |
| } else { | |
| if (!grepl("^v?[0-9]+\\.[0-9]+\\.[0-9]+$", version)) { | |
| stop( | |
| "Invalid `version`: ", version, ". ", | |
| "Use \"latest\", \"vMAJOR.MINOR.PATCH\", or \"MAJOR.MINOR.PATCH\".", | |
| call. = FALSE | |
| ) | |
| } |
| }, add = TRUE) | ||
|
|
||
| cli::cli_alert_info("Downloading {.url {url}}") | ||
| utils::download.file(url, tmp, mode = "wb", quiet = TRUE) |
There was a problem hiding this comment.
utils::download.file() returns a non-zero status on failure without necessarily throwing; currently the return code isn't checked. Please check the return value and raise a clear error that includes the URL when the download fails.
Suggested change
| utils::download.file(url, tmp, mode = "wb", quiet = TRUE) | |
| download_status <- utils::download.file(url, tmp, mode = "wb", quiet = TRUE) | |
| if (!is.numeric(download_status) || | |
| length(download_status) != 1L || | |
| is.na(download_status) || | |
| download_status != 0) { | |
| stop( | |
| sprintf( | |
| "Failed to download uvr asset from %s (download.file status: %s)", | |
| url, | |
| as.character(download_status) | |
| ), | |
| call. = FALSE | |
| ) | |
| } |
|
|
||
| dir.create(dest, recursive = TRUE, showWarnings = FALSE) | ||
| out <- file.path(dest, bin_name) | ||
| file.copy(src, out, overwrite = TRUE) |
There was a problem hiding this comment.
file.copy(src, out, overwrite = TRUE) return value is ignored, so a permission error (e.g. dest not writable) can lead to a later confusing failure when trying to execute uvr. Please check the boolean return from file.copy() and stop() with a clear message if the copy fails.
Suggested change
| file.copy(src, out, overwrite = TRUE) | |
| copied <- file.copy(src, out, overwrite = TRUE) | |
| if (!isTRUE(copied)) { | |
| stop( | |
| "Failed to copy `", src, "` to `", out, | |
| "`. Please check that the destination is writable.", | |
| call. = FALSE | |
| ) | |
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
shiny2docker_uvr()as an experimental sister toshiny2docker(). Generates a Dockerfile that uses nbafrank/uvr to install R inside the container and restore packages fromuvr.lock, instead of anrocker/r-verbase +renv::restore().The base image becomes a free choice (defaults to
debian:stable-slim) since the R version is provided by uvr at build time. Pre-conditions are asserted in R (uvr.toml,uvr.lock,.r-versionpresent and consistent). The launch step writes a small_uvr_start.Rinside the image becauseuvr runonly accepts a script.What's in the patch
R/shiny2docker_uvr.R— public function, lifecycle: experimentalR/utils_uvr.R— helpers: state assertion, release URL resolver, Dockerfile builder, uvr-aware.dockerignoretests/testthat/test-shiny2docker_uvr.R— 8 tests / 23 assertions, no docker / no network neededtests/testthat/test-shiny2docker_uvr-integration.R— env-gated end-to-end test (SHINY2DOCKER_TEST_INTEGRATION=true): builds the image and probes shiny over HTTPtests/testthat/fixtures/uvr-app/— minimal uvr-managed shiny app (app.R,uvr.toml,uvr.lock,.r-version)POC measurements (fixture: shiny + 30 R deps)
Friction found in
uvr0.2.15 (workarounds embedded; upstream issues to file)uvr r installcallsar x <deb>but does not declare/check forbinutils. Cryptic error message ("No such file or directory").uvr r installwrites<r-versions>/<ver>/etc/Renviron.sitebut the parentetc/dir doesn't exist for Posit's R.deb(which putsetc/underlib/R/). Workaround:mkdir -p .../etcbefore install.uvr sync --frozenrejects locks that re-resolve byte-identically inside the container target OS. Currently using plainuvr sync— should be re-enabled once upstream is fixed.uvr lockflattens[r] version = ">=4.3"to[r] version = "*". Workaround: rely on.r-versionfor the pin.uvr rundoes not forward-eto R, only accepts a script — hence the_uvr_start.Rshim.Strategic read
renvon Docker.Test plan