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

[Cmake] Don't blindly overwrite CMAKE_MODULE_PATH. #614

Merged
merged 1 commit into from
Sep 18, 2018

Conversation

marsupial
Copy link
Contributor

Description of Change(s)

Make sure CMAKE_MODULE_PATH passed via command line has precedence over USD.

Fixes Issue(s)

If there are non standard paths/ installs CMAKE_MODULE_PATH is a much easier route than specifying multiple locations, but USD is currently overwriting any CMAKE_MODULE_PATH passed to it.

@meshula
Copy link
Member

meshula commented Sep 12, 2018

I think, but am not positive, that CMake searches first to last? If that is true, wouldn't you want the existing CMAKE_MODULE_PATH to have the lowest precedence?

The documentation is ... lacking. https://cmake.org/cmake/help/latest/variable/CMAKE_MODULE_PATH.html

If this change were to be made, it would probably also be preferable that the cmake scripts in USD be prefixed with USD, e.g. USD_Public.cmake, USD_Packages.cmake, etc., as there is a greater than zero probability that if you allow external to USD CMAKE_MODULE_PATHS, conflicting files may appear in the total search path.

@marsupial
Copy link
Contributor Author

If you don't use -DCMAKE_MODULE_PATH=, then CMAKE_MODULE_PATH is empty.
So like $PATH, you're trying to override behavior in favor of something custom/better.

@jtran56
Copy link

jtran56 commented Sep 14, 2018

Filed as internal issue #USD-4737.

@pixar-oss pixar-oss merged commit 27028f1 into PixarAnimationStudios:dev Sep 18, 2018
pixar-oss added a commit that referenced this pull request Sep 18, 2018
[Cmake] Don't blindly overwrite CMAKE_MODULE_PATH.

(Internal change: 1893591)
@marsupial marsupial deleted the pr/CMAKE_MODULE_PATH branch February 7, 2020 23:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants