Make pxrTargets.cmake relocatable (when built with TBB and OpenSubdiv) #3441

tamaskenez · 2024-11-30T17:58:43Z

Description of Change(s)

The CMake targets now use TBB and OpenSubdiv as targets instead of absolute include and library paths.

Link to proposal (if applicable)

Fixes Issue(s)

The installed OpenUSD library was not relocatable because cmake/pxrTargets.cmake contained absolute paths of include directories and library files.

This PR solves the issue partially: it changes the way oneTBB and OpenSubdiv are used, if those dependencies are found by their CMake config-scripts (as opposed to a find-module).

Therefore, if OpenUSD is built only with the oneTBB (not the old TBB) and OpenSubdiv dependencies which provide a config-script (which is the case when building with build_usd.py), with this PR the installed OpenSubdiv library will be relocatable.

Checklist

[X] I have created this PR based on the dev branch

[X] I have followed the coding conventions

[ ] I have added unit tests that exercise this functionality (Reference:
testing guidelines)

[ ] I have verified that all unit tests pass with the proposed changes

[X] I have submitted a signed Contributor License Agreement (Reference:
Contributor License Agreement instructions)

jesschimein · 2024-12-02T18:17:15Z

Filed as internal issue #USD-10483

❗ Please make sure that a signed CLA has been submitted!

jesschimein · 2024-12-02T18:17:16Z

/AzurePipelines run

azure-pipelines · 2024-12-02T18:17:28Z

Azure Pipelines successfully started running 1 pipeline(s).

tamaskenez · 2024-12-02T20:19:02Z

I've submitted the CLA and I'll be looking into the build errors.

tamaskenez · 2024-12-02T20:56:14Z

/AzurePipelines run

azure-pipelines · 2024-12-02T20:56:19Z

Commenter does not have sufficient privileges for PR 3441 in repo PixarAnimationStudios/OpenUSD

pixar-oss · 2024-12-02T21:06:12Z

/AzurePipelines run

azure-pipelines · 2024-12-02T21:06:22Z

Azure Pipelines successfully started running 1 pipeline(s).

pmolodo

Just to add my $.02 - overall, I think this looks great - in general I'm strongly for anything to moves more towards "modern cmake" with usage of targets instead of baked-in paths.

I had a few suggested tweaks, but they're minor, and I'd still be fine with accepting this as is.

pmolodo · 2024-12-02T21:21:36Z

cmake/defaults/Packages.cmake

+        message(STATUS "OpenSubdiv found as config, OpenSubdiv_DIR is ${OpenSubdiv_DIR}")
+        foreach(t OpenSubdiv::osdCPU OpenSubdiv::osdGPU OpenSubdiv::osdCPU_static OpenSubdiv::osdGPU_static OpenSubdiv::OpenSubdiv)
+            if(TARGET "${t}")
+                message(STATUS "${t} is TARGET")
+            else()
+                message(STATUS "${t} is not TARGET")
+            endif()
+        endforeach()


These messages feel more like debugging messages than things that need to be displayed every build - do you think we could lower the message code from STATUS to VERBOSE or DEBUG?

I'm sorry, they are debugging messages and I removed them in my final commit. I set up a workflow on my fork and I was experimenting with solutions to fix the build error here.

Sorry for the noise, I left the PR in Draft state to indicate it's in progress.

pmolodo · 2024-12-02T21:22:06Z

cmake/defaults/Packages.cmake

+        endforeach()
+    else()
+        find_package(OpenSubdiv 3 REQUIRED)
+        message(STATUS "OpenSubdiv found as module")


Ditto above - can we lower to VERBOSE or DEBUG?

also debugging, will be removed

pmolodo · 2024-12-02T21:36:28Z

pxr/CMakeLists.txt

+if(TBB_DIR)
+    set(PXR_FIND_TBB_IN_CONFIG ON)
+endif()
+if(OpenSubdiv_DIR)
+    set(PXR_FIND_OPENSUBDIV_IN_CONFIG ON)
+endif()


Personally, I would find this more readable if these variables were set in Packages.cmake, in the same branches where we first check to see if the config packages were found. (And also explicitly set to OFF if not found.)

I can see the thinking of wanting to restrict Pixar-specific variables to the pxr subdirectory - but really, everything in the cmake subdir is already very Pixar-specific, and there's already examples of setting PXR_XXX variables in there... so I'd rather it easier to read.

Thanks, changed in 08c8ca9

pmolodo · 2024-12-02T21:43:12Z

pxr/pxrConfig.cmake.in

@@ -47,6 +47,16 @@ if(@PXR_ENABLE_PYTHON_SUPPORT@)
    endif()
 endif()

+include(CMakeFindDependencyMacro)
+
+if(@PXR_FIND_TBB_IN_CONFIG@)


Can we replace this with:

if (NOT DEFINED PXR_FIND_TBB_IN_CONFIG) set(PXR_FIND_TBB_IN_CONFIG "@PXR_FIND_TBB_IN_CONFIG@") endif() if(PXR_FIND_TBB_IN_CONFIG)

I know there's already an established precedent of just doing the "bare" if(@PXR_VAR@), but I feel this makes the resulting pxrConfig.cmake less readable, and prevents downstream users from being able to check (or even override) PXR_FIND_TBB_IN_CONFIG directly themselves.

I agree that lines like if(OFF) or even if() are cryptic. I like your suggestion but why do we need to check if it's defined, why not just setting it?

set(PXR_FIND_TBB_IN_CONFIG "@PXR_FIND_TBB_IN_CONFIG@") if(PXR_FIND_TBB_IN_CONFIG)

or, what about a comment?

if(@PXR_FIND_TBB_IN_CONFIG@) # if(PXR_FIND_TBB_IN_CONFIG)

I'm fine with either one of the 3 above.

I did the second option, please let me know if you prefer your original or the comment. 9decb4e

pmolodo · 2024-12-02T21:46:17Z

pxr/pxrConfig.cmake.in

+    find_dependency(TBB @TBB_VERSION@ CONFIG)
+endif()
+
+if(@PXR_FIND_OPENSUBDIV_IN_CONFIG@)


Similar to above - can we replace with:

if (NOT DEFINED PXR_FIND_OPENSUBDIV_IN_CONFIG) set(PXR_FIND_OPENSUBDIV_IN_CONFIG "@PXR_FIND_OPENSUBDIV_IN_CONFIG@") endif() if(PXR_FIND_OPENSUBDIV_IN_CONFIG)

jesschimein · 2024-12-03T17:46:22Z

/AzurePipelines run

azure-pipelines · 2024-12-03T17:46:33Z

Azure Pipelines successfully started running 1 pipeline(s).

tamaskenez · 2024-12-19T11:37:08Z

@pmolodo Could you please check this again, the checks pass and I addressed your suggestions.

tamaskenez changed the title ~~Make TBB and OpenSubdiv dependencies relocatable~~ Make pxrTargets.cmake relocatable (when built with TBB and OpenSubdiv) Nov 30, 2024

asluk added the build Build-related issue/PR label Dec 2, 2024

Make TBB and OpenSubdiv dependencies relocatable

a714e51

tamaskenez force-pushed the relocatable_config_with_tbb_opensubdiv branch from 7d09fed to a714e51 Compare December 2, 2024 20:25

temporary debugging messages

d8e668e

more debug

c6e63a2

pmolodo reviewed Dec 2, 2024

View reviewed changes

tamaskenez added 5 commits December 2, 2024 23:21

fix attempt

7c591d4

more debug

f6dc727

done

1b5c2b7

set pxrConfig.cmake.in vars in Packages.cmake

08c8ca9

Self-explanatory if in pxrConfig.cmake.in

9decb4e

tamaskenez marked this pull request as ready for review December 2, 2024 23:41

tamaskenez requested a review from pmolodo December 3, 2024 06:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make pxrTargets.cmake relocatable (when built with TBB and OpenSubdiv) #3441

Make pxrTargets.cmake relocatable (when built with TBB and OpenSubdiv) #3441

tamaskenez commented Nov 30, 2024 •

edited

Loading

jesschimein commented Dec 2, 2024

jesschimein commented Dec 2, 2024

azure-pipelines bot commented Dec 2, 2024

tamaskenez commented Dec 2, 2024

tamaskenez commented Dec 2, 2024

azure-pipelines bot commented Dec 2, 2024

pixar-oss commented Dec 2, 2024

azure-pipelines bot commented Dec 2, 2024

pmolodo left a comment

pmolodo Dec 2, 2024

tamaskenez Dec 2, 2024

tamaskenez Dec 2, 2024

pmolodo Dec 2, 2024

tamaskenez Dec 2, 2024

tamaskenez Dec 2, 2024

pmolodo Dec 2, 2024

tamaskenez Dec 2, 2024

pmolodo Dec 2, 2024

tamaskenez Dec 2, 2024 •

edited

Loading

tamaskenez Dec 2, 2024 •

edited

Loading

pmolodo Dec 2, 2024

tamaskenez Dec 2, 2024

jesschimein commented Dec 3, 2024

azure-pipelines bot commented Dec 3, 2024

tamaskenez commented Dec 19, 2024

Make pxrTargets.cmake relocatable (when built with TBB and OpenSubdiv) #3441

Are you sure you want to change the base?

Make pxrTargets.cmake relocatable (when built with TBB and OpenSubdiv) #3441

Conversation

tamaskenez commented Nov 30, 2024 • edited Loading

Description of Change(s)

Link to proposal (if applicable)

Fixes Issue(s)

Checklist

jesschimein commented Dec 2, 2024

jesschimein commented Dec 2, 2024

azure-pipelines bot commented Dec 2, 2024

tamaskenez commented Dec 2, 2024

tamaskenez commented Dec 2, 2024

azure-pipelines bot commented Dec 2, 2024

pixar-oss commented Dec 2, 2024

azure-pipelines bot commented Dec 2, 2024

pmolodo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tamaskenez Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

tamaskenez Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jesschimein commented Dec 3, 2024

azure-pipelines bot commented Dec 3, 2024

tamaskenez commented Dec 19, 2024

tamaskenez commented Nov 30, 2024 •

edited

Loading

tamaskenez Dec 2, 2024 •

edited

Loading

tamaskenez Dec 2, 2024 •

edited

Loading