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

Add MATERIALX_BUILD_MONOLITHIC option #1725

Merged

Conversation

ld-kerley
Copy link
Contributor

Working towards building Frameworks for macOS and IOS, this PR adds the option to build MaterialX as a single monolithic library.

CMake aliases are exported for the standard library targets aliased to the monolithic library to avoid changes being necessary downstream.

@meshula
Copy link

meshula commented May 22, 2024

I'm looking forward to this change; it will make my life easier on iOS in particular, but I think the mx cmake script has already reached a threshold of complexity that IMO necessitates a refactoring. This new change consistently does essentially the same thing per library, which runs a maintenance risk, moving forward, of someone fixing a bug or making an improvement in one place but not all places. I would recommend that rather than repeating the pattern of checking for the monolithic build and setting setting project properties in every sub library, it would good to instead replace all instances of add_library with a new routine in the root CMakeLists.txt, mx_add_library (with appropriate parameters), so that the new pattern is applied identically by virtual of funneling through a single function.

@ld-kerley
Copy link
Contributor Author

Great idea @meshula - and turned out pretty clean I think - I stole some inspiration from USD. I think there is some further clean up that can be done on the MaterialXRenderXXX modules, but I'd like to merge this first and do that in a follow up PR. The extracting the common logic out to the mx_add_library() function made the commonalities easier to see.

@jstone-lucasfilm
Copy link
Member

This looks like an excellent proposal to me, thanks @ld-kerley!

Rather than adding a new Public.cmake module, what would you think about adding the new mx_add_library function directly to the root level CMakeLists.txt file, just after the existing definition of assign_source_group?

Although I'm not against the idea of adding and maintaining an external CMake helper module, my thinking is that keeping the functions local makes them more visible to developers, and that we can take the step of making them external if needed further in the future.

@meshula
Copy link

meshula commented May 24, 2024

I do find this refactoring so much easier to follow, thank you for that :)

@ld-kerley
Copy link
Contributor Author

No strong opinion either way - I was just mirroring what USD did to be honest, but if you'd prefer to avoid the external cmake helper, I'm fine with moving it over.

@jstone-lucasfilm jstone-lucasfilm changed the title Add MATERIALX_BUILD_MONOLITHIC option (default OFF) Add MATERIALX_BUILD_MONOLITHIC option May 24, 2024
Signed-off-by: Jonathan Stone <[email protected]>
Signed-off-by: Jonathan Stone <[email protected]>
Copy link
Member

@jstone-lucasfilm jstone-lucasfilm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a significant improvement to me, thanks @ld-kerley!

@jstone-lucasfilm jstone-lucasfilm merged commit e7654d4 into AcademySoftwareFoundation:dev_1.39 May 24, 2024
34 checks passed
@ld-kerley ld-kerley deleted the monolithic_build branch June 11, 2024 16:19
jstone-lucasfilm pushed a commit that referenced this pull request Jun 25, 2024
Introduced with the monolithic build PR #1725 the cmake export targets no longer had relative paths.

An errant additional `target_include_directories` slipped in.
jstone-lucasfilm pushed a commit that referenced this pull request Jul 8, 2024
The `mx_add_library` refactor introduced in #1725, introduced a few bugs with installing files.

`Generated.h` was incorrectly being ignored, instead `Generated.h.in` was being installed, and also the mdl library files were missing, also some resources were also not being installed.

Should now be corrected in both monolithic and non-monolithic builds.
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.

3 participants