diff --git a/R/pak.R b/R/pak.R index 65801b9e..d7555fb9 100644 --- a/R/pak.R +++ b/R/pak.R @@ -443,6 +443,32 @@ isGH <- function(pkgs) { grepl("^[[:alpha:]]+/.+", pkgs) } +# Returns TRUE iff the constraint (`versionSpec` / `inequality`) is satisfied, +# either by what's on disk (`installedVer`) or by pak's globally-resolved +# version (`pakResolvedVer`) -- and in the latter case only when pak's +# resolution matches what's actually on disk. +# +# The disk-match guard is the regression fix for the binary-lag scenario: +# pak's `pkg_deps` resolver may pick the source version (e.g. reproducible +# 3.1.0) while `pak::pak()` chooses to "keep" the older binary (3.0.0) on +# platforms whose CRAN binary hasn't caught up. Treating `pakResolvedVer` as +# authoritative without checking disk reality marks the unsatisfied install +# as "OK" and the user gets `Installed 1 packages` followed by a still-old +# `packageVersion(pkg)`. +pakConstraintSatisfied <- function(installedVer, versionSpec, inequality, + pakResolvedVer = NA_character_) { + satisfies <- isTRUE(compareVersion2(installedVer, + versionSpec = versionSpec, + inequality = inequality)) + if (!satisfies && !is.na(pakResolvedVer) && nzchar(pakResolvedVer) && + identical(pakResolvedVer, installedVer)) { + satisfies <- isTRUE(compareVersion2(pakResolvedVer, + versionSpec = versionSpec, + inequality = inequality)) + } + satisfies +} + # For each plain CRAN ref in `pkgsForPak` that names a package currently # installed in `libPaths`, rewrite it to `pkg@`. Leaves # GitHub refs, pre-pinned `pkg@X` refs, packages with an explicit user @@ -3327,11 +3353,9 @@ pakInstallFiltered <- function(pkgDT, libPaths, repos, standAlone, verbose, pakVerMap <- get0("pakResolvedVersionMap", envir = pakEnv(), inherits = FALSE) if (!is.null(pakVerMap)) { cand <- pakVerMap[pkg] - if (!is.na(cand) && nzchar(cand)) { - pakRes <- unname(cand) - satisfies <- isTRUE(compareVersion2(pakRes, versionSpec = vSpec, inequality = ineq)) - } + if (!is.na(cand) && nzchar(cand)) pakRes <- unname(cand) } + satisfies <- pakConstraintSatisfied(installedVer, vSpec, ineq, pakRes) } if (!isTRUE(satisfies)) { # We are inside `if (NROW(nowRow))`, i.e. pak HAS something installed @@ -3350,7 +3374,18 @@ pakInstallFiltered <- function(pkgDT, libPaths, repos, standAlone, verbose, # on disk) and warrants the "please change required version" guidance # even when preVer == installedVer (e.g. on a re-Require() call). pakChoseInstalled <- !is.na(pakRes) && identical(pakRes, installedVer) - if (versionChanged || firstTimeInsufficient || pakChoseInstalled) + # pak resolved to a newer version than what's actually on disk + # (typical Mac/Win case: source > binary). Warn the user that the + # install was effectively a no-op even though pak reported success. + pakResolvedNewer <- !is.na(pakRes) && !identical(pakRes, installedVer) && + isTRUE(compareVersion2(pakRes, versionSpec = vSpec, inequality = ineq)) + if (pakResolvedNewer) { + warning(.txtCouldNotBeInstalled, ": ", pkg, " ", ineq, " ", vSpec, + "; pak resolved ", pakRes, " but only ", installedVer, + " is available as a binary on this platform -- ", + "install from source or wait for the binary to be built.", + call. = FALSE) + } else if (versionChanged || firstTimeInsufficient || pakChoseInstalled) warning(msgPleaseChangeRqdVersion(pkg, ineq = ">=", newVersion = installedVer), call. = FALSE) # Always add to warnedDropped: either we already warned above (versionChanged), # or pak ran and chose not to update this package, meaning Require's over-strict diff --git a/tests/testthat/test-16installFailureMetadata_testthat.R b/tests/testthat/test-16installFailureMetadata_testthat.R index 72febb0b..99aab2ab 100644 --- a/tests/testthat/test-16installFailureMetadata_testthat.R +++ b/tests/testthat/test-16installFailureMetadata_testthat.R @@ -694,3 +694,71 @@ test_that("identify-and-defer recovers from PSPclean-style cascade", { expect_true(all(nzchar(failures$reason_brief))) } }) + +# --------------------------------------------------------------------------- +# Regression: binary-lag silent "OK" +# +# Field case (Mac, Require 2.0.0): +# > Require::Install("reproducible (>= 3.1.0)") +# Installed 1 packages in 6.4 secs +# > packageVersion("reproducible") +# [1] '3.0.0' +# +# CRAN had reproducible 3.1.0 in source but only 3.0.0 as a Mac binary, so +# pak's `pkg_deps` resolver pinned 3.1.0 in `pakResolvedVersionMap` while +# `pak::pak()` chose to "keep" the 3.0.0 binary. The post-install +# reconciliation in pakInstallFiltered fell back to `pakResolvedVer` without +# checking that it matched what was actually on disk, so the install was +# reported as successful. The fix: only trust pak's resolved version when +# it matches the version actually present. +# --------------------------------------------------------------------------- +test_that("pakConstraintSatisfied is FALSE when pak resolved newer than installed", { + # Binary-lag scenario: disk has 3.0.0, pak resolved to 3.1.0. + # Must be FALSE -- the install did not satisfy >= 3.1.0. + expect_false(Require:::pakConstraintSatisfied( + installedVer = "3.0.0", + versionSpec = "3.1.0", + inequality = ">=", + pakResolvedVer = "3.1.0" + )) +}) + +test_that("pakConstraintSatisfied is TRUE when installed version meets constraint", { + expect_true(Require:::pakConstraintSatisfied( + installedVer = "3.1.0", + versionSpec = "3.1.0", + inequality = ">=", + pakResolvedVer = "3.1.0" + )) + expect_true(Require:::pakConstraintSatisfied( + installedVer = "3.2.0", + versionSpec = "3.1.0", + inequality = ">=", + pakResolvedVer = NA_character_ + )) +}) + +test_that("pakConstraintSatisfied is FALSE when neither installed nor pakRes meets constraint", { + # pak chose to install installedVer (matches what's on disk) but neither + # satisfies the user constraint -- the install is a real failure. + expect_false(Require:::pakConstraintSatisfied( + installedVer = "3.0.0", + versionSpec = "3.1.0", + inequality = ">=", + pakResolvedVer = "3.0.0" + )) +}) + +test_that("pakConstraintSatisfied tolerates missing / blank pakResolvedVer", { + # When pakResolvedVer is unavailable, fall back solely to installedVer. + expect_false(Require:::pakConstraintSatisfied( + installedVer = "3.0.0", versionSpec = "3.1.0", inequality = ">=")) + expect_false(Require:::pakConstraintSatisfied( + installedVer = "3.0.0", versionSpec = "3.1.0", inequality = ">=", + pakResolvedVer = NA_character_)) + expect_false(Require:::pakConstraintSatisfied( + installedVer = "3.0.0", versionSpec = "3.1.0", inequality = ">=", + pakResolvedVer = "")) + expect_true(Require:::pakConstraintSatisfied( + installedVer = "3.1.0", versionSpec = "3.1.0", inequality = ">=")) +})