-
Notifications
You must be signed in to change notification settings - Fork 648
Look for boost headers for OpenVDBs older than 12 #4873
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
Look for boost headers for OpenVDBs older than 12 #4873
Conversation
|
|
bfde5db to
4b1957d
Compare
src/cmake/externalpackages.cmake
Outdated
| DEPS TBB | ||
| DEFINITIONS USE_OPENVDB=1) | ||
| # Note: OpenVDB older than 12 needs Boost headers. | ||
| if (OpenVDB_FOUND AND OpenVDB_VERSION LESS 12) |
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.
should be VERSION_LESS
|
I'm tempted to think that the extra logic here should instead all be going inside FindOpenVDB.cmake. |
4b1957d to
215dbbf
Compare
215dbbf to
8f87e2d
Compare
|
Well, that kind of backfired, now it's failing a bunch of tests. I'm not quite sure why. |
src/cmake/modules/FindOpenVDB.cmake
Outdated
|
|
||
| # Note: OpenVDB older than 12 needs Boost headers. | ||
| if (OpenVDB_VERSION VERSION_LESS 12) | ||
| checked_find_package (Boost) |
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.
| checked_find_package (Boost) | |
| find_package (Boost) |
I think it's failing because checked_find_package is not recursion-safe.
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.
Yeah, definitely. I can see how the checked_find_package is not at all safe to be called recursively.
So we either need to use find_package here instead of checked_find_package, or else move the boost logic back to externalpackages.cmake (sorry, maybe I steered you wrong).
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.
All good @lgritz I force-pushed the find_package approach and see what the CI will do - sorry I just got some time to get back to this, and if it doesn't work I'll put it back outside...
Signed-off-by: Alex Fuller <[email protected]>
8f87e2d to
12d2b01
Compare
lgritz
left a comment
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.
LGTM to me now, sorry about the runaround
…wareFoundation#4873) Signed-off-by: Alex Fuller <[email protected]>
…wareFoundation#4873) Signed-off-by: Alex Fuller <[email protected]>
…wareFoundation#4873) Signed-off-by: Alex Fuller <[email protected]> Signed-off-by: Zach Lewis <[email protected]>
…wareFoundation#4873) Signed-off-by: Alex Fuller <[email protected]>
Description
Look for boost headers for OpenVDBs older than 12. I was getting issues along the lines of this:
Not too sure if my approach is the best but it fixed it for me on Windows MSVC v143/VS2022. I was building against OpenVDB 11 but I didn't test older versions. OpenVDB 12 I did test and it wasn't needed.
Tests
Not too sure how to make one, it is more or less a compiling bugfix.
Checklist:
need to update the documentation, for example if this is a bug fix that
doesn't change the API.)
(adding new test cases if necessary).
corresponding Python bindings (and if altering ImageBufAlgo functions, also
exposed the new functionality as oiiotool options).
already run clang-format before submitting, I definitely will look at the CI
test that runs clang-format and fix anything that it highlights as being
nonconforming.