PART II: CMake clean ups under lib/mayaUsd and lib/usd #389
Conversation
HamedSabri-adsk
commented
Mar 27, 2020
•
HamedSabri-adsk
commented
- All sub directories under lib/mayaUsd and lib/usd get their own CMakeLists.txt
- fix build/usage requirements for mayaUsdUtils and mayaUsd_Translators, testMayaUsdUI
- don't rely on ${PROJECT_NAME} in lib/usd/schemas because of the folder restructure
- use promote header mechanism across the project
…eLists.txt.
- fix build/usage requirements for mayaUsdUtils and mayaUsd_Translators, testMayaUsdUI
- don't rely on ${PROJECT_NAME} because of the folder restructure.
HamedSabri-adsk
changed the base branch from
dev
to
sabrih/MAYA-104002/clean_up_part_1
There was a problem hiding this comment.
Looks good to me!
Two questions though:
- There are some private headers in ufe that we're promoting/installing?
- I think I get the change from
${PROJECT_NAME}tomayaUsdunder thelib/usddir, but it looks like there's still some usage of `${PROJECT_NAME} in there?
Then just a few other nit-picks about file ordering.
| # ----------------------------------------------------------------------------- | ||
| # promoted headers | ||
| # ----------------------------------------------------------------------------- | ||
| mayaUsd_promoteHeaderList(HEADERS ${headers} SUBDIR ufe/private) | ||
|
|
||
| # ----------------------------------------------------------------------------- | ||
| # install | ||
| # ----------------------------------------------------------------------------- | ||
| install(FILES ${headers} | ||
| DESTINATION ${CMAKE_INSTALL_PREFIX}/include/${PROJECT_NAME}/ufe/private | ||
| ) |
There was a problem hiding this comment.
Seems a little weird that we're promoting and installing headers from a directory called "private"? Wouldn't the preprocessor find them without doing this since they're included with relative paths?
There was a problem hiding this comment.
Ah, that's my mistake. ufe/private is not supposed to be installed.
Fixed in fb592d1
|
@mattyjams CMake can generate graphviz files that could show the dependencies between the targets in a project, libraries which are linked against, etc.. The different dependency types PUBLIC, PRIVATE and INTERFACE are represented as I use it from time to time for sake of visualization. Here is the graphviz only for
|
| @@ -23,5 +23,5 @@ mayaUsd_promoteHeaderList(HEADERS ${headers} SUBDIR render/px_vp20) | |||
| # install | |||
| # ----------------------------------------------------------------------------- | |||
| install(FILES ${headers} | |||
| DESTINATION ${CMAKE_INSTALL_PREFIX}/include/mayaUsd/render/px_vp20 | |||
| DESTINATION ${CMAKE_INSTALL_PREFIX}/include/${PROJECT_NAME}/render/px_vp20 | |||
There was a problem hiding this comment.
We have a lot of these, which are kind of hard-coded paths. I wonder if there is a cmake variable or way to use the relative path from project_name? Such that we could replace "${PROJECT_NAME}/render/px_vp20" with a variable.
| basisCurves.h | ||
| bboxGeom.h | ||
| debugCodes.h | ||
| draw_item.h | ||
| instancer.h | ||
| material.h | ||
| mesh.h | ||
| proxyRenderDelegate.h | ||
| render_delegate.h | ||
| render_param.h | ||
| render_pass.h | ||
| resource_registry.h | ||
| sampler.h | ||
| task_commit.h | ||
| tokens.h |
There was a problem hiding this comment.
Sorry, just noticed this, but it looks like we're now going to be installing all of these headers, not just promoting them. We previously only installed proxyRenderDelegate.h. Maybe we need two variables to separate those that are promoted only versus those that are promoted and installed?
There was a problem hiding this comment.
Thanks @mattyjams . Great catch. I will fix it.
There was a problem hiding this comment.
@mattyjams please see de407da
There was a problem hiding this comment.
It seems odd to have headers that are promoted, but not installed. That would make them in a bit of a gray area between public and private. Are we sure they need to promoted?
I took a random look at one header - draw_item.h - and it seems like it should be fine being not promoted - it's always brought in via #include "draw_item.h", and only from .cpp files in the same directory.
Have we tried not promoting any of these, other than proxyRenderDelegate.h?
There was a problem hiding this comment.
I think the only remaining issue I have here is the unnecessary promotion of headers in vp2RenderDelegate, brought to my attention my @mattyjams here:
