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

[llvm-config] Make llvm-config --system-libs obey LLVM_USE_STATIC_ZSTD. #93754

Merged
merged 1 commit into from
Jun 26, 2024

Conversation

khuey
Copy link
Contributor

@khuey khuey commented May 30, 2024

LLVM's build system does the right thing but LLVM_SYSTEM_LIBS ends up containing the shared library. Emit the static library instead when appropriate.

CC @MaskRay since zstd compression is relevant to his interests.

@llvmbot
Copy link
Member

llvmbot commented May 30, 2024

@llvm/pr-subscribers-llvm-support

Author: Kyle Huey (khuey)

Changes

LLVM's build system does the right thing but LLVM_SYSTEM_LIBS ends up containing the shared library. Emit the static library instead when appropriate.

CC @MaskRay since zstd compression is relevant to his interests.


Full diff: https://github.com/llvm/llvm-project/pull/93754.diff

1 Files Affected:

  • (modified) llvm/lib/Support/CMakeLists.txt (+6-2)
diff --git a/llvm/lib/Support/CMakeLists.txt b/llvm/lib/Support/CMakeLists.txt
index 03e888958a071..1ab5bd370c480 100644
--- a/llvm/lib/Support/CMakeLists.txt
+++ b/llvm/lib/Support/CMakeLists.txt
@@ -321,8 +321,12 @@ if(LLVM_ENABLE_ZSTD)
   if(NOT zstd_library)
     get_property(zstd_library TARGET ${zstd_target} PROPERTY LOCATION)
   endif()
-  get_library_name(${zstd_library} zstd_library)
-  set(llvm_system_libs ${llvm_system_libs} "${zstd_library}")
+  if (zstd_target STREQUAL zstd::libzstd_shared)
+    get_library_name(${zstd_library} zstd_library)
+    set(llvm_system_libs ${llvm_system_libs} "${zstd_library}")
+  else()
+    set(llvm_system_libs ${llvm_system_libs} "${zstd_STATIC_LIBRARY}")
+  endif()
 endif()
 
 if(LLVM_ENABLE_TERMINFO)

@khuey
Copy link
Contributor Author

khuey commented Jun 26, 2024

Bump. @MaskRay if you don't have time to look at this yourself can you at least refer it to someone who does? I can't set reviewers on this PR.

@MaskRay
Copy link
Member

MaskRay commented Jun 26, 2024

Sorry. I receive a lot of review requests. When I am not added as a reviewer, I can easily miss a PR that requires my attention... Weekly ping is fine.

LLVM's build system does the right thing but LLVM_SYSTEM_LIBS ends up containing the shared library. Emit the static library instead when appropriate.

Can you edit the description to show llvm-config --system-libs output differences? This would help folks who are less familiar with CMake (like me).

I checked that this patch does change the output of
llvm-config --system-libs

@MaskRay MaskRay added the cmake Build system in general and CMake in particular label Jun 26, 2024
@MaskRay MaskRay requested a review from mgorny June 26, 2024 02:05
Copy link
Member

@mgorny mgorny left a comment

Choose a reason for hiding this comment

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

I don't see anything wrong with it, though I really hate that we have to do things like that.

LLVM's build system does the right thing but LLVM_SYSTEM_LIBS ends up
containing the shared library. Emit the static library instead when
appropriate.

With LLVM_USE_STATIC_ZSTD, before:

khuey@zhadum:~/dev/llvm-project/build$ ./bin/llvm-config --system-libs
-lrt -ldl -lm -lz -lzstd -lxml2

after:

khuey@zhadum:~/dev/llvm-project/build$ ./bin/llvm-config --system-libs
-lrt -ldl -lm -lz /usr/local/lib/libzstd.a -lxml2
@khuey
Copy link
Contributor Author

khuey commented Jun 26, 2024

Rebased and updated the commit message per @MaskRay's request.

@MaskRay MaskRay merged commit 0f24a46 into llvm:main Jun 26, 2024
5 of 6 checks passed
@MaskRay
Copy link
Member

MaskRay commented Jun 26, 2024

Rebased and updated the commit message per @MaskRay's request.

Thx.

GitHub "Squash and merge" populates the default commit message with the content of the first comment.
So you just need to edit the description. The commit messages are ignored.

khuey added a commit to khuey/llvm-project that referenced this pull request Jun 26, 2024
llvm#93754)

LLVM's build system does the right thing but LLVM_SYSTEM_LIBS ends up
containing the shared library. Emit the static library instead when
appropriate.

With LLVM_USE_STATIC_ZSTD, before:

khuey@zhadum:~/dev/llvm-project/build$ ./bin/llvm-config --system-libs
-lrt -ldl -lm -lz -lzstd -lxml2

after:

khuey@zhadum:~/dev/llvm-project/build$ ./bin/llvm-config --system-libs
-lrt -ldl -lm -lz /usr/local/lib/libzstd.a -lxml2

(cherry-picked from 0f24a46)
nikic pushed a commit to rust-lang/llvm-project that referenced this pull request Jun 30, 2024
llvm#93754)

LLVM's build system does the right thing but LLVM_SYSTEM_LIBS ends up
containing the shared library. Emit the static library instead when
appropriate.

With LLVM_USE_STATIC_ZSTD, before:

khuey@zhadum:~/dev/llvm-project/build$ ./bin/llvm-config --system-libs
-lrt -ldl -lm -lz -lzstd -lxml2

after:

khuey@zhadum:~/dev/llvm-project/build$ ./bin/llvm-config --system-libs
-lrt -ldl -lm -lz /usr/local/lib/libzstd.a -lxml2

(cherry-picked from 0f24a46)
lravenclaw pushed a commit to lravenclaw/llvm-project that referenced this pull request Jul 3, 2024
llvm#93754)

LLVM's build system does the right thing but LLVM_SYSTEM_LIBS ends up
containing the shared library. Emit the static library instead when
appropriate.

With LLVM_USE_STATIC_ZSTD, before:

khuey@zhadum:~/dev/llvm-project/build$ ./bin/llvm-config --system-libs
-lrt -ldl -lm -lz -lzstd -lxml2

after:

khuey@zhadum:~/dev/llvm-project/build$ ./bin/llvm-config --system-libs
-lrt -ldl -lm -lz /usr/local/lib/libzstd.a -lxml2
AlexisPerry pushed a commit to llvm-project-tlp/llvm-project that referenced this pull request Jul 9, 2024
llvm#93754)

LLVM's build system does the right thing but LLVM_SYSTEM_LIBS ends up
containing the shared library. Emit the static library instead when
appropriate.

With LLVM_USE_STATIC_ZSTD, before:

khuey@zhadum:~/dev/llvm-project/build$ ./bin/llvm-config --system-libs
-lrt -ldl -lm -lz -lzstd -lxml2

after:

khuey@zhadum:~/dev/llvm-project/build$ ./bin/llvm-config --system-libs
-lrt -ldl -lm -lz /usr/local/lib/libzstd.a -lxml2
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 llvm:support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants