Skip to content

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.

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

1 similar comment
@conan-center-bot

This comment has been minimized.

@conan-center-bot
Copy link
Contributor

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)

@jcar87 jcar87 self-assigned this Sep 4, 2024
@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.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Martin Valgur seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@conan-center-build-service conan-center-build-service bot locked as off-topic and limited conversation to collaborators Mar 17, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants