Skip to content

[CMake] Only export the LLVM_LINK_LLVM_DYLIB setting if not yet set #135570

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

guitargeek
Copy link

Follows up on these two commits:

  • 3b8b3c2 [CMake] Export the LLVM_LINK_LLVM_DYLIB setting
  • 5fed24a [CMake] Followup for r337366: Only export LLVM_LINK_LLVM_DYLIB if it's set to ON

Commit 3b8b3c2 introduced the export of the LLVM_LINK_LLVM_DYLIB setting, so that clients can check whether the option of dynamic linking is available. However, it left no way for the client to actually set LLVM_LINK_LLVM_DYLIB themselves.

Commit 5fed24a addressed the problem that out-of-tree clients lost the ability to link against the dylib, even if in-tree tools did not.

There is one remaining problem: if LLVM_LINK_LLVM_DYLIB is supported there is no way to configure clients to not link against llvm dynamically anymore. Naively, one would be able to do this for Clang e.g. with -DLLVM_LINK_LLVM_DYLIB=OFF.

This commit suggests to fix this problem by only setting the DYLIB option if it was not already set before.

Follows up on these two commits:

  * 3b8b3c2 [CMake] Export the LLVM_LINK_LLVM_DYLIB setting
  * 5fed24a [CMake] Followup for r337366:
                 Only export LLVM_LINK_LLVM_DYLIB if it's set to ON

Commit 3b8b3c2 introduced the export of the `LLVM_LINK_LLVM_DYLIB`
setting, so that clients can check whether the option of dynamic linking
is available. However, it left no way for the client to actually set
`LLVM_LINK_LLVM_DYLIB` themselves.

Commit 5fed24a addressed the problem that out-of-tree clients lost
the ability to link against the dylib, even if in-tree tools did not.

There is one remaining problem: if LLVM_LINK_LLVM_DYLIB is supported
there is no way to configure clients to not link against llvm
dynamically anymore. Naively, one would be able to do this for Clang
e.g. with `-DLLVM_LINK_LLVM_DYLIB=OFF`.

This commit suggests to fix this problem by only setting the DYLIB
option if it was not already set before.
Copy link

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

@hahnjo
Copy link
Member

hahnjo commented Apr 14, 2025

Note that downstream users can choose by specifying DISABLE_LLVM_LINK_LLVM_DYLIB in their CMake

@guitargeek
Copy link
Author

guitargeek commented Apr 14, 2025

Yes that's true, but if you want to make that configurable, the downstream package (in this case e. g. standalone Clang) would need another flag so you can steer that behavior when building the downstream package. Of course that's also an option, but I think making this change in LLVM is more sustainable because Clang is not the only downstream package affected by this

@guitargeek
Copy link
Author

Note that downstream users can choose by specifying DISABLE_LLVM_LINK_LLVM_DYLIB in their CMake

Maybe to clarify possible confusion from my previous comment: the DISABLE_LLVM_LINK_LLVM_DYLIB argument is not a configuration option for building downstream packages, but a flag that you can set in llvm_add_library: https://github.com/llvm/llvm-project/blob/main/llvm/cmake/modules/AddLLVM.cmake#L500

So if you want to build e.g. Clang with disabled dynamic linking, doing -DDISABLE_LLVM_LINK_LLVM_DYLIB as you imply would not help because this argument is hardcoded in the llvm_add_library calls.

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.

3 participants