MSVC STL version check #149
Conversation
| Because I saw that the changes resulted in broken makefiles at times, I revisited the controversial force-include flag generation for MSVC. I tested the more simpler approach with Ninja, NMake Makefiles and MSBuild (the Visual Studio family generators) and all of them work now with this simplified approach. |
| Hi @MathiasMagnus, sorry it's taken me so long to review this, I've been working on other projects. This is a great heads-up as our developers (some of whom use Visual Studio) weren't aware of this policy! (Abstractly, I'd heard of it, but I didn't realise they were actually doing hard version checks). I will take a look but I think my main recommendation will be that, if possible, we put the MSVC-specific stuff in a separate file - but I'll leave that for the review. |
| mark_as_advanced(COMPUTECPP_COMPILER_FLAGS) | ||
| endif() | ||
| | ||
| # Check if the hosted STL of MSVC is compatible with ComputeCpp |
There was a problem hiding this comment.
Since this whole code section is MSVC-specific, I think I'd be happier if it was part of an opt-in module (i.e. its own file). I feel like that would be a better reflection of the separation of concerns here - it could be called MSVCVersionCheck or similar, and that way users targetting '17 know that they have to deal with it (similarly, once the compiler is updated, they can remove it again).
Let me know what you think!
There was a problem hiding this comment.
I'm generally okay with all this being moved to a file of its own. I'm not sure how many such changes are needed. There is a brief GCC-related test at the beginning, which for the sake of consistency might also be put to a different file... all in all, factoring out compiler-specific stuff to separate files and keeping FindComputeCpp.cmake to the bare minimum of detecting the library might be the best way to go. Also, checks about the executing runtime mentioned in #155 might also be put to a file of its own.
All of this might be the topic of another PR.
There was a problem hiding this comment.
I think I'd prefer if we move it into its own file for now, provided it's not too difficult. Let me know if it looks like it will actually be too onerous.
You're right about the gcc/clang check. Revisiting it, I'm tempted to remove it, because who uses 4.8? 😝 More seriously, if you use old compilers like that, you will get compile issues. Though maybe we should move it to this new file while we still (ostensibly) support Ubuntu 14.04?
Decision time: if you feel like moving it too, please feel free, though I won't mark it as a requirement of this PR by any means. Thanks again!
2 participants
Most of the aggravation of using ComputeCpp with Visual C++ stems from the fact that the STL of MSVC is tested only against latest stable and devel Clang. The check (which is a hard
#error) can be disabled with a macro.The addition in this PR tries to ease the life of users in the hiatus periods, when Compute++ has not yet caught up to latest Clang while MSVC already updated their STL headers. It invokes the device compiler on a minimal source file and checks whether the device compiler fails on the version check. If it does, it tries going around the check. It doesn't do elaborate testing of some feature missing from the device compiler. If it fails compiling even after turning off the check, issues a message saying most likely no STL feature will be available. This may even be a
message(FATAL_ERROR). Without the STL, not many libraries compile. I made it a message not to exclude users with 100% self-hosted code bases.Note
The use of
execute_processis extremely brittle. I wasted an entire day until I uncovered the problem. Passing of the arguments toexecute_processis doneVERBATIM, which goes crazy over the mixed nature ofCOMPUTECPP_DEVICE_COMPILER_FLAGS. Namely its value after invoking computecpp_info.exe iswhich will result in the error
I would advise against trying to come up with something smarter, that is unless you got some spare time to burn.