Improve generated header setup in MaterialXCore #940
Conversation
|
Regarding the CLA check, I have both sent a signed PDF and use the Github check link to sign it. |
theblackunknown
force-pushed
the
fix-materialxcore-generated-header
branch
2 times, most recently
from
3571cd0 to
fbe71b1
Compare
theblackunknown
changed the title
|
@theblackunknown Thanks for putting together this propoal! You can often resolve the EasyCLA detection by adding a comment including the following text: /easycla If that doesn't resolve the issue, then I would recommend writing up an LFX support ticket as described here: |
|
/easycla |
1 similar comment
|
/easycla |
theblackunknown
force-pushed
the
fix-materialxcore-generated-header
branch
from
b97371b to
fc87a98
Compare
|
@jstone-lucasfilm I figured out the EasyCLA issue ! |
|
It seems that I need your approval to run the CI and make sure this is working as expected |
source/MaterialXCore/CMakeLists.txt
Outdated
| @@ -1,7 +1,7 @@ | |||
| configure_file(${CMAKE_CURRENT_SOURCE_DIR}/Generated.h.in ${CMAKE_CURRENT_SOURCE_DIR}/Generated.h) | |||
| configure_file(${CMAKE_CURRENT_LIST_DIR}/Generated.h.in ${CMAKE_CURRENT_BINARY_DIR}/Generated.h) | |||
There was a problem hiding this comment.
I can see the need for the generated header to be in CMAKE_CURRENT_BINARY_DIR, and that part of the change seems like a clear improvement.
Can you provide some clarity on the motivation for referencing source files in CMAKE_CURRENT_LIST_DIR rather than CMAKE_CURRENT_SOURCE_DIR? I'm not against this idea, but I want to make sure that I understand the logic behind the change.
There was a problem hiding this comment.
In this context it does not matter that much and both are equivalent, so I will probably revert this change to keep the diff minimal.
My habit though it to use CMAKE_CURRENT_LIST_DIR when I can as it always refers to the CMake file currently being processed as the other one may not.
Here is a page with a bit more details and examples if you want to dig this : https://cgold.readthedocs.io/en/latest/tutorials/cmake-sources/common.html#cmake-current-list-dir-vs-cmake-current-source-dir
`source/MaterialXCore/.gitignore` is this not needed anymore. This leads to more reliable and reproducible results. I have ran into issues while developing materialx vcpkg port (cf. microsft/vcpkg#24614)
theblackunknown
force-pushed
the
fix-materialxcore-generated-header
branch
from
97a2177 to
fc3667e
Compare
This changelist removes a gitignore file that is no longer required in MaterialXCore, following the generated header improvements in #940.
…ation#940) This PR improves how MaterialXCore generated header is handled in the CMake setup. It writes generated sources in the build tree rather than the source tree, so the MaterialXCore .gitignore file is not needed anymore. I have encountered issues with it while developing a materialx port for vcpkg C++ package manager (cf. microsft/vcpkg#24614).
This changelist removes a gitignore file that is no longer required in MaterialXCore, following the generated header improvements in AcademySoftwareFoundation#940.
Description
This PR improves how
MaterialXCoregenerated header is handled in the CMake setup.It write generated sources in the buildtree rather than the source tree, and uses
CMAKE_CURRENT_LIST_DIRinstead ofCMAKE_CURRENT_SOURCE_DIR, thenMaterialXCore.gitignorefile is not needed anymore.I have encountered issues with it while developing a materialx port for vcpkg C++ package manager (cf. microsft/vcpkg#24614).
Those changes will leads to more reliable and reproducible results.