search

ros-infrastructure / catkin_pkg

Standalone Python library for the catkin build system.
https://github.com/ros/catkin
Other
47 stars 91 forks source link

Fix argparse requirement for python 2.7 #247

Closed cottsay closed 5 years ago

cottsay commented 5 years ago

From https://pypi.org/project/argparse:

As of Python >= 2.7 and >= 3.2, the argparse module is maintained within the Python standard library.

dirk-thomas commented 5 years ago

Can you please describe in the ticket description what motivated the patch. The current version of the package is being used with Python 2.7 for many years without problems.

cottsay commented 5 years ago

I'm not sure how this hasn't been a problem in the past. If you take a look at the other ROS infrastructure packages, they have the correct conditional.

Sorry I didn't give any context, I just figured this was a typo.

dirk-thomas commented 5 years ago

The patch is certainly fine I am just trying to understand what the case was where you ran into a problem without the patch?

Also since neither of the targeted platforms uses Python 2.6 / 3.2 or older anymore maybe this dependency could just be removed?

cottsay commented 5 years ago

There is a recent effort in Fedora to generate RPM dependencies based on the dependencies expressed in the setup.py. So catkin_pkg picked up a new dependency on the virtual package python2.7dist(argparse), which is never provided by anything, so the package can't be installed.

More info on that effort: https://fedoraproject.org/wiki/Changes/EnablingPythonGenerators

CentOS 7 uses Python 2.7, but CentOS 6 uses Python 2.6. We haven't updated the CentOS 6 packages in some time, so I think it would be OK to remove the conditional as far as I am concerned. We should consider doing the same thing to the other ros-infrastructure packages if we do it here, though.

Your call - just let me know which approach you'd prefer.

dirk-thomas commented 5 years ago

There is a recent effort in Fedora to generate RPM dependencies ...

Thanks for the context.

I will go ahead and merge this then (potentially removing the dependency from various packages isn't something I will be able to do atm). Do you need this to be available in a new release or is it enough to be on master?

cottsay commented 5 years ago

Do you need this to be available in a new release or is it enough to be on master?

I added this patch to the distribution package already, so I don't think it is necessary to create a release just for this. Thanks though!