diff --git a/R/path.R b/R/path.R index e08e7822b..84bb1c2c0 100644 --- a/R/path.R +++ b/R/path.R @@ -195,15 +195,12 @@ cmdstan_version_for_comparison <- function(version) { sub("-rc[0-9]+$", "", version) } -# Scalar comparison of versions numbers. Returns -1, 0, or 1. -# Empty strings are used when no native or WSL install was found during path discovery. +# Scalar comparison of version numbers. Returns -1, 0, or 1. Both +# arguments must be versions, so a caller with a possibly missing one +# checks that itself first. cmdstan_version_compare <- function(version, other) { - if (length(version) != 1 || is.na(version) || !nzchar(version)) { - return(-1L) - } - if (length(other) != 1 || is.na(other) || !nzchar(other)) { - return(1L) - } + checkmate::assert_string(version, min.chars = 1) + checkmate::assert_string(other, min.chars = 1) utils::compareVersion( cmdstan_version_for_comparison(version), cmdstan_version_for_comparison(other) @@ -319,11 +316,14 @@ cmdstan_default_path <- function(dir = NULL) { if (!nzchar(latest_cmdstan) && !nzchar(latest_wsl_cmdstan)) { return(NULL) } - if (cmdstan_version_compare(latest_wsl_cmdstan, latest_cmdstan) >= 0) { - return(file.path(wsl_installs_path, latest_wsl_cmdstan)) - } else { + if (!nzchar(latest_wsl_cmdstan)) { return(file.path(installs_path, latest_cmdstan)) } + if (!nzchar(latest_cmdstan) || + cmdstan_version_compare(latest_wsl_cmdstan, latest_cmdstan) >= 0) { + return(file.path(wsl_installs_path, latest_wsl_cmdstan)) + } + return(file.path(installs_path, latest_cmdstan)) } NULL } diff --git a/R/zzz.R b/R/zzz.R index ba5c44c77..decc6cec6 100644 --- a/R/zzz.R +++ b/R/zzz.R @@ -35,11 +35,14 @@ startup_messages <- function() { ) } if (!skip_version_check) { - latest_version <- try(suppressWarnings(latest_released_version(retries = 0)), silent = TRUE) - current_version <- try(cmdstan_version(), silent = TRUE) - if (!inherits(latest_version, "try-error") - && !inherits(current_version, "try-error") - && cmdstan_version_compare(latest_version, current_version) > 0) { + newer <- try( + cmdstan_version_compare( + suppressWarnings(latest_released_version(retries = 0)), + cmdstan_version() + ) > 0, + silent = TRUE + ) + if (isTRUE(newer)) { packageStartupMessage( "\nA newer version of CmdStan is available. See ?install_cmdstan() to install it.", "\nTo disable this check set option or environment variable cmdstanr_no_ver_check=TRUE." diff --git a/dev-notes/compilation-state.md b/dev-notes/compilation-state.md index a417d24cb..a03a741be 100644 --- a/dev-notes/compilation-state.md +++ b/dev-notes/compilation-state.md @@ -3835,22 +3835,22 @@ reintroducing `NA` for unknown in the record reintroduces the collapse. §8 uses `NA` in the public result for the opposite reason, and serialization is the whole of the difference: what never reaches a file keeps its R type. -`cmdstan_version_compare()` is a third instance, in a different costume: it returns -`-1` for a version that is missing, `NA`, or empty (`R/path.R:162-164`), so an -absent version compares as older than everything and every `<` gate fires. That -fallback is correct where it is used, in install-path code where "no CmdStan" -should lose every comparison. It is wrong for a model. - -The guard is also narrower than "unusable", which matters because it removes the -temptation to lean on it: a malformed non-empty string never reaches the `-1` at -all. Both `".."` and `"garbage"` error inside `utils::compareVersion()`, with -`missing value where TRUE/FALSE needed`; `"garbage"` emits `NAs introduced by -coercion` first. So the bad-input behaviour is an error, sometimes with a warning -in front of it, and never the `-1`. Filed as #1260, separately from this design: -the comparison should not answer a question it was not asked, but fixing it is -defence in depth rather than what closes this. That is why §7 makes "an adopted -executable always yields a valid version" a checked invariant rather than an -observation, and why a model without an executable cannot reach a gate (§8). +`cmdstan_version_compare()` was a third instance, in a different costume: it +returned `-1` for a version that was missing, `NA`, or empty, so an absent +version compared as older than everything and every `<` gate fired. That +fallback suited the install-path code, where "no CmdStan" should lose every +comparison, and it was wrong for a model. #1260 removed it: both arguments +must be non-empty strings, and the two callers that can meet a missing +version, `cmdstan_default_path()` and the startup check for a newer release, +handle that case before comparing. A malformed non-empty string never reached +the `-1` in the first place: both `".."` and `"garbage"` error inside +`utils::compareVersion()`, with `missing value where TRUE/FALSE needed`; +`"garbage"` emits `NAs introduced by coercion` first. So the bad-input +behaviour is an error, sometimes with a warning in front of it. Fixing the +comparison is defence in depth rather than what closes this: that is why §7 +makes "an adopted executable always yields a valid version" a checked +invariant rather than an observation, and why a model without an executable +cannot reach a gate (§8). diff --git a/tests/testthat/test-path.R b/tests/testthat/test-path.R index 14b25dad6..bae7c3f95 100644 --- a/tests/testthat/test-path.R +++ b/tests/testthat/test-path.R @@ -256,6 +256,7 @@ test_that("CmdStan version helpers handle invalid inputs", { expect_identical(cmdstan_min_version(), "2.35.0") expect_false(is_supported_cmdstan_version(NULL)) expect_false(is_supported_cmdstan_version("not-a-version")) + expect_error(cmdstan_version_compare("", "2.35.0")) }) test_that("CmdStan version helpers use numeric ordering", {