This is an archive of the discontinued LLVM Phabricator instance.

[cmake] Fix Findzstd.cmake to handle OpenBSD shared libraries
ClosedPublic

Authored by mgorny on Nov 19 2022, 11:59 AM.

Details

Summary

Fix Findzstd CMake to handle shared libraries on OpenBSD correctly.
This userland does not use shared library symlinks without SOVERSION,
so the result of find_library() does not ever end with
zstd_SHARED_LIBRARY_SUFFIX. To work around this, reverse the logic
to compare the result against zstd_STATIC_LIBRARY_SUFFIX and assume
shared library otherwise.

While at it, fix the conditions not to fall back to "result is static
library" path if it actually was recognized as a shared library but
zstd_shared target already existed.

Fixes #59056

Diff Detail

Event Timeline

mgorny created this revision.Nov 19 2022, 11:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 19 2022, 11:59 AM
mgorny requested review of this revision.Nov 19 2022, 11:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 19 2022, 11:59 AM

@phosek, could you test whether this doesn't break MSVC?

I don't know much of the cmake mechanism, so I can only provide tests on Debian testing. I'll leave it to @brad and @phosek
This patch works with the libzstd-dev package.

% grep -i ZSTD /tmp/out/custom2/CMakeCache.txt
//Use zstd for compression/decompression if available. Can be ON,
LLVM_ENABLE_ZSTD:STRING=ON
//Use static version of zstd. Can be TRUE, FALSE
LLVM_USE_STATIC_ZSTD:BOOL=FALSE
zstd_INCLUDE_DIR:PATH=/usr/include
zstd_LIBRARY:FILEPATH=/usr/lib/x86_64-linux-gnu/libzstd.so
zstd_STATIC_LIBRARY:FILEPATH=/usr/lib/x86_64-linux-gnu/libzstd.a
//ADVANCED property for variable: zstd_INCLUDE_DIR
zstd_INCLUDE_DIR-ADVANCED:INTERNAL=1
//ADVANCED property for variable: zstd_LIBRARY
zstd_LIBRARY-ADVANCED:INTERNAL=1
//ADVANCED property for variable: zstd_STATIC_LIBRARY
zstd_STATIC_LIBRARY-ADVANCED:INTERNAL=1

# with a -DCMAKE_PREFIX_PATH=/tmp/p/zstd/out/release build
% grep -i ZSTD /tmp/out/custom2/CMakeCache.txt
CMAKE_PREFIX_PATH:UNINITIALIZED=/tmp/p/zstd/out/release
//Use zstd for compression/decompression if available. Can be ON,
LLVM_ENABLE_ZSTD:STRING=ON
//Use static version of zstd. Can be TRUE, FALSE
LLVM_USE_STATIC_ZSTD:BOOL=FALSE
zstd_INCLUDE_DIR:PATH=/usr/include
zstd_LIBRARY:FILEPATH=/tmp/p/zstd/out/release/lib/libzstd.so
zstd_STATIC_LIBRARY:FILEPATH=/tmp/p/zstd/out/release/lib/libzstd.a
//ADVANCED property for variable: zstd_INCLUDE_DIR
zstd_INCLUDE_DIR-ADVANCED:INTERNAL=1
//ADVANCED property for variable: zstd_LIBRARY
zstd_LIBRARY-ADVANCED:INTERNAL=1
//ADVANCED property for variable: zstd_STATIC_LIBRARY
zstd_STATIC_LIBRARY-ADVANCED:INTERNAL=1
brad added a comment.EditedNov 19 2022, 9:17 PM
humpty$ grep -i zstd CMakeCache.txt
//Use zstd for compression/decompression if available. Can be ON,
LLVM_ENABLE_ZSTD:STRING=ON
//Use static version of zstd. Can be TRUE, FALSE
LLVM_USE_STATIC_ZSTD:BOOL=FALSE
zstd_INCLUDE_DIR:PATH=/usr/local/include
zstd_LIBRARY:FILEPATH=/usr/local/lib/libzstd.so.6.1
zstd_STATIC_LIBRARY:FILEPATH=/usr/local/lib/libzstd.a
//ADVANCED property for variable: zstd_INCLUDE_DIR
zstd_INCLUDE_DIR-ADVANCED:INTERNAL=1
//ADVANCED property for variable: zstd_LIBRARY
zstd_LIBRARY-ADVANCED:INTERNAL=1
//ADVANCED property for variable: zstd_STATIC_LIBRARY
zstd_STATIC_LIBRARY-ADVANCED:INTERNAL=1

No error with the patch applied. Looking at build.ninja it appears to be linking against the zstd shared library. I am running a build.

phosek added inline comments.Nov 20 2022, 1:48 AM
llvm/cmake/modules/Findzstd.cmake
53–74

This could be combined to an elseif block.

54–60

I'd consider combining this if block with the if block above.

mgorny updated this revision to Diff 476745.Nov 20 2022, 7:34 AM

Combine else+if into elseif.

mgorny added inline comments.Nov 20 2022, 7:36 AM
llvm/cmake/modules/Findzstd.cmake
54–60

Do you have anything specific in mind? What we do here depends on the branch taken in the above if but it needs to happen for both branches, i.e. if zstd_LIBRARY was a static lib, then we use that, otherwise we rely on separate static lib lookup result.

brad added a comment.Nov 20 2022, 6:33 PM

Combine else+if into elseif.

This appears to be good.

@phosek, very gentle ping ;-).

MaskRay accepted this revision.Nov 22 2022, 5:09 PM
This revision is now accepted and ready to land.Nov 22 2022, 5:09 PM
phosek accepted this revision.Nov 22 2022, 5:22 PM

LGTM

llvm/cmake/modules/Findzstd.cmake
54–60

I misread the condition, thanks for clarifying.