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..a3e024cf9 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_exe_info <- 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_exe_info(exe_file, tbb_dir) if (result$status != 0) { unknown } else { @@ -363,19 +367,21 @@ reported_features_from_exe <- function(exe_file) { #' 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..cb2154778 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,19 @@ 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"))) { + 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 `", sub("^[^=]*=", "", bad[[1]]), + "`.", + call. = FALSE + ) + } + } cpp_options } 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..19f838ac4 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's one 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..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) | @@ -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..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) | @@ -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-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-compile.R b/tests/testthat/test-build-record-compile.R index 45b08ddd3..a58d8df3f 100644 --- a/tests/testthat/test-build-record-compile.R +++ b/tests/testthat/test-build-record-compile.R @@ -252,6 +252,51 @@ test_that("tbb_dir is the directory the call named or the installation's", { )) }) +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"))) + # No usable record: the selected installation's. + expect_equal(tbb_launch_path(NULL), tbb_path()) +}) + +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_exe_info = 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-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-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")), diff --git a/tests/testthat/test-model-rebuild-rules.R b/tests/testthat/test-model-rebuild-rules.R index e9efdaa79..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 ) @@ -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) }) 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")