Modernize python - #3
Conversation
Use "pip install" instead of "python setup.py install"
| --prefix="%_SETUPTOOLS_INSTALL_PATH%" ^ | ||
| --install-scripts="%_SETUPTOOLS_INSTALL_PATH%\@CATKIN_GLOBAL_BIN_DESTINATION@" ^ | ||
| --root=%_SETUPTOOLS_INSTALL_ROOT%\ | ||
| "@PYTHON_EXECUTABLE@" -m pip install --ignore-installed --no-deps --prefix "%_SETUPTOOLS_INSTALL_PATH%" ^ |
There was a problem hiding this comment.
I think this needs a runtime dependency on python3-pip
|
@jspricke also pointed out that #2 was merged before, but is not in the tree anymore for some undocumented reason. 🙍 I'll build my local workspaces with the change to check whether I find any problems in the wider scope.
Did you consider using |
Why remove it? Leaving it in makes e.g. entry points work (which were always an issue with catkin-based Python packages), and AFAICT it does have no adverse effects besides using up a tiny extra amount of disk space. |
| @@ -1,4 +1,4 @@ | |||
| cmake_minimum_required(VERSION 3.0.2) | |||
| cmake_minimum_required(VERSION 3.16) | |||
There was a problem hiding this comment.
I assume you picked 3.16 because of Ubuntu 20.04. Do we intend to support Focal past its EOL? More generally, do we have (or should we have) a policy for minimum version requirements for ROS-O?
There was a problem hiding this comment.
No, there is no minimum OS version because there are no fixed target distributions, but if we can avoid breakage without consequences it makes sense to keep compatibility.
There is also no single branch for "ROS-O" and while compatibility is important, there is no problem to change branches in the individual builders for target platforms.
The question then is whether any "obese-devel"/"ros1" etc branches should move forward with breaking changes or whether we simply add more branches ("occult-devel" suggests itself ;)).
The egg-info folder is created in the source directory, where it shouldn't stay. I guess it is also installed, which is fine of course. |
Not yet. How to install from there? |
There was a problem hiding this comment.
Not yet. How to install from there?
You should just need to specify the wheel instead of CMAKE_CURRENT_SOURCE_DIR:
pip install ${CMAKE_CURRENT_BINARY_DIR}/build_package.whl
Again, I did not try that approach.
The egg-info folder is created in the source directory, where it shouldn't stay. I guess it is also installed, which is fine of course.
Did you test devel workspaces with this change?
In theory we might migrate to install --editable for them, but I'm sure there are non-trivial reasons why the approach catkin takes can be better in the ROS-O ecosystem.
My core workspace (with 417 basic packages on Debian testing - obviously not all with python setup) builds an install workspace with significantly less warnings with this patch. ❤️
... silencing the pip deprecation warning: DEPRECATION: Building 'xxx' using the legacy setup.py bdist_wheel mechanism, which will be removed in a future version. pip 25.3 will enforce this behaviour change. A possible replacement is to use the standardized build interface by setting the `--use-pep517` option, (possibly combined with `--no-build-isolation`), or adding a `pyproject.toml` file to the source tree of 'xxx'. Discussion can be found at pypa/pip#6334 Co-authored-by: Michael Görner <me@v4hn.de>
Tried now: ubi-agni/catkin@obese-devel...ubi-agni:catkin:python-build+pip-install @v4hn: Could you rebuild your workspace with the latest commits? |
v4hn
left a comment
v4hn
left a comment
There was a problem hiding this comment.
Why remove [the egg-info in src]? Leaving it in makes e.g. entry points work (which were always an issue with catkin-based Python packages), and AFAICT it does have no adverse effects besides using up a tiny extra amount of disk space.
@roehling The whole install logic only touches install workspaces, which need to be standalone and not devel workspaces, where it might make sense to keep files in src/ if it can help python.
It might make sense to use editable installs with devel workspaces and leave the egg-info there, but catkin uses a different awkward mechanism by overlaying all python modules/scripts with wrapper files in devel.
I guess it is also installed, which is fine of course.
@rhaschke I guess you mean the installed *.dist-info in install/?
Crucially, I went on to build my MoveIt workspaces now and hit py_binding_tools which still fails due to a bug in pybind11 that you reported upstream last year with still no response.
You also documented there that you removed the corresponding ros-o commit at that point.
That still seems like a major deal-breaker. Do we have any reasonable options to address this in the moveit repositories?
The workaround forwards the build command through pip to setup.py with an option to use the workspace build folder.
|
Status Update. I started working on a buildfarm-integration of this effort and several small things did not perfectly work out yet. There are several repositories involved. On my side this branch currently describes the automated build attempt for Debian bookworm (I know, trixi is next), listing the repositories and modified branches in the latest commits. |
if DESTDIR is empty, root must not be specified
apparently unconditionally
(debian bookworm)
4 participants
pip installinstead of deprecatedpython setup.py installDrawback: pip performs an in-place build, creating an
.egg-infofolder. This is deleted after installation.