-
Notifications
You must be signed in to change notification settings - Fork 202
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
Clean up build/usage requirements for hdMaya and mayaToHydra targets. #358
Conversation
@@ -14,7 +14,7 @@ | |||
// limitations under the License. | |||
// | |||
|
|||
#include <hdMaya/utils.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Utils is not a private header file. @HamedSabri-adsk please revert this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kxl-adsk Can you clarify what private header mean here?
We have a utils.h and utils.cpp sitting besides each other. Why do I need to do hdMaya/utils.h
instead of simply utils.h
Besides, I have ${CMAKE_CURRENT_SOURCE_DIR}
added to target_include_directories
as PRIVATE
requirement in lib/usd/hdMaya/CMakeLists.txt
.
target_include_directories(${TARGET_NAME}
PRIVATE
${CMAKE_BINARY_DIR}/include
${CMAKE_CURRENT_SOURCE_DIR}
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... so I was curious if this was / could be made a private header or not - and I think we may need to it to be public, because any hdMaya adapters that others might wish to implement will need/want it.
Currently, it's effectively public, since it's installed. However, it's referenced in many places using "../hdMaya/utils.h" syntax, which is slightly confusing, and will need to be fixed to get everything in line with our style conventions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... so I was curious if this was / could be made a private header or not - and I think we may need to it to be public, because any hdMaya adapters that others might wish to implement will need/want it.
Currently, it's effectively public, since it's installed. However, it's referenced in many places using "../hdMaya/utils.h" syntax, which is slightly confusing, and will need to be fixed to get everything in line with our style conventions.
) | ||
|
||
# ----------------------------------------------------------------------------- | ||
# link libraries | ||
# ----------------------------------------------------------------------------- | ||
target_link_libraries(${TARGET_NAME} | ||
PUBLIC | ||
arch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, this all looks great - relying on the project-level modificaitons for this stuff certainly cleans things up here!
Only issue is that I agree with the note Krystian already made - but since he's already got that covered, I'm going to mark this approved!
…ra other than declaring their dependence on mayaUsd. mayaUsd target now declares ${CMAKE_BINARY_DIR}/include publicly.
This is the first of many small PRs to clean up build/usage requirements logics for targets inside maya-usd/lib.