Skip to content

Distro Inconsistencies Framework #186

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.

Sign up for GitHub

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

Closed
sethmlarson wants to merge 6 commits into from
Closed

Distro Inconsistencies Framework #186

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5cb2526
Add get_implementation(), LinuxMintDistribution
sethmlarson Apr 19, 2017
228caa3
Add CloudLinuxDistribution
sethmlarson Apr 19, 2017
e295fef
Add DebianDistribution for stretch/sid
sethmlarson Apr 19, 2017
7f6050b
Fix linter issues
sethmlarson Apr 19, 2017
0287e62
Add specific test for a DebianDistribution branch
sethmlarson Apr 19, 2017
0fa8d9d
Switch to get_distribution()
sethmlarson May 1, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGES
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
**2.0.0 (dev)**

* Added framework for distro to detect and correct reported information in the case of a Linux distro inconsistency.
* Added `get_implementation` which should be used to get a `LinuxDistribution` object.

**1.0.4 (2017.04.01)**

* Fix not being able to read `/etc/` and yet able to read release files inside it.
Expand Down
76 changes: 70 additions & 6 deletions distro.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -842,13 +842,13 @@ def _get_os_release_info(self):
Returns:
A dictionary containing all information items.
"""
if os.path.isfile(self.os_release_file):
try:
with open(self.os_release_file) as release_file:
return self._parse_os_release_content(release_file)
return {}
except (OSError, IOError):
return {}

@staticmethod
def _parse_os_release_content(lines):
def _parse_os_release_content(self, lines):
"""
Parse the lines of an os-release file.

Expand Down Expand Up @@ -898,7 +898,7 @@ def _parse_os_release_content(lines):
codename = codename.strip('()')
codename = codename.strip(',')
codename = codename.strip()
# codename appears within paranthese.
# codename appears within parenthesis.
props['codename'] = codename
else:
props['codename'] = ''
Expand Down Expand Up @@ -1074,7 +1074,71 @@ def _parse_distro_release_content(line):
return distro_info


_distro = LinuxDistribution()
def _normalize_id(distro_id, table):
""" Helper function which normalizes a distro id value. """
distro_id = distro_id.lower().replace(' ', '_')
return table.get(distro_id, distro_id)


class LinuxMintDistribution(LinuxDistribution):
""" LinuxMint will have the same /etc/os-release file
as it's upstream Ubuntu so we should not use any info
from that file except `ID_LIKE`. """
def os_release_attr(self, attribute):
if attribute == 'id_like':
v = super(LinuxMintDistribution, self).os_release_attr(attribute)
Copy link
Contributor

@funkyfuture funkyfuture May 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need for the symbol v here.

Copy link
Contributor Author

@sethmlarson sethmlarson May 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line length constraints. :(

Copy link
Contributor

@funkyfuture funkyfuture May 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can use a line-break in the return statement.

Copy link
Contributor Author

@sethmlarson sethmlarson May 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm aware, I just thought this looked better than this

return (super(LinuxMintDistribution, self)
        .os_release_attr(attribute))

or this

return super(LinuxMintDistribution,
             self).os_release_attr(attribute)

Copy link
Contributor

@funkyfuture funkyfuture May 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rather

return super(LinuxMintDistribution, self)\
    .os_release_attr(attribute)

i actually don't know whether the current state would be optimized upon bytecode-compilation. if so, it doesn't matter anyway.

Copy link
Contributor Author

@sethmlarson sethmlarson May 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would create a linting error because there is a hanging indent that has a multiple of 4 spaces that's not a code indent.

This type of thing isn't optimized away in byte-code, two extra codes for STORE FAST and LOAD FAST before the return.

return v
return ''


class CloudLinuxDistribution(LinuxDistribution):
def id(self):
Copy link
Contributor

@funkyfuture funkyfuture May 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if LinuxDistribution.id was a wrapped as property (which makes sense due to its static-ness during runtime), id could simply be a class-property then:

class CloudLinuxDistribution(LinuxDistribution):
    id = 'cloudlinux'

Copy link
Contributor Author

@sethmlarson sethmlarson May 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we changing id from a function to a property? The same could be said about all other name(), version(), etc

Copy link
Contributor Author

@sethmlarson sethmlarson May 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm in favor of keeping id a function, it would keep things homogenous between the module level id() and the Distribution.id().

Copy link
Contributor

@funkyfuture funkyfuture May 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, it should be canonical. yet, these are all constants.

Copy link
Contributor Author

@sethmlarson sethmlarson May 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They happen to be constants for CloudLinuxDistribution but that's not necessarily the case for all, most distributions have to get the information about id from os_release_attr, lsb_release_attr, and distro_release_attr function calls.

I know we can just use the @property decorator but there are more "constant" values than just id.

Copy link
Contributor

@funkyfuture funkyfuture May 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's my point (not concerning a particular class, but in general) - all these values are constants during runtime, especially from the client code's perspective.

Copy link
Contributor Author

@sethmlarson sethmlarson May 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you're proposing caching the value? There's nothing wrong with a function returning something that is actually a constant. See the class of functions this library is aiming to replace: platform.linux_distribution(), platform.version() etc. Those values won't change during runtime but they're all still functions.

Copy link
Contributor

@funkyfuture funkyfuture May 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i mean both: changing the design and as a side-effect to 'cache'.

def _get_id():
    …

ID = _get_id()

__all__ = ['ID']

though i'd argue that no uppercase reads cleaner in the client code: f'Running on {distro.name}.'

@nir0s shall i open an issue regarding the design discussion? though i can't promise to layout a detailed proposal soon, i see the opportunity to introduce breaking changes with a new major release. sorry for bloating this up here.

Copy link
Contributor Author

@sethmlarson sethmlarson May 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably going to be a little trickier to test compared to our current model of doing things. Agree on keeping things lower-cased.

""" CloudLinux <7 doesn't provide an lsb-release or
os-release file, only a redhat-release file so it's id
is incorrectly detected as `rhel`. """
return 'cloudlinux'


class DebianStretchDistribution(LinuxDistribution):
def _parse_os_release_content(self, lines):
""" Debian 9 (stretch) does not have the codename
in parenthesis or with a comma unlike all other
distributions. This stub """
props = (super(DebianStretchDistribution, self).
_parse_os_release_content(lines))
if 'codename' not in props and props.get('pretty_name', ''):
pretty_name = props['pretty_name']

# Remove the NAME from the PRETTY_NAME entry to prevent getting
# a codename of 'GNU/Linux'.
pretty_name = pretty_name.replace(props.get('name', ''), '')

match = re.search(r'\s([^\s\d]+)$', pretty_name)
if match:
props['codename'] = match.group(1)
return props


def get_distribution(*args):
""" Gets the proper implementation of LinuxDistribution after attempting
to detect distro inconsistencies. Passes all args through to the resulting
LinuxDistribution object. """
ld = LinuxDistribution(*args)
os_release_id = _normalize_id(ld.os_release_attr('id'), NORMALIZED_OS_ID)
lsb_release_id = _normalize_id(ld.lsb_release_attr('distributor_id'),
NORMALIZED_LSB_ID)

if os_release_id == 'ubuntu' and lsb_release_id == 'linuxmint':
return LinuxMintDistribution(*args)
elif ld.id() == 'rhel' and 'CloudLinux' in ld.name():
return CloudLinuxDistribution(*args)
elif ld.id() == 'debian' and ld.codename() == '':
return DebianStretchDistribution(*args)
else:
return ld


_distro = get_distribution()


def main():
Expand Down
6 changes: 6 additions & 0 deletions tests/resources/distros/debian9/etc/os-release
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
PRETTY_NAME="Debian GNU/Linux stretch/sid"
NAME="Debian GNU/Linux"
ID=debian
HOME_URL="https://www.debian.org/"
SUPPORT_URL="https://www.debian.org/support"
BUG_REPORT_URL="https://bugs.debian.org/"
6 changes: 6 additions & 0 deletions tests/resources/special/debian_no_codename/etc/os-release
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
PRETTY_NAME="Debian GNU/Linux"
NAME="Debian GNU/Linux"
ID=debian
HOME_URL="https://www.debian.org/"
SUPPORT_URL="https://www.debian.org/support"
BUG_REPORT_URL="https://bugs.debian.org/"
76 changes: 40 additions & 36 deletions tests/test_distro.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ class TestOSRelease:
def setup_method(self, test_method):
dist = test_method.__name__.split('_')[1]
os_release = os.path.join(DISTROS_DIR, dist, 'etc', 'os-release')
self.distro = distro.LinuxDistribution(False, os_release, 'non')
self.distro = distro.get_distribution(False, os_release, 'non')

def _test_outcome(self, outcome):
assert self.distro.id() == outcome.get('id', '')
Expand Down Expand Up @@ -175,6 +175,15 @@ def test_debian8_os_release(self):
}
self._test_outcome(desired_outcome)

def test_debian9_os_release(self):
desired_outcome = {
'id': 'debian',
'name': 'Debian GNU/Linux',
'pretty_name': 'Debian GNU/Linux stretch/sid',
'codename': 'stretch/sid'
}
self._test_outcome(desired_outcome)

def test_fedora19_os_release(self):
desired_outcome = {
'id': 'fedora',
Expand Down Expand Up @@ -400,7 +409,7 @@ def setup_method(self, test_method):
self.test_method_name = test_method.__name__
dist = test_method.__name__.split('_')[1]
self._setup_for_distro(os.path.join(DISTROS_DIR, dist))
self.distro = distro.LinuxDistribution(True, 'non', 'non')
self.distro = distro.get_distribution(True, 'non', 'non')

def _test_outcome(self, outcome):
assert self.distro.id() == outcome.get('id', '')
Expand Down Expand Up @@ -437,20 +446,6 @@ def test_manjaro1512_lsb_release(self):
'codename': 'Capella'
})

# @pytest.mark.xfail
# def test_openelec6_lsb_release(self):
# # TODO: This should be fixed as part of #109 when dealing
# # with distro inconsistencies
# desired_outcome = {
# 'id': 'openelec',
# 'name': 'OpenELEC',
# 'pretty_name': 'OpenELEC (official) - Version: 6.0.3',
# 'version': '6.0.3',
# 'pretty_version': '6.0.3',
# 'best_version': '6.0.3',
# }
# self._test_outcome(desired_outcome)

def test_ubuntu14normal_lsb_release(self):
self._setup_for_distro(os.path.join(TESTDISTROS, 'lsb',
'ubuntu14_normal'))
Expand Down Expand Up @@ -581,6 +576,19 @@ def test_unknowndistro_release(self):
}
self._test_outcome(desired_outcome)

def test_debian_distribution_pretty_name_has_no_codename(self):
self._setup_for_distro(os.path.join(SPECIAL, 'debian_no_codename'))

self.distro = distro.get_distribution()

desired_outcome = {
'id': 'debian',
'name': 'Debian GNU/Linux',
'pretty_name': 'Debian GNU/Linux',
'codename': '',
}
self._test_outcome(desired_outcome)


@pytest.mark.skipif(not IS_LINUX, reason='Irrelevant on non-linux')
class TestDistroRelease:
Expand All @@ -595,7 +603,7 @@ def _test_outcome(self,
distro_release = os.path.join(
DISTROS_DIR, distro_name + version, 'etc', '{0}-{1}'.format(
release_file_id, release_file_suffix))
self.distro = distro.LinuxDistribution(False, 'non', distro_release)
self.distro = distro.get_distribution(False, 'non', distro_release)

assert self.distro.id() == outcome.get('id', '')
assert self.distro.name() == outcome.get('name', '')
Expand Down Expand Up @@ -811,7 +819,7 @@ def test_cloudlinux5_dist_release(self):
# Uses redhat-release only to get information.
# The id of 'rhel' can only be fixed with issue #109.
desired_outcome = {
'id': 'rhel',
'id': 'cloudlinux',
'codename': 'Vladislav Volkov',
'name': 'CloudLinux Server',
'pretty_name': 'CloudLinux Server 5.11 (Vladislav Volkov)',
Expand All @@ -826,7 +834,7 @@ def test_cloudlinux5_dist_release(self):
def test_cloudlinux6_dist_release(self):
# Same as above, only has redhat-release.
desired_outcome = {
'id': 'rhel',
'id': 'cloudlinux',
'codename': 'Oleg Makarov',
'name': 'CloudLinux Server',
'pretty_name': 'CloudLinux Server 6.8 (Oleg Makarov)',
Expand All @@ -840,7 +848,7 @@ def test_cloudlinux6_dist_release(self):

def test_cloudlinux7_dist_release(self):
desired_outcome = {
'id': 'rhel',
'id': 'cloudlinux',
'codename': 'Yury Malyshev',
'name': 'CloudLinux',
'pretty_name': 'CloudLinux 7.3 (Yury Malyshev)',
Expand Down Expand Up @@ -883,7 +891,7 @@ def setup_method(self, test_method):
super(TestOverall, self).setup_method(test_method)
dist = test_method.__name__.split('_')[1]
self._setup_for_distro(os.path.join(DISTROS_DIR, dist))
self.distro = distro.LinuxDistribution()
self.distro = distro.get_distribution()

def _test_outcome(self, outcome):
assert self.distro.id() == outcome.get('id', '')
Expand Down Expand Up @@ -1076,16 +1084,16 @@ def test_kvmibm1_release(self):

def test_linuxmint17_release(self):
desired_outcome = {
'id': 'ubuntu',
'name': 'Ubuntu',
'pretty_name': 'Ubuntu 14.04.3 LTS',
'version': '14.04',
'pretty_version': '14.04 (Trusty Tahr)',
'best_version': '14.04.3',
'id': 'linuxmint',
'name': 'LinuxMint',
'pretty_name': 'Linux Mint 17.3 Rosa',
'version': '17.3',
'pretty_version': '17.3 (rosa)',
'best_version': '17.3',
'like': 'debian',
'codename': 'Trusty Tahr',
'major_version': '14',
'minor_version': '04'
'codename': 'rosa',
'major_version': '17',
'minor_version': '3'
}
self._test_outcome(desired_outcome)
self._test_non_existing_release_file()
Expand All @@ -1099,7 +1107,6 @@ def test_mageia5_release(self):
'pretty_version': '5 (thornicroft)',
'best_version': '5',
'like': 'mandriva fedora',
# TODO: Codename differs between distro release and lsb_release.
'codename': 'thornicroft',
'major_version': '5'
}
Expand Down Expand Up @@ -1458,10 +1465,8 @@ def test_mandriva2011_release(self):
self._test_release_file_info('mandrake-release', desired_info)

def test_cloudlinux5_release(self):
# Uses redhat-release only to get information.
# The id of 'rhel' can only be fixed with issue #109.
desired_outcome = {
'id': 'rhel',
'id': 'cloudlinux',
'codename': 'Vladislav Volkov',
'name': 'CloudLinux Server',
'pretty_name': 'CloudLinux Server 5.11 (Vladislav Volkov)',
Expand All @@ -1474,9 +1479,8 @@ def test_cloudlinux5_release(self):
self._test_outcome(desired_outcome)

def test_cloudlinux6_release(self):
# Same as above, only has redhat-release.
desired_outcome = {
'id': 'rhel',
'id': 'cloudlinux',
'codename': 'Oleg Makarov',
'name': 'CloudLinux Server',
'pretty_name': 'CloudLinux Server 6.8 (Oleg Makarov)',
Expand Down