Page MenuHomePhabricator

use LLVM_USE_STATIC_ZSTD
ClosedPublic

Authored by ckissane on Sep 2 2022, 12:03 PM.

Details

Summary

removes LLVM_PREFER_STATIC_ZSTD in favor of using a LLVM_USE_STATIC_ZSTD

Diff Detail

Event Timeline

ckissane created this revision.Sep 2 2022, 12:03 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 2 2022, 12:03 PM
ckissane requested review of this revision.Sep 2 2022, 12:03 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 2 2022, 12:03 PM
phosek added a comment.Sep 2 2022, 1:12 PM

I'd prefer LLVM_USE_STATIC_ZSTD name I suggested in D132870. <PROJECT>_USE_STATIC_<DEPENDENCY> is a common convention we already use elsewhere in LLVM. I'd prefer following the existing conventions rather than inventing a new approach for the sake of maintainability.

ckissane updated this revision to Diff 457687.Sep 2 2022, 1:28 PM
  • change to LLVM_USE_STATIC_ZSTD (default FALSE)
ckissane retitled this revision from use LLVM_ZSTD_PREFERENCE_LIST in favor of LLVM_PREFER_STATIC_ZSTD to use LLVM_ZSTD_PREFERENCE_LIST in favor of LLVM_USE_STATIC_ZSTD.Sep 2 2022, 1:28 PM
ckissane edited the summary of this revision. (Show Details)
ckissane retitled this revision from use LLVM_ZSTD_PREFERENCE_LIST in favor of LLVM_USE_STATIC_ZSTD to use LLVM_USE_STATIC_ZSTD.
phosek added inline comments.Sep 2 2022, 1:31 PM
llvm/lib/Support/CMakeLists.txt
27–28

I'd delete this line altogether. In CMake, you don't need to define variables before you use them and there's no suitable default value here.

ckissane updated this revision to Diff 457692.Sep 2 2022, 1:38 PM
ckissane edited the summary of this revision. (Show Details)
  • delete line
phosek accepted this revision.Sep 2 2022, 1:44 PM

LGTM

This revision is now accepted and ready to land.Sep 2 2022, 1:44 PM
This revision was landed with ongoing or failed builds.Sep 2 2022, 2:00 PM
This revision was automatically updated to reflect the committed changes.
tstellar added a subscriber: tstellar.EditedOct 28 2022, 10:33 AM

This change broke the libclc CI job we have for the release branch on MacOS. It looks like the root cause is a change in the output of llvm-config:

Before: -lm;-lz;/usr/local/lib/libzstd.1.5.2.dylib;-lcurses;-lxml2
After: -lm;-lz;-lzstd;-lcurses;-lxml2