-
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
package: llvm/13 - llvm/16 #17509
package: llvm/13 - llvm/16 #17509
Conversation
It looks like continuous integration fails because of a timeout problem. To test this, I'm limitting the configurations that are being built
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 for adding LLVM 17!
I recommend looking over the CMake package template, as you'll want to align with it as much as possible.
@@ -0,0 +1,55 @@ | |||
from conan import ConanFile |
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.
from conan import ConanFile | |
from conan import ConanFile | |
from conan.tools.env import VirtualBuildEnv, VirtualRunEnv |
@@ -0,0 +1,55 @@ | |||
from conan import ConanFile | |||
from conan.tools.cmake import cmake_layout, CMakeDeps, CMakeToolchain, CMake | |||
from conan.tools.build import cross_building |
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.
from conan.tools.build import cross_building | |
from conan.tools.build import can_run |
if not cross_building(self, self.settings): | ||
self.run("./test_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.
This should run under the VirtualRunEnv.
if not cross_building(self, self.settings): | |
self.run("./test_package") | |
if can_run(self): | |
bin_path = os.path.join(self.cpp.build.bindir, "test_package") | |
self.run(bin_path, env="conanrun") |
deps.generate() | ||
tc = CMakeToolchain(self) | ||
tc.generate() | ||
|
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.
For the tool_requires to take effect, use VirtualBuildEnv
.
env = VirtualBuildEnv(self) | |
env.generate() | |
@@ -0,0 +1,32 @@ | |||
from conans import ConanFile, CMake, tools |
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 it's probably alright to just drop the test_v1_package
entirely now, assuming no other reviewers have a good reason for keeping it.
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 I didn't have one in the first place but ci complained about.
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 believe the policy has changed since you opened your PR.
|
||
def source(self): | ||
get(self, **self.conan_data["sources"][self.version], strip_root=True) | ||
apply_conandata_patches(self) |
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.
Is this due to the no_copy_source
option being set? You should apply patches in the build
method instead of the source method.
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.
Oops, nevermind. I see this is exactly what we do for the Qt package.
def is_windows(self): | ||
return self.settings.os == "Windows" | ||
|
||
def is_macos(self): | ||
return self.settings.os == "MacOS" | ||
|
||
def is_linux(self): | ||
return self.settings.os == "Linux" |
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.
To be safe, you should prefix any custom methods or attributes with an _
to avoid conflicts.
release = Version(self.version).major | ||
unsupported_options = [] | ||
if release < 14: | ||
unsupported_options.append('with_curl') | ||
if release < 15: | ||
unsupported_options.append('with_zstd') | ||
unsupported_options.append('with_httplib') | ||
# TODO fix with_zstd and llvm 17 | ||
if release >= 17: | ||
self.output.warning(f"with_zstd will fail in llvm 17 with conan 2") | ||
unsupported_options.append('with_zstd') | ||
for opt in unsupported_options: | ||
self.output.warning(f"{opt} is unsupported in llvm {release}") | ||
self.options.rm_safe(opt) |
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 probably better to delete options based off of the version and any settings
in the config_options
method instead. This way, the options won't be available when consumers try to set them in the first place. You'll want to put that above the configure
method to reflect the evaluation order.
def config_options(self):
if self.settings.os == "Windows":
self.options.rm_safe("fPIC") # FPIC MANAGEMENT (KB-H007)
release = Version(self.version).major
unsupported_options = []
if release < 14:
unsupported_options.append('with_curl')
if release < 15:
unsupported_options.append('with_zstd')
unsupported_options.append('with_httplib')
# TODO fix with_zstd and llvm 17
if release >= 17:
unsupported_options.append('with_zstd')
for opt in unsupported_options:
self.options.rm_safe(opt)
cmake = self._cmake_configure() | ||
cmake.build() | ||
|
||
def _cmake_configure(self): |
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.
This should be migrated to use the toolchain in the generate
method instead of the CMake configure
method.
self.tool_requires("cmake/[>=3.21.3 <4.0.0]") | ||
self.tool_requires("ninja/[>=1.10.0 <2.0.0]") | ||
|
||
def generate(self): |
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 you need VirtualBuildEnv here.
@jwillikers I will test the configs locally first before adding your suggestions, some parts are really what I was looking for, thanks a lot! |
Conan v1 pipeline ✔️All green in build 9 (
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 See details:Failure in build 9 (
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. |
# creating job pools with current free memory | ||
'ram_per_compile_job': '2000', | ||
'ram_per_link_job': '14000', | ||
'conan_center_index_limits': 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.
Instead of an option, you can use a conf
variable to control this, like user.c3i.skip:llvm
.
You can get the value of this variable via self.conf.get
.
self.conf.get('user.c3i.skip:llvm', default=False, check_type=bool)
Then in global.conf
or on the CLI, it can be defined.
conan ... --conf user.c3i.skip:llvm=True
Why is Also, this recipe would be helpful for #20528, which requires components from def package(self):
cmake = self._cmake_configure()
cmake.install()
if self.options.get_safe("with_project_libclc"):
copy(self, "*.bc", os.path.join(self.package_folder, "share", "clc"), os.path.join(self.package_folder, "lib", "clc")) def package_info(self):
self.cpp_info.components["libclc"].set_property("pkg_config_name", "libclc")
self.cpp_info.components["libclc"].libdirs = [os.path.join(self.package_folder, "lib", "clc")] |
Ah, I see now. It appears that Also, with LLVM 17, I noticed that the |
I stumbled upon this too, but didn't thought about taking host profile which sounds reasonable.
Ok, also a good idea for your config_options suggestion, to remove it there. For this recipe and changed config I have already a workflow to check for definition/requirement changes, didn't expect to miss such change, thanks!
So this option shouldn't be part of this recipe. regarding:
Where did you find this? During my search even looked through related issues and never found a note on this, really handy if it works. |
That was recommended here. |
Happy to see this moving along. Thank you for the hard work @jusito |
Building LLVM 15 with a direct_deploy, clang requires libatomic. I wonder if COMPILER_RT_BUILD_STANDALONE_LIBATOMIC will help if COMPILER_RT is needed. |
@jacobfriedman
|
|
It wasn't built natively on mine with llvm 15 using this codebase, unless I
need to update. If you're sure it's included, then all the better.
…On Fri., Nov. 17, 2023, 3:16 a.m. Erin M., ***@***.***> wrote:
@jacobfriedman <https://github.com/jacobfriedman> Just to clarify for
replication, for a llvm 15 build:
- LLVM_ENABLE_PROJECTS should contain clang and compiler-rt
compiler-rt should be in LLVM_ENABLE_RUNTIMES
- there is a dependency missing which may be fixed with
COMPILER_RT_BUILD_STANDALONE_LIBATOMIC, which adds clang_rt.atomic as
dependency to compiler_rt
compiler-rt already includes libatomic. the
COMPILER_RT_BUILD_STANDALONE_LIBATOMIC flag builds clang_rt.atomic
separately, so that you can link to it with -latomic like you would do
with gcc
—
Reply to this email directly, view it on GitHub
<#17509 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAFKHXQOQLWHGSWJKLVNEE3YE4TOXAVCNFSM6AAAAAAX443BHGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMJWGAYDSMJQGA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Those are the targets for which LLVM can generate code. If you remove targets, you will be removing the ability of LLVM to generate code for those architectures. It's not really connected to the conan For example, someone could use LLVM as a library to, say, write a program that compiles CUDA to X86 assembly - but that program could itself be compiled on an X86 host targeting ARM64. |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Please don't close |
We are currently not prioritising packages that add toolchains (rather than libraries or pure executables), as we are prioritising other issues in our backlog. LLVM itself has a myriad of components, and I suspect different users would have different needs. We are opening this issue to track this: #23834 - actual uses cases with examples will help the team gain better insights at how a potential llvm recipe may be used, whether or not it is worth packaging different LLVM components in separate recipes, and so on. Any information of this would be appreciated in that issue so that we can move forward, thanks! |
Why would you close this and open another issue to justify this project? |
Specify library name and version: llvm/13.0.1, llvm/14.0.6, llvm/15.0.7, llvm/16.0.6
Current State 07.2023 waiting for reviews
follow up on previous pull request: #7613
My main focus in this pr is:
What I added:
Tasks:
Feedback and help welcome.