-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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] use cmake build #32309
[Boost] use cmake build #32309
Conversation
Known issues: boost-system not building lib due to intreface target boost-numeric-* issues with modular build
Hm I am properly not going to have time to analyse what is going on with all those dependent port regressions for a while. E.g.: cpprestsdk fails due to:
however locally it builds (CMake 3.26). I assume it has to do with the warning :
|
Probably wrong dep:
|
hmm boost-graph-parallel in CI:
rest of x64-windows regressions is due to boost-math missing the old tr1 and c99 libs. |
include("${CMAKE_CURRENT_LIST_DIR}/ref_sha.cmake") | ||
|
||
vcpkg_from_github( | ||
OUT_SOURCE_PATH SOURCE_PATH_SUPER |
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 we need to separate the vcpkg helpers from proper separate boost component (https://github.com/boostorg/cmake). Separating the vcpkg helper functions into a separate port makes the ownership clear. i.e., these are helper functions authored by vcpkg to package boost, this is the boostorg/cmake core project).
For example, the boost-cmake
port is under the BSL-1.0 license and not under vcpkg's MIT license.
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.
For example, the boost-cmake port is under the BSL-1.0 license and not under vcpkg's MIT license.
Interesting question:
Should I mark the port with license null
since the patches required for vcpkg are probably under MIT
?
Please add it to the maintainer guidelines if you generally want this separation since I prefer it this way if the helper functions only have functionality for a certain very limited group of ports like here or in the case of qt. Secondly, adding a helper port here means that the required cmake logic gets kind of seperated.
How should i call the helper port vcpkg-boost(-(scripts|install|helpers))?
Am I at least allowed to chain it via vcpkg-port-config through boost-cmake
(avoids the issue to add it to all boost manifests)?
You probably only noticed it because boost-cmake
replaces boost-modular-build-helper
according to the git diff github shows ;)
scripts/boost/generate-ports.ps1
Outdated
# 1: boost-cmake/ref_sha.cmake needs manual updating | ||
# 2: Do not trust this script. Platform expressions in dependencies are blindly applied. | ||
# So if you see a configure log with that another boost<lib>Config.cmake is missing | ||
# You probably trusted this script too much. Example: boost-graph -> requires boost-random |
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.
The comment here should only define what does not work. I.e., no " Do not trust this script... You probably trusted this script too much."
@@ -0,0 +1,38 @@ | |||
diff --git a/include/BoostInstall.cmake b/include/BoostInstall.cmake | |||
index 3a00c16..069dde0 100644 |
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.
Why aren't these changes upstream-able?
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.
except for
+ else()
+ string(SUBSTRING "${CMAKE_SYSTEM_PROCESSOR}" 0 1 arch)
+ string(TOLOWER "${arch}" arch)
this is already upstreamed
boostorg/cmake#57
These lines are missing due to boostorg/cmake#57 (comment)
@@ -0,0 +1,109 @@ | |||
diff --git a/CMakeLists.txt b/CMakeLists.txt |
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.
Why is this patch needed?
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.
Because a few ports in vcpkg want to use the legacy boost math libraries.
A follow up PR could make building those a feature instead and then mark the appropriate downstream ports explicitly so that boost-math is header only for most users.
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.
@JavierMatosD: Are there any points I missed to address? I would like to make this ready to merge today. |
Nope looks good to me! Thanks! |
~~arm64-windows: boost-context builds are blocked by a cmake bug (see https://gitlab.kitware.com/cmake/cmake/-/issues/24317)~~ closes microsoft#32274 closes Neumann-A/my-vcpkg-triplets#5 Questions: - [x] ~~Move cmake files to `share/cmake/<name>` ?~~ Not doing it because it is just using `vcpkg_cmake_config_fixup()` - [x] Fix weak dependencies (uwp|emscripten|android|arm)? - [x] Fix library names on !x64 (currently hardcoded to x64 or x86; failure in aricpp since it forces FindBoost module mode.) - [x] ~~Fix arm64-windows boost-context builds -> requires CMake (3.19.2?) update due to bug how the assembler is invoked.~~ (-> CI baseline for now) TODO: - [x] adjust generate ports script - [x] microsoft#37457 --------- Co-authored-by: Cheney-Wang <850426846@qq.com>
arm64-windows: boost-context builds are blocked by a cmake bug (see https://gitlab.kitware.com/cmake/cmake/-/issues/24317)closes #32274
closes Neumann-A/my-vcpkg-triplets#5
Questions:
Move cmake files toNot doing it because it is just usingshare/cmake/<name>
?vcpkg_cmake_config_fixup()
Fix arm64-windows boost-context builds -> requires CMake (3.19.2?) update due to bug how the assembler is invoked.(-> CI baseline for now)TODO: