Conversation
|
Alternatively, |
Are you sure? The reason it's done this way is to ensure CMake doesn't screw up lib order. Ref: #16973 (comment) |
| set(CMAKE_MODULE_PATH ${_curl_cmake_module_path_save}) | ||
|
|
||
| if(CMAKE_C_COMPILER_ID STREQUAL "GNU" AND WIN32 AND NOT TARGET CURL::win32_winsock) | ||
| if(WIN32 AND NOT TARGET CURL::win32_winsock) |
There was a problem hiding this comment.
What about setting CMAKE_C_COMPILER_ID to CMAKE_CXX_COMPILER_ID
if the former is unset and the latter is set?
This may make work not only this line, but also the ~50 other places
where the build expects a value in this variable.
While also keeping this condition, which I think is important to document
that this is gcc/mingw-specifc, and keep this line in sync with it's clone
in CMakeLists.txt.
There was a problem hiding this comment.
Or, because this is for curl-config.cmake only and there is no other use of CMAKE_C_COMPILER_ID variable there for now, perhaps just extend the condition with OR CMAKE_CXX_COMPILER_ID STREQUAL "GNU"?
There was a problem hiding this comment.
No. I deliberately decided for KISS here: Always create the target on WIN32.
(I acknowledge that you added the imported target dance to workaround infrequent CMake quirks. But how this indirection created the next bug is the demonstration of why KISS might be prefered.)
What about setting CMAKE_C_COMPILER_ID to CMAKE_CXX_COMPILER_ID
if the former is unset and the latter is set?
Don't mess with standard variables in user projects.
This may make work not only this line, but also the ~50 other places
where the build expects a value in this variable.
There are zero places in the CMake config template.
There was a problem hiding this comment.
Don't mess with standard variables in user projects.
Agree. This can avoided by introducing an interim variable, but since
it's just a single if(), why isn't it enough to extend the condition with
OR CMAKE_CXX_COMPILER_ID STREQUAL "GNU"?
It must remain in sync with the same logic in core CMakeLists.txt,
and I wouldn't like to extend this workaround to all WIN32 platforms,
because it's a super horrible hack, and nothing else needs it, because
all linkers are sane, except the binutils one.
(It's a binutils quirk, which is unfortunately hit by a CMake quirk.)
There was a problem hiding this comment.
Thinking about it more: the issue with filtering for toolchain in the config file,
is that someone might build libcurl with clang, or MSVC even (which do not
suffer from the binutils ld issue), and that libcurl, together with its cmake config,
may then be used in a project using gcc.
This suggests it's inevitable to extend the workaround to all WIN32?
It's also not only WIN32, the quirk applies to all binutils platforms, and it
also breaks with libcrypto and zlib, in full static builds.
Looking at that code, it's doesn't seem to actually be doing anything for
libcrypto/zlib after this interim cleanup commit: 615c43e,
though it probably should. Maybe this part should remain GCC-only despite
the above; seem ugly to infect all builds with it (possibly causing duplicate
library warnings?), and seems to be a rare issue.
|
Long story short: There is no convincing reason to change this PR. |
I think it's fine, yes. It will create an unused target for libcurl builds that used The unused target part looks like something to fix in Do you agree with the second part, or am I missing something? |
|
Thanks, merged now! |
While working #16973, the binutils ld lib order workaround logic regressed so that it modified the wrong target, writing into the system `ZLIB::ZLIB` and `OpenSSL::Crypto` ones a `INTERFACE_LINK_LIBRARIES` property, instead of creating CURL-namespaced targets. Oddly enough, this also fixed the binutils ld lib ordering issue. It seems this property makes CMake insert each referenced library in two more positions (not at the very end though), which allows ld to resolve all symbols in the cases tested in CI. Fix by creating the indented namespaced targets, and also creating these in `curl-config.cmake` to be available when consuming libcurl. Note that the logic continues doing `get_target_property()` on the two system targets above. If these targets are defined manually and miss the `LOCATION` propery, or are defined as aliases, this command may fail. curl expects these targets be created by CMake's `FindZLIB` and `FindOpenSSL` built-in Find modules (or ones compatible). Ref: #20419 The binutils ld issue is reproduced by these CI jobs: - Linux gcc glibc (amd64, arm64) - Windows gcc zlib-classic (x64) Currently using this curl-for-win revision: curl/curl-for-win@7d12669 Examples: https://github.com/curl/curl/actions/runs/21332437230/job/61399234023?pr=20427 https://github.com/curl/curl/actions/runs/21332437230/job/61399234033?pr=20427 Comparison of lib orders, as passed by CMake to the linker: without workaround (possibly breaking binutils `ld`): ```diff -framework [...] libssl.dylib libcrypto.dylib libz.tbd -lssh2 -lidn2 libldap.tbd liblber.tbd -lbrotlidec -lbrotlicommon -lzstd -lnghttp2 -lpsl -lrtmp -lz -lssl -lcrypto ``` before this patch: ```diff -framework [...] libssl.dylib libcrypto.dylib libz.tbd +libcrypto.dylib <== inserted via `INTERFACE_LINK_LIBRARIES` +libz.tbd <== inserted via `INTERFACE_LINK_LIBRARIES` -lssh2 -lidn2 libldap.tbd liblber.tbd +libcrypto.dylib <== inserted via `INTERFACE_LINK_LIBRARIES` +ibz.tbd <== inserted via `INTERFACE_LINK_LIBRARIES` -lbrotlidec -lbrotlicommon -lzstd -lnghttp2 -lpsl -lrtmp -lz -lssl -lcrypto ``` after this patch: ```diff -framework [...] libssl.dylib libcrypto.dylib libz.tbd -lssh2 -lidn2 libldap.tbd liblber.tbd -lbrotlidec -lbrotlicommon -lzstd -lnghttp2 -lpsl -lrtmp -lz -lssl -lcrypto +libcrypto.dylib <== inserted via `CURL::OpenSSL_Crypto` +libz.tbd <== inserted via `CURL::ZLIB` ``` Bug: #20382 (comment) Reverts: 615c43e Follow-up to 16f073e #16973 Closes #20427
2 participants
The CMake config can be consumed by project which enable only language
CXX.CMAKE_C_COMPILER_IDisn't defined in this case, and the target definition would be missing.But the check for compiler id isn't really needed: The target is namespaced and valid, regardless of actual compiler.
Noticed in microsoft/vcpkg#49518, building cpr.