Make get_cmdstan_flags() robust to noisy make output

NEWS.md

@@ -4,6 +4,8 @@
 keywords (@VisruthSK, #1154)
 * `save_cmdstan_config` and `save_metric` default to `FALSE` but can be 
 set to `TRUE` for an entire R session via new global options. (#1159)
+* `cmdstan_model()` no longer fails when `MAKEFLAGS` enables directory-printing 
+output while reading `STANCFLAGS` from `make`. (#1163)
 
 * CmdStanModel objects created using `compile_model_methods = TRUE` that are
 then saved and reloaded no longer error in model fitting methods. Model methods

R/cpp_opts.R

@@ -174,7 +174,7 @@ assert_valid_threads <- function(threads, cpp_options, multiple_chains = FALSE)
 # exe_info style means off is FALSE
 
 exe_info_style_cpp_options <- function(cpp_options) {
-  if(is.null(cpp_options)) cpp_options <- list()
+  if (is.null(cpp_options)) cpp_options <- list()
   names(cpp_options) <- tolower(names(cpp_options))
   flags_reported_in_exe_info <- c(
     "stan_threads", "stan_mpi", "stan_opencl",

R/model.R

@@ -628,7 +628,7 @@ compile <- function(quiet = TRUE,
   }
   stancflags_combined <- stanc_built_options
   stancflags_local <- get_cmdstan_flags("STANCFLAGS")
-  if (stancflags_local != "") {
+  if (length(stancflags_local) > 0) {
     stancflags_combined <- c(stancflags_combined, stancflags_local)
   }
   stanc_inc_paths <- include_paths_stanc3_args(include_paths, standalone_call = TRUE)
@@ -730,7 +730,7 @@ compile <- function(quiet = TRUE,
   private$precompile_stanc_options_ <- NULL
   private$precompile_include_paths_ <- NULL
 
-  if(!dry_run) {
+  if (!dry_run) {
     if (compile_model_methods) {
       expose_model_methods(env = private$model_methods_env_, verbose = !quiet)
     }

R/utils.R

@@ -561,7 +561,7 @@ wsl_installed <- function() {
     p <- processx::process$new("wsl", "uname")
     for(i in 1:50) {
       Sys.sleep(0.1)
-      if(!p$is_alive()) {
+      if (!p$is_alive()) {
         break
       }
     }
@@ -686,32 +686,55 @@ assert_dir_exists <- checkmate::makeAssertionFunction(check_dir_exists)
 assert_file_exists <- checkmate::makeAssertionFunction(check_file_exists)
 
 # Model methods & expose_functions helpers ------------------------------------------------------
+
+# Extract the requested make variable from `make print-<FLAG>` output while
+# ignoring unrelated lines
+parse_make_print_flag <- function(flag_name, stdout) {
+  lines <- strsplit(stdout, "\r?\n", perl = TRUE)[[1]]
+  pattern <- paste0("^\\s*", flag_name, "\\s*(=|\\+=)\\s*")
+  matches <- grep(pattern, lines, perl = TRUE)
+
+  if (length(matches) == 0) {
+    stop(
+      "Failed to parse `", flag_name, "` from `make print-", flag_name, "` output.\n",
+      "Output was:\n", stdout,
+      call. = FALSE
+    )
+  }
+  if (length(matches) > 1) {
+    stop(
+      "Found multiple `", flag_name, "` lines in `make print-", flag_name, "` output.\n",
+      "Output was:\n", stdout,
+      call. = FALSE
+    )
+  }
+
+  sub(pattern, "", trimws(lines[matches]), perl = TRUE)
+}
+
 get_cmdstan_flags <- function(flag_name) {
   cmdstan_path <- cmdstanr::cmdstan_path()
   withr::with_envvar(
     c("HOME" = short_path(Sys.getenv("HOME"))),
-    flags <- wsl_compatible_run(
+    flags_stdout <- wsl_compatible_run(
       command = "make",
       args = c("-s", paste0("print-", flag_name)),
       wd = cmdstan_path
     )$stdout
   )
-
-  flags <- gsub("\n", "", flags, fixed = TRUE)
-
-  flags <- gsub(
-    pattern = paste0(flag_name, "\\s(=|\\+=)(\\s|$)"),
-    replacement = "", x = flags
-  )
-
-  if (flags == "") {
-    return(flags)
-  }
+  flags <- parse_make_print_flag(flag_name, flags_stdout)
 
   if (flag_name == "STANCFLAGS") {
     # StanC flags need to be returned as a character vector
-    flags_vec <- strsplit(x = flags, split = " ", fixed = TRUE)[[1]]
-    return(flags_vec)
+    if (!nzchar(flags)) {
+      return(character())
+    }
+    flags_vec <- strsplit(x = trimws(flags), split = "\\s+", perl = TRUE)[[1]]
+    return(flags_vec[nzchar(flags_vec)])
+  }
+
+  if (!nzchar(flags)) {
+    return(flags)
   }
 
   if (flag_name %in% c("LDLIBS", "LDFLAGS_TBB")) {
@@ -753,9 +776,11 @@ check_sundials_fpic <- function(verbose) {
     return(invisible(NULL))
   }
   if (interactive()) {
-    message("SUNDIALS needs to be compiled with -fPIC when exposing functions or ",
-            "model methods on Linux.\n",
-            "Updating your make/local file to include -fPIC and rebuilding CmdStan now...")
+    message(
+      "SUNDIALS needs to be compiled with -fPIC when exposing functions or ",
+      "model methods on Linux.\n",
+      "Updating your make/local file to include -fPIC and rebuilding CmdStan now..."
+    )
   }
   cmdstan_make_local(cpp_options = list("CPPFLAGS_SUNDIALS += -fPIC"), append = TRUE)
   rebuild_cmdstan(quiet = !verbose)

tests/testthat/test-model-compile.R

@@ -831,15 +831,27 @@ test_that("dirname of stan_file is used as include path if no other paths suppli
   expect_s3_class(mod_tmp$compile(), "CmdStanModel")
 })
 
-test_that("STANCFLAGS included from make/local", {
-  make_local_old <- cmdstan_make_local()
-  cmdstan_make_local(cpp_options = "STANCFLAGS += --O1", append = TRUE)
-  out <- utils::capture.output(mod$compile(quiet = FALSE, force_recompile = TRUE))
+test_that("STANCFLAGS from get_cmdstan_flags() are included in compile output", {
+  real_get_cmdstan_flags <- get_cmdstan_flags
+  out <- with_mocked_bindings(
+    utils::capture.output(mod$compile(quiet = FALSE, force_recompile = TRUE)),
+    get_cmdstan_flags = function(flag_name) {
+      if (identical(flag_name, "STANCFLAGS")) {
+        c("--O1", "--warn-pedantic")
+      } else {
+        real_get_cmdstan_flags(flag_name)
+      }
+    }
+  )
   if(os_is_windows() && !os_is_wsl()) {
-    out_w_flags <- "bin/stanc.exe --name='bernoulli_model'  --O1 --o"
+    out_w_flags <- "bin/stanc.exe --name='bernoulli_model'[[:space:]]+--O1[[:space:]]+--warn-pedantic[[:space:]]+--o"
   } else {
-    out_w_flags <- "bin/stanc --name='bernoulli_model'  --O1 --o"
+    out_w_flags <- "bin/stanc --name='bernoulli_model'[[:space:]]+--O1[[:space:]]+--warn-pedantic[[:space:]]+--o"
   }
   expect_output(print(out), out_w_flags)
-  cmdstan_make_local(cpp_options = make_local_old, append = FALSE)
+})
+
+test_that("compile() ignores directory chatter from MAKEFLAGS when reading STANCFLAGS", {
+  withr::local_envvar(MAKEFLAGS = "-w -j 4")
+  expect_compilation(mod, quiet = TRUE, force_recompile = TRUE)
 })

tests/testthat/test-utils.R

@@ -277,7 +277,115 @@ test_that("get_cmdstan_flags() can be used recursively in `make`", {
     )
     return(invisible())
   }
-  stdo <- recursive_run$stdout
-  recursive_flags <- readLines(textConnection(stdo))
-  expect_equal(nonrecursive_flags, recursive_flags)
+  expected_stdout <- paste(capture.output(cat(nonrecursive_flags)), collapse = "\n")
+  expect_equal(recursive_run$stdout, expected_stdout)
+})
+
+test_that("parse_make_print_flag() ignores unrelated make output", {
+  stdout <- paste(
+    "make: Entering directory '/tmp/cmdstan'",
+    "STANCFLAGS = --O1 --warn-pedantic --allow-undefined",
+    "make: Leaving directory '/tmp/cmdstan'",
+    sep = "\n"
+  )
+
+  expect_equal(
+    parse_make_print_flag("STANCFLAGS", stdout),
+    "--O1 --warn-pedantic --allow-undefined"
+  )
+})
+
+test_that("parse_make_print_flag() errors if no matching flag line is found", {
+  expect_error(
+    parse_make_print_flag("STANCFLAGS", "make: Entering directory '/tmp/cmdstan'"),
+    "Failed to parse `STANCFLAGS`",
+    fixed = TRUE
+  )
+})
+
+test_that("parse_make_print_flag() errors if multiple matching flag lines are found", {
+  stdout <- paste(
+    "STANCFLAGS = --O1",
+    "STANCFLAGS = --warn-pedantic",
+    sep = "\n"
+  )
+  expect_error(
+    parse_make_print_flag("STANCFLAGS", stdout),
+    "Found multiple `STANCFLAGS` lines",
+    fixed = TRUE
+  )
+})
+
+test_that("get_cmdstan_flags() returns empty STANCFLAGS as character(0)", {
+  with_mocked_bindings(
+    {
+      expect_equal(get_cmdstan_flags("STANCFLAGS"), character(0))
+    },
+    wsl_compatible_run = function(...) {
+      list(stdout = "STANCFLAGS =\n")
+    }
+  )
+})
+
+test_that("get_cmdstan_flags() preserves empty non-STANCFLAGS values", {
+  with_mocked_bindings(
+    {
+      expect_equal(get_cmdstan_flags("CPPFLAGS"), "")
+    },
+    wsl_compatible_run = function(...) {
+      list(stdout = "CPPFLAGS =\n")
+    }
+  )
+})
+
+test_that("get_cmdstan_flags() handles line-continuation STANCFLAGS in make/local", {
+  tmpdir <- withr::local_tempdir()
+  # Build a minimal make setup so we can exercise real make line continuations.
+  writeLines(
+    c(
+      "print-%: ; @echo $* = $($*)",
+      "-include local"
+    ),
+    file.path(tmpdir, "Makefile")
+  )
+  writeLines(
+    c(
+      "STANCFLAGS += --O1 \\",
+      "  --warn-pedantic \\",
+      "  --allow-undefined"
+    ),
+    file.path(tmpdir, "local")
+  )
+  make_run <- processx::run(
+    command = "make",
+    args = c("-s", "print-STANCFLAGS"),
+    wd = tmpdir,
+    error_on_status = FALSE
+  )
+  if (make_run$status != 0) {
+    fail(
+      paste(
+        "Mini make failed.",
+        paste0("status: ", make_run$status),
+        "stdout:",
+        make_run$stdout,
+        "stderr:",
+        make_run$stderr,
+        sep = "\n"
+      )
+    )
+    return(invisible())
+  }
+
+  with_mocked_bindings(
+    {
+      expect_equal(
+        get_cmdstan_flags("STANCFLAGS"),
+        c("--O1", "--warn-pedantic", "--allow-undefined")
+      )
+    },
+    wsl_compatible_run = function(...) {
+      list(stdout = make_run$stdout)
+    }
+  )
 })