v1.0 stage3: transactional record writing - #1265
Open
jgabry wants to merge 14 commits into
Open
Conversation
The four places compile() adds a stanc option (warn-pedantic, use-opencl, allow-undefined and the model name) wrote into the same list as the options the caller supplied, so by the time anything could record the build the two were indistinguishable. They now accumulate in their own list and the two are merged only where they become arguments. No behaviour changes: stanc receives the same arguments in the same order. Part of #1258.
After a successful build, compile() assembles a format 1 build record from what it already holds and writes it beside the executable: the options the caller supplied and the ones cmdstanr added, kept apart; the name stanc was passed; the effective include paths; the hashes and paths of the Stan file, the files stanc reports it included, the user header and make/local; the features the executable reports, present only when known; the CmdStan installation; the TBB directory this build resolved, asked of make with the build's own variables; and whether make/local or the user header includes files nothing here can track. Nothing reads the record yet. Three helpers do the parts compile() cannot do in a line: reading the features the executable reports, asking make for the TBB directory, and running the two untracked-dependency detectors. stanc --info now runs through one function that both the variables method and the record use. An empty cpp_options list is a named list so it writes as a JSON object. Part of #1258.
install_executable() now takes the record and installs both files together: the old executable and the old record are moved aside, the new executable is renamed in, the record is written beside it, and the pair is verified by reading the record back against the installed executable's hash. Any failure after the backups are made puts both previous files back, so a destination never holds a new executable with old provenance or a record describing a binary that is not there. compile() wrote the record after install in the previous commit; it now hands the record to install_executable() instead. verify_build_record() is the check the transaction ends with, on its own so the interleaved-writes case can be tested by writing the four files by hand. Two builds racing to one destination can leave executable B beside record A with every individual write atomic; the hash is what catches it. Part of #1258.
A build whose make/local includes another makefile, or whose user header includes other headers, now prints one note after the record is written, naming force_recompile = TRUE as the way to pick up changes to those files. Nothing is said on a no-op, a dry run or a failed build. NEWS gains entries for the record file and the note, and the internals vignette a subsection on the record's lifecycle: ignore it wherever the executable is ignored, in .gitignore and .Rbuildignore, and move or copy it with the executable. Part of #1258.
install_executable() staged the executable, moved the old pair aside, renamed the new executable in and only then serialised the record, and each of its five failure points had its own recovery branch. Some of those branches checked whether the recovery worked and some did not, so a second filesystem failure during rollback could leave the old executable stranded at its backup path while the error said it had not been modified. A serialisation failure could also leave a partial record in the directory with nothing naming it. The function now stages both files beside the destination first, so nothing there changes until both exist. The moves that follow are a list, run in order and undone in reverse if a later one fails or the installed pair does not read back, with every result checked in one place. The error either says the executable and record are as they were or names the files it could not put back. write_build_record() removes its staging file on exit whether or not it reached the rename. The transaction tests are renumbered for the record's own staging rename, four snapshots carry the new messages, and a test pins that a failed serialisation leaves nothing behind. Part of #1258.
Under WSL stanc is handed /mnt/<drive> paths and reports the files it included in that spelling, which Windows R cannot open, so hashing them failed every multi-file build after make had succeeded. The reported paths now go back through wsl_safe_path(revert = TRUE) before they are hashed. The stanc --info call also reads the temporary copy make compiled rather than the model's own source, so the source hash and the include list describe the same file. reported_features_from_exe() promised that nothing <exe> info printed could fail the build, but a line with nothing before the equals sign passed its filter as a nameless flag and the schema then rejected the record outside the helper. Missing version lines were written as "..". The filter now keeps entries with a name and a stan_version of three dotted integers only. Part of #1258.
test-build-record-compile.R built its mocked models on the shared resources/stan/bernoulli fixture, and the mock writes a text file where the executable goes. Every test file that runs after it alphabetically then ran that text file as the model, which is why CI failed in the sampling tests on three platforms with "./bernoulli: 1: mock: not found" while single-file local runs passed. Each test now copies the program into its own directory first. Part of #1258.
The included-files test now includes two files in non-sorted order with one of them twice and asserts the whole sequence, and pins the include_paths default in the same build. A build with stan_opencl and a user header pins the use-opencl and allow-undefined injection sites. The note test checks a dry run and a failed build say nothing. Part of #1258.
The roxygen and the design's tbb_dir row said the helper exists because get_cmdstan_flags() runs flag-free, which has not been true since Stage 1 gave it make_args. It exists to read both TBB variables in one make call and resolve a relative TBB_LIB. Both now also say the value is recorded as the shell prints it, so repeated spaces and glob characters in a TBB_LIB are not preserved, the same as every make query cmdstanr runs. Contract regenerated. Part of #1258.
When the rollback in install_executable() could not move a newly installed file out of the way but the old file's return then renamed over it, the error still named the restored file as left behind. A successful undo now drops its destination from the stuck list, so the message says the pair is as it was. A fresh install whose new record will not move still names it. Part of #1258.
…e share wsl_safe_path(revert = TRUE) only knew /mnt/<drive>, so an include reached through //wsl$/<distro>/... came back as a bare Linux path that Windows R could not hash, and a successful build failed while assembling its record. A path starting with a single slash and not under /mnt/ now gets the share prefix; host paths already carry a drive or a share and pass through. The three revert calls in csv.R had the same gap for paths CmdStan prints. The vector test gains a native path and a share path, and a new test compiles a model with its include written under the share, run only under WSL. Part of #1258.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v1.0 #1265 +/- ##
=======================================
Coverage ? 92.87%
=======================================
Files ? 16
Lines ? 6916
Branches ? 0
=======================================
Hits ? 6423
Misses ? 493
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Every Windows CI job failed the tbb_dir test at 1718854. CmdStan's print-% rule echoes a variable through RTools sh, which spells the installation's TBB directory as /c/... and drops the backslashes of a supplied Windows path. Printing the value with $(info) from a fragment makefile avoids the shell, but that is a second make-query mechanism plus per-platform path conversion for a corner the Windows launch has never handled. The record now names the directory the call's TBB_LIB gives, resolved against the installation when relative, and the installation's own lib/tbb when the call names none. A TBB_LIB or TBB_BIN set in make/local, ~/.config/stan/make.local or the environment still moves the TBB the binary links against and no longer reaches the record. The design's field row, the environment bullet in the untracked list and the rejected alternatives say so, and the contract is regenerated. Part of #1258.
jgabry
marked this pull request as ready for review
September 13, 2026 00:20
The helper read the raw cpp_options list, so a repeated TBB_LIB recorded the first value where make acts on the last, a vector value errored after a successful make, and FALSE recorded a directory called FALSE where make sees an empty assignment. It now goes through parsed_cpp_options(), the same conversion the record's cpp_options_supplied field uses. A TBB_BIN named on the call is honoured too, in the order the makefile uses: TBB_LIB, or TBB_BIN when TBB_LIB is empty, both resolved against the installation when relative. The design row, the roxygen and the tests say so. Part of #1258.
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 were written with the assistance of AI coding tools. I have reviewed all of the code.
Stage 3 of the v1.0 compilation-state work (#1258): every executable
compile()installs now has a build record beside it, and the two are installed as one transaction. Nothing reads the record yet; the rebuild decision that consumes it is Stage 4. No dev-version bump, since no public contract changes.What lands:
compile()assembles a format 1 record from what it already holds and hands it toinstall_executable(), which installs the executable and the record together: both previous files are moved aside, the new executable is renamed in, the record is written beside it, and the pair is verified by reading the record back against the installed executable's hash. Any failure after the backups are made restores both.stanc_options(warn-pedantic,use-opencl,allow-undefined, the model name) accumulate in their own list and are merged with the caller's only where the arguments are built, sostanc_options_suppliedandstanc_options_injectedare both values the code holds and neither is reconstructed.stanc_nameis recorded raw,my-model_modelformy-model.stan.stanc --inforeports it included, in that order, the user header when there is one andmake/localwhen it exists, each by content hash with the path it was built from.stanc --infonow runs through one function that$variables()and the record share.reported_featurescomes from<exe> infoon the built binary, present only when known;tbb_dirfrom onemakequery run with the build's own variables, so aTBB_LIBsupplied on the call is seen and a relative one is recorded absolute.make/localthat includes another makefile and a user header that includes other headers, fillknown_untracked_dependencies, and a build that has any prints one note namingforce_recompile = TRUE. Nothing on a no-op..gitignore,.Rbuildignoreand copied-executable cases.Every rule has a sentence in
dev-notes/compilation-state-contract.md(§1, §4, §6) and a test that fails if the rule is removed:tests/testthat/test-build-record-compile.Rfor what a build records,test-utils.Rfor the transaction and its rollback,test-build-record.Rfor the interleaved-writes case the hash catches.Part of #1238, #1257 and #1258. Closes nothing on its own.
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: