fix zstd_shared detection on mingw #139945
Conversation
MSVC is not the only compiler that produces Windows dlls, which results in llvm-config printing incorrect paths for --system-libs when libzstd.dll support is enabled.
There was a problem hiding this comment.
The changes look reasonable overall, but I would prefer a bit more explanation of what went wrong before, what the previously MSVC specific codepaths do. It could be good to explicitly call it out in the commit message, that this fixes zstd in mingw builds (instead of implicitly via a negation, about windows compilers that aren't msvc).
| @@ -29,11 +29,11 @@ find_package_handle_standard_args( | |||
| ) | |||
|
|
|||
| if(zstd_FOUND) | |||
| if(zstd_LIBRARY MATCHES "${zstd_STATIC_LIBRARY_SUFFIX}$") | |||
| if(zstd_LIBRARY MATCHES "${zstd_STATIC_LIBRARY_SUFFIX}$" AND NOT zstd_LIBRARY MATCHES "\\.dll\\.a$") | |||
There was a problem hiding this comment.
Is there any cmake variable that contains the .dll.a suffix as a string? But perhaps we don't want to use that here; for MSVC that (.lib) would be equal to the static library suffix, so it would negate the effect of the first part of the condition.
There was a problem hiding this comment.
I don't know of any such variable, though we're more interested here in how zstd was compiled, while this check still incorrectly assumes the conventions are based on how LLVM will be compiled. I wasn't sure it is even possible to correctly identify the kind of file this is just using pattern matching on the name, but this change is trying to just make it behave more consistently. (When compiling MSVC, this tests against "_static.lib", so adding "not .dll.a" has no impact)
There was a problem hiding this comment.
Oh, I missed that this used zstd_STATIC_LIBRARY_SUFFIX, I thought it was the usual CMAKE_STATIC_LIBRARY_SUFFIX.
If it would be the cmake general ones, and if we'd use the variable for import library suffix, like if (zstd_LIBRARY MATCHES "${CMAKE_STATIC_LIBRARY_SUFFIX}$" AND NOT zstd_LIBRARY MATCHES "${CMAKE_IMPORT_LIBRARY_SUFFIX}$"), it'd essentially be if (zstd_LIBRARY MATCHES ".lib$" AND NOT zstd_LIBRARY MATCHES ".lib$").
But this wasn't the case, so nevermind - sorry for the detour.
| set(zstd_STATIC_LIBRARY "${zstd_LIBRARY}") | ||
| elseif (NOT TARGET zstd::libzstd_shared) | ||
| add_library(zstd::libzstd_shared SHARED IMPORTED) | ||
| if(MSVC OR "${CMAKE_CXX_SIMULATE_ID}" STREQUAL "MSVC") | ||
| if(WIN32 OR CYGWIN) | ||
| include(GNUInstallDirs) # For CMAKE_INSTALL_LIBDIR and friends. |
There was a problem hiding this comment.
Can you briefly explain what the code within this conditional branch does? (It's quite unreadable cmake soup as is...) I presume it does some transformation which is essential for using zstd in a mingw build setting.
Is it essential only for using the output of llvm-config, or for building llvm itself as well?
There was a problem hiding this comment.
It transforms the detected zstd_LIBRARY value from an absolute path such as "/usr/lib/libzstd.dll.a" into a linker flag "-lzstd" and marks it in cmake as being a dll. The linker usually doesn't care very much about whether you pass an absolute path or -lzstd and ends up doing mostly the same thing either way, but it leads to inconsistencies when trying to use llvm-config and expecting it to have listed shared libraries with -lzstd
vtjnash
changed the title
|
I updated the github message, so you can just take that text when merging (I don't have merge rights) |
Thanks!
I'd like to nitpick the wording here, "mingw or clang" feels like a bit of an oxymoron (or something...) here. "Mingw" is the name of the environment, not a name that specifically means GCC; and clang can either be in mingw mode or in msvc mode. And clang in msvc mode has the same naming style/behaviour as regular MSVC. Anyway, by just replacing the parenthesis with "(mingw)" it sounds fine to me. I'll approve the patch and I can update the PR description. |
There was a problem hiding this comment.
LGTM.
CCing @petrhosek who I think authored this cmake file, in case he has opinions on it. Otherwise I can probably merge it in a day or two (or whenever I remember it again).
The zstd_shared autodetection was broken for non-MSVC (mingw) compiled LLVM, since it assumed that
*.auniquely identifies a file as being a static library, but typically this is actually an import lib formed as the longer name*.dll.afrom the binary. This leads to confusing output elsewhere, in cmake and llvm-config, when they report this is a static library at an absolute path instead of a shared library named-lzstd.