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.

Sign up for GitHub

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

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

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

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a714e51
Make TBB and OpenSubdiv dependencies relocatable
tamaskenez Dec 2, 2024
d8e668e
temporary debugging messages
tamaskenez Dec 2, 2024
c6e63a2
more debug
tamaskenez Dec 2, 2024
7c591d4
fix attempt
tamaskenez Dec 2, 2024
f6dc727
more debug
tamaskenez Dec 2, 2024
1b5c2b7
done
tamaskenez Dec 2, 2024
08c8ca9
set pxrConfig.cmake.in vars in Packages.cmake
tamaskenez Dec 2, 2024
9decb4e
Self-explanatory `if` in pxrConfig.cmake.in
tamaskenez Dec 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 24 additions & 2 deletions cmake/defaults/Packages.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,14 @@ endif()


# --TBB
find_package(TBB REQUIRED COMPONENTS tbb)
find_package(TBB CONFIG)
if(TBB_DIR)
# Found in CONFIG mode.
set(TBB_tbb_LIBRARY TBB::tbb)
else()
find_package(TBB REQUIRED COMPONENTS tbb)
endif()

add_definitions(${TBB_DEFINITIONS})

# --math
Expand Down Expand Up @@ -251,7 +258,22 @@ if (PXR_BUILD_IMAGING)
endif()
# --Opensubdiv
set(OPENSUBDIV_USE_GPU ${PXR_BUILD_GPU_SUPPORT})
find_package(OpenSubdiv 3 REQUIRED)
find_package(OpenSubdiv 3 CONFIG)
if(OpenSubdiv_DIR)
set(OPENSUBDIV_LIBRARIES OpenSubdiv::osdCPU OpenSubdiv::osdGPU)
set(OPENSUBDIV_OSDCPU_LIBRARY OpenSubdiv::osdCPU)
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()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

else()
find_package(OpenSubdiv 3 REQUIRED)
message(STATUS "OpenSubdiv found as module")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto above - can we lower to VERBOSE or DEBUG?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also debugging, will be removed

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

endif()
# --Ptex
if (PXR_ENABLE_PTEX_SUPPORT)
find_package(PTex REQUIRED)
Expand Down
7 changes: 7 additions & 0 deletions pxr/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,13 @@ pxr_core_epilogue()

export(PACKAGE pxr)

if(TBB_DIR)
set(PXR_FIND_TBB_IN_CONFIG ON)
endif()
if(OpenSubdiv_DIR)
set(PXR_FIND_OPENSUBDIV_IN_CONFIG ON)
endif()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, changed in 08c8ca9


configure_file(pxrConfig.cmake.in
"${PROJECT_BINARY_DIR}/pxrConfig.cmake" @ONLY)
install(FILES
Expand Down
10 changes: 10 additions & 0 deletions pxr/pxrConfig.cmake.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,16 @@ if(@PXR_ENABLE_PYTHON_SUPPORT@)
endif()
endif()

include(CMakeFindDependencyMacro)

if(@PXR_FIND_TBB_IN_CONFIG@)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@tamaskenez tamaskenez Dec 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@tamaskenez tamaskenez Dec 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

find_dependency(TBB @TBB_VERSION@ CONFIG)
endif()

if(@PXR_FIND_OPENSUBDIV_IN_CONFIG@)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

9decb4e

find_dependency(OpenSubdiv @OpenSubdiv_VERSION@ CONFIG)
endif()

# If MaterialX support was enabled for this USD build, try to find the
# associated import targets by invoking the same FindMaterialX.cmake
# module that was used for that build. This can be overridden by
Expand Down
Loading