-
Notifications
You must be signed in to change notification settings - Fork 586
Support Boost 1.88
on Windows
#10419
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
Conversation
Have you tested if this is still required on windows with Currently when running cmake we're getting the warning that the policy is not set which means that we're using cmake's FindBoost module instead of boost's BoostConfig.cmake. The help page to this policy explicitly mentions better support of new boost versions. When going through BoostConfig.cmake I can't see anything that explicitly links |
No, I didn't but even then we can't use it here. The CMake docs for it says this:
And our minimum supported Boost version is Line 5 in a65f2d6
|
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 not to also bump ours to v1.88 while on it?
psapi is not a Boost library but a Windows library, so I don't really see why this should be a reason that we have to link statically.
Does that really raise the Boost version implicitly? Sounds a bit odd if CMake wouldn't fall back to their own config for older Boost versions if they ship it anyways for providing the legacy behavior. |
I don't know, but that's what I get when trying to compile Icinga 2 with that new policy. diff --git a/CMakeLists.txt b/CMakeLists.txt
index 5d71842ce..c342d862b 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -172,7 +172,10 @@ else()
set(LOGROTATE_CREATE "\n\tcreate 644 ${ICINGA2_USER} ${ICINGA2_GROUP}")
endif()
+cmake_policy(PUSH)
+cmake_policy(SET CMP0167 NEW)
find_package(Boost ${BOOST_MIN_VERSION} COMPONENTS coroutine context date_time filesystem iostreams thread system program_options regex REQUIRED)
+cmake_policy(POP)
# Boost.Coroutine2 (the successor of Boost.Coroutine)
# (1) doesn't even exist in old Boost versions and
(END) ---
CMake Error at CMakeLists.txt:177 (find_package):
By not providing "FindBoost.cmake" in CMAKE_MODULE_PATH this project has
asked CMake to find a package configuration file provided by "Boost", but
CMake did not find one.
Could not find a package configuration file provided by "Boost" (requested
version 1.66.0) with any of the following names:
BoostConfig.cmake
boost-config.cmake
Add the installation prefix of "Boost" to CMAKE_PREFIX_PATH or set
"Boost_DIR" to a directory containing one of the above files. If "Boost"
provides a separate development package or SDK, be sure it has been
installed.
-- Configuring incomplete, errors occurred!
DEBUG: 72+ >>>> cd "$sourcePath"
DEBUG: 74+ if ( >>>> $lastexitcode -ne 0) {
DEBUG: 75+ >>>> exit $lastexitcode Do the boost packages we upload to packages.icinga.com include that CMake module? |
Works on my (Linux) machine, though it still gives that policy warning for the tests:
It also works without that warning when I just put
There is nothing special about them, they just mirror the upstream files1. Footnotes
|
I know, it works on Machine (MacOS) too :)! But I'm talking about windows here https://github.com/Icinga/icinga2/actions/runs/15250509514/job/42886209439?pr=10419
Yeah, since it shouldn't have any other side-effects it should be fine without the |
After a quick check on the random Boost version installed on my Windows machine, seems to be there:
Maybe it needs some more/other |
Adding |
Still doesn't fix the actual issue referenced in the PR description though! C:\Users\yhabteab\icinga2\build\lib\remote\remote.dir\Debug\remote_unity.obj
Creating library C:/Users/yhabteab/icinga2/build/Bin/RelWithDebInfo/Debug/check_nscp_api.lib and object C:/Users/y
habteab/icinga2/build/Bin/RelWithDebInfo/Debug/check_nscp_api.exp
base_unity.obj : error LNK2019: unresolved external symbol EnumProcessModules referenced in function "unsigned __int64
__cdecl boost::stacktrace::detail::get_own_proc_addr_base(void const *)" (?get_own_proc_addr_base@detail@stacktrace@boo
st@@YA_KPEBX@Z) [C:\Users\yhabteab\icinga2\build\plugins\check_nscp_api.vcxproj]
base_unity.obj : error LNK2019: unresolved external symbol GetModuleInformation referenced in function "unsigned __int6
4 __cdecl boost::stacktrace::detail::get_own_proc_addr_base(void const *)" (?get_own_proc_addr_base@detail@stacktrace@b
oost@@YA_KPEBX@Z) [C:\Users\yhabteab\icinga2\build\plugins\check_nscp_api.vcxproj]
C:\Users\yhabteab\icinga2\build\Bin\RelWithDebInfo\Debug\check_nscp_api.exe : fatal error LNK1120: 2 unresolved externa
ls [C:\Users\yhabteab\icinga2\build\plugins\check_nscp_api.vcxproj]
Done Building Project "C:\Users\yhabteab\icinga2\build\plugins\check_nscp_api.vcxproj" (default targets) -- FAILED.
Done Building Project "C:\Users\yhabteab\icinga2\build\plugins\check_nscp_api.vcxproj.metaproj" (default targets) -- FA
ILED. |
Ok, here's another reason, according to the docs, if you want to dynamically link that library you've to also load it (probably using LoadLibraryA somewhere in our code base, which I doubt it would make anything better as opposed to the static linking.
|
That's a bit strange, in my intuition, you shouldn't need to care about these details (i.e. linking
Shouldn't the loader take care of this if the binary links against Footnotes
|
Isn't that how it works on Linux as well? I mean, if myapp depends on liba which depends in libb, does myapp directly link against libb? I doubt. So, why not to link Boost dynamically instead, just like OpenSSL?
Only because several Linux OS don't package a newer one. This doesn't mean we have to support v1.66-1.87 on Windows.
Feel free to pull Boost from upstream, but our OpenSSL source provides only the latest version. |
Isn't exactly the point of dynamic linking not having to rebuild an app if a dll changes? |
Does anyone know if that would actually solve the issue? Isn't all of this still pure speculation at this point? If it actually fixes it, yes that could be an option.
For a one-off installation on a machine yes. From an automated job that would be not so nice towards the upstream project.
What DLL is supposed to change? For Boost DLLs, that doesn't really matter as we'd have to ship them anyway (and I doubt there would even be any ABI-compatible updates). For |
The fundamental claim here isn't even correct: using You can see the difference between an import library and one used for static linking when looking at the different
$ '/c/Program Files/7-Zip/7z.exe' l '/c/local/boost_1_79_0/lib64-msvc-14.2/boost_atomic-vc142-mt-x64-1_79.lib'
[...]
..... 2857 2857 1.txt
..... 2857 2857 2.txt
..... 586 586 1.boost_atomic-vc142-mt-x64-1_79.dll
..... 273 273 2.boost_atomic-vc142-mt-x64-1_79.dll
[...]
[...]
2022-04-07 21:47:58 ..... 3677 3677 1.txt
2022-04-07 21:47:58 ..... 3677 3677 2.txt
2022-04-07 21:47:57 ..... 3606 3606 D:\RB\bin.v2\boost\bin.v2\libs\atomic\build\msvc-14.2\release\link-static\threading-multi\wait_on_address.obj
2022-04-07 21:47:57 ..... 1732 1732 D:\RB\bin.v2\boost\bin.v2\libs\atomic\build\msvc-14.2\release\link-static\threading-multi\find_address_sse41.obj
[...] And $ '/c/Program Files/7-Zip/7z.exe' l '/c/Program Files (x86)/Windows Kits/10/Lib/10.0.22621.0/um/x86/psapi.lib'
[...]
..... 2413 2413 1.txt
..... 2413 2413 2.txt
..... 485 485 1.PSAPI.DLL
..... 248 248 2.PSAPI.DLL
[...] |
That still doesn't make a
And now? What am I supposed to do with this information? Disproving that small note in the PR description still doesn't resolve the actual issue, so if you've suggestions to be applied that would actually fix it, it would be nice. Otherwise, this won't bring us much! |
Well, that small note resulted in quite a bit of discussion regarding static and dynamic linking, even for Boost. I just wanted to point out that Regarding progress of the PR, two options:
|
I wouldn't say it's strange. I mean, if some code requires a lib, something has to link against the lib – on all modern platforms. I indeed would expect the shared Boost libs to do that for us, but we don't use them yet. And the latter is the actual strange thing! The static Boost libs have an excuse for not by themselves linking against anything: static libs are not linked by definition. |
0d592e6
to
bc51409
Compare
1.88.0
1.88
on Windows
Boost `1.88.0` introduced a feature [^1] that makes use of the Windows API, but it uses API functions that are only available with `PSAPI_VERSION >= 2` and Windows VISTA only supports `PSAPI_VERSION == 1`. Actually, that new feature can also be disabled by setting the `BOOST_STACKTRACE_DISABLE_OFFSET_ADDR_BASE` macro, but since it seems to be a useful feature and isn't even disabled by default, we can just drop it that ancient Windows version instead of disabling it. [^1]: boostorg/stacktrace#200
bc51409
to
c4ddd48
Compare
The diff is empty, I just fixed a typo VISITA -> VISTA in the commit message 🙈! |
1.88.0
introduced a feature 1 that makes use of the Windows API, but it uses API functions that are only available withPSAPI_VERSION >= 2
and Windows VISTA only supportsPSAPI_VERSION == 1
. Actually, that new feature can also be disabled by setting theBOOST_STACKTRACE_DISABLE_OFFSET_ADDR_BASE
macro, but since it seems to be a useful feature and isn't even disabled by default, we can just drop it that ancient Windows version instead of disabling it.fixes #10412
Footnotes
https://github.com/boostorg/stacktrace/pull/200 ↩