[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.
planetmarshall
force-pushed
the
llvm-core-conan2
branch
3 times, most recently
from
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.
planetmarshall
force-pushed
the
llvm-core-conan2
branch
from
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.
planetmarshall
changed the title
|
I detected other pull requests that are modifying llvm-core/all recipe: This message is automatically generated by https://github.com/ericLemanissier/conan-center-conflicting-prs so don't hesitate to report issues/improvements there. |
This comment has been minimized.
This comment has been minimized.
|
@RubenRBS The build seems to have been stuck in limbo for a while. Are there still CI issues? Thanks. |
This comment has been minimized.
This comment has been minimized.
|
@planetmarshall sorry about that - llvm takes quite a long time to compile - I see that the compilation is now done and the results posted and they are passing, yay!! I'll get a review going for monday, thanks a lot for your patience and for the contribution in the first place, we really aprpeciate it :) |
Thanks, I also appreciate the efforts of the CCI team. |
|
@jusito possible to review this week? I opted not to use llvm-core and your PR instead (llvm-16) but am eagerly waiting on this PR and can help update/test/review the later PRs. @jcar87 It would be a big plus to be able to simply pull down transparent/conan-compiled compilers in Dockerfiles e.g. https://github.com/conan-io/conan-docker-tools/blob/master/modern/clang/Dockerfile |
recipes/llvm-core/all/conanfile.py
Outdated
| url = 'https://github.com/conan-io/conan-center-index' | ||
|
|
||
| settings = ('os', 'arch', 'compiler', 'build_type') | ||
| license = "LLVM-exception" |
There was a problem hiding this comment.
This license change is wrong. Please undo
There was a problem hiding this comment.
Are you sure? This is the SPDX identifier for the LLVM license, according to https://spdx.org/licenses/LLVM-exception.html, and according to the conan docs, "..it is strongly recommended that recipes of Open Source projects use SPDX identifiers from the SPDX license list"
There was a problem hiding this comment.
The existing license was correct. See https://spdx.org/licenses/exceptions-index.html
The SPDX License List includes a list of exceptions. These exceptions grant an exception to a license condition or additional permissions beyond those granted in a license; they are not stand-alone licenses. Exceptions are added to a license using the License Expression operator, "WITH".
There was a problem hiding this comment.
What you linked is just an exception identifier not a license identifier. @planetmarshall
There was a problem hiding this comment.
Reverted - thanks for the clarification.
| # Older cmake versions may have issues generating the graphviz output used | ||
| # to model the components | ||
| build_requires = [ | ||
| 'cmake/3.20.5' |
There was a problem hiding this comment.
Why is it not required anymore?
There was a problem hiding this comment.
Because graphviz is not used to model component dependencies, as noted in the comment.
There was a problem hiding this comment.
To clarify further - the original recipe used graphviz output which was then parsed and sanitized to generate component dependencies. Instead of doing this I opted to just use the information that LLVM already outputs in its CMake files, I thought this was more robust since the best source of cmake dependencies is cmake itself rather than going via graphviz and back again.
| 'DataFlow', | ||
| 'Address;Undefined', | ||
| 'None' | ||
| "shared": [True, False], |
There was a problem hiding this comment.
This pull request is a nightmare to review due to size and patches, you really shouldn't have made formatting changes in such a pull request 🙈
There was a problem hiding this comment.
Sorry that it's been a "nightmare" for you, but the recipe was unmaintained and did not build with conan2. Now it does.
There was a problem hiding this comment.
Thank you for your work! Didn't mean to sound ungrateful or something 🙂
Co-authored-by: Michael Keck <git@cr0ydon.com>
This comment has been minimized.
This comment has been minimized.
Conan v1 pipeline ✔️All green in build 28 (
Conan v2 pipeline ✔️
All green in build 29 ( |
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.