Fix Windows PATH handling for launcher installs

.Rbuildignore

@@ -9,3 +9,4 @@
 ^\.vscode$
 ^deps$
 ^\.git-blame-ignore-revs$
+^\.git$

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
 

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() {

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
 

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()