-
-
Notifications
You must be signed in to change notification settings - Fork 187
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
Add makeflag for building TBB under UCRT automatically #2948
Conversation
Isn't windows.h famously huge? How long does that command take to run? Do we currently use |
Sigh, the "works on my machine" strikes again. Will debug and update |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good but imo it may be nice to only run that grep command if we are using windows. About how long does it take? I don't have a windows computer so I can't test it locally. Also can you use the -m 1
flag with grep so that it just stops on the first match?
I am opposed to calling grep on Windows at all - I'm not sure if it is bundled in rtools, but the standard mingw compiler distribution wouldn't have it by default AFAIK Compiling windows.h and grepping on every single invocation also seems like a bad idea, so if we could have something that persists this to disk somehow that would be great |
Is it
Let's wait to see how long it takes from @andrjohns. Finding the first match of |
This is grepping on the result of the compiler pre-processing windows.h, not just grepping the file itself |
Good call!
It takes about a second for me, and this is running on an Windows ARM VM on an M1 Mac, with
|
And it looks like the Powershell
|
I think sticking to cmd.exe programs is probably the best for compatibility. Findstr might also be faster if you use /l to disable regex searching |
Not much benefit unfortunately:
|
I just did a GHA actions run using the Result:
|
Oh wait hold on, that's 14 seconds and 750 ms. Sheesh never mind |
Back to the drawing board! |
I think it would be fine if this was saved to a file somewhere (like make/os?) as long as it was deleted by “make clean”. 14s isn’t a killer if it only happens during “make build” |
We might be able to skip that completely, I started chasing through the includes in
|
make/compiler_flags
Outdated
ifeq (,$(wildcard $(MATH)make/ucrt)) | ||
UCRT_STRING := $(shell echo "#include <windows.h>" | $(CXX) -E -dM - | findstr _UCRT) | ||
ifneq (,$(UCRT_STRING)) | ||
IS_UCRT ?= true | ||
else | ||
IS_UCRT ?= false | ||
endif | ||
$(shell echo "IS_UCRT ?= $(IS_UCRT)" > $(MATH)make/ucrt) | ||
else | ||
-include $(MATH)make/ucrt | ||
endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can make this more concise by defining a make rule who's result is make/ucrt
, which will then be built when you include
it. This is how the .d
files for dependencies work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh cool, TIL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried out the make rule, but I'm not sure about the best place to put the rule. Thoughts/prefs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think putting it just above where it is included is probably fine? I think you can put rules in places that are conditionally defined
Unfortunately we can't use that Makefile logic since MinGW has their own logic for setting MSVCRT_VERSION, which I'd rather not hardcode. I've instead gone back to the |
This might be a silly question but does |
It will also exist if MinGW is installed (they provide a version as well). But if the system needs UCRT then I'd say it's pretty rare they wouldn't have |
Looks like all passes (hooray!). Should we ask another dev/user with a windows machine to test the basic setup locally:
Just to make sure the initial caching process isn't too obnoxious for the dev workflow (or initial cmdstan build) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two small comments. I will try this branch on my Windows machine tomorrow before final approval.
Thanks @andrjohns
make/tests
Outdated
@@ -106,7 +106,7 @@ else | |||
endif | |||
|
|||
%.hpp-test : %.hpp test/dummy.cpp | |||
$(COMPILE.cpp) $(CXXFLAGS) -O0 -include $^ -o $(DEV_NULL) -Wunused-local-typedefs | |||
$(COMPILE.cpp) $(CXXFLAGS) -O0 -include $^ -S -o $(DEV_NULL) -Wunused-local-typedefs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure where this rule is used, is there a reason for this change in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a MinGW UCRT-specific bug, where the toolchain doesn't recognise the nul
device for some reason: msys2/MINGW-packages#10730
Otherwise it fails with:
Assembler messages:
Fatal error: can't create nul: Invalid argument
But I should have included it only for Win+UCRT, so I've fixed that now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't replicate the failure locally, which makes me think it's related to either:
- rtools43 uses gcc 13.2 while GHA uses 12.2
- rtools43 gcc built by MSYS2, GHA by Mingw-W64
RTools43 g++:
g++.exe (Rev2, Built by MSYS2 project) 13.2.0
GHA g++:
g++.exe (x86_64-posix-seh-rev2, Built by MinGW-W64 project) 12.2.0
@@ -125,6 +125,7 @@ clean-deps: | |||
@$(RM) $(call findfiles,test,*.d.*) | |||
@$(RM) $(call findfiles,lib,*.d.*) | |||
@$(RM) $(call findfiles,stan,*.dSYM) | |||
@$(RM) $(call findfiles,make,ucrt) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noting so we remember to do it: it seems like stan and cmdstan do not call the clean target of their submodules, so we will need to manually add something which deletes this higher up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried with both rtools43 (UCRT) and rtools40 on this branch, both work. The caching makes it so the build time is essentially no different
Thanks @andrjohns
Summary
Windows users (and cmdstan interfaces) currently need to add
-D_UCRT
tomake/local
when building on windows with a UCRT toolchain. This PR updates the makefiles to detect whether a UCRT toolchain is being used on Windows, and to add the flag automatically if needed.We can detect UCRT-ness by checking whether the
UCRT
macro is defined after pre-processing thewindows.h
headerUCRT Toolchain:
Non-UCRT Toolchain:
I've tested that the TBB (and Stan) successfully build under both the UCRT
mingw-w64-ucrt-x86_64-gcc
and the non-UCRTmingw-w64-x86_64-gcc
toolchainsTests
N/A
Side Effects
N/A
Release notes
Automatically detect UCRT toolchain use on Windows
Checklist
Math issue #(issue number)
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