Set ROS_PYTHON_VERSION if unset #708

sloretz · 2019-09-25T19:16:01Z

Requires ros-infrastructure/rosdistro#145

This sets ROS_PYTHON_VERSION if it is unset. This enables rosdep to resolve conditional dependencies using ROS_PYTHON_VERSION before building a ROS distribution from source.

If --rosdistro is given, it tries to use the python_version field from the rosdistro index. Otherwise, it uses the same version of python used to invoke rosdep as mentioned in ros-infrastructure/rep#201

To try out the python_version attribute, update using a custom rosdistro index url

ROSDISTRO_INDEX_URL=https://raw.githubusercontent.com/ros/rosdistro/dirk-thomas/add-python_version-to-index/index-v4.yaml rosdep update

To test, get a package using conditional dependencies and run the commands below with no ROS workspace sourced.

git clone https://github.com/ros/urdf_parser_py.git -b py3-port

Before this PR

(env3) sloretz@7895d0788081:/tmp$ rosdep check --from-paths urdf_parser_py/ --ignore-src --skip-keys="rospy catkin"
All system dependencies have been satisfied

no rosdistro after this PR

sloretz@b0e9deb48a5e:~$ rosdep check --from-paths urdf_parser_py/ --ignore-src --skip-keys="rospy catkin"
WARNING: ROS_PYTHON_VERSION is unset. Defaulting to 3
System dependencies have not been satisfied:
apt	python3-lxml
apt	python3-mock

melodic

sloretz@b0e9deb48a5e:~$ rosdep check --rosdistro melodic --from-paths urdf_parser_py/ --ignore-src --skip-keys="rospy catkin"
System dependencies have not been satisfied:
apt	python-lxml
apt	python-yaml
apt	python-mock

noetic

sloretz@b0e9deb48a5e:~$ rosdep check --rosdistro noetic --from-paths urdf_parser_py/ --ignore-src --skip-keys="rospy catkin"
System dependencies have not been satisfied:
apt	python3-lxml
apt	python3-mock

wjwwood

lgtm

nuclearsandwich

We discussed this a bit offline today. My main concern is that I think many devs use the python2 or python3 versions of the tools exclusively since they cannot be co-installed system-wide. Which means that it's somewhat luck-of-the-draw whether a developer is using python2 or python3 tools with a python2 or python3 workspace. If this were a tool that operated inside a ROS workspace context, this would make a lot of sense, but since rosdep is system-wide I think that this default needs a warning since it has nearly a 50-50 chance of being the wrong choice.

dirk-thomas · 2019-10-01T22:24:44Z

With ros-infrastructure/rep#207 in flight that information could be used here too.

codecov-io · 2019-10-03T16:00:43Z

Codecov Report

Merging #708 into master will increase coverage by 0.31%.
The diff coverage is 82.05%.

@@            Coverage Diff             @@
##           master     #708      +/-   ##
==========================================
+ Coverage   75.82%   76.14%   +0.31%     
==========================================
  Files          38       40       +2     
  Lines        3032     3106      +74     
==========================================
+ Hits         2299     2365      +66     
- Misses        733      741       +8

Impacted Files	Coverage Δ
src/rosdep2/meta.py	`100% <100%> (ø)`
src/rosdep2/main.py	`49.36% <50%> (+0.01%)`	⬆️
src/rosdep2/sources_list.py	`87.78% <66.66%> (+1.86%)`	⬆️
src/rosdep2/cache_tools.py	`73.46% <73.46%> (ø)`

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 39cf725...dd4bf62. Read the comment docs.

sloretz · 2019-10-03T16:01:46Z

@nuclearsandwich added warning in 88816ef

dirk-thomas

This should use the information from the updated REP 153.

sloretz · 2019-10-03T16:15:36Z

This should use the information from the updated REP 153.

REP 153 python_version can be used instead of sys.version[0] when --rosdistro is set. That seems more like an enhancement than a blocker.

dirk-thomas · 2019-10-03T16:18:57Z

REP 153 python_version can be used instead of sys.version[0] when --rosdistro is set. That seems more like an enhancement than a blocker.

it has nearly a 50-50 chance of being the wrong choice.

With the high probability of doing the wrong thing I wouldn't call this an enhancement. I think we should do it right when we can (when --rosdistro is available).

Warn when ROS_PYTHON_VERSION is being defaulted Signed-off-by: Shane Loretz <[email protected]>

Import from new file cache_tools.py Signed-off-by: Shane Loretz <[email protected]>

Adds a new cache directory for meta data. Stores python version for each rosdistro in the metadata cache. Signed-off-by: Shane Loretz<[email protected]> Signed-off-by: Shane Loretz <[email protected]>

Major changes since review

sloretz · 2019-10-03T19:03:52Z

I think we should do it right when we can (when --rosdistro is available).

@dirk-thomas PR and PR description updated

wjwwood

lgtm, with one question.

wjwwood · 2019-10-03T19:08:28Z

src/rosdep2/main.py

@@ -391,6 +392,18 @@ def _rosdep_main(args):
        check_for_sources_list_init(options.sources_cache_dir)
    elif command not in ['fix-permissions']:
        setup_proxy_opener()
+
+    if 'ROS_PYTHON_VERSION' not in os.environ and options.ros_distro:


Shouldn't options.ros_distro take precedence over the environment variable?

options.ros_distro overriding ROS_PYTHON_VERSION might be a problem for Arch Linux users. They seem to be using Python 3: https://aur.archlinux.org/packages/ros-melodic-catkin/ with melodic. If options.ros_distro takes precedence then with #709 pip dependencies would be installed with Python 2 using Melodic on Arch.

See #708 (comment) why on other platforms the rosdistro configured value must have precedence.

@wjwwood I think this is resolved by dd4bf62 (explanation in #708 (comment)). Would you mind having another look at this PR?

dirk-thomas

I haven't looked at the added MetaDatabase logic. I will leave that up to the maintainer(s) of rosdep.

src/rosdep2/main.py

src/rosdep2/sources_list.py

Signed-off-by: Shane Loretz <[email protected]>

Python 2 compatibility Missing imports Signed-off-by: Shane Loretz<[email protected]> Signed-off-by: Shane Loretz <[email protected]>

sloretz · 2019-10-07T18:16:10Z

I haven't looked at the added MetaDatabase logic. I will leave that up to the maintainer(s) of rosdep.

Added tests in 830db97. They pass in Python 2 and Python 3 as of 8a2f954.

Signed-off-by: Shane Loretz <[email protected]>

This allows users with a melodic workspace sourced to install dependencies for a noetic workspace, while still allowing users on community supported platforms to override the python version. Signed-off-by: Shane Loretz<[email protected]> Signed-off-by: Shane Loretz <[email protected]>

sloretz · 2019-10-10T17:39:03Z

CI failure looks like a temporary network issue

======================================================================

ERROR: test.test_rosdep_source.test_install_source

----------------------------------------------------------------------

Traceback (most recent call last):

  File "/opt/python/3.6.7/lib/python3.6/urllib/request.py", line 1318, in do_open

    encode_chunked=req.has_header('Transfer-encoding'))

  File "/opt/python/3.6.7/lib/python3.6/http/client.py", line 1239, in request

    self._send_request(method, url, body, headers, encode_chunked)

  File "/opt/python/3.6.7/lib/python3.6/http/client.py", line 1285, in _send_request

    self.endheaders(body, encode_chunked=encode_chunked)

  File "/opt/python/3.6.7/lib/python3.6/http/client.py", line 1234, in endheaders

    self._send_output(message_body, encode_chunked=encode_chunked)

  File "/opt/python/3.6.7/lib/python3.6/http/client.py", line 1026, in _send_output

    self.send(msg)

  File "/opt/python/3.6.7/lib/python3.6/http/client.py", line 964, in send

    self.connect()

  File "/opt/python/3.6.7/lib/python3.6/http/client.py", line 1392, in connect

    super().connect()

  File "/opt/python/3.6.7/lib/python3.6/http/client.py", line 936, in connect

    (self.host,self.port), self.timeout, self.source_address)

  File "/opt/python/3.6.7/lib/python3.6/socket.py", line 724, in create_connection

    raise err

  File "/opt/python/3.6.7/lib/python3.6/socket.py", line 713, in create_connection

    sock.connect(sa)

TimeoutError: [Errno 110] Connection timed out

During handling of the above exception, another exception occurred:

Traceback (most recent call last):

  File "/home/travis/virtualenv/python3.6.7/lib/python3.6/site-packages/nose/case.py", line 198, in runTest

    self.test(*self.arg)

  File "/home/travis/build/ros-infrastructure/rosdep/test/test_rosdep_source.py", line 318, in test_install_source

    install_source(resolved)

  File "/home/travis/build/ros-infrastructure/rosdep/src/rosdep2/platforms/source.py", line 278, in install_source

    f = urlretrieve(resolved.tarball, filename)

  File "/opt/python/3.6.7/lib/python3.6/urllib/request.py", line 248, in urlretrieve

    with contextlib.closing(urlopen(url, data)) as fp:

  File "/opt/python/3.6.7/lib/python3.6/urllib/request.py", line 223, in urlopen

    return opener.open(url, data, timeout)

  File "/opt/python/3.6.7/lib/python3.6/urllib/request.py", line 526, in open

    response = self._open(req, data)

  File "/opt/python/3.6.7/lib/python3.6/urllib/request.py", line 544, in _open

    '_open', req)

  File "/opt/python/3.6.7/lib/python3.6/urllib/request.py", line 504, in _call_chain

    result = func(*args)

  File "/opt/python/3.6.7/lib/python3.6/urllib/request.py", line 1361, in https_open

    context=self._context, check_hostname=self._check_hostname)

  File "/opt/python/3.6.7/lib/python3.6/urllib/request.py", line 1320, in do_open

    raise URLError(err)

urllib.error.URLError: <urlopen error [Errno 110] Connection timed out>

acarrillo · 2019-10-18T23:20:41Z

src/rosdep2/main.py

+
+    if 'ROS_PYTHON_VERSION' not in os.environ and 'ROS_DISTRO' in os.environ:
+        # Set python version to version used by ROS distro
+        python_versions = MetaDatabase().get('ROS_PYTHON_VERSION')


Looks like MetaDatabase().get('ROS_PYTHON_VERSION') can return None here, it's causing my CI builds to fail after I upgraded to rosdep 0.16.2: #720

Fix proposed in #721

sloretz added the enhancement label Sep 25, 2019

sloretz self-assigned this Sep 25, 2019

sloretz mentioned this pull request Sep 25, 2019

Pip respects ROS_PYTHON_VERSION #709

Merged

wjwwood previously approved these changes Sep 26, 2019

View reviewed changes

nuclearsandwich previously requested changes Oct 1, 2019

View reviewed changes

sloretz requested a review from nuclearsandwich October 3, 2019 16:01

dirk-thomas previously requested changes Oct 3, 2019

View reviewed changes

sloretz added 3 commits October 3, 2019 11:54

Set ROS_PYTHON_VERSION if unset

c0bfa27

Warn when ROS_PYTHON_VERSION is being defaulted Signed-off-by: Shane Loretz <[email protected]>

Move functions unchanged to new file

08df723

Import from new file cache_tools.py Signed-off-by: Shane Loretz <[email protected]>

Respect python_version in rosdistro index

d72c4a3

Adds a new cache directory for meta data. Stores python version for each rosdistro in the metadata cache. Signed-off-by: Shane Loretz<[email protected]> Signed-off-by: Shane Loretz <[email protected]>

sloretz force-pushed the sloretz/ros-python-version branch from 88816ef to d72c4a3 Compare October 3, 2019 18:55

sloretz requested review from dirk-thomas and wjwwood October 3, 2019 19:02

wjwwood approved these changes Oct 3, 2019

View reviewed changes

dirk-thomas reviewed Oct 3, 2019

View reviewed changes

src/rosdep2/main.py Outdated Show resolved Hide resolved

src/rosdep2/sources_list.py Show resolved Hide resolved

sloretz added 3 commits October 7, 2019 10:26

Depend rosdistro >= 0.7.5

f32222f

Signed-off-by: Shane Loretz <[email protected]>

Add tests for MetaDatabase

830db97

Signed-off-by: Shane Loretz <[email protected]>

Fix tests

8a2f954

Python 2 compatibility Missing imports Signed-off-by: Shane Loretz<[email protected]> Signed-off-by: Shane Loretz <[email protected]>

sloretz added 2 commits October 10, 2019 09:31

Disable debug print()

5a3e8e8

Signed-off-by: Shane Loretz <[email protected]>

sloretz requested a review from wjwwood October 10, 2019 17:36

wjwwood approved these changes Oct 10, 2019

View reviewed changes

sloretz merged commit 42c8fed into master Oct 10, 2019

sloretz deleted the sloretz/ros-python-version branch October 10, 2019 18:38

acarrillo reviewed Oct 18, 2019

View reviewed changes

acarrillo mentioned this pull request Oct 18, 2019

rosdep resolve on version 0.16.2 yields TypeError #720

Closed

sloretz mentioned this pull request Oct 19, 2019

rosdep update throws ROS_PYTHON_VERSION warning #722

Closed

MrMoe830 mentioned this pull request Mar 27, 2022

Intel-ROS(RDK) IntelRealSense/librealsense#10341

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set ROS_PYTHON_VERSION if unset #708

Set ROS_PYTHON_VERSION if unset #708

sloretz commented Sep 25, 2019 •

edited

Loading

wjwwood left a comment

nuclearsandwich left a comment

dirk-thomas commented Oct 1, 2019

codecov-io commented Oct 3, 2019 •

edited

Loading

sloretz commented Oct 3, 2019

dirk-thomas left a comment

sloretz commented Oct 3, 2019

dirk-thomas commented Oct 3, 2019

sloretz commented Oct 3, 2019 •

edited

Loading

wjwwood left a comment

wjwwood Oct 3, 2019

sloretz Oct 7, 2019

dirk-thomas Oct 7, 2019

sloretz Oct 10, 2019

dirk-thomas left a comment

sloretz commented Oct 7, 2019

sloretz commented Oct 10, 2019

acarrillo Oct 18, 2019 •

edited

Loading

sloretz Oct 18, 2019

Set ROS_PYTHON_VERSION if unset #708

Set ROS_PYTHON_VERSION if unset #708

Conversation

sloretz commented Sep 25, 2019 • edited Loading

wjwwood left a comment

Choose a reason for hiding this comment

nuclearsandwich left a comment

Choose a reason for hiding this comment

dirk-thomas commented Oct 1, 2019

codecov-io commented Oct 3, 2019 • edited Loading

Codecov Report

sloretz commented Oct 3, 2019

dirk-thomas left a comment

Choose a reason for hiding this comment

sloretz commented Oct 3, 2019

dirk-thomas commented Oct 3, 2019

sloretz commented Oct 3, 2019 • edited Loading

wjwwood left a comment

Choose a reason for hiding this comment

wjwwood Oct 3, 2019

Choose a reason for hiding this comment

sloretz Oct 7, 2019

Choose a reason for hiding this comment

dirk-thomas Oct 7, 2019

Choose a reason for hiding this comment

sloretz Oct 10, 2019

Choose a reason for hiding this comment

dirk-thomas left a comment

Choose a reason for hiding this comment

sloretz commented Oct 7, 2019

sloretz commented Oct 10, 2019

acarrillo Oct 18, 2019 • edited Loading

Choose a reason for hiding this comment

sloretz Oct 18, 2019

Choose a reason for hiding this comment

sloretz commented Sep 25, 2019 •

edited

Loading

codecov-io commented Oct 3, 2019 •

edited

Loading

sloretz commented Oct 3, 2019 •

edited

Loading

acarrillo Oct 18, 2019 •

edited

Loading