-
Notifications
You must be signed in to change notification settings - Fork 6.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
[boost] stop renaming libs and add has_synchro for boost-atomic #27694
Closed
Closed
Changes from all commits
Commits
Show all changes
76 commits
Select commit
Hold shift + click to select a range
92f23c1
add compiler version to vcpkg-cmake-get-vars
Neumann-A 30cba5f
boost library naming stuff
Neumann-A a82885e
fix missing ,
Neumann-A 3d72073
has_synchronization_lib check for boost-atomic.
Neumann-A 5d6c981
use correct ver dep
Neumann-A 24ac419
v db
Neumann-A 3395c01
update helper script
Neumann-A f74ac8b
Merge remote-tracking branch 'origin/master' into HEAD
BillyONeal c32d32e
install CMake 3.24.3 FindBoost.cmake
Neumann-A 60c2435
remove more renaming.
Neumann-A fb66ad5
v db
Neumann-A cd3c953
Merge remote-tracking branch 'upstream/master' into fix_boost_clang_cl
Neumann-A 87f2900
more suffixes restored.
Neumann-A 806c1cd
v db
Neumann-A e7f64cd
guard policy 102
Neumann-A d3bc6b9
test in CI
Neumann-A 8ae0528
more policy adjustments
Neumann-A f31254a
add pass entry to ci baseline
Neumann-A 30aa236
fix vcpkg-ci-boost on arm
Neumann-A ebcd03a
more fixes
Neumann-A 3f1edd0
more dep adjustment
Neumann-A f8a9515
more platform deps
Neumann-A d44217d
more dep and supports fixes
Neumann-A 4b32e9c
remove uwp from testing
Neumann-A 040d793
v db and script
Neumann-A 28d00f9
Merge branch 'microsoft:master' into fix_boost_clang_cl
Neumann-A 481e069
Merge remote-tracking branch 'upstream/master' into fix_boost_clang_cl
Neumann-A ea70fa2
v db
Neumann-A f8402bf
Merge branch 'microsoft:master' into fix_boost_clang_cl
Neumann-A a84154c
Merge branch 'microsoft:master' into fix_boost_clang_cl
Neumann-A ee6a9a3
Merge remote-tracking branch 'upstream/master' into fix_boost_clang_cl
Neumann-A 4877304
details
Neumann-A 9fd2c02
v db
Neumann-A d7f25d4
Merge remote-tracking branch 'upstream/master' into fix_boost_clang_cl
Neumann-A 9e3cff0
v db
Neumann-A e5f4c7c
Merge remote-tracking branch 'upstream/master' into fix_boost_clang_cl
Neumann-A 504a863
revert v db fix 2023
Neumann-A 5e49004
v db
Neumann-A 71c1126
Merge branch 'microsoft:master' into fix_boost_clang_cl
Neumann-A 088058b
Merge branch 'microsoft:master' into fix_boost_clang_cl
Neumann-A 66c202f
Merge branch 'microsoft:master' into fix_boost_clang_cl
Neumann-A 6d77309
Merge remote-tracking branch 'upstream/master' into fix_boost_clang_cl
Neumann-A 62ac681
port version bump
Neumann-A 8c0c931
v db
Neumann-A 0608d38
restore removal of renaming
Neumann-A 2fbcda1
v db
Neumann-A 994a6b9
Merge remote-tracking branch 'upstream/master' into fix_boost_clang_cl
Neumann-A dff3332
v db
Neumann-A 4ba314e
Merge remote-tracking branch 'upstream/master' into fix_boost_clang_cl
Neumann-A ee70360
v db
Neumann-A e1289cc
Merge remote-tracking branch 'upstream/master' into fix_boost_clang_cl
Neumann-A 016860e
revert vdb and boost-random
Neumann-A 691d499
generate update
Neumann-A 7b73f99
v db
Neumann-A 016589e
Merge branch 'microsoft:master' into fix_boost_clang_cl
Neumann-A 20b6ea4
Merge remote-tracking branch 'upstream/master' into fix_boost_clang_cl
Neumann-A 13442c7
v db
Neumann-A ee0067c
remove ws
Neumann-A aec8ad6
v db
Neumann-A 05de907
Merge remote-tracking branch 'upstream/master' into fix_boost_clang_cl
Neumann-A 0827185
v db
Neumann-A f8515fd
v changes
Neumann-A ea84a1f
v db
Neumann-A fd7fe2b
Merge branch 'master' of https://github.com/microsoft/vcpkg into fix_…
JonLiu1993 619f3a9
fix android triplets
Neumann-A 84c8720
v db
Neumann-A 4e51b1d
merge
JavierMatosD 052810f
Merge remote-tracking branch 'origin/master' into HEAD
JavierMatosD 07782c6
remove boost-atomic stuff since it's already merged
JavierMatosD de9e3c0
newline at end of file
JavierMatosD 7eaefb8
Merge remote-tracking branch 'origin/master' into HEAD
BillyONeal 8e3df40
Revert do-nothing changes.
BillyONeal 9a7f23f
run generate-ports
JavierMatosD 7cc2ec2
revert mystery change from running generate-ports
JavierMatosD a94f089
version db
JavierMatosD 1861d39
fix boost-asio
JavierMatosD File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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've been messing around with this to see if this would work with older versions of CMake. This doesn't seem to work with CMake 3.12 or below.
To repro you can do the following:
vcpkg install boost
vcpkg install boost-regex
I have a list file that looks like this:
//main.cpp
cmake-3.12.0.exe -G "Ninja" -DCMAKE_TOOLCHAIN_FILE=C:\dev\vcpkg\scripts\buildsystems\vcpkg.cmake ..
You get the following error:
The project compiles and links successfully when I use cmake 3.24.3 as expected.
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.
Did your CMake 3.12 test use VS2017 or below? If not the test is invalid, if yes please provide a --trace-expand log.
(Also mixing a boost build with VS2022 and consuming it in VS2017 intentionally won't work with this PR; So you need to build everything with VS2017)
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 tested with VS2022, but I was actually able to build successfully by setting
Boost_COMPILER
to "-vc143" in thevcpkg-cmake-wrapper.cmake
.I believe this is a scenario that we care about and was possible before this PR. I.e., VS2022 with CMake 3.12
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 the scenario you care about is the other way around. Building boost with VS2017 and consuming with VS2022. The other way around is unsupported by VS itself but was possible before.
I am not fixing virtual problems. VS2022 + CMake 3.12 isn't supported by CMake itself. As such it is probably unable to deduce
MSVC_TOOLSET_VERSION
correctly which is required for FindBoost.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.
Putting aside my backward compatibility concerns for the moment, could you please expand on how this PR addresses your three points?
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.
Boost_COMPILER
set_boost_COMPILER
inFindBoost
here:https://gitlab.kitware.com/cmake/cmake/-/blob/master/Modules/FindBoost.cmake#L1913
Lookup for library names is done e.g. around here :
https://gitlab.kitware.com/cmake/cmake/-/blob/master/Modules/FindBoost.cmake#L2230
So by setting
Boost_COMPILER
you are telling FindBoost to look for boost libs with a fixed infix (v140
) however using clang the infix is (clangw<major>
). Renaming the infix should be avoided sincelld-link
andlink
linked libraries are not necessary link compatible iflld-link
left IR metadata in the library (due to-flto
or something similar)The whole name guesing FindBoost normally does is below
https://gitlab.kitware.com/cmake/cmake/-/blob/master/Modules/FindBoost.cmake#L861
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 is super helpful, thank you.
I have a few observations on this though:
-flto
and the libraries are link compatible. It seems like extremely bad behavior to break things that work just to slightly shift around where the failure occurs in a narrow set of circumstances (passing-flto
AND mixing compilers).Given this, I think pinning
Boost_COMPILER
sounds like the best path forward given all the concerns. It lets us remove our boost library name customizations (Yay!) while still giving customers the same unsurprising experience they get with every other library. It also, importantly, doesn't break any cases that would work[2].Another PS from above: it makes total sense that we don't care about supporting new Visual Studio Generators with old CMake. However, using a simple developer command prompt with Ninja is broadly compatible between new and old CMake/MSVC and we'd like to avoid breaking that without a highly compelling reason.
[1] I think wxwidgets might also do something similar
[2] Where "work" excludes suspicious ODR-violating things like having multiple copies of Boost available to find and relying on CMake to always pick the "right" one.
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 should probably forbidden as soon as vcpkg installs the wrapper. However, the current state also makes exactly this possible with clang-cl (and even msvc if the dep is forgotten). The libs get installed with the lib infix
-clangw<ver>-
vcpkg hardcodes the compiler to bev140
. Somewhere on a cmake search path I have either the libs with the infix or i have them without them since FindBoost also looks for libs names without it: https://gitlab.kitware.com/cmake/cmake/-/blob/master/Modules/FindBoost.cmake#L2234So the shift-right is currently also possible independent of this change. (and also for MSVC if you for example forget to install a boost dependency but still look it up and have it available somewhere).
The above would also happen with per port customization switching compilers between clang-cl and cl in the boost ports but that is probably niche usage ;)
I also wouldn't consider this massiv shift-right as too important since to get to that point there has to be a massiv incompetence from the user side. E.g. currently you could simply set
Boost_DIR|ROOT
to somewhere external or useCONFIG
to get a different boost version then provided by vcpkg.LTO was just one example there are more cases where clang-cl/lld-link will generate a binary which cannot be read/linked with
link.exe
. Ask BillyONeal which metadata he skipped when he implemented the dumpbin check into vcpkg. But that is really not the point here.but to which value? Remember that the wrapper is installed by boost-uninstall and technically there could be a switch in compiler between different boost ports. From my perspective vcpkg either uses the correct infix without setting the variable or removes the infix itself completely (which would make external lookup due to the lib name impossible unless other cmake variables are used to control the lookup behavior).
For me the optimal way forward would be if boost would generate the cmake configs the monolithic build installs instead of assuming https://gitlab.kitware.com/cmake/cmake/-/blob/master/Modules/FindBoost.cmake#L1013 and below will be correct. However getting b2 to do that is not quite as simply since it requires the whole information about the dependents and is probably not a quick solution. I tried once a few years ago to get it to work however failed to find enough information about how b2 works to get it to work. Basically you copy the rule form the monolithic build and adjust it to whatever vcpkg does. however fixing the missing parts wasn't as easy/straight forward.
If If FindBoost does not support the boost version you are installing you are out of luck any way (https://gitlab.kitware.com/cmake/cmake/-/blob/master/Modules/FindBoost.cmake#L1657)
That being said newer boost + old cmake + newer VS + ninja seems like a very niche combination for me especially since newer VS + old cmake might fail proper compiler detection logic. (It is not all about the VS Generator; Ninja also needs to pass compiler/feature detection which might be ok in general I just wouldn't bet on it.)
So TL;DR:
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.
In my experience, the vast majority of projects that build successfully with boost version N will also succeed on boost version N+1 with everything else held constant (though the CMake warning is very annoying). The main problem I've seen with old CMake + new Boost is that new libraries aren't available for
find_package(Boost COMPONENTS ...)
. It's theoretically possible that new dependency edges could break the static libraries, but I haven't seen that to happen with any frequency in practice.This is a very good point. I think this indicates that we must pin the infix to a consistent value (possibly empty) for any sort of sane result. (edit: Possible alternative at the end)
Some observations:
My recollection of the past is that we couldn't use the "system" naming scheme (the closest approximation to an "empty" infix) because that broke multi-config generators in older CMake versions -- the "system" naming scheme did not include a debug annotation.
As long as we
set(Boost_COMPILER ...)
in the wrapper to some value (possibly determined at port build time) and homogenize the library names, that should fix clang-cl (and all other compilers).I'm still open to making the library names track the "default" names as closely as possible, which sounds like
boost-uninstall
orboost-modular-build-helper
should run a compiler query pass to pick an infix, which will then be consumed and adhered to by every other boost port (regardless of port customizations). This does mean that if you choose to, for example, buildboost-filesystem
with MSVC v142 instead of v143 the resulting binaries may be mislabeled -- all boost binaries would be marked as "v143" -- but it seems to be as close as we can possibly get given my understanding of the circumstances.The one other moonshot that might be interesting to explore is whether it's possible to pass the library names into FindBoost directly, skipping all detection. If that's the case, we could just have each boost-xyz port install its own pseudo-targets file and glob them all into the
boost-uninstall
wrapper:If this approach works, we wouldn't need to add any compiler detection pass and we could remove ALL of our lib name regularization.
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.
correct that is why
system
naming is out of the questionThat is always possible since FindBoost just uses
find_library
and thus CACHE variables. However, I am neither commiting time for either setting the infix to empty or prefilling the CACHE. I am currently happy with my overlay which is this PR and I would rather spend exploring a solution for #32274