-
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!
…nan 2. * [llvm-core] bring package up to date. Initial support for 13.0.0 * [llvm-core] add support for job pool resource limits from @jusito * [llvm-core] add cmake patch for 12.0.0 * [llvm-core] add cmake patch for 11.1.0 * [llvm-core] allow conan 1.x * [llvm-core] fix zlib detection for 11.1.0 * [llvm-core] additional patches for missing includes and patch metadata * [llvm-core] resolve v1 hooks issues * [llvm-core] use llvm generated config file to extract components * [llvm-core] add custom cmake build script * [llvm-core] use run env during build and force ninja * [llvm-core] fix patches for libxml * [llvm-core] bump up default ram per link job * [llvm-core] backport split-dwarf patches from 14.x * [llvm-core] add support for cross compiling * [llvm-core] fix patch files for split dwarf * [llvm-core] disable cross compilation * [llvm-core] add CCI build service specific behaviour * [llvm-core] fix cci resource limitations * [llvm-core] implement component dependencies * [llvm-core] sanitize component names * [llvm-core] bump ram per link job for cci shared debug * [llvm-core] exclude cmake find modules * [llvm-core] skip rpath on macos * [llvm-core] reformat and fix linting issues * [llvm-core] delete 18x patch (added in error) * [llvm-core] rename references to LLVM-Config * [llvm-core] retain LLVMConfigExtensions.cmake * [llvm-core] add fake targets for *-gen * [llvm-core] use the actual cmake target name as component name * [llvm-core] fix component dependencies * [llvm-core] add target options * [llvm-core] add editline requirement (WIP) * [llvm-core] support for libedit (editline) in 13.x * [llvm-core] disable shared debug for CCI, update license identifier and refactor * [llvm-core] require cxx std 14 for test package * [llvm-core] handle edge case of static libiconv being linked into a dynamic library on macos * [llvm-core] skip rpath handling for build on macos * [llvm-core] propagate DYLD_LIBRARY_PATH to tblgen during build * [llvm-core] propagate DYLD_LIBRARY_PATH to tblgen during build * [llvm-core] do not build shared libs with the install rpath * [llvm-core] use original test package * [llvm-core] do not package static libraries with shared build * [llvm-core] fix test_package * [llvm-core] support linking in lib for native codegen * [llvm-core] add windows support * [llvm-core] remove pdb files * [llvm-core] require cmake_path support in test package * [llvm-core] handle legacy conan msvcrt setting * [llvm-core] missing patch type * Apply suggestions from code review Co-authored-by: Michael Keck <[email protected]> * [llvm-core] revert changes to license * [llvm-core] apply review suggestions * [llvm-core] add patch source for include file fix * [llvm-core] use user-config for compiler resource settings * [llvm-core] add link for further info on cross compilation * [llvm-core] use PurePosixPath only * Trying 'as_posix()' function * More 'as_posix()' function * Typo * [llvm-core] revert usage of 'load' to workaround regex error * [llvm-core] try relaxing restrictions on cross-building * [llvm-core] use semi-colon separated string for targets instead of individual options * [llvm-core] set default fPIC fallback to 'True' * [llvm-core] use version range for ninja * [llvm-core] move cci check to 'validate_build' * [llvm-core] allow CCI to use llvm-core user config for resource limits * [llvm-core] dont activate build environment for llvm-tblgen * [llvm-core] add patch for link failure with lld * [llvm-core] explicitly list shared libraries * [llvm-core] revert to using * Revert "[llvm-core] try relaxing restrictions on cross-building" This reverts commit 7b2e085 * [llvm-core] workaround for conan-io#13560 * [llvm-core] increase ram available for link jobs in CCI * [llvm-core] remove the split-dwarf patch * [llvm-core] disable shared debug builds entirely and debug builds for linux * [llvm-core] remove remaining split-dwarf capability --------- Co-authored-by: Michael Keck <[email protected]> Co-authored-by: Francisco Ramirez de Anton <[email protected]>
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.