From 22522413c3775569a82509881e774f2c895402b0 Mon Sep 17 00:00:00 2001 From: jgabry Date: Tue, 22 Sep 2026 10:07:42 -0600 Subject: [PATCH 1/5] Launch the model executable with the TBB it was built against On Windows the executable finds tbb.dll through PATH, and every launch put the selected installation's lib/tbb there, which is the wrong TBB once a different installation is selected. The four sites that launch the model executable now use the directory the build record holds. If that directory is gone nothing is added, so the executable is never pointed at another installation's TBB. Without a usable record the selected installation's is used as before. When a launch fails and the recorded directory is gone, the error says so and adds reinstalling it as a remedy. The Windows skip added to the adoption test in #1280 is removed, since that is the case the record now covers. Closes #1261. Part of #1258. --- NEWS.md | 4 ++ R/args.R | 2 + R/build.R | 5 ++- R/build_record.R | 12 ++++-- R/model.R | 22 +++++++--- R/path.R | 25 ++++++++++++ R/run.R | 41 ++++++++++++++----- dev-notes/compilation-state-contract.md | 2 +- dev-notes/compilation-state.md | 23 ++++++----- tests/testthat/test-build-record-compile.R | 47 ++++++++++++++++++++++ tests/testthat/test-model-rebuild-rules.R | 30 ++++++++++++-- tests/testthat/test-model-rebuild.R | 6 +-- 12 files changed, 180 insertions(+), 39 deletions(-) diff --git a/NEWS.md b/NEWS.md index dfbb557c8..4fa05d44c 100644 --- a/NEWS.md +++ b/NEWS.md @@ -237,6 +237,10 @@ as the other methods. (#1205) or was built for another platform, now gives an error naming the executable and saying how to rebuild it. Previously the fitting methods and `$cmdstan_defaults()` surfaced a raw `processx` error. (#1246) +* On Windows a model executable is now launched with the TBB it was built +against, which is recorded at build time. Previously the selected CmdStan +installation's TBB was used, which was wrong once `set_cmdstan_path()` had +selected a different one. (#1261) ## Removed and deprecated diff --git a/R/args.R b/R/args.R index 10c2f9d7e..7e09491d3 100644 --- a/R/args.R +++ b/R/args.R @@ -31,6 +31,7 @@ CmdStanArgs <- R6::R6Class( model_methods_env = NULL, standalone_env = NULL, exe_file, + tbb_dir = NULL, proc_ids, method_args, data_file = NULL, @@ -49,6 +50,7 @@ CmdStanArgs <- R6::R6Class( self$stan_file <- stan_file self$stan_code <- stan_code self$exe_file <- exe_file + self$tbb_dir <- tbb_dir self$model_methods_env <- model_methods_env self$standalone_env <- standalone_env self$proc_ids <- proc_ids diff --git a/R/build.R b/R/build.R index caecedb55..51dbb03ce 100644 --- a/R/build.R +++ b/R/build.R @@ -159,16 +159,17 @@ build_executable <- function(stan_file, run_make( c(wsl_safe_path(repair_path(tmp_exe)), make_vars, stancflags_make), quiet ) + tbb_dir <- tbb_dir_from_options(cpp_options) record <- new_build_record( configuration = append( configuration, list(stanc_options_from_make = as.list(from_make)), after = 3 ), - reported_features = reported_features_from_exe(tmp_exe), + reported_features = reported_features_from_exe(tmp_exe, tbb_dir), dependencies = current$dependencies, executable_hash = hash_file(tmp_exe), cmdstan = current$cmdstan, - tbb_dir = tbb_dir_from_options(cpp_options), + tbb_dir = tbb_dir, untracked_dependencies = untracked_dependencies( current$dependencies$make_local$built_from, user_header ) diff --git a/R/build_record.R b/R/build_record.R index ddf75f9f6..fb821d3aa 100644 --- a/R/build_record.R +++ b/R/build_record.R @@ -288,13 +288,15 @@ new_build_record <- function(configuration, reported_features, dependencies, #' Run an executable's `info` command #' #' @param exe_file Path to the executable. +#' @param tbb_dir The TBB directory the build resolved, or `NULL` when there +#' is no usable record. #' @return The `processx::run()` result. A non-zero exit is not an error. #' @noRd -run_info_cli <- function(exe_file) { +run_info_cli <- function(exe_file, tbb_dir = NULL) { withr::with_path( c( toolchain_PATH_env_var(), - tbb_path() + tbb_launch_path(tbb_dir) ), wsl_compatible_run( command = wsl_safe_path(exe_file), @@ -341,12 +343,14 @@ parse_exe_info_string <- function(ret_stdout) { #' every feature unknown rather than failing the build. #' #' @param exe_file Path to the executable. +#' @param tbb_dir The TBB directory the build resolved, or `NULL` when there +#' is no usable record. #' @return A named list of what was kept, empty when nothing was. #' @noRd -reported_features_from_exe <- function(exe_file) { +reported_features_from_exe <- function(exe_file, tbb_dir = NULL) { unknown <- structure(list(), names = character()) tryCatch({ - result <- run_info_cli(exe_file) + result <- run_info_cli(exe_file, tbb_dir) if (result$status != 0) { unknown } else { diff --git a/R/model.R b/R/model.R index 942f4a412..0ba62a52e 100644 --- a/R/model.R +++ b/R/model.R @@ -1066,6 +1066,7 @@ sample <- function(data = NULL, standalone_env = private$standalone_functions(), model_name = self$model_name(), exe_file = self$exe_file(), + tbb_dir = private$record_$tbb_dir, proc_ids = checkmate::assert_integerish(chain_ids, lower = 1, len = chains, unique = TRUE, null.ok = FALSE), data_file = process_data(data, model_variables), save_latent_dynamics = save_latent_dynamics, @@ -1225,6 +1226,7 @@ sample_mpi <- function(data = NULL, standalone_env = private$standalone_functions(), model_name = self$model_name(), exe_file = self$exe_file(), + tbb_dir = private$record_$tbb_dir, proc_ids = checkmate::assert_integerish(chain_ids, lower = 1, len = chains, unique = TRUE, null.ok = FALSE), data_file = process_data(data, model_variables), save_latent_dynamics = save_latent_dynamics, @@ -1360,6 +1362,7 @@ optimize <- function(data = NULL, standalone_env = private$standalone_functions(), model_name = self$model_name(), exe_file = self$exe_file(), + tbb_dir = private$record_$tbb_dir, proc_ids = 1, data_file = process_data(data, model_variables), save_latent_dynamics = FALSE, @@ -1532,6 +1535,7 @@ laplace <- function(data = NULL, standalone_env = private$standalone_functions(), model_name = self$model_name(), exe_file = self$exe_file(), + tbb_dir = private$record_$tbb_dir, proc_ids = 1, data_file = process_data(data, model_variables), save_latent_dynamics = FALSE, @@ -1664,6 +1668,7 @@ variational <- function(data = NULL, standalone_env = private$standalone_functions(), model_name = self$model_name(), exe_file = self$exe_file(), + tbb_dir = private$record_$tbb_dir, proc_ids = 1, data_file = process_data(data, model_variables), save_latent_dynamics = save_latent_dynamics, @@ -1846,6 +1851,7 @@ pathfinder <- function(data = NULL, standalone_env = private$standalone_functions(), model_name = self$model_name(), exe_file = self$exe_file(), + tbb_dir = private$record_$tbb_dir, proc_ids = 1, data_file = process_data(data, model_variables), save_latent_dynamics = FALSE, @@ -1983,6 +1989,7 @@ generate_quantities <- function(fitted_params, standalone_env = private$standalone_functions(), model_name = self$model_name(), exe_file = self$exe_file(), + tbb_dir = private$record_$tbb_dir, proc_ids = seq_along(fitted_params_files), data_file = process_data(data, model_variables), seed = seed, @@ -2055,6 +2062,7 @@ diagnose <- function(data = NULL, standalone_env = private$standalone_functions(), model_name = self$model_name(), exe_file = self$exe_file(), + tbb_dir = private$record_$tbb_dir, proc_ids = 1, data_file = process_data(data, model_variables), seed = seed, @@ -2160,7 +2168,9 @@ cmdstan_defaults <- function(method = c("sample", "optimize", "variational", "pathfinder", "laplace")) { method <- match.arg(method) private$assert_current() - parse_cmdstan_args(self$exe_file(), method, self$stan_file()) + parse_cmdstan_args( + self$exe_file(), method, self$stan_file(), private$record_$tbb_dir + ) } CmdStanModel$set("public", name = "cmdstan_defaults", value = cmdstan_defaults) @@ -2289,13 +2299,14 @@ assert_stan_file_exists <- function(stan_file) { #' `"variational"`, `"pathfinder"`, or `"laplace"`. #' @param stan_file The model's Stan file, for the error when the binary #' will not run. +#' @param tbb_dir The record's `tbb_dir`, or `NULL` without a usable record. #' @return A named list with cmdstanr-style argument names and default #' values. -parse_cmdstan_args <- function(model_binary, method, stan_file) { +parse_cmdstan_args <- function(model_binary, method, stan_file, tbb_dir) { withr::with_path( c( toolchain_PATH_env_var(), - tbb_path() + tbb_launch_path(tbb_dir) ), ret <- tryCatch( wsl_compatible_run( @@ -2304,7 +2315,7 @@ parse_cmdstan_args <- function(model_binary, method, stan_file) { error_on_status = FALSE ), error = function(e) { - stop_cannot_run(model_binary, stan_file, conditionMessage(e)) + stop_cannot_run(model_binary, stan_file, conditionMessage(e), tbb_dir) } ) ) @@ -2314,7 +2325,8 @@ parse_cmdstan_args <- function(model_binary, method, stan_file) { output <- trimws(paste0(ret$stderr, ret$stdout)) stop_cannot_run( model_binary, stan_file, - if (nzchar(output)) output else paste("exit status", ret$status) + if (nzchar(output)) output else paste("exit status", ret$status), + tbb_dir ) } # CmdStan may write help text to stdout or stderr depending on the platform diff --git a/R/path.R b/R/path.R index 0b3ff11fb..e08e7822b 100644 --- a/R/path.R +++ b/R/path.R @@ -441,3 +441,28 @@ tbb_path <- function(dir = NULL) { } path_to_TBB } + +#' The TBB directory to put on PATH when launching a model executable +#' +#' On Windows there's no rpath, so the executable looks for tbb.dll on PATH and +#' we need to put a TBB directory there. We use the one from the build record, +#' i.e., the TBB the model was actually built against, even if the user has +#' selected a different CmdStan installation since then. If that directory no +#' longer exists we don't add anything, in which case the executable either uses +#' whatever TBB is already on PATH or fails to load if there isn't one. If +#' there's no usable build record we fall back to the selected installation's +#' TBB, which is what we always did before. +#' +#' @param tbb_dir The record's `tbb_dir`, or `NULL` when there is no usable +#' record. +#' @return The directory, or `NULL` when nothing should go on PATH. +#' @noRd +tbb_launch_path <- function(tbb_dir) { + if (!os_is_windows()) { + return(NULL) + } + if (is.null(tbb_dir)) { + return(tbb_path()) + } + if (dir.exists(tbb_dir)) tbb_dir else NULL +} diff --git a/R/run.R b/R/run.R index 07a3ac96d..7d07033ae 100644 --- a/R/run.R +++ b/R/run.R @@ -474,22 +474,39 @@ check_target_exe <- function(exe) { #' relative path like `./bernoulli` and an errno. This one names the #' executable, keeps the system's reason (for example "Permission denied") #' or the executable's own output, and says how to rebuild it, or that -#' there is no Stan file to rebuild it from. +#' there is no Stan file to rebuild it from. When the TBB the build linked +#' against is no longer there it says so, since that is the likely cause +#' and reinstalling it is the other way out. #' #' @param exe_file Path to the executable. #' @param stan_file The model's Stan file, empty for a model created from an #' executable alone. #' @param reason processx's error message, or what the executable printed. +#' @param tbb_dir The record's `tbb_dir`, or `NULL` without a usable record. #' @noRd -stop_cannot_run <- function(exe_file, stan_file, reason) { +stop_cannot_run <- function(exe_file, stan_file, reason, tbb_dir = NULL) { system_error <- regmatches( reason, regexec("\\(system error [0-9]+, ([^)]*)\\)", reason) )[[1]] if (length(system_error) == 2) { reason <- system_error[[2]] } - remedy <- if (length(stan_file) > 0) { + tbb_gone <- !is.null(tbb_dir) && !dir.exists(tbb_dir) + if (tbb_gone) { + reason <- paste0( + reason, "\nThe TBB it was built against at '", tbb_dir, + "' no longer exists." + ) + } + remedy <- if (length(stan_file) > 0 && tbb_gone) { + paste( + "Reinstall it there or run cmdstan_model() with force_recompile = TRUE", + "to rebuild it." + ) + } else if (length(stan_file) > 0) { "Run cmdstan_model() with force_recompile = TRUE to rebuild it." + } else if (tbb_gone) { + "Reinstall it there; there is no Stan file to rebuild it from." } else { "There is no Stan file to rebuild it from." } @@ -554,6 +571,7 @@ stop_cannot_run <- function(exe_file, stan_file, reason) { args = self$command_args()[[chain_id]], exe_file = self$exe_file(), stan_file = self$args$stan_file, + tbb_dir = self$args$tbb_dir, mpi_cmd = mpi_cmd, mpi_args = mpi_args ) @@ -616,7 +634,8 @@ CmdStanRun$set("private", name = "run_sample_", value = .run_sample) command = self$command(), args = self$command_args()[[chain_id]], exe_file = self$exe_file(), - stan_file = self$args$stan_file + stan_file = self$args$stan_file, + tbb_dir = self$args$tbb_dir ) procs$mark_proc_start(chain_id) procs$set_active_procs(procs$active_procs() + 1) @@ -652,7 +671,8 @@ CmdStanRun$set("private", name = "run_generate_quantities_", value = .run_genera command = self$command(), args = self$command_args()[[id]], exe_file = self$exe_file(), - stan_file = self$args$stan_file + stan_file = self$args$stan_file, + tbb_dir = self$args$tbb_dir ) procs$set_active_procs(1) procs$mark_proc_start(id) @@ -702,7 +722,7 @@ CmdStanRun$set("private", name = "run_pathfinder_", value = .run_other) withr::with_path( c( toolchain_PATH_env_var(), - tbb_path() + tbb_launch_path(self$args$tbb_dir) ), ret <- tryCatch( wsl_compatible_run( @@ -716,7 +736,8 @@ CmdStanRun$set("private", name = "run_pathfinder_", value = .run_other) ), error = function(e) { stop_cannot_run( - self$exe_file(), self$args$stan_file, conditionMessage(e) + self$exe_file(), self$args$stan_file, conditionMessage(e), + self$args$tbb_dir ) } ) @@ -818,7 +839,7 @@ CmdStanProcs <- R6::R6Class( get_proc = function(id) { private$processes_[[id]] }, - new_proc = function(id, command, args, exe_file, stan_file, + new_proc = function(id, command, args, exe_file, stan_file, tbb_dir, mpi_cmd = NULL, mpi_args = NULL) { if (!is.null(mpi_cmd)) { exe_name <- mpi_args[["exe"]] @@ -833,7 +854,7 @@ CmdStanProcs <- R6::R6Class( withr::with_path( c( toolchain_PATH_env_var(), - tbb_path() + tbb_launch_path(tbb_dir) ), private$processes_[[id]] <- tryCatch( wsl_compatible_process_new( @@ -850,7 +871,7 @@ CmdStanProcs <- R6::R6Class( if (!is.null(mpi_cmd)) { stop(e) } - stop_cannot_run(exe_file, stan_file, conditionMessage(e)) + stop_cannot_run(exe_file, stan_file, conditionMessage(e), tbb_dir) } ) ) diff --git a/dev-notes/compilation-state-contract.md b/dev-notes/compilation-state-contract.md index 0222caf85..ab153aaf5 100644 --- a/dev-notes/compilation-state-contract.md +++ b/dev-notes/compilation-state-contract.md @@ -639,7 +639,7 @@ part of `cmdstan`. `stan_build_info()` reports the builder with `exists = FALSE`, the treatment §7 already gives recorded sources that are gone, and a launch failure -becomes an error naming the recorded installation, with reinstalling it or +becomes an error naming the recorded TBB directory, with reinstalling it or rebuilding from source as the two remedies. **A selected installation that is gone is its own error, checked where it is used.** diff --git a/dev-notes/compilation-state.md b/dev-notes/compilation-state.md index 4297eb62e..ab9898bcf 100644 --- a/dev-notes/compilation-state.md +++ b/dev-notes/compilation-state.md @@ -2055,11 +2055,15 @@ the directory is written at build time rather than derived at launch (§4). **Which TBB the executable is launched with is a launch-side rule, not an assessment one.** On macOS and Linux the rpath settles it. On Windows there is no -rpath and cmdstanr supplies the directory on `PATH`, today the selected -installation's rather than the builder's (`R/run.R:1238-1247`), which is wrong as -soon as the two differ. The fix reads `tbb_dir` from the record at the sites that -launch the model binary and is #1261. It lands after Stage 3, needs no format change -because the field is already recorded, and changes no verdict here. +rpath and cmdstanr supplies the directory on `PATH`: the record's `tbb_dir` when +it still exists, nothing when it is gone, and the selected installation's only +when there is no usable record (#1261, `tbb_launch_path()`). A gone directory is +not a reason to substitute another: for a default-layout build that is the +builder going, and running the binary against whatever installation is selected +now is the fault #1261 removed, by a second route. The sites that launch an +installation program, `stanc`, `make` and `bin/stansummary`, keep the selected +installation's, since that installation owns the program. None of it changes a +verdict here. @@ -2081,7 +2085,7 @@ deleted. Recording the TBB directory separately from the builder is what lets th Windows launch treat a gone builder and a gone TBB as two events, and that build's TBB is still there. `stan_build_info()` reports the builder with `exists = FALSE`, the treatment §7 already gives recorded sources that are gone, and a launch failure -becomes an error naming the recorded installation, with reinstalling it or +becomes an error naming the recorded TBB directory, with reinstalling it or rebuilding from source as the two remedies. @@ -2389,7 +2393,8 @@ from `$cmdstan_defaults()`, one that started but could not answer `help-all`, th loader's own message standing as the reason. A chain that starts and dies in the loader is reported as a chain that finished unexpectedly, with the loader's output, because nothing short of classifying stderr tells that apart from an ordinary -nonzero exit; the TBB-specific remedy is #1261's. It names the executable, and for a source-backed +nonzero exit. When the record's TBB directory is gone the error says so +and adds reinstalling it as a remedy (#1261, above). It names the executable, and for a source-backed model says that `force_recompile = TRUE` rebuilds it; with only an executable (§7) there is nothing to rebuild and it says so instead. @@ -3750,8 +3755,8 @@ reports the installed CmdStan, not the one that built the executable, caused by `R/model.R:318` and not `dry_run`). Small, user-visible, and the natural work to pick up while Stage 0 is in review. -**#1261** (launch the model executable with the TBB its build resolved) is gated -only on Stage 3 having written `tbb_dir`, and lands any time after it. +**#1261** (launch the model executable with the TBB its build resolved) needed +only Stage 3's `tbb_dir` and landed after it. --- diff --git a/tests/testthat/test-build-record-compile.R b/tests/testthat/test-build-record-compile.R index 45b08ddd3..030ff6650 100644 --- a/tests/testthat/test-build-record-compile.R +++ b/tests/testthat/test-build-record-compile.R @@ -252,6 +252,53 @@ test_that("tbb_dir is the directory the call named or the installation's", { )) }) +test_that("a launch on Windows puts the recorded TBB directory on PATH", { + local_mocked_bindings(os_is_windows = function() TRUE) + recorded <- withr::local_tempdir() + expect_equal(tbb_launch_path(recorded), recorded) + # Gone: nothing goes on PATH rather than some other TBB. + expect_null(tbb_launch_path(file.path(recorded, "gone"))) + # No usable record: the selected installation's. + expect_equal(tbb_launch_path(NULL), tbb_path()) +}) + +test_that("off Windows a launch adds no TBB directory", { + local_mocked_bindings(os_is_windows = function() FALSE) + expect_null(tbb_launch_path(withr::local_tempdir())) + expect_null(tbb_launch_path(NULL)) +}) + +test_that("the model is launched with the TBB its record names", { + stan_file <- local_bernoulli() + tbb <- withr::local_tempdir() + mod <- mock_compile( + stan_file, cpp_options = list(tbb_lib = wsl_safe_path(tbb)) + ) + handed <- "unset" + local_mocked_bindings( + tbb_launch_path = function(tbb_dir) { + handed <<- tbb_dir + NULL + }, + # Only the model's help-all is mocked; stanc still runs for real. + wsl_compatible_run = function(command, args, ...) { + if ("help-all" %in% args) { + return(list(status = 0L, stdout = "", stderr = "")) + } + real_wcr(command = command, args = args, ...) + } + ) + mod$cmdstan_defaults() + expect_true(same_path(handed, tbb)) + + # Without a usable record there is nothing to hand over. + file.remove(build_record_path(mod$exe_file())) + local_mocked_bindings(run_info_cli = function(...) default_info_ret) + adopted <- cmdstan_model(exe_file = mod$exe_file()) + adopted$cmdstan_defaults() + expect_null(handed) +}) + test_that("a build with an untracked dependency records it", { local_cmdstan_make_local(cpp_options = list("-include other.mk")) make_local <- file.path(cmdstan_path(), "make", "local") diff --git a/tests/testthat/test-model-rebuild-rules.R b/tests/testthat/test-model-rebuild-rules.R index e9efdaa79..5dfa9e812 100644 --- a/tests/testthat/test-model-rebuild-rules.R +++ b/tests/testthat/test-model-rebuild-rules.R @@ -24,9 +24,9 @@ local_info_launches <- function(.local_envir = parent.frame()) { counter$n <- 0L real_run_info_cli <- run_info_cli local_mocked_bindings( - run_info_cli = function(exe_file) { + run_info_cli = function(exe_file, ...) { counter$n <- counter$n + 1L - real_run_info_cli(exe_file) + real_run_info_cli(exe_file, ...) }, .env = .local_envir ) @@ -449,7 +449,9 @@ test_that("an executable that starts but cannot answer help-all is an error", { ) }) expect_error( - parse_cmdstan_args("/models/bern", "sample", "/models/bern.stan"), + parse_cmdstan_args( + "/models/bern", "sample", "/models/bern.stan", tbb_dir = NULL + ), paste0( "^The executable at '/models/bern' could not be run: ", "error while loading shared libraries: libtbb.so: not found\n", @@ -458,6 +460,28 @@ test_that("an executable that starts but cannot answer help-all is an error", { ) }) +test_that("a failed launch says when the recorded TBB is gone", { + local_mocked_bindings(wsl_compatible_run = function(...) { + list(status = 127L, stdout = "", stderr = "") + }) + gone <- file.path(withr::local_tempdir(), "tbb") + expect_error( + parse_cmdstan_args("/models/bern", "sample", "/models/bern.stan", gone), + paste0( + "could not be run: exit status 127\n", + "The TBB it was built against at '", gone, "' no longer exists.\n", + "Reinstall it there or run cmdstan_model() with force_recompile = TRUE ", + "to rebuild it." + ), + fixed = TRUE + ) + expect_error( + parse_cmdstan_args("/models/bern", "sample", character(), gone), + "Reinstall it there; there is no Stan file to rebuild it from.", + fixed = TRUE + ) +}) + test_that("a record member with a longer name is not read as the user header", { stan_file <- local_bernoulli() a <- mock_cmdstan_model(stan_file) diff --git a/tests/testthat/test-model-rebuild.R b/tests/testthat/test-model-rebuild.R index 96515ad79..fb9965efe 100644 --- a/tests/testthat/test-model-rebuild.R +++ b/tests/testthat/test-model-rebuild.R @@ -119,11 +119,7 @@ test_that("adoption without a record verifies by the hash it captured", { test_that("adoption from a record needs no installation", { gone <- local_gone_installation() mod_e <- cmdstan_model(exe_file = mod_b$exe_file()) - # On Windows the executable finds tbb.dll through the selected installation - # until #1261 takes the directory from the record instead. - if (!os_is_windows()) { - expect_no_error(mod_e$cmdstan_defaults()) - } + expect_no_error(mod_e$cmdstan_defaults()) expect_error(cmdstan_model(replaced_stan_file), gone, fixed = TRUE) }) From 56b3cdaccd841e03c9fea1456290aa0c2f88563e Mon Sep 17 00:00:00 2001 From: jgabry Date: Tue, 22 Sep 2026 10:16:05 -0600 Subject: [PATCH 2/5] Test platform branches on the platform they apply to The TBB launch helper's Windows branch and the WSLENV assembly were tested everywhere by mocking os_is_windows() and os_is_wsl(). The Windows and WSL CI jobs run those branches for real, so the tests now check whichever branch the platform takes. --- tests/testthat/test-build-record-compile.R | 16 +++++++--------- tests/testthat/test-threads.R | 2 +- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/tests/testthat/test-build-record-compile.R b/tests/testthat/test-build-record-compile.R index 030ff6650..a58d8df3f 100644 --- a/tests/testthat/test-build-record-compile.R +++ b/tests/testthat/test-build-record-compile.R @@ -252,9 +252,13 @@ test_that("tbb_dir is the directory the call named or the installation's", { )) }) -test_that("a launch on Windows puts the recorded TBB directory on PATH", { - local_mocked_bindings(os_is_windows = function() TRUE) +test_that("a launch puts the recorded TBB directory on PATH only on Windows", { recorded <- withr::local_tempdir() + if (!os_is_windows()) { + expect_null(tbb_launch_path(recorded)) + expect_null(tbb_launch_path(NULL)) + skip("the rest applies on Windows") + } expect_equal(tbb_launch_path(recorded), recorded) # Gone: nothing goes on PATH rather than some other TBB. expect_null(tbb_launch_path(file.path(recorded, "gone"))) @@ -262,12 +266,6 @@ test_that("a launch on Windows puts the recorded TBB directory on PATH", { expect_equal(tbb_launch_path(NULL), tbb_path()) }) -test_that("off Windows a launch adds no TBB directory", { - local_mocked_bindings(os_is_windows = function() FALSE) - expect_null(tbb_launch_path(withr::local_tempdir())) - expect_null(tbb_launch_path(NULL)) -}) - test_that("the model is launched with the TBB its record names", { stan_file <- local_bernoulli() tbb <- withr::local_tempdir() @@ -293,7 +291,7 @@ test_that("the model is launched with the TBB its record names", { # Without a usable record there is nothing to hand over. file.remove(build_record_path(mod$exe_file())) - local_mocked_bindings(run_info_cli = function(...) default_info_ret) + local_mocked_bindings(run_exe_info = function(...) default_info_ret) adopted <- cmdstan_model(exe_file = mod$exe_file()) adopted$cmdstan_defaults() expect_null(handed) diff --git a/tests/testthat/test-threads.R b/tests/testthat/test-threads.R index 25efb01da..062356d83 100644 --- a/tests/testthat/test-threads.R +++ b/tests/testthat/test-threads.R @@ -58,7 +58,7 @@ test_that("the thread count reaches the child process and not the session", { }) test_that("WSLENV keeps the entries the session already exports", { - local_mocked_bindings(os_is_wsl = function() TRUE, .package = "cmdstanr") + skip_if(!os_is_wsl()) withr::local_envvar(WSLENV = "A/u:STAN_NUM_THREADS/u:B/p") expect_equal(cmdstan_process_env(4)[["WSLENV"]], "A/u:B/p:STAN_NUM_THREADS/u") From 7b5569779ac6f681a4795b6da5cb5a42f3b1f9d0 Mon Sep 17 00:00:00 2001 From: jgabry Date: Tue, 22 Sep 2026 10:16:05 -0600 Subject: [PATCH 3/5] Rename run_info_cli() to run_exe_info() What it runs is the executable's own info command, and the new name reads naturally beside reported_features_from_exe(). --- R/build_record.R | 4 ++-- tests/testthat/test-build-info.R | 22 +++++++++++----------- tests/testthat/test-build-record.R | 2 +- tests/testthat/test-model-rebuild-rules.R | 6 +++--- 4 files changed, 17 insertions(+), 17 deletions(-) diff --git a/R/build_record.R b/R/build_record.R index fb821d3aa..89695d4d3 100644 --- a/R/build_record.R +++ b/R/build_record.R @@ -292,7 +292,7 @@ new_build_record <- function(configuration, reported_features, dependencies, #' is no usable record. #' @return The `processx::run()` result. A non-zero exit is not an error. #' @noRd -run_info_cli <- function(exe_file, tbb_dir = NULL) { +run_exe_info <- function(exe_file, tbb_dir = NULL) { withr::with_path( c( toolchain_PATH_env_var(), @@ -350,7 +350,7 @@ parse_exe_info_string <- function(ret_stdout) { reported_features_from_exe <- function(exe_file, tbb_dir = NULL) { unknown <- structure(list(), names = character()) tryCatch({ - result <- run_info_cli(exe_file, tbb_dir) + result <- run_exe_info(exe_file, tbb_dir) if (result$status != 0) { unknown } else { diff --git a/tests/testthat/test-build-info.R b/tests/testthat/test-build-info.R index 74a85ecc6..b2d81a1b8 100644 --- a/tests/testthat/test-build-info.R +++ b/tests/testthat/test-build-info.R @@ -1,6 +1,6 @@ # Write an example record beside a fake executable and read it back with # stan_build_info(). `edit` changes the record before it is written. The -# record is valid, so the executable must never be run: run_info_cli() is +# record is valid, so the executable must never be run: run_exe_info() is # mocked to count calls and the count must stay zero. Counting matters # because reported_features_from_exe() swallows errors, so a mock that # only stops would let an unneeded run go unnoticed. @@ -9,7 +9,7 @@ available_result <- function(edit = identity) { write_build_record(edit(example_record(exe)), exe) launches <- 0 local_mocked_bindings( - run_info_cli = function(...) launches <<- launches + 1, + run_exe_info = function(...) launches <<- launches + 1, .package = "cmdstanr" ) result <- stan_build_info(exe) @@ -113,7 +113,7 @@ test_that("fields the record withholds are absent from the result", { test_that("a missing build record falls back to the executable's own info", { exe <- local_fake_exe() local_mocked_bindings( - run_info_cli = function(...) default_info_ret, + run_exe_info = function(...) default_info_ret, .package = "cmdstanr" ) result <- stan_build_info(exe) @@ -133,7 +133,7 @@ test_that("an unreadable build record falls back to the executable's own info", jsonlite::write_json(record, build_record_path(exe), auto_unbox = TRUE) local_mocked_bindings( - run_info_cli = function(...) default_info_ret, + run_exe_info = function(...) default_info_ret, .package = "cmdstanr" ) result <- stan_build_info(exe) @@ -147,7 +147,7 @@ test_that("an unreadable build record falls back to the executable's own info", test_that("an unsupported record format reports its version and nothing else", { local_mocked_bindings( - run_info_cli = function(...) default_info_ret, + run_exe_info = function(...) default_info_ret, .package = "cmdstanr" ) @@ -189,7 +189,7 @@ test_that("an executable mismatch discards the recorded features for the binary' "STAN_THREADS=true", "STAN_THREADS=false", mismatched_info$stdout, fixed = TRUE ) local_mocked_bindings( - run_info_cli = function(...) mismatched_info, + run_exe_info = function(...) mismatched_info, .package = "cmdstanr" ) result <- stan_build_info(exe) @@ -273,7 +273,7 @@ test_that("an empty untracked dependencies list differs from having no record at exe <- local_fake_exe() local_mocked_bindings( - run_info_cli = function(...) default_info_ret, + run_exe_info = function(...) default_info_ret, .package = "cmdstanr" ) missing_result <- stan_build_info(exe) @@ -314,7 +314,7 @@ test_that("a real user header is reported under dependencies and nowhere else", launches <- 0 local_mocked_bindings( - run_info_cli = function(...) launches <<- launches + 1, + run_exe_info = function(...) launches <<- launches + 1, .package = "cmdstanr" ) result <- stan_build_info(mod$exe_file()) @@ -410,7 +410,7 @@ test_that("stan_build_info() errors on unusable paths and unidentifiable executa failed_exe <- local_fake_exe("failed") local_mocked_bindings( - run_info_cli = function(...) list(status = 1, stdout = ""), + run_exe_info = function(...) list(status = 1, stdout = ""), .package = "cmdstanr" ) expect_error( @@ -426,7 +426,7 @@ test_that("stan_build_info() errors on unusable paths and unidentifiable executa no_version_exe <- local_fake_exe("no_version") local_mocked_bindings( - run_info_cli = function(...) list(status = 0, stdout = "STAN_THREADS=true\n"), + run_exe_info = function(...) list(status = 0, stdout = "STAN_THREADS=true\n"), .package = "cmdstanr" ) expect_error( @@ -443,7 +443,7 @@ test_that("stan_build_info() errors on unusable paths and unidentifiable executa test_that("features reported by the binary have the fixed shape", { exe <- local_fake_exe() local_mocked_bindings( - run_info_cli = function(...) default_info_ret, + run_exe_info = function(...) default_info_ret, .package = "cmdstanr" ) result <- stan_build_info(exe) diff --git a/tests/testthat/test-build-record.R b/tests/testthat/test-build-record.R index cb2153642..67667c156 100644 --- a/tests/testthat/test-build-record.R +++ b/tests/testthat/test-build-record.R @@ -576,7 +576,7 @@ test_that("reported features the record cannot hold are left unknown", { exe <- local_fake_exe() # A nameless entry and a missing version are both dropped, not written. local_mocked_bindings( - run_info_cli = function(...) { + run_exe_info = function(...) { list(status = 0, stdout = " = true\nSTAN_THREADS=true\n") } ) diff --git a/tests/testthat/test-model-rebuild-rules.R b/tests/testthat/test-model-rebuild-rules.R index 5dfa9e812..bc4b2a799 100644 --- a/tests/testthat/test-model-rebuild-rules.R +++ b/tests/testthat/test-model-rebuild-rules.R @@ -22,11 +22,11 @@ local_bernoulli <- function(.local_envir = parent.frame()) { local_info_launches <- function(.local_envir = parent.frame()) { counter <- new.env() counter$n <- 0L - real_run_info_cli <- run_info_cli + real_run_exe_info <- run_exe_info local_mocked_bindings( - run_info_cli = function(exe_file, ...) { + run_exe_info = function(exe_file, ...) { counter$n <- counter$n + 1L - real_run_info_cli(exe_file, ...) + real_run_exe_info(exe_file, ...) }, .env = .local_envir ) From 449a03172ebcad3878be3833f00c8dfc14b82425 Mon Sep 17 00:00:00 2001 From: jgabry Date: Tue, 22 Sep 2026 12:39:23 -0600 Subject: [PATCH 4/5] Reject a make expression in the TBB directory options A TBB_LIB or TBB_BIN value like "$(MATH)lib/tbb" was recorded as written, so the record named a directory that never existed and, on Windows, the launch put nothing on PATH. Rebuilding recorded the same thing again, so the launch error's remedy didn't help. cmdstan_model() now rejects such a value before the build and asks for a literal directory. Also softens the stop_cannot_run() roxygen, since a gone TBB is one likely cause of a failed launch rather than the likely one. Part of #1258. --- R/build_record.R | 28 +++++++++++++------------ R/cpp_options.R | 14 ++++++++++++- R/run.R | 2 +- dev-notes/compilation-state-contract.md | 2 +- dev-notes/compilation-state.md | 2 +- tests/testthat/test-cpp-options.R | 21 +++++++++++++++++++ 6 files changed, 52 insertions(+), 17 deletions(-) diff --git a/R/build_record.R b/R/build_record.R index 89695d4d3..a3e024cf9 100644 --- a/R/build_record.R +++ b/R/build_record.R @@ -367,19 +367,21 @@ reported_features_from_exe <- function(exe_file, tbb_dir = NULL) { #' Where the record says the TBB is #' -#' The first non-empty of `TBB_LIB` and `TBB_BIN` from the call's -#' `cpp_options`, then the installation's own copy, which is the order the -#' makefile links in. The options are read as make receives them: the last -#' assignment wins and `FALSE` is an empty one. A relative directory is -#' resolved against the installation, where make runs. -#' -#' Make can also pick up both variables from `make/local`, -#' `~/.config/stan/make.local` or the environment. The record does not look -#' there, so a build configured that way links against one TBB while the -#' record names the installation's. On Windows the launch puts the recorded -#' directory on PATH, so such a build runs with the installation's TBB first, -#' which is what happens today anyway. We decided not to ask make for the real -#' answer for now, and could reconsider if there is demand for it. +#' We use `TBB_LIB` if the call set it, otherwise `TBB_BIN`, otherwise the +#' installation's own copy, which is the order the makefile uses when it +#' links. The options are read the way make reads them, so the last +#' assignment wins and `FALSE` counts as empty. A relative directory is +#' relative to the installation, since that's where make runs. We take the +#' value as written; `assert_valid_cpp_options()` has already rejected a +#' make expression, because nothing here could expand it. +#' +#' The two variables can also reach make from `make/local`, +#' `~/.config/stan/make.local` or the environment, and we don't look there. +#' So a build set up that way links against a different TBB than the record +#' names, which is the installation's. On Windows that means the launch puts the +#' installation's TBB on PATH, which is what happened before too. We decided +#' not to ask make for the real answer for now and can revisit if anyone +#' needs it. #' #' @param cpp_options The call's `cpp_options`, as given. #' @return The directory. Under WSL it is the Windows path. diff --git a/R/cpp_options.R b/R/cpp_options.R index aad904596..30b2ef185 100644 --- a/R/cpp_options.R +++ b/R/cpp_options.R @@ -49,7 +49,8 @@ make_variable_name_pattern <- "[A-Za-z_][A-Za-z0-9_]*" #' Every entry must be named and every name must be a Make variable name. The #' names are uppercased here so that one spelling reaches everything downstream. #' The user header and the stanc flags have their own arguments, so setting them -#' here is an error. +#' here is an error. A TBB directory has to be a literal path, since the build +#' record keeps it as written and never asks make what it expands to. #' #' @param cpp_options What the user passed, or `NULL`. #' @return The options with their names uppercased, or an empty list for @@ -87,6 +88,17 @@ assert_valid_cpp_options <- function(cpp_options) { if ("STANCFLAGS" %in% names(cpp_options)) { stop(stancflags_cpp_option_message(), call. = FALSE) } + for (tbb_at in which(names(cpp_options) %in% c("TBB_LIB", "TBB_BIN"))) { + value <- cpp_options[[tbb_at]] + if (is.character(value) && any(grepl("$", value, fixed = TRUE))) { + stop( + "`", names(cpp_options)[[tbb_at]], "` must be a literal directory. ", + "cmdstanr records it to launch the model with the right TBB and ", + "doesn't expand make expressions like `", value, "`.", + call. = FALSE + ) + } + } cpp_options } diff --git a/R/run.R b/R/run.R index 7d07033ae..19f838ac4 100644 --- a/R/run.R +++ b/R/run.R @@ -475,7 +475,7 @@ check_target_exe <- function(exe) { #' executable, keeps the system's reason (for example "Permission denied") #' or the executable's own output, and says how to rebuild it, or that #' there is no Stan file to rebuild it from. When the TBB the build linked -#' against is no longer there it says so, since that is the likely cause +#' against is no longer there it says so, since that's one likely cause #' and reinstalling it is the other way out. #' #' @param exe_file Path to the executable. diff --git a/dev-notes/compilation-state-contract.md b/dev-notes/compilation-state-contract.md index ab153aaf5..396398543 100644 --- a/dev-notes/compilation-state-contract.md +++ b/dev-notes/compilation-state-contract.md @@ -248,7 +248,7 @@ contains.** | `dependencies.included_files` | yes | yes | **ordered sequence**, duplicates preserved (§6) | | `executable_hash` | yes | yes | hash of the executable this record describes | | `cmdstan` | yes | yes | normalized installation path and version | -| `tbb_dir` | yes | **no** | the absolute TBB directory the call named, read as `make` receives the call's `cpp_options` (§3: last assignment wins, `FALSE` is an empty one): `TBB_LIB`, or `TBB_BIN` when `TBB_LIB` is empty, which is the order `compiler_flags` uses; resolved against the installation when relative, since `make` runs there; and the installation's own `lib/tbb` when the call named neither. Filled from the call, not asked of `make`: a `TBB_LIB` or `TBB_BIN` set in `make/local`, `~/.config/stan/make.local` or the environment moves the TBB the binary links against and not this field (§6). Recorded because Windows needs it at launch and a later `set_cmdstan_path()` must not move it (#1261 consumes it; no verdict here turns on it). Not compared: both routes it sees are compared already, through `cpp_options` and `cmdstan` | +| `tbb_dir` | yes | **no** | the absolute TBB directory the call named, read as `make` receives the call's `cpp_options` (§3: last assignment wins, `FALSE` is an empty one): `TBB_LIB`, or `TBB_BIN` when `TBB_LIB` is empty, which is the order `compiler_flags` uses; resolved against the installation when relative, since `make` runs there; and the installation's own `lib/tbb` when the call named neither. The value is taken as written, so a make expression in it (`$(MATH)lib/tbb`) is rejected before the build rather than recorded as a directory that does not exist. Filled from the call, not asked of `make`: a `TBB_LIB` or `TBB_BIN` set in `make/local`, `~/.config/stan/make.local` or the environment moves the TBB the binary links against and not this field (§6). Recorded because Windows needs it at launch and a later `set_cmdstan_path()` must not move it (#1261 consumes it; no verdict here turns on it). Not compared: both routes it sees are compared already, through `cpp_options` and `cmdstan` | | `untracked_dependencies` | yes | no | reported (§6), never a trigger | | `format_version` | yes | **no** | not a comparison: the reader either reads the record's version or does not, which is an artifact-side reason like unreadable JSON (§6) | diff --git a/dev-notes/compilation-state.md b/dev-notes/compilation-state.md index ab9898bcf..f48415033 100644 --- a/dev-notes/compilation-state.md +++ b/dev-notes/compilation-state.md @@ -878,7 +878,7 @@ must not restate it. A rule written in two places is a future inconsistency. | `dependencies.included_files` | yes | yes | **ordered sequence**, duplicates preserved (§6) | | `executable_hash` | yes | yes | hash of the executable this record describes | | `cmdstan` | yes | yes | normalized installation path and version | -| `tbb_dir` | yes | **no** | the absolute TBB directory the call named, read as `make` receives the call's `cpp_options` (§3: last assignment wins, `FALSE` is an empty one): `TBB_LIB`, or `TBB_BIN` when `TBB_LIB` is empty, which is the order `compiler_flags` uses; resolved against the installation when relative, since `make` runs there; and the installation's own `lib/tbb` when the call named neither. Filled from the call, not asked of `make`: a `TBB_LIB` or `TBB_BIN` set in `make/local`, `~/.config/stan/make.local` or the environment moves the TBB the binary links against and not this field (§6). Recorded because Windows needs it at launch and a later `set_cmdstan_path()` must not move it (#1261 consumes it; no verdict here turns on it). Not compared: both routes it sees are compared already, through `cpp_options` and `cmdstan` | +| `tbb_dir` | yes | **no** | the absolute TBB directory the call named, read as `make` receives the call's `cpp_options` (§3: last assignment wins, `FALSE` is an empty one): `TBB_LIB`, or `TBB_BIN` when `TBB_LIB` is empty, which is the order `compiler_flags` uses; resolved against the installation when relative, since `make` runs there; and the installation's own `lib/tbb` when the call named neither. The value is taken as written, so a make expression in it (`$(MATH)lib/tbb`) is rejected before the build rather than recorded as a directory that does not exist. Filled from the call, not asked of `make`: a `TBB_LIB` or `TBB_BIN` set in `make/local`, `~/.config/stan/make.local` or the environment moves the TBB the binary links against and not this field (§6). Recorded because Windows needs it at launch and a later `set_cmdstan_path()` must not move it (#1261 consumes it; no verdict here turns on it). Not compared: both routes it sees are compared already, through `cpp_options` and `cmdstan` | | `untracked_dependencies` | yes | no | reported (§6), never a trigger | | `format_version` | yes | **no** | not a comparison: the reader either reads the record's version or does not, which is an artifact-side reason like unreadable JSON (§6) | diff --git a/tests/testthat/test-cpp-options.R b/tests/testthat/test-cpp-options.R index 08bc8f5a3..563641f68 100644 --- a/tests/testthat/test-cpp-options.R +++ b/tests/testthat/test-cpp-options.R @@ -63,6 +63,27 @@ test_that("assert_valid_cpp_options points an empty user header at NULL", { ) }) +test_that("assert_valid_cpp_options wants a literal TBB directory", { + expect_error( + assert_valid_cpp_options(list(tbb_bin = "$(MATH)lib/tbb")), + paste( + "`TBB_BIN` must be a literal directory. cmdstanr records it to launch", + "the model with the right TBB and doesn't expand make expressions", + "like `$(MATH)lib/tbb`." + ), + fixed = TRUE + ) + expect_error( + assert_valid_cpp_options(list(TBB_LIB = "${HOME}/tbb")), + "`TBB_LIB` must be a literal directory.", + fixed = TRUE + ) + expect_identical( + assert_valid_cpp_options(list(TBB_LIB = "/opt/tbb", TBB_BIN = FALSE)), + list(TBB_LIB = "/opt/tbb", TBB_BIN = FALSE) + ) +}) + test_that("assert_valid_cpp_options rejects an unnamed assignment", { expect_error( assert_valid_cpp_options(list("FOO=1")), From e2bd1ce0c5d6c560a19ce427905b396aa9bb57c1 Mon Sep 17 00:00:00 2001 From: jgabry Date: Tue, 22 Sep 2026 12:42:17 -0600 Subject: [PATCH 5/5] Check the TBB directory options as make receives them The check for a make expression looked only at character values, so a factor or a list carrying one still reached make as the expression. Checking the assignment strings the options turn into covers every type the same way and drops the type guard. --- R/cpp_options.R | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/R/cpp_options.R b/R/cpp_options.R index 30b2ef185..cb2154778 100644 --- a/R/cpp_options.R +++ b/R/cpp_options.R @@ -89,12 +89,14 @@ assert_valid_cpp_options <- function(cpp_options) { stop(stancflags_cpp_option_message(), call. = FALSE) } for (tbb_at in which(names(cpp_options) %in% c("TBB_LIB", "TBB_BIN"))) { - value <- cpp_options[[tbb_at]] - if (is.character(value) && any(grepl("$", value, fixed = TRUE))) { + flags <- cpp_options_to_compile_flags(cpp_options[tbb_at]) + bad <- grep("$", flags, fixed = TRUE, value = TRUE) + if (length(bad) > 0) { stop( "`", names(cpp_options)[[tbb_at]], "` must be a literal directory. ", "cmdstanr records it to launch the model with the right TBB and ", - "doesn't expand make expressions like `", value, "`.", + "doesn't expand make expressions like `", sub("^[^=]*=", "", bad[[1]]), + "`.", call. = FALSE ) }