-
-
Notifications
You must be signed in to change notification settings - Fork 43
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
find_package(LLVM)
fails to find shared libztd and errors out on Windows, when using Clang
#306
Comments
Filed llvm/llvm-project#121345. |
|
That doesn't seem to be the case. It is possible that I've messed something up but I've just set Visual Studio 2022, and Conda environment with Clang. Clang reports https://cmake.org/cmake/help/latest/variable/MSVC.html#variable:MSVC says:
And indeed, if I set |
And indeed, if I replace |
I've proposed llvm/llvm-project#121437 to do that. https://dev.azure.com/conda-forge/feedstock-builds/_build/results?buildId=1129626&view=logs&j=535b614b-9e28-51ef-eb67-d0aa83d01d61&t=c0e5205c-8699-555e-ce93-e93093f402f2 seems to confirm that it fixed the issue in question (note the |
|
Thanks, I was wondering if MinGW could be affected — but this is definitely cleaner than trying to maintain an opt-out list of compilers or guessing based on static library suffix. |
Instead of unconditionally assuming MSVC-alike behavior for WIN32 that could negatively impact MSVC, use `CMAKE_CXX_SIMULATE_ID` to detect Clang using MSVC ABI. Note that this variable is unset for MSVC itself, so use it alternatively to MSVC. Thanks to @isuruf for the suggestion (conda-forge/llvmdev-feedstock#306 (comment)).
@mgorny, are you still planning to push the backport for llvm/llvm-project#121345 to llvm 19? It shouldn't be hard to fix the cherry-pick (I guess) - I could also give it a shot if you want. 19.1.7 is scheduled in about a week, so not a huge amount of time left (given that all backports need approval). |
Yes, I would. I think the merge conflict results from the change I've requested backporting in llvm/llvm-project#121410. I was hoping there would be some gap between the pull requests being merged and the release that would let me request another backport PR once that one's merged. Or perhaps I should close that and open a single PR with both changes instead. |
Okay, found out how to make the bot take both commits: llvm/llvm-project#121755 now. |
If you only want to backport one commit that doesn't cherry-pick cleanly, you can fix it up manually, push a branch, and then tell the backport bot about it. |
Yeah, but we need both, and one would conflict with the other either way. |
Solution to issue cannot be found in the documentation.
Issue
When trying to get triton-feedstock working on Windows, with Clang used as the compiler, CMake would fail with:
Full Azure log: 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)):
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
(andlibzstd.lib
would too) triggers the second clause in thezstd_FOUND
condition, causing it to perform explicit lookup forbin/zstd.dll
. On the other hand, when using Clang, it just treatslibzstd.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, since LLVM in Conda is built with MSVC, it uses the shared zstd library and includes references to the dependency on this library in the installed CMake files. However, these files don't include the zstd library target itself — instead, they're triggering another lookup in reverse dependencies. Since the lookup is now performed with Clang and it doesn't find the shared library, the dependencies are unsatisfied.
And perhaps the worst part, it is possible that I've actually broken it there. I specifically see the shared library logic for MSVC has been added in llvm/llvm-project@e7fc754 — perhaps the condition there should check for Windows rather than MSVC. But it's outside my expertise to change that.
Installed packages
Environment info
The text was updated successfully, but these errors were encountered: