-
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
Add support for pip install #1941
Add support for pip install #1941
Conversation
749848c
to
226233c
Compare
2b346b4
to
add2475
Compare
Why is the CLA check failing? |
Just pushed a commit and it's failing for me as well, let me message John... |
/easycla |
openvdb/openvdb/CMakeLists.txt
Outdated
ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR} | ||
) | ||
endif() | ||
if(SKBUILD) |
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.
Wondering if there's a cleaner way to handle this kind of case without all of this conditional branching. What about using an INSTALL_PREFIX and including the forward slash in the prefix (although potentially needs thought for multi-platforms):
if (SKBUILD)
set(INSTALL_PREFIX "openvdb/")
else
set(INSTALL_PREFIX "")
endif()
install(TARGETS openvdb_static
RUNTIME_DESTINATION ${INSTALL_PREFIX}${CMAKE_INSTALL_BINDIR}
)
Ultimately, it would be nice to use less if (SKBUILD)
logic if possible?
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, I'm not a huge fan of all the SKBUILD
logic either. I'm happy to take another look.
In an ideal world, it'd be possible to separate them entirely, but in practice Python packaging has very limited support for two modules in a single package. Thus, we need to delegate a lot of the pip install location logic to CMake since Python just doesn't provide us with the expressiveness to do so hence the SKBUILD
custom logic.
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 suggestions @danrbailey ! I've consolidated the conditional branching significantly. Please take another look and let me know what you think.
add2475
to
2c46af8
Compare
Signed-off-by: Matthew Cong <[email protected]>
Signed-off-by: Matthew Cong <[email protected]>
2c46af8
to
40d1089
Compare
Signed-off-by: Matthew Cong <[email protected]>
717a157
to
18ad210
Compare
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.
This is much better, thanks Matt! Just one small comment which I think is probably a find-replace typo...
set(NANOVDB_INSTALL_MATH_DIR ${NANOVDB_INSTALL_INCLUDE_DIR}/math) | ||
set(NANOVDB_INSTALL_TOOLS_DIR ${NANOVDB_INSTALL_INCLUDE_DIR}/tools) | ||
set(NANOVDB_INSTALL_ROOT_DIR ${NANOVDB_INSTALL_INCLUDEDIR}/nanovdb) | ||
set(NANOVDB_INSTALL_CUDA_DIR ${NANOVDB_INSTALL_ROOT_DIR}/cuda) |
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.
There are a few of these variables that are swapping INCLUDE paths for ROOT paths?
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.
Not a typo I think.
Prior to this change, NANOVDB_INSTALL_INCLUDE_DIR
(note the underscore between include and dir) was already defined. I've renamed NANOVDB_INSTALL_INCLUDE_DIR
to NANOVDB_INSTALL_ROOT_DIR
in order to avoid confusion with the newly introduced/very similarly named NANOVDB_INSTALL_INCLUDEDIR
(that defaults to CMAKE_INSTALL_INCLUDEDIR
).
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.
Ah, that makes sense, thanks for explaining.
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.
Tested on Linux with the latest changes. LGTM.
58c8d3a
into
AcademySoftwareFoundation:master
This PR adds preliminary support for pip install. Assuming that the dependencies required for building OpenVDB and NanoVDB are installed, then
pip install <openvdb_source_root>
willsite-packages
directoryimport openvdb
andimport nanovdb
when launching Python.This is a first-step towards making installation of OpenVDB easier for new users, and will help users dive into the library via the Python interface.