Launch the model executable with the TBB it was built against - #1281
Merged
Merged
Conversation
On Windows the executable finds tbb.dll through PATH, and every launch put the selected installation's lib/tbb there, which is the wrong TBB once a different installation is selected. The four sites that launch the model executable now use the directory the build record holds. If that directory is gone nothing is added, so the executable is never pointed at another installation's TBB. Without a usable record the selected installation's is used as before. When a launch fails and the recorded directory is gone, the error says so and adds reinstalling it as a remedy. The Windows skip added to the adoption test in #1280 is removed, since that is the case the record now covers. Closes #1261. Part of #1258.
The TBB launch helper's Windows branch and the WSLENV assembly were tested everywhere by mocking os_is_windows() and os_is_wsl(). The Windows and WSL CI jobs run those branches for real, so the tests now check whichever branch the platform takes.
What it runs is the executable's own info command, and the new name reads naturally beside reported_features_from_exe().
4 tasks
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v1.0 #1281 +/- ##
=======================================
Coverage ? 93.42%
=======================================
Files ? 19
Lines ? 7312
Branches ? 0
=======================================
Hits ? 6831
Misses ? 481
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
A TBB_LIB or TBB_BIN value like "$(MATH)lib/tbb" was recorded as written, so the record named a directory that never existed and, on Windows, the launch put nothing on PATH. Rebuilding recorded the same thing again, so the launch error's remedy didn't help. cmdstan_model() now rejects such a value before the build and asks for a literal directory. Also softens the stop_cannot_run() roxygen, since a gone TBB is one likely cause of a failed launch rather than the likely one. Part of #1258.
The check for a make expression looked only at character values, so a factor or a list carrying one still reached make as the expression. Checking the assignment strings the options turn into covers every type the same way and drops the type guard.
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 made with the assistance of AI. I have reviewed all code and doc.
Closes #1261. Part of #1258.
On Windows the executable finds tbb.dll through PATH, and every launch put
the selected installation's
lib/tbbthere. That is the wrong TBB as soonas the selected installation is not the one that built the model: build
with 2.39,
set_cmdstan_path()to 2.40, sample, and the 2.39 binary runsagainst 2.40's TBB. The build record already says which directory the
build linked against (
tbb_dir), and nothing read it.Now the four sites that launch the model executable (the chains, diagnose,
$cmdstan_defaults()and the post-buildinfocall) put the recordeddirectory on PATH. If that directory is gone nothing is added, so the
executable runs with whatever TBB is already on PATH or fails to load, but
it is never pointed at a different installation's TBB. Without a usable
record the selected installation's directory is used as before. The
installation programs (make, stanc, stansummary) are untouched.
When a launch fails and the recorded directory is gone, the error from
#1280 says so and adds reinstalling it as a remedy:
This also removes the Windows skip added to the adoption test in #1280:
that test deselects the installation, which is exactly the case the record
now covers.
The second commit replaces two tests that mocked
os_is_windows()andos_is_wsl()with tests that check whichever branch the platform takes,since the Windows and WSL jobs run the other branches for real. The third
renames
run_info_cli()torun_exe_info(), since what it runs is theexecutable's own
infocommand.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: