Generate Setuptools Dict Helper Method

[DLu]

Fix for #122. catkin_pkg.python_setup.generate_distutils_setup is a very useful tool in catkin for ensuring that you don't need to fill in redundant data in the package.xml and setup.py. This PR builds on that structure to generate the same sort of dictionary for ament packages.

Signed-off-by: David Lu!!

[tylerjw]

Looks really useful. It probably needs a license/copyright comment at the top. You use ament_copyright to add that for you.

[DLu]

What do you think @mabelzhang / @audrow ?

[audrow]

Thanks for the PR. This looks useful to me.

A couple thoughts:

• I'd like tests to be written for it.
• I assume that functions other than generate_setuptools_dict will not be used outside of this file. It probably makes sense to indicate that the are private with a leading underscore (_).

I'd also like some sort of documentation for this feature. Although, I'm new to maintaining ament and I'm not that familiar with it's documentation. Where do you think it would make sense to document this? Maybe in the creating a package tutorial on ROS2 index?

[audrow]

Looks pretty good.

I think it would be more descriptive to name the test file test_generate_setuptools_dict.py.

Also, I ran CI and there are a few linter errors in the test file. I think the easiest way to get around this is to use pytest instead of unittest in the test file. Basically this involves using functions instead of classes and using assert instead of the unittest functions like assertEqual. Here's CI if you want to take a look:

• Linux

[audrow]

Looks good. I'll merge it with green CI.

[audrow]

• Linux
• Linux-aarch64
• macOS
• Windows

[audrow]

Actually, on giving this one more look over, would you change ament_package/python_setup.py to something more descriptive of what is in the file, such as ament_package/generate_setuptools_dict.py? (I should have mentioned this in my earlier review.)

[audrow]

• Linux
• Linux-aarch64
• macOS
• Windows

[audrow]

@DLu, thanks for the PR!

[nuclearsandwich]

This PR has a critical problem when it comes to releasing packages onto the ROS buildfarm. sourcedeb jobs which create our debian source packages are designed to run without any dependencies beyond the Debian toolchain and as a result any package trying to use import non-standard library packages in setup.py will fail when the clean stage of the source package build is run.

In ROS 1, the package supplying this library, catkin_pkg is a necessity of the buildfarm scripts which run inside the containers and so is present for the sourcedeb jobs. The scripts in the current build farm go back to 2014 and it's unclear if the addition of generate_distutils_setup was meant to be intentionally covered by the blanket installation of catkin-pkg or not.

[christophebedard]

sourcedeb jobs which create our debian source packages are designed to run without any dependencies beyond the Debian toolchain and as a result any package trying to use import non-standard library packages in setup.py will fail when the clean stage of the source package build is run.

I thought that this was what <build_depend>, or at the very least <buildtool_depend>, was for.

[nuclearsandwich]

I thought that this was what <build_depend>, or at the very least <buildtool_depend>, was for.

Those declarations ensure that the packages are available at build time but source package creation is a pre-build time process which we previously have not had dependencies installed for (and for which installing any build time dependencies would significantly affect sourcedeb creation time. This issue is present in Python projects generally since anything there is no way to know what must be installed in order to process setup.py if you can't first process setup.py. As far as I know all Python solutions to this problem involve some out-of-band method to indicate required dependencies of setup.py itself.

This problem is what led to shuttering of the https://github.com/ament/ament_python library which was an earlier attempt to add helpers for ament python packages.

This helper works fine in a colcon build context or anywhere that uses package.xml and not setup.py for resolving dependencies but does not work in contexts where setup.py alone is used.

I would like to make the following proposal and I'm very interested in feedback from @DLu, @audrow, and @christophebedard.

Given that this helper cannot be used on the buildfarm at this time, and that I pretty much consider the ROS infrastructure in a "platform freeze" as we prepare a new ROS distribution for release I would suggest either raising an error when the function is used or reverting its inclusion altogether in order to avoid situations where packages are built around it and hit this wall when they perform a buildfarm release.

I recognize that the only problem that resolves is the buildfarm strife and the original goal of this change was to remove the necessity of keeping your ROS package.xml and Python setup.py synchronized.

To that end I think it is worth beginning a discussion about what would be required to provide helper functions like this which may be used in setup.py via a resurrected ament_python package. As for why ament_python and not ament_package, I have a bunch of thoughts behind that feeling but I think they could wait since the eventual home of these helpers is a secondary concern to them having a functional place at all.

What do folks think about that?

[audrow]

Thanks for the explanation. I didn't understand all of this before.

I would suggest either raising an error when the function is used or reverting its inclusion altogether in order to avoid situations where packages are built around it and hit this wall when they perform a buildfarm release.

Either is fine to me. A revert seems preferable. We can then have a discussion about how to better have helper functions after the Galactic release.

[christophebedard]

Same, I wasn't aware of this. Thanks for the explanation!

Given that this helper cannot be used on the buildfarm at this time, and that I pretty much consider the ROS infrastructure in a "platform freeze" as we prepare a new ROS distribution for release I would suggest either raising an error when the function is used or reverting its inclusion altogether in order to avoid situations where packages are built around it and hit this wall when they perform a buildfarm release.

That's fine with me! I will revert/remove the use of generate_distutils_setup for now.

I agree that it is worth having this discussion (after Galactic). I would very much like to avoid having to keep package.xml and setup.py in sync, but yeah other than that I don't have much to say.

the eventual home of these helpers is a secondary concern to them having a functional place at all.

👍

[DLu]

It sure would have been nice to know this back when I raised the original issue. #122

I think revert makes sense, since this cannot be used, but its intensely frustrating, as I was hoping to get the fix into both foxy and galactic.

The fact that we have to go back and reimplement work that work that has been done twice already (once in ROS1, and another in ament_python) is why I am continually reluctant to use ROS2. I would be interested to hear what we gain from building debians without any external dependencies.

[nuclearsandwich]

I think revert makes sense, since this cannot be used, but its intensely frustrating, as I was hoping to get the fix into both foxy and galactic.

It's unfortunate that it wasn't noticed earlier. The internal workings of the buildfarm aren't meant to be required knowledge but whenever making changes that affect packaging it sometimes is. I'm sorry the issue here went undetected until it got put into practice, to both yourself and @christophebedard.

The fact that we have to go back and reimplement work that work that has been done twice already (once in ROS1, and another in ament_python) is why I am continually reluctant to use ROS2.

There's a documentation gap which I want to start addressing with ros2/ros2_documentation#1385 but we've been talking about the developer experience gap between ROS 1 and ROS 2 recently and this definitely fits into that conversation. I appreciate the feedback on both counts and I hope that ros2/ros2#1132 puts us on a path to close the gap