-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
remove vcpkg in favor of boostorg/math standalone #2151
remove vcpkg in favor of boostorg/math standalone #2151
Conversation
This is awesome, thank you! 😻
|
Upstream PR: boostorg/math#678 |
@@ -90,6 +79,7 @@ get_filename_component(TOOLSET_ROOT_DIR "${TOOLSET_ROOT_DIR}" DIRECTORY) # $\VC\ | |||
|
|||
set(TOOLSET_LIB "${TOOLSET_ROOT_DIR}/lib/${VCLIBS_X86_OR_X64}") | |||
|
|||
add_subdirectory(boost-math) |
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.
No change requested, just explaining for other reviewers. You mentioned:
The standalone mode requires the definition of the preprocessor macro
BOOST_MATH_STANDALONE
.
I found that this is automatically defined by boost-math's CMakeLists.txt
:
cmake_dependent_option(BOOST_MATH_STANDALONE "Use Boost.Math in standalone mode" ON "NOT BOOST_SUPERPROJECT_VERSION" OFF)
message(STATUS "Boost.Math: standalone mode ${BOOST_MATH_STANDALONE}")
And running cmake
for the STL repo prints:
-- Boost.Math: standalone mode ON
@@ -90,6 +79,7 @@ get_filename_component(TOOLSET_ROOT_DIR "${TOOLSET_ROOT_DIR}" DIRECTORY) # $\VC\ | |||
|
|||
set(TOOLSET_LIB "${TOOLSET_ROOT_DIR}/lib/${VCLIBS_X86_OR_X64}") | |||
|
|||
add_subdirectory(boost-math) | |||
add_subdirectory(stl) | |||
|
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.
No change requested - I noticed that special_math.cpp
is defining the following macros:
Lines 17 to 18 in f75c7f5
#define BOOST_CHRONO_HEADER_ONLY | |
#define BOOST_CONFIG_SUPPRESS_OUTDATED_MESSAGE |
I suspect that we can drop them in standalone mode. However, that would force us to immediately convert the MSVC-internal build to also use standalone mode. While I think we should eventually do that, it looks like that will be more work than I initially thought, so it should be separate.
Specifically, I ran our internal Special Math tests (currently not in this repo for boring reasons; this is on our GitHub Migration backlog) and found that while they pass with the binaries shipping in VS 2022 17.0 Preview 3 (built with ancient Boost 1.66.0), there are 34 failures with Boost.Math standalone. The good news is that these are exactly the same failures as with microsoft/STL main
using vcpkg, so that shouldn't block this PR. (Apparently Boost.Math did change behavior after 1.66.0 and we just hadn't noticed yet.)
I suspect that we need to regenerate the tests (in addition to updating them so they can run without a full copy of Boost) and that this doesn't indicate any physical problem with Boost.Math or our usage of it.
I'm mirroring this to an MSVC-internal PR. Changes can still be pushed during final review, but please notify me if that happens. |
Note that during the final merge process, there will be a conflict with #2144's changes to |
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.
Resolved merge conflict in README.md
, dropping the cd STL
command as nothing follows it now. The x86/x64 sections still say "Change directories to the previously cloned STL
directory."
Thanks for this amazing build/infrastructure improvement that simplifies our Getting Started workflow too! 🎉 😻 🎉 |
Regarding #1718 this PR tries to drop the
vcpkg
dependency in favor of the new standalone mode ofBoost::math
.boostorg/math
are included as a submodule matching the existing style of including external sources.boostorg/math
.BOOST_MATH_STANDALONE
. From the include hierarchy ofspecial_math.cpp
, this affects the following filesThough it is generally not required to drop
vcpkg
I did not see any benefit in keeping it. Maybe I have overseen something but I could not find any reference to boost besidesspecial_math.cpp
. There againBoost::math
could also be included using the download functions ofcmake
(ie.ExternalProject_Add
) if a submodule is not applicable in this case.