diff --git a/.Rbuildignore b/.Rbuildignore index 5f19960..bb578bb 100644 --- a/.Rbuildignore +++ b/.Rbuildignore @@ -9,3 +9,4 @@ ^\.vscode$ ^deps$ ^\.git-blame-ignore-revs$ +^\.git$ diff --git a/NEWS.md b/NEWS.md index 6c22466..23d0137 100644 --- a/NEWS.md +++ b/NEWS.md @@ -45,6 +45,11 @@ directory to the user's zsh profile on macOS when it is not already on `PATH`. It respects `ZDOTDIR` and warns if the profile cannot be updated (#35). +- On Windows, `install_pkg_cli_apps()` no longer replaces the current + process `PATH` with only the user-level `Path` after adding launcher + directories. It now avoids duplicate entries, uses a short path when + available, and reports too-long user-level `Path` values with a + remediation hint (#38). # Rapp 0.3.0 diff --git a/R/install.R b/R/install.R index 15ae403..b7c9575 100644 --- a/R/install.R +++ b/R/install.R @@ -483,46 +483,84 @@ ensure_path_windows <- function(destdir = rapp_install_dir()) { return(FALSE) } stopifnot(.Platform$OS.type == "windows") - destdir <- normalizePath(destdir, winslash = "\\", mustWork = TRUE) - - # Check if we're already on PATH. If we are, do nothing - # Read current PATH from HKCU\Environment - path <- get_env_win_registry("Path") - path <- strsplit(path, ";", fixed = TRUE)[[1L]] - path <- path[nzchar(path)] - path <- unique(path) - - path_norm <- function(x) tolower(normalizePath(x, mustWork = FALSE)) - present <- path_norm(destdir) %in% path_norm(path) - if (present) { + destdir <- normalizePath(destdir, mustWork = TRUE) + path_entry <- utils::shortPathName(destdir) + if ( + is.na(path_entry) || + !nzchar(path_entry) || + nchar(path_entry) >= nchar(destdir) + ) { + path_entry <- destdir + } + + is_on_path <- function(env_path = Sys.getenv("PATH")) { + path_has_dir(destdir, env_path, ignore_case = TRUE) || + path_has_dir(path_entry, env_path, ignore_case = TRUE) + } + update_process_path <- function(env_path = Sys.getenv("PATH")) { + if (is_on_path(env_path)) { + return(FALSE) + } + Sys.setenv( + "PATH" = paste( + c(path_entry, path_entries(env_path, normalize = FALSE)), + collapse = .Platform$path.sep + ) + ) + TRUE + } + + # Check if we're already on the user-level PATH. If we are, do nothing. + # Read current PATH from HKCU\Environment. + user_path <- get_env_win_registry("Path") + if (is_on_path(user_path)) { + update_process_path() return(FALSE) } # We are not on the PATH yet, we have to add it # Pass the new path entry via envvar to avoid quoting and encoding shenanigans - # also, propogate the new updated PATH from the registry to this R session + # and also propagate the new entry to this R session. old <- Sys.getenv("RAPP_NEW_PATH_ENTRY", NA_character_) - Sys.setenv("RAPP_NEW_PATH_ENTRY" = destdir) + old_path <- Sys.getenv("PATH") + Sys.setenv("RAPP_NEW_PATH_ENTRY" = path_entry) on.exit({ if (is.na(old)) { Sys.unsetenv("RAPP_NEW_PATH_ENTRY") } else { Sys.setenv("RAPP_NEW_PATH_ENTRY" = old) } - Sys.setenv("PATH" = get_env_win_registry("Path")) }) script <- shQuote(utils::shortPathName( system.file("add-path-entry.ps1", package = "Rapp") )) args <- c("-NoProfile", "-ExecutionPolicy", "Bypass", "-File", script) - system2("powershell", args) + status <- run_powershell(args) + if (!identical(status, 0L)) { + warning( + "Could not add ", + path_entry, + " to the user-level PATH; powershell exited with status ", + status, + ".", + call. = FALSE + ) + return(FALSE) + } + + update_process_path(old_path) + TRUE } get_env_win_registry <- function(name) { utils::readRegistry("Environment", hive = "HCU", view = "default")[[name]] } +run_powershell <- function(args) { + system2("powershell", args) +} + ensure_path_macos <- function(destdir = rapp_install_dir()) { if (Sys.getenv("RAPP_NO_MODIFY_PATH") != "") { return(FALSE) @@ -629,14 +667,34 @@ write_macos_path_lines <- function(profile, lines, profile_display) { ) } -path_has_dir <- function(dir, env_path = Sys.getenv("PATH")) { - normalizePath(dir, mustWork = FALSE) %in% path_entries(env_path) +path_has_dir <- function( + dir, + env_path = Sys.getenv("PATH"), + ignore_case = is_windows() +) { + dir <- normalizePath(dir, mustWork = FALSE) + entries <- path_entries(env_path) + + if (ignore_case) { + dir <- tolower(dir) + entries <- tolower(entries) + } + + dir %in% entries } -path_entries <- function(env_path = Sys.getenv("PATH")) { +path_entries <- function(env_path = Sys.getenv("PATH"), normalize = TRUE) { + if (is.null(env_path) || !length(env_path) || is.na(env_path)) { + return(character()) + } + entries <- strsplit(env_path, .Platform$path.sep, fixed = TRUE)[[1L]] entries <- entries[nzchar(entries)] - normalizePath(entries, mustWork = FALSE) + if (normalize) { + normalizePath(entries, mustWork = FALSE) + } else { + entries + } } is_macos <- function() { diff --git a/inst/add-path-entry.ps1 b/inst/add-path-entry.ps1 index de85c13..d61d3ed 100644 --- a/inst/add-path-entry.ps1 +++ b/inst/add-path-entry.ps1 @@ -10,18 +10,50 @@ $NewPathEntry = if ($env:RAPP_NEW_PATH_ENTRY) { $env:RAPP_NEW_PATH_ENTRY } elsei Write-Verbose "Adding $NewPathEntry to your user-level PATH" +function Normalize-PathEntry([string]$PathEntry) { + try { + $PathEntry = (Resolve-Path -LiteralPath $PathEntry -ErrorAction Stop).ProviderPath + } catch { + } + + try { + $PathEntry = [System.IO.Path]::GetFullPath($PathEntry) + } catch { + } + + return $PathEntry.TrimEnd('\').ToLowerInvariant() +} + # Read unexpanded PATH components $PathEntries = (Get-Item -LiteralPath $RegistryPath).GetValue( 'Path', '', 'DoNotExpandEnvironmentNames') -split ';' -ne '' -if ($NewPathEntry -in $PathEntries) { +$NewPathEntryNorm = Normalize-PathEntry $NewPathEntry +$PathEntryNorms = $PathEntries | ForEach-Object { Normalize-PathEntry $_ } + +if ($NewPathEntryNorm -in $PathEntryNorms) { Write-Verbose "Install directory $NewPathEntry already on PATH!" - exit 1 + exit 0 } # Prepend to PATH $NewPath = (,$NewPathEntry + $PathEntries) -join ';' +if ($NewPath.Length -gt 32767) { + $Hint = @( + "Rapp uses a Windows short path when one is available." + "Remove stale entries from your user-level PATH or choose a shorter launcher directory with RAPP_BIN_DIR." + "Windows long-path support does not increase this environment-variable limit." + ) -join ' ' + $Message = @( + "Adding $NewPathEntry would make your user-level PATH $($NewPath.Length) characters," + "exceeding the Windows environment variable limit of 32767." + $Hint + ) -join ' ' + Write-Error $Message + exit 3 +} + # Update registry as REG_EXPAND_SZ Set-ItemProperty -Type ExpandString -LiteralPath $RegistryPath Path -Value $NewPath diff --git a/tests/testthat/test-install.R b/tests/testthat/test-install.R index 7268a85..1ee8938 100644 --- a/tests/testthat/test-install.R +++ b/tests/testthat/test-install.R @@ -213,6 +213,54 @@ test_that("non-Rapp executables respect overwrite flag", { }) +test_that("install_pkg_cli_apps persists Windows PATH when destdir is only transient", { + skip_if_not(is_windows()) + + destdir <- tempfile("rapp-bin-win-path") + dir.create(destdir, recursive = TRUE) + on.exit(unlink(destdir, recursive = TRUE), add = TRUE) + + current_path <- Sys.getenv("PATH") + path_with_destdir <- paste(destdir, current_path, sep = .Platform$path.sep) + withr::local_envvar( + RAPP_NO_MODIFY_PATH = NA, + PATH = path_with_destdir + ) + + powershell_calls <- list() + testthat::local_mocked_bindings( + get_env_win_registry = function(name) { + expect_identical(name, "Path") + "C:\\Windows\\system32" + }, + run_powershell = function(args) { + powershell_calls <<- append(powershell_calls, list(list( + args = args, + path_entry = Sys.getenv("RAPP_NEW_PATH_ENTRY") + ))) + 0L + }, + .package = "Rapp" + ) + + expect_true(Rapp:::ensure_path_windows(destdir)) + expect_identical(Sys.getenv("PATH"), path_with_destdir) + expect_length(powershell_calls, 1L) + expect_same_path(powershell_calls[[1L]][["path_entry"]], destdir) +}) + + +test_that("path_entries can keep the existing process PATH raw", { + env_path <- paste("first", "second", sep = .Platform$path.sep) + + expect_identical( + Rapp:::path_entries(env_path, normalize = FALSE), + c("first", "second") + ) + expect_identical(Rapp:::path_entries("", normalize = FALSE), character()) +}) + + test_that("install_pkg_cli_apps adds default macOS install dir to PATH setup", { skip_if_not(identical(Sys.info()[["sysname"]], "Darwin")) skip_on_cran()