colcon / colcon-argcomplete

Completion for colcon command lines using argcomplete
http://colcon.readthedocs.io
Apache License 2.0
2 stars 3 forks source link

Fix custom install of completion files during deb build #24

Closed cottsay closed 5 years ago

cottsay commented 5 years ago

I think this was just a little bug introduced by the most recent release.

dirk-thomas commented 5 years ago

This fails to run ros_release_python deb3.

cottsay commented 5 years ago

What version of setuptools? Also, are you specifying BUILD_DEBIAN_PACKAGE?

I believe that the 0.3.0 deb on packages.ros.org doesn't include these files because of this bug.

dirk-thomas commented 5 years ago

What version of setuptools?

The latest one: 40.6.3

are you specifying BUILD_DEBIAN_PACKAGE?

Yes, that happens automatically from the stdeb,cfg file: https://github.com/colcon/colcon-argcomplete/blob/071dc86f675612bab08a1eb0ff6e1cd3579e0857/stdeb.cfg#L5

I believe that the 0.3.0 deb on packages.ros.org doesn't include these files because of this bug.

:+1:

cottsay commented 5 years ago

Okay. I did my testing with older setuptools, and had to apply the patch manually to get the packaging script to run. Right at the beginning, the setup.py is invoked to get the package name and version. Two issues with this:

  1. It doesn't specify BUILD_DEBIAN_PACKAGE, so the version trap kills it.
  2. If I manually specify BUILD_DEBIAN_PACKAGE, the setup.cfg is malformed because the script hasn't run sdist_dsc to apply the patch to setup.cfg yet.

So right now, you need to jump through hoops to use older setuptools, which is where I did my testing. This is a separate issue, though.

When trying this patch alone with newer setuptools, it looks like sdist_dsc copies the data files to the staging location, and that causes the code path which populates dst_prefix to be skipped. Really, BUILD_DEBIAN_PACKAGE isn't necessary in this situation because we have a sufficient version of setuptools to begin with, so we can skip the CustomInstallCommand entirely.

EDIT: I didn't re-read my comment after I realized that debhelper somehow uses an older setuptools? I don't understand it...

So right now, you need newer setuptools, even though much of the BUILD_DEBIAN_PACKAGE stuff is designed specifically to work with older setuptools.

In any case, if I apply #23 as well, everything works as expected with the newest setuptools, and if I patch setup.cfg manually, it works with older setuptools as well.

dirk-thomas commented 5 years ago

had to apply the patch manually to get the packaging script to run. Right at the beginning, the setup.py is invoked to get the package name and version.

What packaging script are you referring to here?

you need to jump through hoops to use older setuptools

In order to install the hooks into share it requires setuptools 40.5.0 or higher. That is a requirement when creating the release for PyPI using ros_release_python pip.

To create a Debian package using ros_release_python deb3 it can only rely on the Debian package version of setuptools which is too old to use [options.data_files] in the setup.cfg file. That is why it needs the Debian patch removing the section from the setup.cfg files since it would otherwise fail as well as all the custom logic in the setup.py file to achieve the same installation of hooks (which is only being triggered when building the Debian package using the environment variable defined in the stdeb.cfg file).

In any case, if I apply #23 as well, everything works as expected with the newest setuptools

It is unclear to me what you try when "everything works". Can you actually invoke ros_release_python pip and ros_release_python deb3 with these two patches? As mentioned in the other ticket that fails for me.

if I patch setup.cfg manually, it works with older setuptools as well.

You should never need to apply the patch manually. stdeb called by ros_release_python does that automatically. So it sounds like you are doing something custom.

dirk-thomas commented 5 years ago

I will go ahead and merge this patch since it fixed the case that CustomInstallCommand is actually used. I will create a follow up PR to fix the Debian packaging...