Skip to content
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

Merged
merged 83 commits into from
Sep 24, 2024

Conversation

planetmarshall
Copy link
Contributor

@planetmarshall planetmarshall commented Mar 5, 2024

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.


Copy link
Contributor

github-actions bot commented Mar 5, 2024

🤖 Beep Boop! This pull request is making changes to 'recipes/llvm-core//'.

👋 @Hopobcn @paulharris you might be interested. 😉

@planetmarshall planetmarshall marked this pull request as draft March 5, 2024 21:24
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@planetmarshall planetmarshall force-pushed the llvm-core-conan2 branch 3 times, most recently from b8f48e8 to 471320c Compare March 5, 2024 22:41
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@planetmarshall planetmarshall changed the title [llvm-core] bring package up to date. Initial support for 13.0.0 [llvm-core] bring package up to date. Support for Conan 2. Mar 7, 2024
@planetmarshall planetmarshall marked this pull request as ready for review March 7, 2024 22:48
@planetmarshall
Copy link
Contributor Author

@jcar87 Most recent CI build failed but i'm unable to view any logs.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.


components = defaultdict(list)
for lib, dep in deps:
if not lib.startswith('LLVM'):
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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).

Copy link
Contributor Author

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.

Copy link
Contributor Author

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-center-bot
Copy link
Collaborator

Conan v1 pipeline ✔️

All green in build 43 (bd3f92f10901ca9fe5487ffaca59e3ed312454a9):

  • llvm-core/11.1.0:
    All packages built successfully! (All logs)

  • llvm-core/13.0.0:
    All packages built successfully! (All logs)

  • llvm-core/12.0.0:
    All packages built successfully! (All logs)


Conan v2 pipeline ✔️

Note: Conan v2 builds are now mandatory. Please read our discussion about it.

All green in build 44 (bd3f92f10901ca9fe5487ffaca59e3ed312454a9):

  • llvm-core/13.0.0:
    All packages built successfully! (All logs)

  • llvm-core/12.0.0:
    All packages built successfully! (All logs)

  • llvm-core/11.1.0:
    All packages built successfully! (All logs)

Copy link
Contributor

@jcar87 jcar87 left a 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!

@conan-center-bot conan-center-bot merged commit d3985d3 into conan-io:master Sep 24, 2024
31 checks passed
@planetmarshall planetmarshall deleted the llvm-core-conan2 branch September 24, 2024 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

llvm-core/13.0.0: recipe is broken (conan 2)