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

pcl: add v1.14.0, use llvm-openmp for OpenMP #22152

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

Conversation

valgur
Copy link
Contributor

@valgur valgur commented Jan 4, 2024

I scanned the diff against previous version and there were no changes to the list of components or their dependencies, either internal or external ones.

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

@conan-center-bot

This comment has been minimized.

@perseoGI
Copy link
Contributor

perseoGI commented Jul 9, 2024

Hi @valgur, thank you for your contributions!
As mentioned in #24562 v1.14.1 has fixed some errors which were being patched.
Instead of adding v1.14.0 could you better add the latest release?

Thank you in advance!

@perseoGI perseoGI self-assigned this Jul 9, 2024
@valgur valgur marked this pull request as draft July 10, 2024 14:52
@weypro
Copy link

weypro commented Aug 27, 2024

I want PCL 1.14.1, and I'm really looking forward to it being merged.

@perseoGI
Copy link
Contributor

Hi @valgur is this PR still in draft?

@conan-center-bot

This comment has been minimized.

@valgur
Copy link
Contributor Author

valgur commented Aug 27, 2024

It should be ready to be merged. Needs a build to verify that, though.

Of course, I would love to use openmp/system instead of copying the flag export logic for the hundredth time (#22360), but it should work regardless of that.

@valgur valgur marked this pull request as ready for review August 27, 2024 17:07
@valgur valgur changed the title pcl: add v1.14.0, use llvm-openmp for OpenMP, avoid cache_variables pcl: add v1.14.0, use llvm-openmp for OpenMP Aug 27, 2024
@conan-center-bot

This comment has been minimized.

@perseoGI
Copy link
Contributor

perseoGI commented Aug 28, 2024

It should be ready to be merged. Needs a build to verify that, though.

We are under maintaining right now, I'll launch them manually

Of course, I would love to use openmp/system instead of copying the flag export logic for the hundredth time (#22360), but it should work regardless of that.

We have been discussing with the team and the potential risk of mixing different openMP backends and other problems we sow do not compensate the extra copy-paste code.
I know how you feel, I also do not like to repeat myself.
We will study more in deep in a near future to find a robust solution. Until that, this is a good to go way :)

@valgur
Copy link
Contributor Author

valgur commented Aug 28, 2024

We have been discussing with the team and the potential risk of mixing different openMP backends and other problems we sow do not compensate the extra copy-paste code.

Thanks, I appreciate the consideration and caution you have put into the topic with the team. The mixing of OpenMP runtimes was a valid concern for the initial compromise suggestion of defaulting to llvm-openmp as the OpenMP provider on CCI. The openmp/system recipe avoids that altogether and only uses llvm-openmp for CLang and AppleClang, so it is worth another look, if you haven't already. Unless you are concerned about the minor risk of mixing system libomp with one from Conan as well?

@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
Copy link
Collaborator

Conan v1 pipeline ❌

Failure in build 18 (8b12b8df50e97a93b2bf5bf0eb896559beacf60f):

An unexpected error happened and has been reported. Help is on its way! 🏇


Conan v2 pipeline ❌

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

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 @conan-io/barbarians on the PR and we will help you.

Failure in build 18 (8b12b8df50e97a93b2bf5bf0eb896559beacf60f):

  • pcl/1.14.1:
    An unexpected error happened and has been reported

  • pcl/1.13.1:
    Didn't run or was cancelled before finishing


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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants