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

Fix: Windows TBB path logic fails if BridgeStan is not yet downloaded #149

Merged
merged 10 commits into from
Jun 30, 2023

Conversation

WardBrian
Copy link
Collaborator

Closes #148

This also adds a very trivial test: Can the package be imported without BRIDGESTAN being set ahead of time.

@WardBrian WardBrian added bug Something isn't working python labels Jun 30, 2023
@WardBrian WardBrian requested a review from roualdes June 30, 2023 14:06
@WardBrian
Copy link
Collaborator Author

I'd like to publish a 2.1.1 shortly after this, assuming nothing new comes in

@aseyboldt
Copy link
Contributor

Seems I can't easily test this PR directly in the nutpie CI, because checking out stan fails with a "path too long" error...
https://github.com/pymc-devs/nutpie/actions/runs/5424318833/jobs/9863510219?pr=42#step:8:32

But this looks good to me, assuming tests pass.

@WardBrian
Copy link
Collaborator Author

fails with a "path too long" error...

Oh man, I recently dealt with this for Facebook's prophet package. Pip seems to like really long base path names, and a lot of the bash-on-Windows tools (including git, but also things like mingw32-make), do not respect the LONG_PATHS setting and fail if the path is over ~280 characters. Super annoying

You may be able to get things going by setting %TEMPDIR% to somewhere in the top-level of C:\, but that might still be too long. Git cloning it somewhere and then pip install -e . in that location would probably work.

@WardBrian
Copy link
Collaborator Author

@roualdes thoughts on this fix and a .1 release?

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.

Ya, this all seems reasonable to me. Is it possible for @aseyboldt to test this once it is accepted, but before we do a .1 release?

@aseyboldt
Copy link
Contributor

This does indeed fix the issue.
I had to call the setup function manually though, loading the library failed without it.

@WardBrian
Copy link
Collaborator Author

Calling the setup function manually seems like a reasonable requirement for users who intend to load the dll not using the Python class we provide

@WardBrian WardBrian merged commit 41ffee8 into main Jun 30, 2023
19 checks passed
@WardBrian WardBrian deleted the fix/148-tbb-path branch June 30, 2023 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TBB path issue on Windows
3 participants