Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Windows: add TBB (and MinGW DLLs) to load path #118

Merged
merged 3 commits into from
May 8, 2023

Conversation

WardBrian
Copy link
Collaborator

This is in response to #117. It isn't a full "fix" (arbitrary user configs could still run into similar problems and need to set their own %PATH%s correctly), but it attempts to be a bit friendlier.

This is similar to code in cmdstanpy. The main difference is (in Python) it also uses os.add_dll_directory. This appears to have fixed the issue which was preventing us from running our Python tests on Windows in Github Actions, so I've re-enabled those.

I'll do a follow on PR which tries to split the Windows CI in to separate jobs per-interface.

Note: This PR does not change the R interface, since R doesn't (yet) have any handling of where BridgeStan is installed (see #100)

@WardBrian
Copy link
Collaborator Author

It turns out that whether the os.add_dll_directory call is necessary is OS (windows only), version (behaves differently in 3.8 and 3.10), AND distribution (anaconda python doesn’t need it, python from python.org does) specific.

I have confirmed that it never hurts in situations where it isn’t required, but that explains why I could never get the tests to run in CI but could locally.

@WardBrian WardBrian requested a review from roualdes May 6, 2023 14:22
Copy link
Owner

@roualdes roualdes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just my one question, which you likely already thought through. I just want to make sure I'm on the same page. Thanks.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic here seems slightly different, as compared to the logic in Python. So this comment is just meant to double check on that difference.

Under Julia, if where.exe can find tbb.dll, then the Julia ENV["PATH"] will be able to successfully find tbb.dll, is this correct? And it's only if where.exe can't find tbb.dll that we need to add tbb.dll to the path?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, Python specifically does not look in PATH for dependent DLLs, which as far as I can tell all other languages do.

@WardBrian WardBrian merged commit e70d303 into roualdes:main May 8, 2023
@WardBrian WardBrian deleted the windows/add-tbb-on-load branch May 8, 2023 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants