Commit cpp_options only on a real compilation

A dry run builds nothing, so it no longer records cpp_options_ or moves
hpp_file_; the latter previously pointed $hpp_file() at a temporary file
the dry run never wrote. exe_file_ and cmdstan_version_ stay in the tail,
commented as the deliberate exceptions: during a dry run they are also
the configured destination and the toolchain version, so they are
assigned on dry runs and on success but never on a failure.

cmdstan_version() is now evaluated into a local before any compilation
work rather than after the executable is installed. It is not infallible
despite being an accessor: set_cmdstan_path() stores PATH and VERSION
together, and when read_cmdstan_version() returns NULL the guard falls
through and leaves PATH set with VERSION NULL. In that state stanc and
make both run and only this call errors. It cannot be hoisted any
higher, since an ordinary no-op returns before ever reaching it and
would gain a failure mode it does not have today.

If discarding the staged candidate fails while another error is being
raised, the diagnostic now names the leftover path instead of implying
it was removed.

The two tests that asserted on $cpp_options() after a dry run move to
mocked successful compiles, and the header precedence test asserts that
the ignored spelling is dropped rather than retained.

Author: jgabry

NEWS.md

@@ -40,6 +40,30 @@ failed because those inputs were dropped. (#1234)
 * A `user_header` supplied to `cmdstan_model()` is now used by a later
 `$compile()`. Previously it was only honored when the model was compiled
 immediately. (#1234)
+* `$compile()` now accepts `user_header = NULL` to compile without a user
+header. Previously a header, once supplied, could not be removed. (#1235)
+* `$compile()` now recompiles when the user header changes. Previously a
+different header was ignored if the executable was otherwise up to date. (#1235)
+* `$compile()` now reduces duplicate `USER_HEADER`/`user_header` entries in
+`cpp_options` to the one actually used, so `$cpp_options()` no longer reports
+the ignored spelling after a successful compilation. (#1235)
+* A `$compile()` call that finds the executable up to date no longer erases
+`$cpp_options()`. Previously the recorded options were replaced with whatever
+the call supplied, so a bare `$compile()` on a model built with
+`cpp_options = list(stan_threads = TRUE)` dropped `stan_threads` and the
+executable then ran single-threaded. (#1235)
+* `$expose_functions()` now works after a `$compile()` call that found the
+executable up to date. It previously failed with "not possible with a
+pre-compiled Stan model" on a model that had compiled itself. (#1235)
+* A failed compilation no longer moves `$exe_file()` or replaces the generated
+C++ used by `$hpp_file()` and `fit$init_model_methods()`. Previously a failure
+at the C++ stage left the old executable paired with model methods generated
+from the new program. A `dry_run = TRUE` compilation likewise no longer moves
+`$hpp_file()`, which previously pointed at a temporary file it never wrote.
+(#1235)
+* `$compile()` now errors if the newly compiled executable cannot be installed,
+restoring the previous executable. Previously the replacement was unchecked, so
+a failure could silently leave the model with no executable at all. (#1235)
 * CmdStanModel methods now correctly handle `#include` directories with spaces
 in their paths. (#820)
 * `$include_paths()` now returns absolute paths, and relative include paths are

R/model.R

@@ -496,10 +496,13 @@ NULL
 #'   the model object is created (or when `$compile()` is called) and stored as
 #'   absolute paths, so subsequent changes to the working directory do not
 #'   affect them. If `$compile()` is called again without `include_paths`, the
-#'   paths used for the previous compilation are reused.
+#'   most recently supplied paths are reused.
 #' @param user_header (string) The path to a C++ file (with a .hpp extension)
 #'   to compile with the Stan model. If `$compile()` is called again without
-#'   `user_header`, the header used for the previous compilation is reused.
+#'   `user_header`, the most recently supplied header is reused, and changing
+#'   it forces recompilation. Pass `user_header = NULL` to compile without one.
+#'   A header can also be supplied via `cpp_options` as `USER_HEADER` or
+#'   `user_header`; the `user_header` argument takes precedence over both.
 #' @param cpp_options (list) Any makefile options to be used when compiling the
 #'   model (`stan_threads`, `stan_mpi`, `stan_opencl`, etc.). Anything you would
 #'   otherwise write in the `make/local` file. For an example of using threading
@@ -738,6 +741,14 @@ compile <- function(quiet = TRUE,
     }
   }
 
+  # Evaluated here rather than in the tail: cmdstan_version() is not infallible
+  # despite being an accessor, because set_cmdstan_path() can leave PATH set
+  # while VERSION stays NULL, and in that state stanc and make both run but this
+  # errors. Staging it removes a failure point after the executable is already
+  # installed. It cannot be hoisted above the no-op return, which today never
+  # reaches this call at all.
+  compiled_cmdstan_version <- cmdstan_version()
+
   if (os_is_wsl() && (compile_model_methods || compile_standalone)) {
     warning("Additional model methods and standalone functions are not ",
             "currently available with WSLv1 CmdStan and will not be compiled.",
@@ -862,6 +873,7 @@ compile <- function(quiet = TRUE,
     private$user_header_dirty_ <- FALSE
     private$hpp_file_ <- hpp_file
     private$model_methods_env_ <- model_methods_env
+    private$cpp_options_ <- cpp_options
     private$precompile_cpp_options_ <- NULL
     private$precompile_stanc_options_ <- NULL
     private$precompile_include_paths_ <- NULL
@@ -871,9 +883,12 @@ compile <- function(quiet = TRUE,
     }
   } # End - if(!dry_run)
 
-  private$cmdstan_version_ <- cmdstan_version()
+  # Both are exceptions to the rule that state describing the compiled artifact
+  # is committed only above: during a dry run they are also the configured
+  # destination and the toolchain version, so they are assigned on a dry run and
+  # on success -- and, for exe_file_, on a no-op -- but never on a failure.
+  private$cmdstan_version_ <- compiled_cmdstan_version
   private$exe_file_ <- exe
-  private$cpp_options_ <- cpp_options
 
   if (!dry_run) {
     if (compile_model_methods) {

R/utils.R

@@ -303,6 +303,17 @@ copy_temp_files <-
 #'   describing it.
 install_executable <- function(from, to) {
   candidate <- tempfile(pattern = "exe-new-", tmpdir = dirname(to))
+  # Discarding the staged copy can fail too, so the diagnostics say where it was
+  # left rather than implying it is gone and sending the user looking for a file
+  # that is still there.
+  discard_candidate <- function() {
+    if (unlink(candidate) == 0L) {
+      ""
+    } else {
+      paste0(" The staged copy has been left at '", candidate, "'.")
+    }
+  }
+
   if (!isTRUE(suppressWarnings(file.copy(from, candidate)))) {
     stop(
       "Could not stage the compiled executable at '", candidate, "'. ",
@@ -317,10 +328,10 @@ install_executable <- function(from, to) {
       error_on_status = FALSE
     )
     if (is.na(chmod$status) || chmod$status != 0) {
-      unlink(candidate)
       stop(
         "Could not make the compiled executable executable. ",
         "The model executable at '", to, "' was not modified.",
+        discard_candidate(),
         call. = FALSE
       )
     }
@@ -330,20 +341,21 @@ install_executable <- function(from, to) {
   if (file.exists(to)) {
     backup <- tempfile(pattern = "exe-old-", tmpdir = dirname(to))
     if (!isTRUE(suppressWarnings(file.rename(to, backup)))) {
-      unlink(candidate)
       stop(
         "Could not move the existing executable '", to, "' aside. ",
         "It was not modified.",
+        discard_candidate(),
         call. = FALSE
       )
     }
   }
 
   if (!isTRUE(suppressWarnings(file.rename(candidate, to)))) {
-    unlink(candidate)
+    leftover_candidate <- discard_candidate()
     if (is.null(backup)) {
       stop(
         "Could not install the compiled executable at '", to, "'.",
+        leftover_candidate,
         call. = FALSE
       )
     }
@@ -352,12 +364,14 @@ install_executable <- function(from, to) {
         "Could not install the compiled executable at '", to, "' and the ",
         "previously compiled executable could not be restored. It has been ",
         "kept at '", backup, "'.",
+        leftover_candidate,
         call. = FALSE
       )
     }
     stop(
       "Could not install the compiled executable at '", to, "'. ",
       "The previously compiled executable has been restored.",
+      leftover_candidate,
       call. = FALSE
     )
   }

man/model-method-compile.Rd

@@ -142,7 +142,6 @@ test_that("a no-op compile preserves a header supplied via cpp_options", {
 })
 
 test_that("compile() uses a user header supplied to cmdstan_model()", {
-  stan_file <- testing_stan_file("bernoulli_external")
   user_header <- withr::local_tempfile(lines = "", fileext = ".hpp")
   received_stancflags <- list()
   local_mocked_bindings(
@@ -153,8 +152,18 @@ test_that("compile() uses a user header supplied to cmdstan_model()", {
     }
   )
 
-  model <- cmdstan_model(stan_file, user_header = user_header, compile = FALSE)
-  model$compile(force_recompile = TRUE, dry_run = TRUE)
+  model <- cmdstan_model(
+    local_external_model(),
+    user_header = user_header,
+    compile = FALSE
+  )
+  # A mocked compile rather than a dry run: a dry run builds nothing, so it
+  # records nothing about a compiled artifact.
+  with_mocked_cli(
+    compile_ret = list(status = 0),
+    info_ret = list(status = 1),
+    code = model$compile(force_recompile = TRUE)
+  )
 
   expect_equal(
     model$cpp_options()[["USER_HEADER"]],
@@ -482,15 +491,18 @@ test_that("cmdstan_model works with user_header with mock", {
 
 test_that("wsl path conversion is done as expected", {
   tmp_file <- withr::local_tempfile(lines = hpp, fileext = ".hpp")
+  local_mocked_stanc()
+  # Mocked successful compiles rather than dry runs: only a compilation that
+  # produced an executable records the options describing it.
+
  # Case 1: arg
   with_mocked_cli(
-    compile_ret = list(status = 1),
-    info_ret = list(),
+    compile_ret = list(status = 0),
+    info_ret = list(status = 1),
     code = {
       mod <- cmdstan_model(
-        stan_file = testing_stan_file("bernoulli_external"),
-        user_header = tmp_file,
-        dry_run = TRUE
+        stan_file = local_external_model(),
+        user_header = tmp_file
       )
     }
   )
@@ -502,15 +514,14 @@ test_that("wsl path conversion is done as expected", {
 
   # Case 2: cpp opt USER_HEADER
   with_mocked_cli(
-    compile_ret = list(status = 1),
-    info_ret = list(),
+    compile_ret = list(status = 0),
+    info_ret = list(status = 1),
     code = {
       mod <- cmdstan_model(
-        stan_file = testing_stan_file("bernoulli_external"),
+        stan_file = local_external_model(),
         cpp_options = list(
           USER_HEADER = tmp_file
-        ),
-        dry_run = TRUE
+        )
       )
     }
   )
@@ -522,15 +533,14 @@ test_that("wsl path conversion is done as expected", {
 
   # Case # 3: only user_header opt
   with_mocked_cli(
-    compile_ret = list(status = 1),
-    info_ret = list(),
+    compile_ret = list(status = 0),
+    info_ret = list(status = 1),
     code = {
       mod <- cmdstan_model(
-        stan_file = testing_stan_file("bernoulli_external"),
+        stan_file = local_external_model(),
         cpp_options = list(
           user_header = tmp_file
-        ),
-        dry_run = TRUE
+        )
       )
     }
   )

tests/testthat/test-model-compile-user_header.R

@@ -692,9 +692,15 @@ test_that("*hpp_file() functions work", {
   expect_equal(mod$hpp_file(), file.path(dirname(mod$stan_file()), "bernoulli.hpp"))
   mod$save_hpp_file(tmp_dir)
   expect_equal(mod$hpp_file(), file.path(tmp_dir, "bernoulli.hpp"))
+  # A dry run generates no C++, so it leaves the saved location alone rather
+  # than pointing $hpp_file() at a temporary file that was never written.
   mod$compile(force_recompile = TRUE, dry_run = TRUE)
+  expect_equal(mod$hpp_file(), file.path(tmp_dir, "bernoulli.hpp"))
+  # A real recompilation does write it, to a fresh temporary location.
+  expect_call_compilation(mod$compile(force_recompile = TRUE))
   expect_false(isTRUE(all.equal(mod$hpp_file(), file.path(tmp_dir, "bernoulli.hpp"))))
   expect_false(isTRUE(all.equal(mod$hpp_file(), file.path(dirname(mod$stan_file()), "bernoulli.hpp"))))
+  checkmate::expect_file_exists(mod$hpp_file())
 })
 
 test_that("check_syntax() works", {