rosdep: Added m2r package

[peci1]

Please add the following dependency to the rosdep database.

Package name:

m2r

Package Upstream Source:

https://github.com/miyakogi/m2r

Purpose of using this:

This package allows including Markdown files in RST docs, as e.g. mentioned here: https://stackoverflow.com/a/46012023/1076564 .

Links to Distribution Packages

• Debian: https://packages.debian.org/bullseye/m2r (buster via pip)
• Ubuntu: https://packages.ubuntu.com/jammy/m2r (bionic via pip)
• Fedora: https://packages.fedoraproject.org/pkgs/python-m2r/python3-m2r/
• Arch: not available (pip)
• Gentoo: not available (pip)
• macOS: not available (pip)
• Alpine: not available (pip)
• NixOS/nixpkgs: https://search.nixos.org/packages?channel=22.11&show=m2r&from=0&size=50&sort=relevance&type=packages&query=m2r
• openSUSE: https://software.opensuse.org/package/python3-m2r

penSUSE: python-cbor2

Checks

• All packages have a declared license in the package.xml
• This repository has a LICENSE file
• This package is expected to build on the submitted rosdistro

[peci1]

I had to force version 0.2.1 in pip on bionic because the newest version can be installed, but it is broken (probably python3-only, but released to python 2). I think the syntax I used should work, but I'm apparently the first one to do it so I'm rather checking if you think the same.

[peci1]

Hmm, this is the output of rosdep install with the PR:

executing command [sudo -H pip2 install -U m2r==0.2.1]
Collecting m2r==0.2.1
  Downloading https://files.pythonhosted.org/packages/39/e7/9fae11a45f5e1a3a21d8a98d02948e597c4afd7848a0dbe1a1ebd235f13e/m2r-0.2.1.tar.gz
Collecting mistune (from m2r==0.2.1)
  Downloading https://files.pythonhosted.org/packages/3a/c7/b0a4413a4d9b7a4fda0d710fd90dba62375f0d0c4544e848dc7656757c0c/mistune-2.0.4-py2.py3-none-any.whl
Collecting docutils (from m2r==0.2.1)
  Downloading https://files.pythonhosted.org/packages/8d/14/69b4bad34e3f250afe29a854da03acb6747711f3df06c359fa053fae4e76/docutils-0.18.1-py2.py3-none-any.whl (570kB)
    100% |████████████████████████████████| 573kB 1.9MB/s 
Building wheels for collected packages: m2r
  Running setup.py bdist_wheel for m2r ... done
  Stored in directory: /root/.cache/pip/wheels/47/f8/dc/80f56bc4abf785834d422c2f5c864a14bf34576612aeb03492
Successfully built m2r
Installing collected packages: mistune, docutils, m2r
  Found existing installation: mistune 0.8.4
    Uninstalling mistune-0.8.4:
      Successfully uninstalled mistune-0.8.4
  Found existing installation: docutils 0.14
    Not uninstalling docutils at /usr/lib/python2.7/dist-packages, outside environment /usr
Successfully installed docutils-0.18.1 m2r-0.2.1 mistune-2.0.4
ERROR: the following rosdeps failed to install
  pip: Failed to detect successful installation of [m2r==0.2.1]

Apparently installing the package is not a problem, but verifying installation is.

[peci1]

Solution for the pip version constraint provided in ros-infrastructure/rosdep#909 .

[clalancette]

Two things here:

1. This seems to be a python package, so I would have expected this to be in rosdep/python.yaml.
2. The upstream repository is now archived and says "no more updates". Should we really be adding this in at this time?

[peci1]

1. This can be used both as a Python package and as a CLI tool. That's why I've added it in base, but I can move it over to Python if you like.

2. The Ubuntu and Debian packages will of course receive maintenance during the lifetime of the respective distros. The pip package will not. Pip is used for a few distros and most importantly for Python 2.7 distros of Ubuntu and Debian. These will soon lose support, so it might be possible getting in a package that got archived in Nov 2022?

Alternatively, there is https://github.com/CrossNox/m2r2 which is a fork updated for newer versions of Sphinx. This could be used for the Python 3.7+ distros, but not for Python 2.7 (bionic) or Python 3.6 (focal). m2r2 says it is API-compatible with m2r, although it might be compatible with slightly different versions of tooling (regarding Sphinx, I'm not sure if it's compatible with Sphinx 1, or only with Sphinx 2).

So, what about the following?

m2r:
  alpine:
    pip:
      packages: [m2r2]
  arch:
    pip:
      packages: [m2r2]
  debian:
    '*': [m2r]
    buster:
      pip:
        packages: ['m2r<0.3']
  fedora: [python3-m2r]
  gentoo:
    pip:
      packages: [m2r2]
  nixos: [m2r]
  opensuse: [python3-m2r]
  osx:
    pip:
      packages: [m2r2]
  ubuntu:
    '*': [m2r]
    bionic:
      pip:
        packages: ['m2r<0.3']

This would use distro packages for m2r where available, m2r2 on the "rolling" distros which tend to be very up-to-date, and pip m2r < 0.3 for bionic and focal which use Python 2.7 (still pending ros-infrastructure/rosdep#909).

[nuclearsandwich]

@peci1 this turned into something of a big one, thanks for your patience while we review it.

1. This can be used both as a Python package and as a CLI tool. That's why I've added it in base, but I can move it over to Python if you like.

This is, I think a bit of a gut-call. I tend to follow Debian's lead, if the Debian package is python3-m2r then it's best in python.yaml. But there are other equally valid heuristics like whether the package contains a python module, or whether the key has definitions using pip.
I actually think that any rosdep key with definitions from pip belongs in python.yaml by our current practices.

I think the syntax I used should work, but I'm apparently the first one to do it so I'm rather checking if you think the same.

I flagged this in my review comments before catching up on the conversation that you tested and found the same results (and submitted a PR which potentially fixes it!) yourself. In order to review the rest of this change without a significant review and release on the rosdep side I still advocate removing this form, and thus, leaving this key undefined on bionic.

Alternatively, there is https://github.com/CrossNox/m2r2 which is a fork updated for newer versions of Sphinx. This could be used for the Python 3.7+ distros, but not for Python 2.7 (bionic) or Python 3.6 (focal). m2r2 says it is API-compatible with m2r, although it might be compatible with slightly different versions of tooling (regarding Sphinx, I'm not sure if it's compatible with Sphinx 1, or only with Sphinx 2).

Unless m2r2 is a drop in replacement which provides a compatible m2r module (and thus would likely conflict with the original m2r package I do not think they should be mixed in the same key definition and instead m2r2 should be a separate rosdep key. A package which is specifically set up to use either as appropriate could then declare the correct dependency using package.xml conditions.

[nuclearsandwich]

One of the key definitions is not compatible with current versions of rosdep and should not be merged at this time. The rest looks good as is.

[nuclearsandwich]

Suggested change

	packages: ['m2r<0.3']
	packages: ['m2r']

This form will not error during rosdep install but rosdep does not support detecting this as installed. If the "current" version of m2r does not work on Bionic and there is no packaged version, I think the best thing is leaving it undefined.

[peci1]

Thanks for the extensive feedback, @nuclearsandwich ;)

I actually think that any rosdep key with definitions from pip belongs in python.yaml by our current practices.

No problem, I'll move it there.

In order to review the rest of this change without a significant review and release on the rosdep side I still advocate removing this form, and thus, leaving this key undefined on bionic.

One more idea: I could create a rdmanifest file in some of our repos and specify source installer instead of pip. In the rdmanifest, it could be quite easy to do the pip magic correctly. Would this be acceptable?

Unless m2r2 is a drop in replacement which provides a compatible m2r module

Ah, right, I've missed that there would be a name difference... It is API compatible, but up to this "detail" :) I'll thus make one m2r-only key and one m2r2-only.

[nuclearsandwich]

One more idea: I could create a rdmanifest file in some of our repos and specify source installer instead of pip. In the rdmanifest, it could be quite easy to do the pip magic correctly. Would this be acceptable?

I am boring and prefer to keep things conventional. But I appreciate the amount of effort being put in to making this dependency available wherever possible even if "unconventional". My advice is: if you're not going to be the one using this dependency on the affected platforms, then leaving it blank for now is the way to go.

If you're working to support these platforms because you need that support there, it's another matter entirely. I would lean toward a solution that above all else: does not require a user running rosdep install to do anything else out of the ordinary in order to ensure that the setup will work and secondarily: does not require someone adding a dependency on the rosdep key to do anything unusual in their repository setup to make it work. Which is why adding an rdmanifest to a specific repository doesn't seem like a general solution to me.

In either, case you always have the option of using custom rosdep sources in addition to the upstream sources in your project! Since these are pip/source dependencies any way there is no way to use them in the official ROS package builds.

Ah, right, I've missed that there would be a name difference... It is API compatible, but up to this "detail" :) I'll thus make one m2r-only key and one m2r2-only.

Yeah, my guiding principle here is that if the source code / package itself has to be aware of the difference between two different dependency solutions then those should be different rosdep keys.

[peci1]

My advice is: if you're not going to be the one using this dependency on the affected platforms, then leaving it blank for now is the way to go.

I actually am the one who wants to use it =) And since Melodic EOL is coming, I'd like to leave the project in a good shape before the EOL.

does not require a user running rosdep install to do anything else out of the ordinary in order to ensure that the setup will work

I have the impression that source installs are handled "out of the box" by rosdep (only the buildfarm ignores them). Or am I wrong?

does not require someone adding a dependency on the rosdep key to do anything unusual in their repository setup to make it work

What do you mean by unusual? The proposed rdmanifest would just run something like python2 -m pip install 'm2r<0.3'. Pip is already installed because of rosdep, so there should really be no annoyances.

In either, case you always have the option of using custom rosdep sources in addition to the upstream sources in your project!

Sure.

Yeah, my guiding principle here is that if the source code / package itself has to be aware of the difference between two different dependency solutions then those should be different rosdep keys.

I agree.

---

You as a maintainer have the last word and I think we've already discussed everything related. So please go on and give a definitive decision whether the bionic/buster keys should be added or not.

[nuclearsandwich]

I have the impression that source installs are handled "out of the box" by rosdep (only the buildfarm ignores them). Or am I wrong?

I have no problem admitting that my own experience is so concentrated on the build farm delivery that I have no idea. All rosdistro PRs require two reviewers and I think it's time for me to call in backup early. @cottsay do you have more experience with the source definitions for rosdep? I am at the point where I would have to experiment.

You as a maintainer have the last word and I think we've already discussed everything related. So please go on and give a definitive decision whether the bionic/buster keys should be added or not.

I appreciate the acknowledgement. I'm definitely not trying to wear you down but also recognize that this has been a long one. My motivation for asking whether you were going to be using the atypical definitions was to figure out whether continuing to iterate was worthwhile. Since you are and you're trying to set Melodic packages up to go out with up-to-date support, I do not want to shut that down prematurely. If the discussion is blocking forward progress elsewhere, we could split the buster and bionic handling into a follow-up PR.

[nuclearsandwich]

@peci1 how do you want to proceed here. Due to rosdep's current behavior. I don't think that the official definitions can include a version discriminator in the key definition.

My preference would be to leave the key undefined in the official sources if the later versions are unsuitable on bionic, and allow you and others who need it to add it in via custom sources.

[cottsay]

cottsay, do you have more experience with the source definitions for rosdep?

I have none, sorry :(

[peci1]

I think the proposed solution is the best now taking into account Melodic EOL. I'll update the PR.

[peci1]

I've updated the PR as suggested.

[nuclearsandwich]

Somehow managed to dismiss my own review. It does require a second to merge.