Update CMake and setuptools logic for the Python bindings #233
Conversation
diegoferigo
force-pushed
the
feature/setuptools
branch
from
bdf5096 to
815a8eb
Compare
Codecov Report
@@ Coverage Diff @@
## devel #233 +/- ##
==========================================
+ Coverage 97.51% 97.90% +0.39%
==========================================
Files 49 49
Lines 1531 1531
==========================================
+ Hits 1493 1499 +6
+ Misses 38 32 -6 |
|
Hi, |
Great! I'm going to debug the failure in CI, I'm not sure why |
|
Mmh Github is blocking CI since this is my first contribution. @artivis I suspect you have to manually approve every run. I've just enabled Actions in my fork, I'll do a force push to trigger the build and continue in this way until I solve the problem. |
diegoferigo
force-pushed
the
feature/setuptools
branch
from
62e0669 to
14b96c5
Compare
There was a problem hiding this comment.
Mmh Github is blocking CI since this is my first contribution. @artivis I suspect you have to manually approve every run. I've just enabled Actions in my fork, I'll do a force push to trigger the build and continue in this way until I solve the problem.
I had approved the run already this morning. Could it be because of the force-push?
diegoferigo
force-pushed
the
feature/setuptools
branch
14 times, most recently
from
76aaff1 to
9b762bc
Compare
diegoferigo
force-pushed
the
feature/setuptools
branch
4 times, most recently
from
9f461f3 to
6613b79
Compare
diegoferigo
force-pushed
the
feature/setuptools
branch
from
adf6fe6 to
280a56b
Compare
|
I addressed all the comments and cleaned the history. There's a last remark I have to make and it regards the branching strategy that is used. The version of the Python package is determined by pypa/setuptools_scm from the closest tag found from the HEAD. Currently, What I recommend in order to get correct development tags in
|
Say no more, I'm well aware it is kind of crappy 😅 as it makes my own work more complicated. I'll somehow merge both branches asap and possibly get rid of Thanks again for iterating on this PR. |
A single branch with a quick release cycle is a great choice, especially in research :) The logic of the automatic versioning is a bit obscure and indeed makes these branching needs a bit odd, but after a couple of years of usage I have to say that it is handy, especially if you maintain a nightly channel corresponding to new commits landing the development branch (that could be
Thank you for the feedback and no problem for the delay, have a nice vacation and good rest 😉 |
| @@ -1,5 +1,5 @@ | |||
| set(PYBIND11_CPP_STANDARD -std=c++11) | |||
| pybind11_add_module(manifpy | |||
| pybind11_add_module(manifpy MODULE | |||
There was a problem hiding this comment.
Note to myself, MODULE is the default when nothing's specified.
|
Hi @diegoferigo, So I'm back on this PR and gave it a shot yesterday in a clean 18.04 container. Unfortunately the plain CMake build of the Python bindings does not work anymore. It first fails finding Python3 (not sure why since we provide the module) and then on the install rule. I've revisited the FindPython logic and will push that on a separate branch later today to run it through the CI (don't worry about fixing anything at the moment). As for the install rule, I'll open a separate comment. |
python/CMakeLists.txt
Outdated
| option( | ||
| DETECT_ACTIVE_PYTHON_SITEPACKAGES | ||
| "Do you want manif to detect and use the active site-package directory? (it could be a system dir)" | ||
| FALSE) | ||
|
|
||
| # Installation in the active site-packages folder (could be a system folder) | ||
| if(DETECT_ACTIVE_PYTHON_SITEPACKAGES) | ||
| set(PYTHON_INSTDIR "${Python3_SITELIB}/manifpy") | ||
| # Installation from the build extension of setup.py | ||
| elseif(CALL_FROM_SETUP_PY) | ||
| set(PYTHON_INSTDIR "${CMAKE_INSTALL_PREFIX}") | ||
| # Installation in the CMAKE_INSTALL_PREFIX | ||
| else() | ||
| execute_process( | ||
| COMMAND | ||
| ${Python3_EXECUTABLE} -c | ||
| "import distutils.sysconfig; print(distutils.sysconfig.get_python_lib(plat_specific=True, standard_lib=False, prefix=''))" | ||
| OUTPUT_VARIABLE _PYTHON_INSTDIR) | ||
| string(STRIP ${_PYTHON_INSTDIR} _PYTHON_INSTDIR_CLEAN) | ||
| set(PYTHON_INSTDIR "${_PYTHON_INSTDIR_CLEAN}/manifpy") | ||
| endif() |
There was a problem hiding this comment.
So, I don't quite understand this chunk. Correct me if I'm wrong but the overall idea is to choose whether to install in the CMake install path or the Python's site-package right?
Now the logic seems odd; aren't the last two blocks inverted? Shouldn't it be
elseif(CALL_FROM_SETUP_PY)
execute_process(...)
else()
set(PYTHON_INSTDIR "${CMAKE_INSTALL_PREFIX}")??
Furthermore, the variable Python3_SITELIB is provided by the FindPython3 module and is the results of distutils.sysconfig.get_python_lib(plat_specific=True, standard_lib=False) (see doc). So why do we have two distinct cases leading to the same result?
I don't know what's the common practice (if any) but shouldn't we install by default in the Python site-package?
There was a problem hiding this comment.
Scratch my previous comment about the 'if' block logic being odd, I went down the rabbit hole 😭.
Still, the CMAKE_INSTALL_PREFIX variable populated by cmake-build-extension is anything but a prefix 😅.
There was a problem hiding this comment.
Yeah sorry for replying too late, it would have saved you quite some digging 😅 For the records, in our projects we always find odd when a CMake projects installs files outside CMAKE_INSTALL_PREFIX, this is the rationale behind this logic. This is a behavior that could surprise users and third-party packagers and we try to avoid it.
However, in this specific case, using the detected site-packages folder (that could be a system folder) makes sense in some use cases. In fact, installing in a custom CMAKE_INSTALL_PREFIX, in most cases, requires the user to extend manually PYTHONPATH if they are using the system interpreter. Nowadays I believe this is not the most common case and this is the reason why the default behavior of this CMake logic in to install the bindings inside the CMAKE_INSTALL_PREFIX, where the right installation path has to be structured. Here is where distutils enters in the picture.
There was a problem hiding this comment.
No worries, it's actually a good thing that I investigated as it allowed me to better understand what's going on.
I generally agree with your point on installing things under the CMAKE_INSTALL_PREFIX but for the particular case of python bindings installed from plain CMake, I'm not sure if that makes sense and at the very least it is not convenient since it may requires (as you noted) to augment the PYTHONPATH. Now, I don't actually expect anyone to use the plain CMake install for the bindings tho, and I'd encourage using pip.
Python install fixes
| *.so | ||
| *__pycache__ | ||
| .pytest_cache | ||
| dist |
There was a problem hiding this comment.
On the long term, you can consider using the official gitignore for Python:
https://github.com/github/gitignore/blob/master/Python.gitignore
|
All green! Thanks a lot @diegoferigo for the feature and iterating this PR! Merging :) |
This PR refactors both the CMake project and the setuptools files as follows:
CMAKE_INSTALL_PREFIX, and the path is computed as recommended for third-party platform dependent projectsDETECT_ACTIVE_PYTHON_SITEPACKAGESCMake option allows to install the bindings in the active python directory (this means outside the install prefix), that could be a system foldersetup.pyhas been simplified and most of its logic moved to a modernsetup.cfgpyproject.tomlwas added to support PEP517 and PEP518setup.cfgfileRelated to #228. However, this PR does not implement any CI/CD pipeline towards PyPI.
Test this PR with:
Building the wheel: output