Skip to content
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

[cmake] Findzstd.cmake cannot find shared zstd library in win-64 conda environment when using Clang #121345

Closed
mgorny opened this issue Dec 30, 2024 · 6 comments · Fixed by #121437
Labels
cmake Build system in general and CMake in particular platform:windows release:backport

Comments

@mgorny
Copy link
Member

mgorny commented Dec 30, 2024

Also reported for the feedstock but I'm pretty sure the problem is in the CMake file: conda-forge/llvmdev-feedstock#306

Example Azure log from downstream package indirectly broken by this: azure-log.txt

I don't have a Windows setup to test this properly, but my reading of the code leads to the following conclusion (that I've originally stated in conda-forge/triton-feedstock#29 (comment)):

  │ │   -- Found zstd: %PREFIX%/Library/lib/libzstd.lib

https://github.com/llvm/llvm-project/blob/e21dc4bd5474d04b8e62d7331362edcc5648d7e5/llvm/cmake/modules/Findzstd.cmake

When MSVC is used, zstd_STATIC_LIBRARY_SUFFIX is _static.lib$, otherwise it's just .lib$. So when using MSVC, zstd.lib (and libzstd.lib would too) triggers the second clause in the zstd_FOUND condition, causing it to perform explicit lookup for bin/zstd.dll. On the other hand, when using Clang, it just treats libzstd.lib as a static library (the first clause) and doesn't bother beyond that. So effectively, with MSVC it finds both static and shared libraries, and with Clang only the former.


Now, this is particularly problematic, when LLVM itself is built with MSVC (as it is in conda-forge). It uses the shared zstd library and includes references to the dependency on this library in the installed CMake files. However, when reverse dependencies use Clang instead, and try to find_package(LLVM), the lookup is performed again. Since it is now performed with Clang and it doesn't find the shared library, the dependencies are unsatisfied and CMake errors out:

 │ │   CMake Error at %BUILD_PREFIX%/Library/lib/cmake/llvm/LLVMExports.cmake:64 (set_target_properties):
 │ │     The link interface of target "LLVMSupport" contains:
 │ │       zstd::libzstd_shared
 │ │     but the target was not found.  Possible reasons include:
 │ │       * There is a typo in the target name.
 │ │       * A find_package call is missing for an IMPORTED target.
 │ │       * An ALIAS target is missing.
 │ │   Call Stack (most recent call first):
 │ │     %BUILD_PREFIX%/Library/lib/cmake/llvm/LLVMConfig.cmake:349 (include)
 │ │     %BUILD_PREFIX%/Library/lib/cmake/mlir/MLIRConfig.cmake:10 (find_package)
 │ │     CMakeLists.txt:61 (find_package)

And perhaps the worst part, it is possible that I've actually broken that code before. I specifically see the shared library logic for MSVC has been added in e7fc754 — perhaps the condition there should check for Windows rather than MSVC. But it's outside my expertise to change that.

CC @nga888

@mgorny mgorny added cmake Build system in general and CMake in particular platform:windows labels Dec 30, 2024
mgorny added a commit to mgorny/llvm-project that referenced this issue Jan 1, 2025
Extend the special logic for finding `zstd.dll` in `Findzstd` to apply
to all Windows configurations rather than just MSVC.  From what
I understand, this naming scheme is specific to Windows in general,
rather than "Windows with MSVC", and `.lib` files are always used to
link to shared libraries.  I could reproduce the original problem when
using Clang with Conda-installed LLVM, and extending the logic to
`WIN32` seems to fix it for me.

That said, I'm not an expert on Windows and I have only done very
limited testing, so I'd appreciate if someone could double check that
I'm not breaking some workflow.

Fixes llvm#121345
mgorny added a commit that referenced this issue Jan 2, 2025
Extend the special logic for finding `zstd.dll` in `Findzstd` to apply
to all MSVC-compatible configurations such as Clang targeting MSVC.

Fixes #121345
@mgorny mgorny reopened this Jan 3, 2025
@mgorny
Copy link
Member Author

mgorny commented Jan 3, 2025

/cherry-pick 62d0aff

@llvmbot
Copy link
Member

llvmbot commented Jan 3, 2025

/cherry-pick 62d0aff

Error: Command failed due to missing milestone.

@mgorny
Copy link
Member Author

mgorny commented Jan 3, 2025

/cherry-pick 62d0aff

@llvmbot
Copy link
Member

llvmbot commented Jan 3, 2025

Failed to cherry-pick: 62d0aff

https://github.com/llvm/llvm-project/actions/runs/12600635807

Please manually backport the fix and push it to your github fork. Once this is done, please create a pull request

@mgorny
Copy link
Member Author

mgorny commented Jan 6, 2025

/cherry-pick 5bbd598 62d0aff

@llvmbot
Copy link
Member

llvmbot commented Jan 6, 2025

/pull-request #121755

tru pushed a commit to llvmbot/llvm-project that referenced this issue Jan 13, 2025
Extend the special logic for finding `zstd.dll` in `Findzstd` to apply
to all MSVC-compatible configurations such as Clang targeting MSVC.

Fixes llvm#121345

(cherry picked from commit 62d0aff)
tru pushed a commit to llvmbot/llvm-project that referenced this issue Jan 14, 2025
Extend the special logic for finding `zstd.dll` in `Findzstd` to apply
to all MSVC-compatible configurations such as Clang targeting MSVC.

Fixes llvm#121345

(cherry picked from commit 62d0aff)
tru pushed a commit to llvmbot/llvm-project that referenced this issue Jan 14, 2025
Extend the special logic for finding `zstd.dll` in `Findzstd` to apply
to all MSVC-compatible configurations such as Clang targeting MSVC.

Fixes llvm#121345

(cherry picked from commit 62d0aff)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmake Build system in general and CMake in particular platform:windows release:backport
Projects
Development

Successfully merging a pull request may close this issue.

2 participants