Page MenuHomePhabricator

[CMake] Provide Findzstd module
ClosedPublic

Authored by phosek on Sep 30 2022, 1:36 PM.

Details

Summary

This module is used to find the system zstd library. The imported
targets intentionally use the same name as the generate zstd config
CMake file so these can be used interchangeably.

Diff Detail

Event Timeline

phosek created this revision.Sep 30 2022, 1:36 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 30 2022, 1:36 PM
Herald added a subscriber: StephenFan. · View Herald Transcript
phosek requested review of this revision.Sep 30 2022, 1:36 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 30 2022, 1:36 PM

Thanks for adding the support! However, I get such a warning and my built ninja lld does not link against libzstd...

CMake Warning (dev) at /usr/share/cmake-3.24/Modules/FindPackageHandleStandardArgs.cmake:438 (message):
  The package name passed to `find_package_handle_standard_args` (ZSTD) does
  not match the name of the calling package (zstd).  This can lead to
  problems in calling code that expects `find_package` result variables
  (e.g., `_FOUND`) to follow a certain pattern.
Call Stack (most recent call first):
  cmake/modules/Findzstd.cmake:16 (find_package_handle_standard_args)
  cmake/config-ix.cmake:149 (find_package)
  CMakeLists.txt:843 (include)
This warning is for project developers.  Use -Wno-dev to suppress it.

Thanks a lot for doing this. Please consider the more fancy suggestions non-obligatory, I think just find_package_handle_standard_args needs to be fixed.

By the way, you may also look at FindLibEdit.cmake on how to grab directory hints from pkg-config.

However, if you don't feel like doing all the extra things, I can look into making a followup patches for them.

llvm/cmake/modules/Findzstd.cmake
14

Well, I think this is good enough for Gentoo since we hate static libraries but given all the fancy logic in LLVM you may want to try finding both shared and static. Thinking about it, you could issue a second find_library call like:

find_library(zstd_STATIC_LIBRARY_NAMES libzstd.a)
18

As @MaskRay pointed out, you probably need to make the zstd lowercase. I'd also make all the other ZSTD_* vars lowercase for consistency but not sure if that's necessary.

mgorny added inline comments.Oct 2 2022, 8:46 AM
llvm/cmake/modules/Findzstd.cmake
23

Pointed out by Rolf Eike Beer, the man with the coolest name in Gentoo ;-).

30
mgorny added a comment.Oct 4 2022, 5:21 AM

I'm a little impatient and need a distraction, so I hope you don't mind if I try to finish your patch.

mgorny added a comment.Oct 4 2022, 6:06 AM

Submitted my version as D135153.

I apologize about the delay, I implemented the suggested changes, but then I decided to also test this module on Windows and found out that more changes are going to be needed and haven't had time to implement those. I hope to find some time today or tomorrow.

phosek updated this revision to Diff 465320.Oct 5 2022, 1:43 AM
phosek marked 3 inline comments as done.
mgorny accepted this revision.Oct 5 2022, 2:57 AM

I think it's good enough for us. I wish you'd have implemented the improvements from my version but I can work on top of this.

This revision is now accepted and ready to land.Oct 5 2022, 2:57 AM
This revision was landed with ongoing or failed builds.Oct 6 2022, 1:23 AM
This revision was automatically updated to reflect the committed changes.
phosek added a comment.Oct 6 2022, 1:26 AM

I think it's good enough for us. I wish you'd have implemented the improvements from my version but I can work on top of this.

I'd prefer adding functionality such as the pkg-config support in a follow up change. We would need to ensure it works correctly on systems that don't have pkg-config like Windows. I'd also prefer doing this consistently across all Find*.cmake modules; we currently don't support pkg-config in any of them.

nikic added a subscriber: nikic.Oct 6 2022, 3:05 AM

I've reverted this patch because it breaks running LLVM tests with:

llvm-lit: /home/npopov/repos/llvm-project/llvm/utils/lit/lit/TestingConfig.py:138: fatal: unable to parse config file '/home/npopov/repos/llvm-project/build/test/lit.site.cfg.py', traceback: Traceback (most recent call last):
  File "/home/npopov/repos/llvm-project/llvm/utils/lit/lit/TestingConfig.py", line 127, in load_from_path
    exec(compile(data, path, 'exec'), cfg_globals, None)
  File "/home/npopov/repos/llvm-project/build/test/lit.site.cfg.py", line 48, in <module>
    config.have_zstd = FALSE
NameError: name 'FALSE' is not defined

I expect this is just a matter of a missing llvm_canonicalize_cmake_booleans entry, but it's not completely obvious to me how this is related to this patch.

mgorny added a comment.Oct 6 2022, 5:45 AM

I expect this is just a matter of a missing llvm_canonicalize_cmake_booleans entry, but it's not completely obvious to me how this is related to this patch.

I've noticed this earlier today but couldn't submit patch then. Now D135357.

mgorny reopened this revision.Oct 6 2022, 8:07 AM

My fix has landed, so I suppose you can try again.

This revision is now accepted and ready to land.Oct 6 2022, 8:07 AM
This revision was automatically updated to reflect the committed changes.

Thanks to both of you! A regular build of clang/lld/etc now use zstd.