-
Notifications
You must be signed in to change notification settings - Fork 1.9k
llvm-core 19.1.7 #24317
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 19.1.7 #24317
Conversation
This comment has been minimized.
This comment has been minimized.
bda25f1
to
c9a21a3
Compare
This comment has been minimized.
This comment has been minimized.
c9a21a3
to
f3b497b
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.
Hooks produced the following warnings for commit 5d75052llvm-core/19.1.0@#a45fb0c3ae4a7a65121f822e088e1c9f
|
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.
68959cf
to
fb68722
Compare
This comment has been minimized.
This comment has been minimized.
Hooks produced the following warnings for commit fb68722llvm-core/19.1.1@#9a372edc7a3f021f8cb4a83c23f67ad7
|
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.
Conan v1 pipeline ❌Warning Conan Center will stop receiving updates for Conan 1.x packages soon - please see announcement. Failure in build 17 (
Note: To save resources, CI tries to finish as soon as an error is found. For this reason you might find that not all the references have been launched or not all the configurations for a given reference. Also, take into account that we cannot guarantee the order of execution as it depends on CI workload and workers availability. Conan v2 pipeline ❌
The v2 pipeline failed. Please, review the errors and note this is required for pull requests to be merged. In case this recipe is still not ported to Conan 2.x, please, ping Failure in build 17 (
Note: To save resources, CI tries to finish as soon as an error is found. For this reason you might find that not all the references have been launched or not all the configurations for a given reference. Also, take into account that we cannot guarantee the order of execution as it depends on CI workload and workers availability. |
7663daa
to
c235c3e
Compare
c235c3e
to
d936c90
Compare
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.
@planetmarshall Thank you for your PR, updating the llvm-core project.
I see this is not a simple bump version, but has several changes and misses details about the motivation behind these changes. Could please add more details, as this recipe is really complex and hard to maintain? Thank you!
recipes/llvm-core/all/conandata.yml
Outdated
"third-party": | ||
url: https://github.com/llvm/llvm-project/releases/download/llvmorg-19.1.7/third-party-19.1.7.src.tar.xz | ||
sha256: b96deca1d3097c7ffd4ff2bb904a50bdd56bec7ed1413ffb0d1d01af87b72c12 |
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.
"third-party": | |
url: https://github.com/llvm/llvm-project/releases/download/llvmorg-19.1.7/third-party-19.1.7.src.tar.xz | |
sha256: b96deca1d3097c7ffd4ff2bb904a50bdd56bec7ed1413ffb0d1d01af87b72c12 |
What is the case needed third-party? It only contains unittest and benchmark folders. Both are unrelated when consuming or packaging the project in most cases. The benchmark headers can be disabled by using the CMake definitions LLVM_INCLUDE_BENCHMARKS=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.
Removed.
@@ -52,13 +53,75 @@ | |||
"XCore" | |||
} | |||
|
|||
def components_from_dotfile(dotfile): |
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.
Why is it required only now? I saw your feature request in Conan client for Dot (Graphviz), still, this is a bit complex to maintain, so I would avoid it if possible. Boost is a good example of how opaque the recipe becomes after receiving customizations.
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.
See this comment from #22997 for the context behind this decision.
In summary, parsing CMake Graphviz output was how the original (Conan 1.x) LLVM recipe generated component dependencies - I removed this in favour of parsing CMake output, however in the above referenced comment it was suggested that Graphviz was the "least-worst" option, so I've added it back in here.
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.
Could you please add more detailed comments in the recipe, including the method description, and what is happening in the key steps? As I commented before, it not only increases the recipe's complexity but also makes its maintenance harder. More information will be welcome to understand it in future PRs.
recipes/llvm-core/all/conanfile.py
Outdated
@@ -137,6 +211,11 @@ def config_options(self): | |||
if self.settings.os == "Windows": | |||
del self.options.fPIC | |||
del self.options.with_libedit # not supported on windows | |||
if Version(self.version) < 18: |
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.
Do you have a reference about zstd inclusion? Could please add a comment in the recipe, I did not find a reference in https://releases.llvm.org/18.1.0/projects/libcxx/docs/ReleaseNotes/18.html
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.
It's not explicit in the release notes. It's available since LLVM 15 - see https://reviews.llvm.org/D128465
recipes/llvm-core/all/conanfile.py
Outdated
if is_msvc(self): | ||
# https://discourse.llvm.org/t/failing-to-compile-llvm-on-windows-with-clang | ||
cmake_variables["MSVC"] = True | ||
|
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.
if is_msvc(self): | |
# https://discourse.llvm.org/t/failing-to-compile-llvm-on-windows-with-clang | |
cmake_variables["MSVC"] = True |
I would not enforce this native cmake variable. Using clang on Windows has several flavours, msvc is only one case: https://blog.conan.io/2022/10/13/Different-flavors-Clang-compiler-Windows.html
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.
Removed. This was fixed in llvm/llvm-project#95505
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.
@planetmarshall Thank you for keeping this PR, I saw your last changes and CI is passing again. Still, I have some comments.
recipes/llvm-core/all/conanfile.py
Outdated
if Version(self.version) < 18: | ||
get(self, **sources, strip_root=True) | ||
else: | ||
get(self, **sources["llvm"], destination='llvm-main', strip_root=True) | ||
get(self, **sources["cmake"], destination='cmake', strip_root=True) |
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.
if Version(self.version) < 18: | |
get(self, **sources, strip_root=True) | |
else: | |
get(self, **sources["llvm"], destination='llvm-main', strip_root=True) | |
get(self, **sources["cmake"], destination='cmake', strip_root=True) | |
if Version(self.version) < 15: | |
get(self, **sources, strip_root=True) | |
else: | |
# LLVM >=15 split up several components in its release, including cmake | |
get(self, **sources["llvm"], destination='llvm-main', strip_root=True) | |
get(self, **sources["cmake"], destination='cmake', strip_root=True) |
recipes/llvm-core/all/conanfile.py
Outdated
@@ -231,6 +318,7 @@ def generate(self): | |||
tc = CMakeToolchain(self, generator="Ninja") | |||
# https://releases.llvm.org/12.0.0/docs/CMake.html | |||
# https://releases.llvm.org/13.0.0/docs/CMake.html | |||
# https://releases.llvm.org/19.1.1/docs/CMake.html |
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.
# https://releases.llvm.org/19.1.1/docs/CMake.html | |
# https://releases.llvm.org/19.1.0/docs/CMake.html |
Other patch versions has no online documentation, they will return HTTP 404.
@@ -52,13 +53,75 @@ | |||
"XCore" | |||
} | |||
|
|||
def components_from_dotfile(dotfile): |
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.
Could you please add more detailed comments in the recipe, including the method description, and what is happening in the key steps? As I commented before, it not only increases the recipe's complexity but also makes its maintenance harder. More information will be welcome to understand it in future PRs.
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.
In case needing to touch those patches because CMakeDeps.set_property was not able to fit the expected native names by llvm, then I would avoid using it. My intention was to avoid patching anything, using CMakeDeps to match what is expected by llvm project without using any patch.
cc48553
to
5a3b20f
Compare
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.
AS this PR has several changes, I did a diff between the package llvm-core/13.0.0 in Conan Center and the produced by this PR:
The artifacts are still there, no missing library/header. Some configuration changed, or just moved the position, which is fine IMO.
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.
@planetmarshall THank you for updating this recipe, it's more clear now how the dot file is working to list llvm dependencies.
It took a while to be reviewed, most because this recipe not updates a version, but also changes the way of processing the llvm dependencies. Please, consider doing separated PRs in the future, when having more than one topic in the changes, so it may be faster to have merged. Regards!
Merging this as to not delay this any further - but I don't understand why
Please sympathise with the fact that we have a high volume of pull requests to review. This recipe is bringing back an implementation that was removed here #22997 - and both versions of this implementation are far from trivial and difficult to review. I would very strongly advise that if there is any sense of urgency in merging a PR, to limit the scope of the changes to only what's necessary to achieve the goal of the PR (e.g. adding the new version) - and if there are changes that are not trivial to a casual reviewer (e.g PosixPath), please explain them. We tend to limit the use of "pure python" imports to make sure recipes remain compatible irrespective of version of Python. |
Summary
Changes to recipe: llvm-core/19.1.7
Depends on #22997
Motivation
Add The latest LLVM release
Details
19.1.619.1.7