v1.0 stage1: make-option correctness - #1262
Merged
Merged
Conversation
…paths for Make
get_cmdstan_flags("STANCFLAGS") read the variable with `make print-STANCFLAGS`,
whose recipe echoes the value through the shell. The shell strips the quotes, so
a value like --filename-in-msg='/my dir/model.stan' came back as two words and
the direct stanc calls in compile() passed them as two arguments; stanc then
refused the second one as an extra positional argument. The value is now read
with a rule of our own that prints $(STANCFLAGS) one argument per line after
the shell has split it, so what comes back is exactly what stanc's recipe gets.
The rule goes in a temporary makefile passed with a second -f rather than in an
--eval argument, since users may have a make too old for --eval (the one Apple
ships with macOS is). Every line carries a prefix so directory-change messages
from a recursive make are ignored. The recipe needs sh, so the call puts the
toolchain on PATH the way compile() does, and the makefile is written with LF
endings so a Linux make under WSL can read it.
Since the local flags now arrive as words, the make path requotes them before
appending them to the STANCFLAGS value handed back to make.
include_paths_stanc3_args() quoted a path for Make only when it contained a
space, so a quote in the path broke the shell syntax and a dollar sign was
expanded by Make. make_shell_quote() single-quotes each path that holds a
character the shell could interpret and doubles `$` for Make. It quotes one
element at a time because shQuote() switches a whole vector to double quotes
when any element holds a single quote. Paths without such characters are
unchanged.
Tests run real make against a minimal makefile that includes a local file with
the fixtures, hand quoted words to make and check the shell delivers them back
unchanged, and compile a model with the quoted fixture in make/local without
mocking either stanc call.
closes #1230
closes #1232
Both unit test workflows only ran on pull requests against master, so a pull request against the v1.0 branch got no Windows or WSL coverage. Add v1.0 to the branch filter of each.
A header passed through cpp_options as USER_HEADER or user_header is now an error that points at the user_header argument. The header still reaches make as USER_HEADER=, but as a flag built beside the cpp_options flags, so it no longer appears in $cpp_options(). A new $user_header() accessor returns the recorded path. resolve_user_header() and its conflict warnings are gone; what is left is inlined at the two call sites. assert_valid_cpp_options() holds the rejection and will take the rest of the cpp_options checks in the next commit. The design note's two references to the deleted function now read in the past tense. Part of #1258.
`assert_valid_cpp_options()` is now the whole check for `cpp_options`. Every entry must be named, every name must be a Make variable name (`^[A-Za-z_][A-Za-z0-9_]*$`), and names are uppercased there so a single spelling reaches everything downstream. `$cpp_options()` reports names in their make spelling, so `list(stan_threads = TRUE)` comes back as `STAN_THREADS`. `USER_HEADER` and `STANCFLAGS` are rejected as literals with errors naming the `user_header` and `stanc_options` arguments. Unnamed entries get an error that names the route for what the caller wrote: `list(NAME = "value")` for a plain assignment, `cmdstan_make_local()` for `+=` and the other makefile operators and for make's own flags, and the owning argument when the entry assigns `USER_HEADER` or `STANCFLAGS`. A name like `CXXFLAGS+` is rejected by the name grammar, since it would otherwise reach make as a live `+=`. A logical `FALSE` now reaches make as the empty assignment `NAME=`, which disables the option and overrides `make/local`, the same as `NULL`. The string `"FALSE"` is still passed as a value. `validate_cpp_options()`, whose one remaining job was to warn that `FALSE` would enable an option, is deleted with its test. Since names are canonical on entry, `parsed_cpp_options()`, `exe_info_reflects_cpp_options()`, `exe_info_style_cpp_options()` and `merge_exe_info_cpp_options()` stop folding case, and the parser no longer keeps an exclusion list or an opaque class for unnamed entries. `STAN_VERSION` is compared like any other option. `cpp_option_value()` is unchanged because it also reads lowercase executable metadata. The design note passages that described the deleted code as current are rewritten in the past tense. The generated contract is unchanged. Part of #1258. closes #1250 closes #1251
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v1.0 #1262 +/- ##
==========================================
+ Coverage 92.27% 92.53% +0.25%
==========================================
Files 15 15
Lines 6489 6633 +144
==========================================
+ Hits 5988 6138 +150
+ Misses 501 495 -6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
`assert_valid_stanc_options()` now rejects `include-paths`, `warn-pedantic`,
`allow-undefined`, `use-opencl` and `name`, each with an error naming what
owns the setting: the `include_paths` argument, `pedantic = TRUE`, the
`user_header` argument, `cpp_options = list(stan_opencl = TRUE)`, and the
name of the Stan file. The match is on where the flag name occurs, the
entry's name when it is named and its value otherwise, taking the text
before the first `=`, so every spelling of a flag is caught including the
named `FALSE` and `NA` that emit nothing. A named entry whose name contains
`=` is a shape error that shows the `list("flag" = "value")` spelling.
`$check_syntax()` validates its own `stanc_options` list the same way. It
never had, not even for a leading hyphen, so a flag with a dedicated
argument had two channels there.
With `name` rejected the `--name` injections in `$compile()` and
`$check_syntax()` run unconditionally.
Source-only operations always pass `--allow-undefined`: `$check_syntax()`,
`$format()` and `$variables()` run stanc against a program and never link
anything, so whether a function has a definition is a build's concern.
Before this a model with an external function and no header could not be
checked or formatted without passing the flag by hand. Only `$compile()`
derives the flag from `user_header` now, and with its readers gone the
private `using_user_header_` field is deleted.
The design note's §3 rejection rule now says it covers every
`stanc_options` list a method accepts, `$check_syntax()`'s included, and
the generated contract is regenerated.
Part of #1258.
The first CI run of this branch found no defect in the code and three in the tests added with the make/local tokenizer. The include path test predicted quotes only for a path with a space. On the Windows runners tempdir() is a short path, `C:/Users/RUNNER~1/...`, and the `~` is outside the safe set `make_shell_quote()` accepts, so the path is quoted and the test disagreed. The expectation for that path now comes from `make_shell_quote()` itself; the two hand-written paths below it still pin the quoting rule. The `costs $5` fixture is skipped under WSL. The wsl launcher passes arguments through a shell, so `.wsl_check_exists()` looks for `costs ` and reports the directory missing. That is a limit of the WSL path handling for a `$` in a path's last component, older than this branch, and not what the fixture tests. The mini make/local helper in test-utils.R mocked `wsl_compatible_run()` with a native make call, while `stancflags_from_make()` had already converted its rule file path to `/mnt/...` for WSL, so make could not open it. The helper now calls the real runner with the temporary directory as its working directory, and writes its two files in binary mode so the Linux make reads LF endings. Part of #1258.
…s win An include path in make/local's STANCFLAGS now stops the build with an error that names the include_paths argument. Any element that contains --include-paths or begins with -I counts, a substring test on what make resolved rather than a parse of the file, so a flag arriving through a makefile that make/local includes is caught too. Only include_paths can set the search path now, which keeps the build and stanc --info resolving the same files. A flag the call emits, supplied in stanc_options or injected from pedantic, user_header, stan_opencl or the file name, now wins over the same flag in make/local. drop_overridden_stancflags() removes the make/local element whose flag matches one of the call's, and when that element is the bare flag it also takes the next element if it does not begin with a hyphen, since that is the flag's value given as a separate word. -fno-soa after a make/local --warn-pedantic is the next flag and stays. cmdstanr passes STANCFLAGS on the make command line, which replaces the makefile's own value, so dropping from the vector it appends removes the flag from make's stanc call and the two direct calls alike. Before this a make/local --warn-pedantic beside pedantic = TRUE reached stanc twice, which 2.37 refuses. The include-path rejection runs first and is not subject to the drop. Part of #1258.
cmdstan_model(exe_file = path) with no stan_file adopts an executable as it is. Supplying cpp_options, stanc_options, include_paths, user_header, force_recompile or pedantic to such a call is now an error, since each can only be honoured by building or by reading a Stan program and there is neither. The check is on whether the argument was supplied, so an explicit NULL counts as omission and force_recompile = FALSE is refused like TRUE. It runs before the executable is checked for existence, so a caller learns about the bad argument even when the path is wrong too. Before this the arguments were accepted and ignored, and include_paths was resolved and recorded on a model that had no program to resolve includes for. With include_paths rejected on this path, the executable branch of initialize() takes its include paths from the precompile field alone; the fallback to the raw argument could no longer be reached. The cmdstanr_force_recompile option keeps its place in the $compile() signature. The check reads the constructor's own argument list, which holds only what the caller passed, so the option default never reaches it. The option's help page says it has no effect on an executable-only model. Part of #1258.
"executable metadata takes precedence over compile options" compiled with cpp_options = list(stan_threads = FALSE) and expected the executable to have threading anyway, so that sampling without threads_per_chain errored. It passed only because FALSE reached make as STAN_THREADS=FALSE, a non-empty value that ifdef treats as set. That is the #1251 bug, fixed earlier on this branch, and CI found the test on the first run that included the fix. The test now pins the corrected behaviour end to end: the executable built with stan_threads = FALSE reports threading off, samples without threads_per_chain, and warns that threads_per_chain has no effect. Its snapshot of the old error goes with it. Part of #1258.
Version 0.9.0.9002 becomes 0.9.0.9003. The NEWS entries cover what this branch changed: include_paths and user_header as the only channels for their settings, the stanc flags rejected from stanc_options, source-only operations always allowing undefined functions, named cpp_options with Make spellings and FALSE disabling an option, the call's stanc flags winning over make/local, the make/local STANCFLAGS tokenizer and include path quoting, and the arguments an executable-only model refuses. Four unreleased entries about a user header passed through cpp_options are removed, since that spelling is now an error. The man pages are regenerated from the roxygen edited on this branch. Part of #1258.
…ns messages The rule file stancflags_from_make() hands to make appended itself to MAKEFILE_LIST, so a make/local value that reads $(lastword $(MAKEFILE_LIST)) named the temporary file instead of local. The file's first line now removes it from the list. Four migration messages suggested replacements that were wrong or did not parse. Values are now written as R literals with encodeString(), an empty USER_HEADER points at user_header = NULL, -B points at force_recompile = TRUE and -f at an include line in make/local. The design note said the cpp_options name check runs after uppercasing. The code checks the name as written and uppercases what passes, and the contract now says so. The exe_file docs say NULL counts as omitting the build arguments, the cpp_options docs say an empty assignment empties the variable rather than disabling an option, and the #1230 NEWS entry names the WSL dollar-sign limit. Found by the external review of #1262. Part of #1258.
CmdStan's make/program adds --use-opencl to STANCFLAGS when STAN_OPENCL is set. The query that reads STANCFLAGS from make ran with make/local alone, so with STAN_OPENCL=true in make/local and cpp_options = list(stan_opencl = FALSE) both stanc calls got --use-opencl while the C++ build got STAN_OPENCL=, and the generated code did not compile. Pre-existing for NULL, exposed by FALSE now meaning off. stancflags_from_make() and get_cmdstan_flags() take the call's Make assignments and put them on make's command line, so make resolves STANCFLAGS for this build. compile() computes the assignments once and hands the same vector to the query and to the build. The contract sentence in §6 says so. The #1251 NEWS entry is shortened to the outcome: FALSE turns an option off, and wins over make/local. Two mini-makefile tests pin the helper, and a dry-run compile against a real make/local holding STAN_OPENCL=true asserts --use-opencl reaches both stanc calls zero times for FALSE and once for TRUE and for make/local alone. Test mocks of get_cmdstan_flags() accept the new argument. Found by the external review of #1262. Part of #1258.
The message that rejects a user header in cpp_options now writes the path with encodeString(), so on Windows the backslashes come out doubled. The test built its expected text from the raw path and failed on the windows-latest runner. It now encodes the path the same way. Part of #1258.
The query that reads make/local STANCFLAGS received the call's cpp_options but not the USER_HEADER assignment, which was built afterwards and reached only the build. A make/local rule conditioned on USER_HEADER resolved one way for the query and another for the build. The header assignment now joins the cpp_options in a single vector of make variables that both the query and the build call use, and the contract sentence in §6 names user_header alongside cpp_options. A dry-run test with a conditional make/local pins it. The migration message for a -f make flag wrote its make/local example without quoting, so a path containing a quote produced invalid R. It now encodes the R literal the way the operator message does. The §3 sentence saying entries are normalized ahead of validation contradicted the later sentence that checks the name shape as written and then uppercases. The §3 sentence no longer states an ordering; the sentence that owns it is the one in the operator section. Part of #1258.
jgabry
marked this pull request as ready for review
September 10, 2026 15:00
This was referenced Sep 10, 2026
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Submission Checklist
Summary
This PR and the summary below was written with the help of AI coding tools. I have reviewed the code myself.
Stage 1 of the v1.0 compilation-state work (#1258): every option that reaches
makeorstancnow arrives through one channel, is spelled the way the tool reads it, and is checked before anything runs. Targetsv1.0, notmaster.What changes for users (NEWS has the full list):
cpp_optionsentries must be named with a Make variable name. Names are uppercased on entry and$cpp_options()reports the Make spelling.FALSEorNULLpasses an empty assignment, which turns the option off and overridesmake/local. Previouslystan_threads = FALSEenabled threading (cpp_options = list(stan_threads = FALSE) enables threading instead of disabling it #1251), and unnamed entries such as"STAN_THREADS=TRUE"reachedmakebut were invisible to everything keyed on names (Unnamed raw cpp_options assignments reach make but are invisible to everything that keys on names #1250).user_headeris the only way to supply a user header;cpp_options = list(USER_HEADER = ...)is an error naming the argument. The new$user_header()method returns the path.include_pathsis the only way to givestancinclude paths.include-pathsinstanc_options,STANCFLAGSincpp_optionsand an include path inmake/local'sSTANCFLAGSare all errors.stanc_optionsrejects the flags cmdstanr sets from its own arguments (warn-pedantic,allow-undefined,use-opencl,include-paths,name) in every spelling.$check_syntax()checks its list the same way.$check_syntax(),$format()and$variables()always pass--allow-undefined; they read a program and link nothing.make/local'sSTANCFLAGSare read the way the shell splits them (Quoted values in make/local STANCFLAGS are split on whitespace and break direct stanc calls #1232), and are resolved with the call'scpp_optionsapplied, sostan_opencl = FALSEalso keeps the--use-openclthat CmdStan's makefiles add from reachingstanc. A flag the call sets wins over the same flag inmake/local.makeare quoted for Make and the shell (Include paths for Make are hand-quoted rather than properly escaped #1230).cmdstan_model(exe_file = )with nostan_filerejectscpp_options,stanc_options,include_paths,user_header,force_recompileandpedantic. There is nothing to build, so the executable is used as it is.Each rule has a sentence in
dev-notes/compilation-state-contract.md(§3, §6, §7) and a test that fails if the rule is removed.Closes #1230, #1232, #1250, #1251.
Reviewed externally before this was marked ready; the review found one real defect (the
STANCFLAGSquery ignored the call'scpp_options, fixed in the last commit) and a handful of message and doc corrections, all in the two final commits. One finding is deferred on purpose and recorded on #1258: the executable-only check reads the captured arguments by exact name, so an abbreviation such asforce = TRUEpasses it. Removing$compile()later in v1.0 makes those arguments the constructor's own formals and closes that without matching code.Copyright and Licensing
Please list the copyright holder for the work you are submitting
(this will be you or your assignee, such as a university or company):
Jonah Gabry
By submitting this pull request, the copyright holder is agreeing to
license the submitted work under the following licenses: