remove dependency on ament_python and perform customizations in setup.py #158
Conversation
demo_nodes_py/setup.py
Outdated
|
|
||
| # install scripts into libexec path | ||
| if 'develop' in sys.argv: | ||
| sys.argv[sys.argv.index('develop') + 1:sys.argv.index('develop') + 1] = \ |
There was a problem hiding this comment.
This appears to insert the arguments but I don't see how it replaces a script-dir argument already part of argv the way that the ament_python version of this code did. https://github.com/ament/ament_python/blob/eafd268376cda400f68b1b5b8429986531b856ad/ament_python/script_dir.py#L27-L29
Same for the install command just below and the install-scripts argument which we do specify during debian packaging in order to make sure the non-libexec scripts are placed into /opt/ros/$ROSDISTRO/bin
topic_monitor/setup.py
Outdated
|
|
||
| # install scripts into libexec path | ||
| if 'develop' in sys.argv: | ||
| sys.argv[sys.argv.index('develop') + 1:sys.argv.index('develop') + 1] = \ |
There was a problem hiding this comment.
Same question as above (#158 (comment)). How does this replace a manually specified script-dir argument?
There was a problem hiding this comment.
It doesn't. But I don't think it needs to either. ament/ament_tools#156 removes passing those from the build tool. So if no custom location is passed the package installs it's scripts into libexec. If a custom location is passed it is being honored (imo better than being ignored/overridden as it was before). The Debian rules file will need to do the same as the build tool (pass a prefix but no script specific location) and the behavior should not change.
There was a problem hiding this comment.
The Debian rules file will need to do the same as the build tool (pass a prefix but no script specific location) and the behavior should not change.
I'm not sure that this will work. Earlier iterations of the bloom changes passed only the prefix and that was not sufficient to put everything in the right place. I needed to pass both --install-lib and --install-scripts explicitly. Blooming this branch in a throwaway repository, patching out the explicit --install-scripts and using a local binary job script would be enough to indicate whether or not this would actually work.
There was a problem hiding this comment.
I'm not sure that this will work.
Please try this PR together with the one on ament_tools. It installs the scripts into the libexec path as desired. Since that works I am pretty confident the Debian rules file is able to achieve the same behavior by invoking the same command.
There was a problem hiding this comment.
this PR together with the one on ament_tool
ament tools isn't part of the debian package build. Can you explain why the PR to ament_tools would be needed to validate a debian build with this change?
With dh_python responsible for the direct packaging actions, we don't actually get to invoke setup.py directly. Instead, arguments are passed through the exported environment variables. [1].
The link above is to the ROS 2 bloom fork, the in-progress refactor uses separate templates for the different build types but interaction with setuptools via pybuild environment variables is still used. The ROS 2 bloom fork initially installed python packages into the default debian filesystem hierarchy, after some local testing, https://github.com/ros2/bloom/commit/9969bc18b0890324884e3763c864ace860f26945 and a typo fix for it, https://github.com/ros2/bloom/commit/22e272014ed96e720f74b6363914bb80dd54396d were added. In local testing the prefix alone wasn't sufficient to install python library files in the ROS installation prefix.
We added the install scripts together while I was in CA with https://github.com/ros2/bloom/commit/31fee87b20f6dfbbcf01aedc7776f8f50c64eab4 and its escape fixes https://github.com/ros2/bloom/commit/f221ee781754f661b03e9baec7cbc019d1cc0ce7 because the setting of --prefix alone wasn't enough to put the non-libexec binaries in the ROS installation prefix.
Unless the reason prefix was not being honored is identified and resolved for Bloom merging this as-is will require maintainers who want their scripts installed as libexec rather than bin will need to modify the bloom generated templates.
There was a problem hiding this comment.
ament tools isn't part of the debian package build. Can you explain why the PR to ament_tools would be needed to validate a debian build with this change?
I wasn't saying that. I just wanted to point out that you can take a closer look to how ament_tools builds Python packages. Since it works in that case the Debian rules file should be able to replicate the same invocations and achieve the same behavior.
We added the install scripts together while I was in CA with ros2/bloom@31fee87 and its escape fixes ros2/bloom@f221ee7 because the setting of
--prefixalone wasn't enough to put the non-libexec binaries in the ROS installation prefix.
Please try a local build with the modified ament_tools package. It doesn't pass the --install-scripts argument explicitly yet all executables are installed into the right location (at least for me). If you see a different behavior we need to debug why that is the case. But if that works for you too it should be possible to also achieve the same behavior from the Debian rule (without the need to pass that argument).
|
@nuclearsandwich can you try updating the rules to test the approach suggested in these PRs and we will then propagate and get rid of ament_python dependency in the codebase |
Yes, as it happens I just finished setting up the reproduction. To try it yourself:
I understand the behavior using only When I investigated this issue previously, I found that the explicit |
|
I didn't mention Your above example proves that the executables are installed into the |
|
@nuclearsandwich Any update on this? |
Yes, but this was never in dispute. The precise issue I am raising is that we cannot remove the explicit
It's not related to the script location but it is absolutely related to the feedback I've been providing. Specifically that the assertion (made here: #158 (comment)) that The reproduction for demo_nodes_py show in a concise fashion that bloom changes you would mandate in this PR do not create a correct package. When either of For further clarification, I've created a second demonstration of why changes requested in bloom for this PR to be sufficient do not work. Below is a list of package contents for ros2cli with the changes to bloom that would be required to support this PR as-is. You can reproduce it following the steps above substituting the package name Here are the package contents: With only This PR currently will not replace an earlier edit: corrected bloom commit link |
Can you please share the steps how to reproduce this. Thanks. |
|
dirk-thomas
force-pushed
the
python_package_installation
branch
from
4c4a3ca
to
e1850fd
Compare
….py and setup.cfg
dirk-thomas
force-pushed
the
python_package_installation
branch
from
e1850fd
to
2fc9cc9
Compare
|
The missing piece of information was in
The generated From that file only Changing the cfg generated by Instead I updated this PR as well as ros2/examples#178 to configure the libexec location for the scripts in a package specific |
I tested this in two configurations. To reproduce them you can fork https://github.com/nuclearsandwich/build-python-repro-testing and set the revision of demo_nodes_py in rosdistro.yaml and r2b2-rosdistro-cache.yaml.
Using a setup.cfg file is definitely a clearer, preferable solution to inserting command line arguments. The setup.cfg is still overridden by the explicit command line arguments passed via the current Bloom debian/rules template. In order to honor the setup.cfg we'll need to remove I am also strongly (like 9/10) in favor of adding install-lib to the setup.cfg as well. It keeps the template magic to a minimum and keeps the prefix-relative setup options together. Since every python package needs a setup.cfg whether it is installing scripts to bin or libexec, it doesn't increase the number of packages we need to modify to enact this change. |
|
I don't think a package developer should need to specify
No package has a setup.cfg file at the moment. And only the ones which customize the script install location need one. |
Can you show your work here? What combination of debian/rules template and setup.cfg allows packages installing into the ROS prefix bin not to need a setup.cfg? |
The current set of PRs together with the removal of Only the packages which want to install scripts into libexec have a [ |
Are you proposing we remove --install-scripts from the bloom templates in source or patch them out for packages using libexec. Removing |
You are right. So the |
To my knowledge or @wjwwood's bloom doesn't currently look at package contents beyond the package.xml and I am not sure from a hygiene / design perspective whether we really want it testing file presence let alone parsing other package files. Everything else bloom needs comes from the package.xml. Could we put something there to indicate either 1. where to install scripts (defaulting to bin) or 2. to leave options off and trust that a setup.cfg is present. |
I don't think this information belongs into the manifest.
That would require every package to have a |
|
Offline discussion lead to the conclusion that we will have bloom intelligently add the scripts argument if there is no
We considered requiring all python packages have a |
|
Based on the conclusion it would be great if someone can review ament/ament_python#3 as well as all connected PRs so that they can get merged. The changes to |
See the discussion in https://github.com/ros2/bloom/pull/2 and ros2/demos#158
Connect to ament/ament_python#3.
The same changes are necessary for the
example_rclpy_*packages. Once we agree this is the right way to move forward I will create a PR for those.Update: ros2/examples#178