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

Modernize TBB CMake setup by exclusively relying on CMake namespace targets #3207

Open
wants to merge 6 commits into
base: dev
Choose a base branch
from

Conversation

theblackunknown
Copy link

@theblackunknown theblackunknown commented Aug 5, 2024

Description of Change(s)

Using CMake namespace target is the modern approach, it forwards definitions, include directories and link libraries transitively to consumers.
One TBB::tbb is already produced by the FindTBB.cmake module.
As the main CMakeLists.txt already requires CMake 3.14 this should already be available to all CMake clients.

Additionally try to resolve TBB Config package before trying the module one.
Miscellaneous associated cleanups are made with remaining commits.

  • I have verified that all unit tests pass with the proposed changes
  • I have submitted a signed Contributor License Agreement

…argets

Using CMake namespace target is the modern approach, it forward definitions, include directories and link libraries transitively to consumers.
And one is already setup by the `FindTBB.cmake` module.
As the main CMakeLists.txt already requires CMake 3.14 this should already be available to all CMake clients.

Additionally try to resolve TBB Config package before trying the module one.
@theblackunknown theblackunknown marked this pull request as ready for review August 5, 2024 08:16
@jesschimein
Copy link
Collaborator

Filed as internal issue #USD-9934

@jesschimein
Copy link
Collaborator

/AzurePipelines run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@theblackunknown
Copy link
Author

theblackunknown commented Aug 6, 2024

Hey @jesschimein I have reproduced yesterday issues locally when building without using oneTBB and I have pushed a new fix to accommodate those.

@jesschimein
Copy link
Collaborator

/AzurePipelines run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@theblackunknown
Copy link
Author

Hi @jesschimein , I am not sure to understand whether the last azure pipeline run for windows is a genuine error or not.
I am not able to reproduce the timeout issue locally.

@jesschimein
Copy link
Collaborator

/AzurePipelines run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jesschimein
Copy link
Collaborator

Yeah, some of the timeouts are a little fickle, and we usually rerun to check. Looks like Windows successfully built this time! 🙌🏻

@theblackunknown
Copy link
Author

Hi @jesschimein there is now a large list of conflicts, if I resolve them is there a good chance to integrate my change for the next release ?

@asluk asluk added the build Build-related issue/PR label Oct 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Build-related issue/PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants