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] Extend zstd.dll finding logic from MSVC to WIN32 in general #121437

Merged
merged 2 commits into from
Jan 2, 2025

Conversation

mgorny
Copy link
Member

@mgorny mgorny commented 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 #121345

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
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 mgorny requested a review from MaskRay January 2, 2025 10:32
@mgorny
Copy link
Member Author

mgorny commented Jan 2, 2025

Updated to avoid breaking MinGW.

Copy link
Collaborator

@nga888 nga888 left a comment

Choose a reason for hiding this comment

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

LGTM.

@mgorny mgorny merged commit 62d0aff into llvm:main Jan 2, 2025
8 checks passed
@mgorny mgorny deleted the cmake-findzstd-win branch January 2, 2025 15:43
@llvm-ci
Copy link
Collaborator

llvm-ci commented Jan 2, 2025

LLVM Buildbot has detected a new failure on builder lld-x86_64-win running on as-worker-93 while building llvm at step 7 "test-build-unified-tree-check-all".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/146/builds/1977

Here is the relevant piece of the build log for the reference
Step 7 (test-build-unified-tree-check-all) failure: test (failure)
******************** TEST 'LLVM-Unit :: Support/./SupportTests.exe/38/87' FAILED ********************
Script(shard):
--
GTEST_OUTPUT=json:C:\a\lld-x86_64-win\build\unittests\Support\.\SupportTests.exe-LLVM-Unit-24204-38-87.json GTEST_SHUFFLE=0 GTEST_TOTAL_SHARDS=87 GTEST_SHARD_INDEX=38 C:\a\lld-x86_64-win\build\unittests\Support\.\SupportTests.exe
--

Script:
--
C:\a\lld-x86_64-win\build\unittests\Support\.\SupportTests.exe --gtest_filter=ProgramEnvTest.CreateProcessLongPath
--
C:\a\lld-x86_64-win\llvm-project\llvm\unittests\Support\ProgramTest.cpp(160): error: Expected equality of these values:
  0
  RC
    Which is: -2

C:\a\lld-x86_64-win\llvm-project\llvm\unittests\Support\ProgramTest.cpp(163): error: fs::remove(Twine(LongPath)): did not return errc::success.
error number: 13
error message: permission denied



C:\a\lld-x86_64-win\llvm-project\llvm\unittests\Support\ProgramTest.cpp:160
Expected equality of these values:
  0
  RC
    Which is: -2

C:\a\lld-x86_64-win\llvm-project\llvm\unittests\Support\ProgramTest.cpp:163
fs::remove(Twine(LongPath)): did not return errc::success.
error number: 13
error message: permission denied




********************


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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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