-
-
Notifications
You must be signed in to change notification settings - Fork 189
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
Minor patches to TBB Windows build for compatibility with RTools make #2999
Conversation
I've also left the existing usage of |
@WardBrian Any chance we could sneak this through in the 2.34 release? It would help simplify the windows setup steps (only need to set PATH now) @wds15 would you happen to remember the POSIX issues that led to the |
@WardBrian good call! I've updated the makefiles to first do a check for the presence of |
Changes look good now, thanks! As for this in the 2.34 release, I'm torn. Calling the fact that @rok-cesnovar thoughts? |
First of all, this is awesome. Not requiring mingw32-make is a huge huge win! But cmdstan scripts still require it right now and I think we would need to clean up the Cmdstan makefiles, cleanup docs and installation instructions before the release. We could do that, but it feels like it would be a bit rushed to me. And yeah, more testing time would be helpful as well. Asking Windows users to test the RC because of a somewhat big change would be helpful as well. The only other option is making an RC2 and prolonging the freeze period. |
Ah yeah good point. This change isn't particularly urgent, better to play it safe |
Agreed @rok-cesnovar, this is a great change! I had always suspected that the issues with Getting the build system out of our way is also the current barrier to using the Microsoft compilers (or at least Clang-for-MSVC) for Stan, which is a goal of mine. One day |
@andrjohns I only recall that |
@andrjohns would you mind looking into what needs to be changed in the stan and cmdstan repos? |
Summary
This PR applies some minor changes to the TBB's build rules on Windows so that it can be compiled with the version of
make
bundled with RTools - users will no longer need to additionally installmingw32-make
on windows.These changes are based on
RcppParallel
's changes.I've run a GHA workflow which shows that the Math library & TBB successfully build with these changes under RTools40, RTools42, and RTools43 - without needing to install any additional packages
Tests
N/A - existing tests should still pass, testing workflow already run to confirm successful build
Side Effects
N/A
Release notes
Updated TBB Windows build rules for compatibility with RTools make
Checklist
Copyright holder: Andrew Johnson
The copyright holder is typically you or your assignee, such as a university or company. By submitting this pull request, the copyright holder is agreeing to the license the submitted work under the following licenses:
- Code: BSD 3-clause (https://opensource.org/licenses/BSD-3-Clause)
- Documentation: CC-BY 4.0 (https://creativecommons.org/licenses/by/4.0/)
the basic tests are passing
./runTests.py test/unit
)make test-headers
)make test-math-dependencies
)make doxygen
)make cpplint
)the code is written in idiomatic C++ and changes are documented in the doxygen
the new changes are tested