Consolidate shared CMake logic in PyMaterialX#2713
Consolidate shared CMake logic in PyMaterialX#2713jstone-lucasfilm wants to merge 2 commits intoAcademySoftwareFoundation:mainfrom
Conversation
This changelist consolidates shared CMake logic in PyMaterialX, defining a new `mx_add_python_module` function to reduce duplication and improve maintainability.
| DESTINATION "${MATERIALX_PYTHON_FOLDER_NAME}") | ||
| mx_add_python_module(PyMaterialXGenMdl | ||
| LIBRARIES | ||
| MaterialXGenMdl) |
There was a problem hiding this comment.
Possibly add PyMaterialXGenShader for consistency with the other generators?
The simplification makes it easy to spot inconsistencies in this review.
| mx_add_python_module(PyMaterialXRenderOsl | ||
| LIBRARIES | ||
| PyMaterialXRender | ||
| MaterialXRenderOsl) |
There was a problem hiding this comment.
Inconsistent with others, would need:
PyMaterialXGenOsl
MaterialXGenOsl
ld-kerley
left a comment
ld-kerley
left a comment
There was a problem hiding this comment.
Looks like its heading in a good direction to me - sorry I had written this when you posted it but forgot to hit send
| endif() | ||
|
|
||
| # Gather source and header files | ||
| file(GLOB module_source "${CMAKE_CURRENT_SOURCE_DIR}/*.${args_SOURCE_EXTENSION}") |
There was a problem hiding this comment.
Might be cleaner - and simpler in the cmake - if we leave the file globing in the respective modules - as we do for the regular MaterialX library modules.
We wouldn't need the code to handle different file extensions, or excluded files then.
This makes it easier if there are ever special case file exceptions etc, as well as following the existing convention.
As some point in the future, personally, I think we should follow CMake recommendation, and not glob for the source files at all - but instead list them explicitly - but thats a debate for another day.
| endif() | ||
| endif() | ||
|
|
||
| function(mx_add_python_module MODULE_NAME) |
There was a problem hiding this comment.
When I added the similar functions for the main MaterialX modules it was suggested these functions should all live in the main CMakeLists.txt file for easy location. So maybe we should follow that convention.
Personally I would prefer to move all the function to a new cmake/utils.cmake file - and simplify the amount of code that live in the main CMakeLists.txt file. Certalizing all the functions - we might find places we can re-use code even more - perhaps.
3 participants
This changelist consolidates shared CMake logic in PyMaterialX, defining a new
mx_add_python_modulefunction to reduce duplication and improve maintainability.