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

openmp/system: new recipe #22360

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

openmp/system: new recipe #22360

wants to merge 12 commits into from

Conversation

valgur
Copy link
Contributor

@valgur valgur commented Jan 15, 2024

Adds a new openmp/system package for a much more convenient and less bug-prone OpenMP support in recipes.

This avoids the need for listing the appropriate flags in every recipe using OpenMP, e.g. https://github.com/conan-io/conan-center-index/blob/master/recipes/lightgbm/all/conanfile.py#L123-L133

It relies on the default OpenMP provided by the compiler for compilers that ship with one (GCC and MSVC) and falls back to llvm-openmp from CCI for others (Clang and AppleClang).

transitive_headers=True, transitive_libs=True can be used to set the correct visibility for libraries that use #pragma omp in their public headers.

See #24577 for more context.

Requires

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@valgur valgur marked this pull request as ready for review July 9, 2024 13:15
@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.

@valgur valgur mentioned this pull request Jul 18, 2024
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@valgur valgur changed the title openmp: new meta-package openmp/system: new recipe Jul 19, 2024
@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline ✔️

All green in build 11 (fda846f782bf90dfa592b294bee0f26f1acc6fd7):

  • openmp/system:
    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 11 (fda846f782bf90dfa592b294bee0f26f1acc6fd7):

  • openmp/system:
    All packages built successfully! (All logs)

@jacobfriedman
Copy link

jacobfriedman commented Oct 2, 2024

Why not just use the llvm-openmp recipe, ignore the wrapping, and append the openmp argument in downstream recipes?

Edit: Adding yet another openmp to conan is not ideal. I recommend recipes add the argument, rather than bloating the ecosystem @jcar87 @planetmarshall @memsharded

@memsharded
Copy link
Member

Edit: Adding yet another openmp to conan is not ideal. I recommend recipes add the argument,

Recall that it is also possible to inject compiler flags with tools.build:cxxflags, so adding arbitrary flags can be done from the user side without really modifying libraries.

@valgur
Copy link
Contributor Author

valgur commented Oct 2, 2024

Edit: Adding yet another openmp to conan is not ideal. I recommend recipes add the argument,

Recall that it is also possible to inject compiler flags with tools.build:cxxflags, so adding arbitrary flags can be done from the user side without really modifying libraries.

@memsharded I do recall that, but the proposed openmp/system is mostly concerned with correct handling OpenMP as a part of the dependency graph (that is, correct visibility of the runtime library dependency via the transitive_libs=True attribute and propagating the preprocessor flag via transitive_headers=True for public headers that rely on OpenMP features) rather than a mere optimization flag. Also, I hope you're not seriously suggesting force-enabling OpenMP for all recipes that might support it (#24577 (comment))?

@jacobfriedman
Copy link

I would explicitly define requirements in packages, rather than wrapping them, which is a workaround. I wouldn't expect force-enabling openmp, rather, it would be an option- for all recipes that might support it.

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.

5 participants