Fixes for Python 3.8 #182

jspricke · 2019-11-24T09:46:46Z

No description provided.

dirk-thomas

Please address the failing CI - one of the existing test fails due to some change in behavior it seems.

setup.py

dirk-thomas

Btw CI is still failing for the same reason as before.

stdeb.cfg

jspricke · 2019-11-29T18:08:43Z

This should be fine now, can you have another look?

dirk-thomas · 2019-12-12T21:28:31Z

The fact that changing the import order breaks the test is a problem because that means the behavior of rospkg is changing with Python 3.8. We need to make sure that the way it changes (returning ubuntu rather than Ubuntu) doesn't cause issues downstream. Eventually that means we have to patch code using the API to work with both responses.

jspricke · 2019-12-12T21:51:47Z

I'm not sure what you are talking about, on Ubuntu 18 I get Ubuntu with the distro module. I assume that you are seeing some artifact of pip installing the distro module on Ubuntu 16. As this will only be needed for Ubuntu 20, I swapped the import logic on purpose.

dirk-thomas · 2019-12-12T21:56:38Z

I tried the distro module on Ubuntu 18.04 and it returned lower case ubuntu instead of Ubuntu (which is what platform returns).

jspricke · 2019-12-12T21:59:17Z

>>> import distro
>>> distro.linux_distribution()
(u'Ubuntu', u'18.04', u'Bionic Beaver')

with python-distro 1.0.1-2
what did you try?

dirk-thomas · 2019-12-12T22:01:59Z

I use the latest version of distro installed via pip: 1.4.0

And call the function (the same way as in this patch): distro.linux_distribution(full_distribution_name=0) and it returns (u'ubuntu', u'18.04', u'bionic').

jspricke · 2019-12-12T22:06:31Z

looks like that's unstable anyhow, for example in Debian I get:

>>> import platform
>>> platform.linux_distribution(full_distribution_name=0)
('debian', 'bullseye/sid', '')

dirk-thomas · 2019-12-12T22:26:16Z

I split out the Python 3.8 specific fixes for the new SyntaxWarnings and added Python 3.7/3.8 CI: see #183

I am going to rebase this one after that PR has been merged.

See https://docs.python.org/3.7/library/platform.html#unix-platforms

Also only depend on python3-distro if python3 doesn't provide the platform module (i.e. starting with version 3.8).

dirk-thomas · 2019-12-12T22:46:23Z

It took me a bit of debugging (a93fa32) but as it turns out the inverted import logic exposed that the import distro line was never doing what it was supposed to (in Python 2). It was loading the relative module within rospkg. See 3fa713b for the fix.

dirk-thomas · 2019-12-12T22:47:06Z

I also replaced ModuleNotFoundError with ImportError (ed980c9` ) since the former is only available as of Python 3.6.

dirk-thomas · 2019-12-12T23:03:03Z

With all the back and forth and debugging I felt the need to rewrite one more time. Beside the original rebased commits I only applied two necessary fixes:

force absolute imports since the rospkg package also contains a module with the name distro: 017549e
ModuleNotFoundError is only available as of Python 3.6: 94ded0a

I don't see any evidence of a case mismatch on Travis so I won't change the matching logic from case sensitive to insensitive for now.

@jspricke It would be great if you could take another look and comment if that looks good to you, too.

tfoote

At least the modules have the same API. This seems good to me.

I double checked that we're not breaking users from source/pip and they get the distro module backported so the install_requires not being conditional is ok.

jspricke · 2019-12-13T06:38:16Z

@dirk-thomas afaik your two commits change nothing as the platform module is part of Python up to 3.8, so the import will always prefer the platform module when it's available. Starting with 3.8 it will switch to the distro module, but in 3.8 absolute_import is the default as well as the ModuleNotFoundError is available. So LGTM.

dirk-thomas · 2019-12-13T15:43:35Z

Thanks.

dirk-thomas · 2019-12-13T21:13:56Z

This caused #185.

jacobperron · 2019-12-18T01:06:13Z

AFAICT, this is causing CI to fail here: ros-infrastructure/rosdep#735

Although the platform module is available in Python 3.8, linux_distribution and dist have been removed, so lsb_info is always None:

rospkg/src/rospkg/os_detect.py

Lines 140 to 145 in 2c027b1

    
           if hasattr(distro, "linux_distribution"): 
        
               self.lsb_info = distro.linux_distribution(full_distribution_name=0) 
        
           elif hasattr(distro, "dist"): 
        
               self.lsb_info = distro.dist() 
        
           else: 
        
               self.lsb_info = None

jacobperron · 2019-12-18T01:19:19Z

Also, rospkg tests are failing locally for me with Python 3.8:

Test output


$ # nosetests --with-coverage --cover-package=rospkg --with-xunit test
.EE........../root/rospkg/src/rospkg/distro.py:195: YAMLLoadWarning: calling yaml.load() without Loader=... is deprecated, as the default Loader is unsafe. Please read https://msg.pyyaml.org/load for full details.
  raw_data = yaml.load(f.read())
.../root/rospkg/test/test_rospkg_distro.py:318: YAMLLoadWarning: calling yaml.load() without Loader=... is deprecated, as the default Loader is unsafe. Please read https://msg.pyyaml.org/load for full details.
  assert yaml.load(open(p)) == distro.raw_data, distro.raw_data
./root/rospkg/test/test_rospkg_distro.py:337: YAMLLoadWarning: calling yaml.load() without Loader=... is deprecated, as the default Loader is unsafe. Please read https://msg.pyyaml.org/load for full details.
  assert yaml.load(open(p)) == distro.raw_data, distro.raw_data
./root/rospkg/test/test_rospkg_distro.py:353: YAMLLoadWarning: calling yaml.load() without Loader=... is deprecated, as the default Loader is unsafe. Please read https://msg.pyyaml.org/load for full details.
  raw_data = yaml.load("""variants:
................................E...................................................
 ======================================================================
ERROR: test.test_rospkg_catkin_packages.test_get_manifest
 ----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/local/lib/python3.8/dist-packages/nose/case.py", line 198, in runTest
    self.test(*self.arg)
  File "/root/rospkg/test/test_rospkg_catkin_packages.py", line 58, in test_get_manifest
    manif = manager.get_manifest("foo")
  File "/root/rospkg/src/rospkg/rospack.py", line 167, in get_manifest
    return self._load_manifest(name)
  File "/root/rospkg/src/rospkg/rospack.py", line 211, in _load_manifest
    retval = self._manifests[name] = parse_manifest_file(self.get_path(name), self._manifest_name, rospack=self)
  File "/root/rospkg/src/rospkg/manifest.py", line 414, in parse_manifest_file
    _static_rosdep_view = init_rospack_interface()
  File "/root/rosdep/src/rosdep2/rospack.py", line 59, in init_rospack_interface
    lookup = _get_default_RosdepLookup(Options())
  File "/root/rosdep/src/rosdep2/main.py", line 132, in _get_default_RosdepLookup
    sources_loader = SourcesListLoader.create_default(sources_cache_dir=options.sources_cache_dir,
  File "/root/rosdep/src/rosdep2/sources_list.py", line 581, in create_default
    matcher = DataSourceMatcher.create_default(os_override=os_override)
  File "/root/rosdep/src/rosdep2/sources_list.py", line 293, in create_default
    os_name, os_version, os_codename = os_detect.detect_os()
  File "/root/rospkg/src/rospkg/os_detect.py", line 664, in detect_os
    raise OsNotDetected("Could not detect OS, tried %s" % attempted)
rospkg.os_detect.OsNotDetected: Could not detect OS, tried ['zorin', 'windows', 'nixos', 'clearlinux', 'ubuntu', 'slackware', 'rhel', 'qnx', 'osx', 'tizen', 'opensuse', 'opensuse', 'opensuse', 'opensuse', 'openembedded', 'neon', 'mx', 'mint', 'linaro', 'gentoo', 'funtoo', 'freebsd', 'fedora', 'elementary', 'elementary', 'debian', 'cygwin', 'centos', 'manjaro', 'arch', 'alpine']

======================================================================
ERROR: test.test_rospkg_catkin_packages.test_licenses
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/local/lib/python3.8/dist-packages/nose/case.py", line 198, in runTest
    self.test(*self.arg)
  File "/root/rospkg/test/test_rospkg_catkin_packages.py", line 65, in test_licenses
    manif = rospack.get_manifest("foo")
  File "/root/rospkg/src/rospkg/rospack.py", line 167, in get_manifest
    return self._load_manifest(name)
  File "/root/rospkg/src/rospkg/rospack.py", line 211, in _load_manifest
    retval = self._manifests[name] = parse_manifest_file(self.get_path(name), self._manifest_name, rospack=self)
  File "/root/rospkg/src/rospkg/manifest.py", line 414, in parse_manifest_file
    _static_rosdep_view = init_rospack_interface()
  File "/root/rosdep/src/rosdep2/rospack.py", line 59, in init_rospack_interface
    lookup = _get_default_RosdepLookup(Options())
  File "/root/rosdep/src/rosdep2/main.py", line 132, in _get_default_RosdepLookup
    sources_loader = SourcesListLoader.create_default(sources_cache_dir=options.sources_cache_dir,
  File "/root/rosdep/src/rosdep2/sources_list.py", line 581, in create_default
    matcher = DataSourceMatcher.create_default(os_override=os_override)
  File "/root/rosdep/src/rosdep2/sources_list.py", line 293, in create_default
    os_name, os_version, os_codename = os_detect.detect_os()
  File "/root/rospkg/src/rospkg/os_detect.py", line 664, in detect_os
    raise OsNotDetected("Could not detect OS, tried %s" % attempted)
rospkg.os_detect.OsNotDetected: Could not detect OS, tried ['zorin', 'windows', 'nixos', 'clearlinux', 'ubuntu', 'slackware', 'rhel', 'qnx', 'osx', 'tizen', 'opensuse', 'opensuse', 'opensuse', 'opensuse', 'openembedded', 'neon', 'mx', 'mint', 'linaro', 'gentoo', 'funtoo', 'freebsd', 'fedora', 'elementary', 'elementary', 'debian', 'cygwin', 'centos', 'manjaro', 'arch', 'alpine']

======================================================================
ERROR: test.test_rospkg_os_detect.test_osx
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/local/lib/python3.8/dist-packages/nose/case.py", line 198, in runTest
    self.test(*self.arg)
  File "/root/rospkg/test/test_rospkg_os_detect.py", line 169, in test_osx
    assert detect.get_codename() == 'snow'
  File "/root/rospkg/src/rospkg/os_detect.py", line 378, in get_codename
    ver = distutils.version.StrictVersion(version).version
AttributeError: 'StrictVersion' object has no attribute 'version'

Name                    Stmts   Miss  Cover
-------------------------------------------
rospkg/__init__.py          7      0   100%
rospkg/common.py           15      0   100%
rospkg/distro.py          316     10    97%
rospkg/environment.py      68      2    97%
rospkg/manifest.py        259     46    82%
rospkg/os_detect.py       469     87    81%
rospkg/rospack.py         223     13    94%
rospkg/stack.py           123     22    82%
-------------------------------------------
TOTAL                    1480    180    88%
----------------------------------------------------------------------
Ran 102 tests in 0.466s

FAILED (errors=3)

dirk-thomas · 2019-12-18T01:28:09Z

this is causing CI to fail here: ros-infrastructure/rosdep#735

Have you seen the comment above ("This caused #185.") as well as the related fix from #186?

rospkg tests are failing locally for me with Python 3.8:

Do you have the Python package distro installed? CI passed with all supported Python version for #186 which pretty much reflects what is on master / in the latest release.

jacobperron · 2019-12-18T02:14:08Z

Yes, distro was installed (I ran the commands in the travis script).
It's possible I installed rosdep before rospkg from source and something strange happened. I tried with a fresh docker container and only one test error remains:

ERROR: test.test_rospkg_os_detect.test_osx
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/local/lib/python3.8/dist-packages/nose/case.py", line 198, in runTest
    self.test(*self.arg)
  File "/rospkg/test/test_rospkg_os_detect.py", line 169, in test_osx
    assert detect.get_codename() == 'snow'
  File "/rospkg/src/rospkg/os_detect.py", line 378, in get_codename
    ver = distutils.version.StrictVersion(version).version
AttributeError: 'StrictVersion' object has no attribute 'version'

I'm not on osx, so maybe it's expected?

In any case, if I install rospkg at master, I can't seem to confirm the distro on Ubuntu Focal:

>>> from rospkg.os_detect import LsbDetect
>>> foo = LsbDetect("ubuntu")
>>> foo.is_os()
False

Upper case "Ubuntu" doesn't work either. Here's the Dockerfile I used to produce.

sloretz · 2019-12-30T18:39:52Z

I'm not on osx, so maybe it's expected?

StrictVersion only has a version attribute if the version string has a patch component. The OSX detector gets the version string by executing a file. That file has a shebang expects an executable called python, but with that Dockerfile there is none, causing stdout to be an empty string. Installing Python 2 fixes the test failure, but I think we could get rid of that test dependency by making the shebang /bin/sh and using echo.

rospkg/test/os_detect/osx/sw_vers

Lines 1 to 3 in fc3f2b8

    
           #!/usr/bin/env python 
        
           import sys 
        
           sys.stdout.write("10.6.5\n")

dirk-thomas reviewed Nov 25, 2019

View reviewed changes

setup.py Outdated Show resolved Hide resolved

jspricke force-pushed the python_3.8 branch from c13b37a to b13d223 Compare November 25, 2019 23:10

dirk-thomas reviewed Nov 25, 2019

View reviewed changes

stdeb.cfg Outdated Show resolved Hide resolved

dirk-thomas added the requires-changes label Dec 12, 2019

jspricke added 4 commits December 12, 2019 14:30

Replace deprecated platform functions with distro

dd05cfa

See https://docs.python.org/3.7/library/platform.html#unix-platforms

Use distro module in test

0b69f63

Use platform module as a fallback (for Ubuntu Xenial)

86f33e1

Also only depend on python3-distro if python3 doesn't provide the platform module (i.e. starting with version 3.8).

Prefer old platform module to fix Xenial

2c5c183

dirk-thomas force-pushed the python_3.8 branch from 9067f69 to 0bc639b Compare December 12, 2019 22:30

dirk-thomas force-pushed the python_3.8 branch from f921ef3 to 88795e1 Compare December 12, 2019 22:52

force absolute imports

017549e

dirk-thomas force-pushed the python_3.8 branch from 88795e1 to 017549e Compare December 12, 2019 22:59

ModuleNotFoundError is only available as of Python 3.6

94ded0a

dirk-thomas added bug and removed requires-changes labels Dec 12, 2019

dirk-thomas requested a review from tfoote December 12, 2019 23:03

tfoote approved these changes Dec 13, 2019

View reviewed changes

dirk-thomas merged commit fc3f2b8 into ros-infrastructure:master Dec 13, 2019

tfoote mentioned this pull request Dec 13, 2019

only conditionally declare distro dependency in setup.py #186

Merged

jacobperron mentioned this pull request Dec 18, 2019

Add Python 3.7 and 3.8 to test matrix ros-infrastructure/rosdep#735

Closed

This was referenced Dec 30, 2019

Use /bin/sh instead of python 2 #188

Merged

Case insensitive LSB name checking #189

Merged

seanyen mentioned this pull request Jan 8, 2020

add missing dependency - distro conda-forge/rospkg-feedstock#7

Merged

5 tasks

dirk-thomas modified the milestones: 1.2.0, 1.2.1 Jun 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes for Python 3.8 #182

Fixes for Python 3.8 #182

jspricke commented Nov 24, 2019

dirk-thomas left a comment

dirk-thomas left a comment

jspricke commented Nov 29, 2019

dirk-thomas commented Dec 12, 2019

jspricke commented Dec 12, 2019

dirk-thomas commented Dec 12, 2019 •

edited

jspricke commented Dec 12, 2019

dirk-thomas commented Dec 12, 2019 •

edited

jspricke commented Dec 12, 2019

dirk-thomas commented Dec 12, 2019

dirk-thomas commented Dec 12, 2019

dirk-thomas commented Dec 12, 2019

dirk-thomas commented Dec 12, 2019

tfoote left a comment

jspricke commented Dec 13, 2019

dirk-thomas commented Dec 13, 2019

dirk-thomas commented Dec 13, 2019

jacobperron commented Dec 18, 2019 •

edited

jacobperron commented Dec 18, 2019

dirk-thomas commented Dec 18, 2019

jacobperron commented Dec 18, 2019

sloretz commented Dec 30, 2019

Fixes for Python 3.8 #182

Fixes for Python 3.8 #182

Conversation

jspricke commented Nov 24, 2019

dirk-thomas left a comment

Choose a reason for hiding this comment

dirk-thomas left a comment

Choose a reason for hiding this comment

jspricke commented Nov 29, 2019

dirk-thomas commented Dec 12, 2019

jspricke commented Dec 12, 2019

dirk-thomas commented Dec 12, 2019 • edited

jspricke commented Dec 12, 2019

dirk-thomas commented Dec 12, 2019 • edited

jspricke commented Dec 12, 2019

dirk-thomas commented Dec 12, 2019

dirk-thomas commented Dec 12, 2019

dirk-thomas commented Dec 12, 2019

dirk-thomas commented Dec 12, 2019

tfoote left a comment

Choose a reason for hiding this comment

jspricke commented Dec 13, 2019

dirk-thomas commented Dec 13, 2019

dirk-thomas commented Dec 13, 2019

jacobperron commented Dec 18, 2019 • edited

jacobperron commented Dec 18, 2019

dirk-thomas commented Dec 18, 2019

jacobperron commented Dec 18, 2019

sloretz commented Dec 30, 2019

dirk-thomas commented Dec 12, 2019 •

edited

dirk-thomas commented Dec 12, 2019 •

edited

jacobperron commented Dec 18, 2019 •

edited