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

remove vcpkg in favor of boostorg/math standalone #2151

Merged

Conversation

michael-herwig
Copy link

Regarding #1718 this PR tries to drop the vcpkg dependency in favor of the new standalone mode of Boost::math.

  • The sources of boostorg/math are included as a submodule matching the existing style of including external sources.
  • The submodule points to a fork I created to remove a few lines of endianness detection, causing an error for arm-based architectures, see ticket. It may be better to have a workaround, create a company-owned fork, or wait for a solution published at boostorg/math.
  • The submodule upgrades to 1.77.0 and may introduce breaking changes which are left to be verified.
  • The standalone mode requires the definition of the preprocessor macro BOOST_MATH_STANDALONE. From the include hierarchy of special_math.cpp, this affects the following files
    boost-math/include\boost/math/tools/config.hpp
    boost-math/include\boost/math/tools/is_standalone.hpp
    boost-math/include\boost/math/tools/assert.hpp
    boost-math/include\boost/math/tools/throw_exception.hpp
    boost-math/include\boost/math/tools/lexical_cast.hpp
    boost-math/include\boost/math/special_functions/detail/fp_traits.hpp
    

Though 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 besides special_math.cpp. There again Boost::math could also be included using the download functions of cmake (ie. ExternalProject_Add) if a submodule is not applicable in this case.

@StephanTLavavej StephanTLavavej added build Related to the build system infrastructure Related to repository automation labels Aug 23, 2021
@StephanTLavavej
Copy link
Member

This is awesome, thank you! 😻

  • I think this will also make it possible to eliminate long-standing divergence between the MSVC-internal and GitHub builds, where the internal build uses a patched copy of Boost 1.66.0 (Dec 2017 🙀) because it didn't use vcpkg. (We'll need to make those internal changes when mirroring this PR or afterwards - that machinery lives outside of the stl/msbuild subdirectory.)
  • Adding boostorg/math as a submodule and removing vcpkg is correct - we don't use vcpkg for anything else. Nor do I think we're likely to in the future - consuming anything directly raises the specter of special_math.cpp: Statically linked library contains and depends on Boost symbols #362. Using Boost.Math standalone doesn't affect that, but using vcpkg for anything else in the future would be similarly affected. I believe we're likely to follow the examples of Ryu (modified/_Uglified source code) and libfmt (new code heavily inspired by existing code).
  • standalone: c++ usage in preprocessor evaluation boostorg/math#677 should be fixed upstream so we can use the official repo before merging this.
  • No concerns with upgrading to Boost 1.77.0; we have upgraded Boost several times without issues.

@StephanTLavavej
Copy link
Member

Upstream PR: boostorg/math#678

@michael-herwig michael-herwig marked this pull request as ready for review August 24, 2021 17:07
@michael-herwig michael-herwig requested a review from a team as a code owner August 24, 2021 17:07
@@ -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)
Copy link
Member

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)

Copy link
Member

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:

#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.

.gitignore Show resolved Hide resolved
@StephanTLavavej
Copy link
Member

I'm mirroring this to an MSVC-internal PR. Changes can still be pushed during final review, but please notify me if that happens.

@StephanTLavavej
Copy link
Member

Note that during the final merge process, there will be a conflict with #2144's changes to README.md. I'll push changes for this immediately before merging.

@CaseyCarter CaseyCarter removed their assignment Aug 26, 2021
Copy link
Member

@StephanTLavavej StephanTLavavej left a 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."

@StephanTLavavej StephanTLavavej merged commit 78ff461 into microsoft:main Aug 27, 2021
@StephanTLavavej
Copy link
Member

Thanks for this amazing build/infrastructure improvement that simplifies our Getting Started workflow too!

🎉 😻 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Related to the build system infrastructure Related to repository automation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants