-
Notifications
You must be signed in to change notification settings - Fork 1.4k
cmake: add support for staged install dirs #3935
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
base: dev
Are you sure you want to change the base?
Conversation
|
Filed as internal issue #USD-11756 ❗ Please make sure that a signed CLA has been submitted! (This is an automated message. See here for more information.) |
|
@davvid - so for: cmake: add support for staged install dirs PixarAnimationStudios/OpenUSD#3935 ...it's not immediately clear how the PR relates to https://github.com/aousd/build-ig-initiatives/issues/15 - ie, making the pxrConfig.cmake portable. Can you elaborate on that? Also - personally, I'm not a fan of using env vars - they end up creating "secret inputs" that are hard to track down. Could you add a cmake option/variable instead? EDIT: Ok, I see now that DESTDIR as an env var is something of a standard: https://www.gnu.org/prep/standards/html_node/DESTDIR.html However, it's only useful if something is setting DESTDIR - and it seems like more of a standard for makefiles. Is setting DESTDIR something that CMake itself generally does? Is there standard tooling used in the default USD build pipeline (or your build pipeline) that sets it? Or is the assumption that the end user sets it themselves before launching a build (as opposed to a tool setting it)? EDIT 2: Ok, I'm educating myself here - DESTDIR env is explicitly supported by cmake: https://cmake.org/cmake/help/latest/variable/CMAKE_INSTALL_PREFIX.html#variable:CMAKE_INSTALL_PREFIX
|
cmake/macros/Public.cmake
Outdated
| --package pxr --module ${pxrPythonModulesStr} \ | ||
| --inputIndex ${BUILT_XML_DOCS}/index.xml \ | ||
| --pythonPath ${CMAKE_INSTALL_PREFIX}/lib/python \ | ||
| --pythonPath \$ENV{DESTDIR}${CMAKE_INSTALL_PREFIX}/lib/python \ |
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 seems odd that we add the DESTDIR prefix to the python path here - where it's presumably supposed to find the USD python libs that have already been written out - without a corresponding change to the code for where those libs get written out (or installed). Is there existing CMake machinery that already incorporates DESTDIR when writing/installing the python libs?
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.
Yes, cmake in both Ninja and Makefile mode both honor DESTDIR. This is used heavily by Linux distro packaging (where the install prefix, e.g. /usr) is not writable, so we want to be able to write into a temporary "staging" directory from which the packages are created.
You'll notice that I didn't have to touch other parts -- that's because the rest of the build already supports DESTDIR. It's just these parts that are deficient.
It's also used by packaging systems that are familiar with this standard.
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.
gotcha, makes sense
cmake/macros/Public.cmake
Outdated
| --package pxr --module ${pxrPythonModulesStr} \ | ||
| --inputIndex ${BUILT_XML_DOCS}/index.xml \ | ||
| --pythonPath ${CMAKE_INSTALL_PREFIX}/lib/python \ | ||
| --pythonPath \$ENV{DESTDIR}${CMAKE_INSTALL_PREFIX}/lib/python \ |
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 we make these changes conditional on not being on windows?
DESTDIRisn't a convention on windows (and worst case, someone may be using it for something else there)- On windows, if
CMAKE_INSTALL_PREFIXis an absolute path likeC:\some\dir, prependingC:\destdirin front will result in an invalid path - the CMake docs for
DESTDIRexplicitly say not to use DESTDIR on windows
We could either do that here, or add a --destdir flag to the python command...
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.
I hadn't considered adding a --destdir flag. Handling it in the script is appealing because it lets us avoid having a complicated conditional in the cmake code and it should be easier to disable the feature inside of the script when it's running on Windows.
If we wanted to make it fully insulated, maybe we could not even have a command-line flag at all. We could just make it implicit (just like the rest of the cmake DESTDIR support) and it'd enable itself when the environment variable is defined and we're running on non-Windows.
What do you think about pushing this down logic down into the script without adding a --destdir flag? This would effectively keep the environment variable as the opt-in mechanism, but only on non-Windows.
Add DESTDIR support to the convertDoxygen.py script. DESTDIR is supported by cmake's generators on UNIX but is not recommended on Windows. References: https://www.gnu.org/prep/standards/html_node/DESTDIR.html
5abf08d to
45a2050
Compare
|
I just pushed an update. The logic is now contained within the script and it only enables this feature when on posix (unix) platforms. Note for reviewers ~ the install prefix is already an absolute path (starts with |
Add DESTDIR support to the documentation steps.
Related-to: https://github.com/aousd/build-ig-initiatives/issues/15