Skip to content

Conversation

@alxbilger
Copy link
Contributor

Integrate Taskflow as a header-only library, update CMake configuration to locate and fetch Taskflow if necessary, and include relevant headers.

taskflow: https://github.com/taskflow/taskflow

More information:


By submitting this pull request, I acknowledge that
I have read, understand, and agree SOFA Developer Certificate of Origin (DCO).


Reviewers will merge this pull-request only if

  • it builds with SUCCESS for all platforms on the CI.
  • it does not generate new warnings.
  • it does not generate new unit test failures.
  • it does not generate new scene test failures.
  • it does not break API compatibility.
  • it is more than 1 week old (or has fast-merge label).

Integrate Taskflow as a header-only library, update CMake configuration to locate and fetch Taskflow if necessary, and include relevant headers.
@damienmarchal
Copy link
Contributor

It remind me so much something we did with PJBensoussan around 2006 in SOFA.

@alxbilger
Copy link
Contributor Author

[ci-build][with-all-tests][force-full-build]

@alxbilger alxbilger added the pr: status to review To notify reviewers to review this pull-request label Oct 9, 2025
@olivier-roussel
Copy link
Contributor

olivier-roussel commented Oct 13, 2025

Modern cmake usage is more a suggestion here (my commits 05691e0 and cec0c7a ). Both solutions work fine and looks good to me. It was mainly driven by the idea of being consistent with cmake config files provided by the Taskflow library itself.
Don't know if anybody has an opinion about that... I can revert these commits if necessary

@alxbilger
Copy link
Contributor Author

alxbilger commented Oct 13, 2025

About cec0c7a, it is was a conclusion in the last dev meeting. So, thank you for having done the job!

Another question: is there something to do regarding the package/installation process?

@olivier-roussel
Copy link
Contributor

olivier-roussel commented Oct 13, 2025

The only thing I think is still missing here is the find package for the generated cmake config file of Sofa.Helper, i.e. adding in the Sofa.HelperConfig.cmake.in :

if(NOT TARGET Taskflow::Taskflow)
    sofa_find_package(Taskflow QUIET REQUIRED)
endif()

But strangely, find_package calls for other required libs (Json, STB, DiffLib) are missing as well here. So either it is an omission, either I am wrong and missing something...
Ok my bad: Json, STB and DiffLib are not using targets so they do not need to be explicitly found here. Everything is OK for them. We just need this find_pacakge in the Sofa.HelperConfig.cmake.in if we use the modern cmake version.

@olivier-roussel
Copy link
Contributor

[ci-build][with-all-tests]

@olivier-roussel
Copy link
Contributor

[ci-build][with-all-tests]

@olivier-roussel
Copy link
Contributor

[ci-build][with-all-tests][force-full-build]

@olivier-roussel
Copy link
Contributor

Just for information, there would/will be some extra steps necessary after looking for the Taskflow package in the Sofa.Helper CMakeLists.txt to install the library (headers + cmake config files) in the case of building the binary package on Windows. But this when we'll have migrated our Windows support on Pixi and it's purely related to it, so I'll do it in #5252.

@hugtalbot hugtalbot added the pr: new feature Implement a new feature label Oct 16, 2025
@bakpaul bakpaul added pr: status postponed To keep in mind that this PR was interesting but no one has time to make it mergeable now. and removed pr: status to review To notify reviewers to review this pull-request labels Oct 30, 2025
@hugtalbot hugtalbot added the pr: highlighted in next release Highlight this contribution in the notes of the upcoming release label Oct 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr: highlighted in next release Highlight this contribution in the notes of the upcoming release pr: new feature Implement a new feature pr: status postponed To keep in mind that this PR was interesting but no one has time to make it mergeable now.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants