Fix CMake build rule for resource installation#2687
Fix CMake build rule for resource installation#2687jstone-lucasfilm merged 5 commits intoAcademySoftwareFoundation:mainfrom
Conversation
|
This looks very promising, thanks @kwokcb, and I'll just add a note that we should time our build process before and after this change, to make sure we're maintaining build performance. |
|
It doesn't have any affect on the initial build since resources are always installed so CI build time won't change. To check however, I put a timer in the build step and get this so it's pretty negligable. Copying OSL utilities to build directory...
Elapsed time (seconds): 0.00699933
[ 68%] Built target MaterialXTestOslUtilities
[ 68%] Copying resources to build directory
Copying resources to build directory...
Elapsed time (seconds): 0.0909475I could check in the timer code but don't think it's needed. |
Reduce overhead of resources copy to only copy if files were modified or created.
|
After chatting with @lkerley on Slack, I've made the small change to only copy if files have changed to minimize the time spent in case the file system is slower. |
| ${CMAKE_CURRENT_SOURCE_DIR}/../../resources ${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/resources) | ||
| # Copy resources to build directory - always runs during build | ||
| add_custom_target(MaterialXTestResources ALL | ||
| COMMAND ${CMAKE_COMMAND} -E copy_directory_if_different |
There was a problem hiding this comment.
Copy only files which have changed.
|
I've done some timings on my machine which is a 4 yr old Alienware personal laptop. Clean builds show about 2 seconds for resources. libraries which we already copy is about a second. Incremental builds are about 1 second. // Clean build
[MaterialXTestData] Copying data libraries...
Elapsed time (seconds): 0.944269
Copying OSL utilities to build directory
[MaterialXTestOslUtilities] Copying OSL utilities...
Elapsed time (seconds): 0.0731214
Copying resources to build directory
[MaterialXTestResources] Copying resources...
Elapsed time (seconds): 1.86759
[MaterialXTestData] Copying data libraries...
Elapsed time (seconds): 0.951778
Copying OSL utilities to build directory
[MaterialXTestOslUtilities] Copying OSL utilities...
Elapsed time (seconds): 0.0723189
Copying resources to build directory
[MaterialXTestResources] Copying resources...
Elapsed time (seconds): 1.87808
// Incremental build
[MaterialXTestData] Copying data libraries...
Elapsed time (seconds): 0.206294
Copying OSL utilities to build directory
[MaterialXTestOslUtilities] Copying OSL utilities...
Elapsed time (seconds): 0.0529004
Copying resources to build directory
[MaterialXTestResources] Copying resources...
Elapsed time (seconds): 0.728033
[MaterialXTestData] Copying data libraries...
Elapsed time (seconds): 0.194733
Copying OSL utilities to build directory
[MaterialXTestOslUtilities] Copying OSL utilities...
Elapsed time (seconds): 0.0547186
Copying resources to build directory
[MaterialXTestResources] Copying resources...
Elapsed time (seconds): 0.705728 |
ld-kerley
left a comment
There was a problem hiding this comment.
I've been running with this locally for a little bit and it's much better than what we currently have - thanks for putting this together.
jstone-lucasfilm
left a comment
There was a problem hiding this comment.
This looks good to me in local tests, thanks @kwokcb!
f50c349
into
AcademySoftwareFoundation:main
Issue
The post build step for MaterialXTest does no trigger so resources are not copied to the
build area on a build.
This means developers must manually copy this over when they are changing any files affecting the resources folder. If the lack of copy is not noticed / done then it can get confusing as to why source changes are not reflected in the build tests.
Change
Add an explicit target for resource copy and make MaterialXTest dependeng on it.