Skip to content

python/CMakeLists.txt: Fix install destination - #184

Open
podsvirov wants to merge 1 commit into
google:masterfrom
podsvirov:cmake-python-install-destination
Open

python/CMakeLists.txt: Fix install destination#184
podsvirov wants to merge 1 commit into
google:masterfrom
podsvirov:cmake-python-install-destination

Conversation

@podsvirov

@podsvirovpodsvirov commented Apr 18, 2021

Copy link
Copy Markdown
Contributor

Added CMAKE_INSTALL_PYLIBDIR cached variable.
Default value retrivet from Python3_EXECUTABLE.

Current install destination Python3_SITELIB (absolute path from FindPython3 module).
An absolute path is not acceptable when creating an installation package.

Comment threadsrc/python/CMakeLists.txt Outdated
set_property(TEST pywraps2_test PROPERTY ENVIRONMENT
"PYTHONPATH=$ENV{PYTHONPATH}:${PROJECT_BINARY_DIR}/python")

set(CMAKE_INSTALL_SITELIBDIR "${Python3_SITELIB}"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling this CMAKE_ seems a bit strange. I would assume that's only for variables defined by CMake itself.

Can you point me at some CMake Python documentation or a canonical example? It seems no one has used this variable name before.

https://www.google.com/search?q=%22CMAKE_INSTALL_SITELIBDIR%22
https://github.com/search?q=CMAKE_INSTALL_SITELIBDIR

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, what problem are you trying to solve? Use of this variable should also be documented somewhere.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jmr, please review #186 (this should clarify my motivation).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like there should be a relevant directory somewhere here:
https://cmake.org/cmake/help/latest/module/FindPython3.html

How do other swig/python/windows/cmake projects do it?

@jmr

jmr commented Apr 22, 2021

Copy link
Copy Markdown
Member

Does #186 make this unnecessary?

@podsvirov

Copy link
Copy Markdown
ContributorAuthor

These changes are not directly related to the #186. These changes solve the problem of hard-coded absolute installation path for the python module. Absolute install paths add additional problems when creating packages for distribution (in this case, it becomes necessary to install to a path other than the default).

@podsvirov
podsvirovforce-pushed the cmake-python-install-destination branch from 2fe6579 to 2ca43b5CompareApril 27, 2021 21:19
@podsvirov
podsvirov requested a review from jmrApril 27, 2021 21:26
@jmr

jmr commented Jul 10, 2025

Copy link
Copy Markdown
Member

This has merge conflicts. Is it still relevant?

@podsvirov
podsvirovforce-pushed the cmake-python-install-destination branch from 2ca43b5 to 893680dCompareJuly 10, 2025 18:44
@podsvirov
podsvirovforce-pushed the cmake-python-install-destination branch from 893680d to cc27ffcCompareJuly 10, 2025 18:46
@jmr

jmr commented Jul 11, 2025

Copy link
Copy Markdown
Member

What do other Python people think? @andyneff

@andyneff

Copy link
Copy Markdown
Contributor

It seems fine to me. Normally I would try to do the string manipulation in cmake, but I don't see an obvious way to have the python's sys.prefix without calling python (which will often be different from the normal prefix /usr).

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@podsvirov@jmr@andyneff