-
Notifications
You must be signed in to change notification settings - Fork 665
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
Fix nanovdb CI and use the correct MSVC_RUNTIME library tag. #1695
Fix nanovdb CI and use the correct MSVC_RUNTIME library tag. #1695
Conversation
.github/workflows/nanovdb.yml
Outdated
- { vc: 'x64-windows-static', crt: 'MT', build: 'Release', cmake: '-A x64 -G \"Visual Studio 17 2022\" -DOPENVDB_CORE_SHARED=OFF -DUSE_STATIC_DEPENDENCIES=ON -DBLOSC_USE_EXTERNAL_SOURCES=ON' } | ||
- { vc: 'x64-windows-static', crt: 'MTd', build: 'Debug', cmake: '-A x64 -G \"Visual Studio 17 2022\" -DOPENVDB_CORE_SHARED=OFF -DUSE_STATIC_DEPENDENCIES=ON -DBLOSC_USE_EXTERNAL_SOURCES=ON' } | ||
- { vc: 'x64-windows', crt: 'MD', build: 'Release', cmake: '-A x64 -G \"Visual Studio 17 2022\" -DOPENVDB_CORE_STATIC=OFF' } | ||
- { vc: 'x64-windows', crt: 'MDd', build: 'Debug', cmake: '-A x64 -G \"Visual Studio 17 2022\" -DOPENVDB_CORE_STATIC=OFF' } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding this 'crt' entry here won't do anything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's still failing, even if I add 'crt'. I wonder if I can pass CMAKE_MSVC_RUNTIME_LIBRARY
to the cmake flag per this post: https://discourse.cmake.org/t/msvc-runtime-library-ignored-on-command-line/1644.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you apply this commit ontop of whatever changes you're making where the build breaks so it's clear what this is addressing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's trying to address this issue: https://github.com/apradhana/openvdb/actions/runs/6660668233/job/18102225571
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of using 'crt', I passed the tag -DCMAKE_MSVC_RUNTIME_LIBRARY=MultiThreaded
explicitly. It seems to fix the problem: https://github.com/apradhana/openvdb/actions/runs/6662495109
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a variable in the strategy list (in this case 'crt') won't do anything by itself. It's an entry into the CI matrix which you can then use in the job however you want, its not a special value that the CI is reading in any way.
I've just taken a look and know whats happening, you need to apply the VDB_MSVC_RUNTIME_SELECTION
variable to .cu files if you want o fix this natively. Take a look at the NANOVDB_USE_CUDA
branch in openvdb/nanovdb/nanovdb/examples/benchmark/CMakeLists.txt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the pointer, Nick. For the time being, is it OK if we changed the following two lines in nanovdb.yml
:
- { vc: 'x64-windows-static', build: 'Release', cmake: '-A x64 -G \"Visual Studio 17 2022\" -DOPENVDB_CORE_SHARED=OFF -DUSE_STATIC_DEPENDENCIES=ON -DBLOSC_USE_EXTERNAL_SOURCES=ON -DCMAKE_MSVC_RUNTIME_LIBRARY=MultiThreaded' }
- { vc: 'x64-windows-static', build: 'Debug', cmake: '-A x64 -G \"Visual Studio 17 2022\" -DOPENVDB_CORE_SHARED=OFF -DUSE_STATIC_DEPENDENCIES=ON -DBLOSC_USE_EXTERNAL_SOURCES=ON -DCMAKE_MSVC_RUNTIME_LIBRARY=MultiThreadedDebug' }
This is what is written in the code right now (after I deleted the 'crt' variable in the strategy list). In place of that, I passed -DCMAKE_MSVC_RUNTIME_LIBRARY=MultiThreaded
and DCMAKE_MSVC_RUNTIME_LIBRARY=MultiThreadedDebug
for static and static-debug build respectively. Cf this link.
The NanoVDB CI is passing on master so I'm not sure what this "fixes"? But seems fine, albeit you've submitted changes which will have no effect |
This PR is made to make #1651 pass the CI. |
Signed-off-by: apradhana <[email protected]>
d05a127
to
e36bedb
Compare
Fixing NanoVDB CI for VDB11 release: