-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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-core] bring package up to date. Support for Conan 2. #22997
[llvm-core] bring package up to date. Support for Conan 2. #22997
Conversation
🤖 Beep Boop! This pull request is making changes to 'recipes/llvm-core//'. 👋 @Hopobcn @paulharris you might be interested. 😉 |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
b8f48e8
to
471320c
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
50c4543
to
e45b3bc
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@jcar87 Most recent CI build failed but i'm unable to view any logs. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
||
components = defaultdict(list) | ||
for lib, dep in deps: | ||
if not lib.startswith('LLVM'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this needs to be kept. Without something like this, I’m seeing shared libraries (libRemarks.so
and libLTO.so
) in the produced package even when shared=False
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might also want to delete *.so
and *.so.*
from the package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
libRemarks and libLTO are part of the LLVM infrastructure and not available as static libraries.
libLTO specifically is used by some out of tree linkers but not by LLVM itself - See https://discourse.llvm.org/t/how-to-use-dynamic-library-liblto-so/74541 for example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand these are not available statically. However I don’t think it’s correct to include shared libs in a static package. For example, if my intention of using a static package is to produce an executable that doesn’t depend on any shared libs for ease of deployment, now I’m forced to link to these 2 shared libs when I link to llvm-core::llvm::core
, and the executable will not run in the target environment as intended.
I think we should delete them, and the users of the static package should simply be aware that these 2 components are not available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
llvm-core::llvm-core
is a conan-generated target and would not ordinarily be available to consumers of LLVM, and I would expect users of the package to use the targets in the official docs, as the test package does.
One issue with deleting the libraries for static builds is that a shared library build is not supported (by LLVM) for Windows, so a static build is the only way to build those libraries. Deleting the libraries for all platforms apart from Windows seems to me inconsistent behaviour.
It's not a perfect solution - but currently the package works exactly as described in the official LLVM documentation. I'm sure further contributions to the recipe to improve the linking behaviour will be welcome.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
llvm-core::llvm-core
is a conan-generated target and would not ordinarily be available to consumers of LLVM
By “not available” do you just mean “not recommended”? Because I certainly have code using it, and it works fine (except for linking to the 2 shared libs).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean it is a target provided by conan, not by LLVM. So if you are linking to llvm-core::llvm-core
, your application will work with the conan package but it would not work against a package built directly from the LLVM sources just using cmake.
The test_package in the repository should still work whether LLVM is provided by conan or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do think that LTO
and Remarks
should probably be made separate components for the shared build, but IMHO this is something that can be added as a future improvement and shouldn't block this PR.
Conan v1 pipeline ✔️All green in build 43 (
Conan v2 pipeline ✔️
All green in build 44 ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @planetmarshall and everyone involved - this was not an easy recipe to port, and hopefully this can unblock some of the usages that are required by the community.
There's still a few things to look into further down the line - the issue with libiconv in macOS, and the parsing of CMake files (it is obvious from the recipe code that this can be fragile) - however, I dont think these are an impediment to merge the recipe as proposed in this PR.
Thanks so much!
Specify library name and version: llvm-core/*
Attempt to bring the llvm-core recipe up to date. Includes contributions from @jusito and #17509. Fixes #20339
Note that I have opted not to add the latest version (18.1.3) as part of this PR, as the existing recipe needed updating and is already quite complex. I (or someone else) will add the latest version as a followup.