Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for downloading a python3 version of the rosdep database. #607

Closed
wants to merge 2 commits into from

Conversation

clalancette
Copy link
Contributor

@clalancette clalancette commented May 18, 2018

What we do here is to add an alternate version of https://raw.githubusercontent.com/ros/rosdistro/master/rosdep/sources.list.d/20-default.list, called https://raw.githubusercontent.com/ros/rosdistro/master/rosdep/sources.list.d/20-default-py3.list . The 20-default.list stays the same, while the 20-default-py3.list points to a yaml file called python3.yaml (example). That version of the yaml file maps rosdep keys to their python3 equivalents. python3.yaml is only used if the ROS_PYTHON_VERSION environment variable is set to 3, otherwise it defaults to using the standard python.yaml (in order to use Python 3 successfully, users must have ROS_PYTHON_VERSION set to 3 both during sudo rosdep init and during rosdep update).

Doing it this way has a few different benefits and drawbacks.

Benefits:

  • Keeps using existing Python 2 support for the case where ROS_PYTHON_VERSION is not set and where it is set to 2.
  • The Python 3 keys are kept in an entirely different database so there is no confusion in the python.yaml file.
  • To switch over to Python 3 by default in a future ROS release, we only have to change the 20-default.list file to point to the python3.yaml file, and change ROS_PYTHON_VERSION to be 3 over in ros_environment.

Drawbacks:

  • In order to switch over to the Python 3 version of the rosdep database, users must do a few things:
    • Remove ~/.ros/rosdep/sources.cache/*
    • Remove (using sudo) /etc/ros/rosdep/sources.list.d/20-default.list
    • Set ROS_PYTHON_VERSION to 3 in their user account.
    • Set ROS_PYTHON_VERSION to 3 in the root account.
    • Run sudo rosdep init
    • Run rosdep update
  • We are not using the <os>_python3 keys that are already in the existing python.yaml file (e.g. https://github.com/ros/rosdistro/blob/master/rosdep/python.yaml#L226)

@dirk-thomas
Copy link
Member

Can you please add a description to this PR how this is expected to be used and what the pros/cons of this approach are.

Signed-off-by: Chris Lalancette <[email protected]>
@codecov-io
Copy link

codecov-io commented May 18, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@30a631f). Click here to learn what that means.
The diff coverage is 81.25%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #607   +/-   ##
=========================================
  Coverage          ?   75.31%           
=========================================
  Files             ?       30           
  Lines             ?     2892           
  Branches          ?        0           
=========================================
  Hits              ?     2178           
  Misses            ?      714           
  Partials          ?        0
Impacted Files Coverage Δ
src/rosdep2/main.py 48% <0%> (ø)
src/rosdep2/sources_list.py 87.32% <92.85%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 30a631f...43b05a9. Read the comment docs.

@clalancette
Copy link
Contributor Author

Can you please add a description to this PR how this is expected to be used and what the pros/cons of this approach are.

I updated the description with more information. I'll probably add a few more things presently, but it is a start.

@dirk-thomas
Copy link
Member

Since the drawback of switching the rosdep database is significant I would like to see a comparison to alternative approaches (naming your proposed approach A).


Currently we use a _python3 suffix behind the distribution name for the rosdep keys to identify Python 3 packages (naming this approach B). That approach has the disadvantage of requiring to replicate a lot of rosdep keys since Python 2 and Python 3 mappings are defined under the same OS key. Also the wildcard feature can't be used in this scenario. E.g.:

python:
  ...
  ubuntu:
    bionic: [python-dev]
    bionic_python3: [python3-dev]
    ...
    trusty: [python-dev]
    trusty_python3: [python3-dev]
    ...
    xenial: [python-dev]
    xenial_python3: [python3-dev]
    ...

Another alternative (named C)would be to "detect" a different OS name (e.g. ubuntu3). That would allow to keep the Python 2 and Python 3 packages separate and each group could utilize the wildcard feature.

This approach would require some kind of "fallback" though in order to not require duplicating all rosdep keys for non-Python packages.


How difficult would e.g. C be over A? Would the extra effort for updating rosdep be justified by the advantage? Maybe there are other alternatives not mentioned here?


Also if we consider to switch to Python 3 by the Ubuntu release 20.04 it would be straight forward to use Python 3 packages from that Ubuntu distro forward (avoid any duplication). So the usefulness of any solution could be limited to the transition period.

@clalancette
Copy link
Contributor Author

I'm going to answer the last question first:

Maybe there are other alternatives not mentioned here?

We originally had 6 ideas:

  1. Create parallel “python3.yaml” alongside “python.yaml” in rosdistro.
  2. Add a different set of “OS codename” that are “_python3” (e.g. “xenial_python3”), leave the keys the same, then let rosdep choose between them.
  3. Add a different set of “OS name” that are “_python3” (e.g. “ubuntu_python3”), leave the keys the same, then let rosdep choose between them.
  4. Add rosdep keys with “python2” and “python3” prefixes, then use conditional dependencies in package format 3 to manage them.
  5. Leave the keys as they are today (‘python-foo’), then for OSs that we want to be Python3, have those map to the python3 packages.
  6. Hybrid of d and e - Have python-foo keys map to default for OS (python2 for Xenial, etc, python3 for 20.04, etc). Optionally, additionally, have python3-foo key that is explicitly python3, and python2-foo key that is explicitly python2. Have linter that complains if package.xml has “magic” ‘python-foo’ key outside of conditional. Provide python3.yaml file temporarily for testing ahead of time (to replace python-foo keys with python3 versions).

During our discussion, there was a lot of support for idea 6 above, which this PR moves us towards (I'll note that this is also similar to Number 1, with the difference that in number 6, python3.yaml is temporary). To finish it off we'd have to additionally add in more keys to both python.yaml and python3.yaml, but in that scenario, python3.yaml goes away during N-Turtle (or whenever we make the hard-cut over to Python 3). Number 2 is what we currently have today, but we don't think it is ideal. You are suggesting Number 3, which is probably doable but unclear on whether it is better. Number 4 was too much work for consumers of the rosdep keys, so I won't consider it further.

How difficult would e.g. C be over A? Would the extra effort for updating rosdep be justified by the advantage?

The initial effort wouldn't be too hard; the complication is going to be the fallback mechanism you mentioned. I'll do a quick proof-of-concept to see what it would look like, then we can compare. It's unclear to me whether that solution is better than this one. It keeps all of the keys in one file, but it's going to be somewhat confusing for contributors to understand that xenial_python3 is deprecated while ubuntu_python3 is not.

@dirk-thomas
Copy link
Member

Closing in favor of ros-infrastructure/rep#201 which decided on just adding python3-* keys in parallel to the existing python-* keys. The choice happens within a package manifest, see e.g. https://github.com/ros/catkin/blob/86afdf7f1b59af72c84e3b4854134e288dfde97e/package.xml#L22-L23.

@dirk-thomas dirk-thomas deleted the melodic-py3 branch November 27, 2019 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants