Skip to content

fix zstd_shared detection on mingw #139945

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
May 24, 2025
Merged

fix zstd_shared detection on mingw #139945

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions llvm/cmake/modules/Findzstd.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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$")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@petrhosek petrhosek May 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @mstorsjo that it'd be preferable to use a standard variable rather than hardcoding .dll.a. I did a quick search through CMake codebase and it looks like CMAKE_IMPORT_LIBRARY_SUFFIX is set to .dll.a when the platform is Windows-GNU, would that work here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upstream, the variable used in similar situations appears to be a hard coded list of ".dll.a" combined with CMAKE_FIND_LIBRARY_SUFFIXES:
https://github.com/Kitware/CMake/blob/ede59aac3f28a0ecb535f13bdcb81fbd5c109983/Modules/FindZLIB.cmake#L162-L171

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upstream also seems to consider the use of CMAKE_IMPORT_LIBRARY_SUFFIX in this file to be unsupported behavior (https://gitlab.kitware.com/cmake/cmake/-/issues/21180#note_828866), since it is a configuration variable for the llvm target, and not relevant to the zstd dependencies, as it is used for here, so I would rather not add more uses of it

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

# IMPORTED_LOCATION is the path to the DLL and IMPORTED_IMPLIB is the "library".
get_filename_component(zstd_DIRNAME "${zstd_LIBRARY}" DIRECTORY)
Expand Down