Distro Inconsistencies Framework by sethmlarson · Pull Request #186 · python-distro/distro

sethmlarson · 2017-04-19T15:03:00Z

These are some of the changes for working towards v2.

Changes

Added get_implementation() which will choose which LinuxDistribution sub-class to use based on known inconsistencies within Linux distros.
Added LinuxMintDistribution which will not use most values from /etc/os-release if the file is not changed from upstream Ubuntu.
Added CloudLinuxDistribution which will always return an id of cloudlinux even if there are only /etc/redhat-release files available.
Added DebianDistribution which will detect the new format that Debian is using for codenames within /etc/os-release PRETTY_NAME value in the case that lsb_release is not available. (Note I still don't have the complete set of /etc/*-release files from Debian 9, I didn't actually check. I was just going off the data that was provided in Provide way to deal with distro inconsistencies #109.

References

cc: @nir0s @xavfernandez @andy-maier @MartijnBraam @funkyfuture

funkyfuture

a'ight, i don't know why i was asked for a review, but i did it gladly as far as i could. my view on the code is actually like i'd never seen it, since it's a long time ago i dealt with it and haven't followed any development or discussion since. that's not the worst of all perspectives.

as this is leading to a new major release, i suggest to decorate the methods of LinuxDistribution that don't take any arguments as property. i could also argue that the data some module-level functions return would semantically more appropriate if represented as constants. e.g. rather distro.ID than distro.id().

funkyfuture · 2017-04-19T17:47:12Z

distro.py

+        return props
+
+
+def get_implementation(*args):


is this supposed to be a public interface? the name doesn't tell me what this function is about, and i get the docstring's meaning also rather vaguely.
this is the first time i encounter the term 'implementation' in the context of distributions. as a non-native speaker.

It's public-ish. It's probably not going to be used by a single non-power user because most users will just use distro.id(), distro.name(), etc. The word implementation was chosen because @nir0s suggested it in a prior thread. I thought that naming it get_distribution would clash too closely to the other function that I know will be defined distribution() when we add in support for Windows and Mac OS.

@funkyfuture, what do you suggest?

looking at the docs and the code, i would say this function does get_linux_distribution or detect_…?!

Not necessarily Linux considering that we're thinking of adding windows and mac, but yes, get_distribution would be make sense. The problem with it is the fact that it might be ambiguous as to what you're getting. You're actually getting an object, not the distribution itself.

sethmlarson · 2017-04-19T20:37:03Z

Sorry, I added you as a reviewer here because you were pinged for comments on the previous PR.

nir0s · 2017-04-24T11:51:39Z

distro.py

+        return 'cloudlinux'
+
+
+class DebianDistribution(LinuxDistribution):


So.. I'm wondering.. shouldn't we have a class per distribution version rather than per distribution? For instance, in Debian Stretch, we might stumble into something that will later will change again in another version but that touches the same attribute (e.g. version)

Unless what you meant to do is solve something that is general to all Debian distributions and then a specific implementation will inherit that class.

It seems to me, that according to the current implementation, everytime the id is debian you'll get the inconsistency version of that distribution's class.

Additionally, it seems like this doesn't return a "version".. for some reason.

I was wondering your opinion on this as well. I will convert it to be version-specific. It doesn't return a version because I used to a very constrained set of /etc/*-release files in order to get this issue to show itself. Specifically not using lsb-release as was the case in the GitHub issue where this was reported.

Right. I'll open an issue for the version thing.

Regarding the version specific thing.. it would be great if a version-agnostic class of each distribution where there are constant inconsistencies would exist for version specific ones to inherit from.. so if you found that there are distros where we need a version agnostic implementation, I wouldn't add a version to those implementations.

Okay, I'll add a DebianStretchDistribution class. :) I'm guessing because Linux Mint has the os-release problem it'll stay with a single class until Linux Mint fixes that issue?

I'm grabbing the latest Debian 9 ISO and getting all the release and version files, then the TestOverall testcase for Debian 9 will return the version.

nir0s · 2017-04-24T15:09:59Z

distro.py

+
+
+class DebianDistribution(LinuxDistribution):
+    """ Starting with Debian 9 (stretch) the information inside of


In general, I think the docstring should be inside each method of a class so that we can explain things even if we have multiple inconsistencies per distribution.

Sure thing!

nir0s · 2017-04-24T15:11:09Z

distro.py

+        return props
+
+
+def get_implementation(*args):


@funkyfuture, what do you suggest?

sethmlarson · 2017-04-30T23:21:43Z

Okay I'm working on the comments now! 🚂

sethmlarson · 2017-05-04T22:28:27Z

This should be ready for re-review @nir0s :)

funkyfuture · 2017-05-06T12:01:50Z

distro.py

+    from that file except `ID_LIKE`. """
+    def os_release_attr(self, attribute):
+        if attribute == 'id_like':
+            v = super(LinuxMintDistribution, self).os_release_attr(attribute)


no need for the symbol v here.

Line length constraints. :(

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

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)

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.

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.

funkyfuture · 2017-05-06T12:05:59Z

distro.py

+
+
+class CloudLinuxDistribution(LinuxDistribution):
+    def id(self):


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'

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

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

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

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.

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.

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.

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.

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.

sethmlarson · 2017-06-28T20:21:29Z

Would still like these changes to be merged. Ping?

nir0s · 2017-08-02T12:19:28Z

I deeply apologize. I'll get back to this asap. @SethMichaelLarson

Sent from my Google Nexus 5X using FastHub

sethmlarson · 2017-08-02T13:20:40Z

No worries! :)

nir0s · 2017-10-11T17:50:15Z

@SethMichaelLarson, excuse me for the extremely lengthy delay (personal issues..). I'd like to get this merged while addressing in the docs that there are breaking changes. Would you mind solving the conflicts?

Thank you!

sethmlarson · 2017-10-11T20:03:20Z

On vacation currently, when I return I'll take a look at this again.

zztalker · 2018-12-01T02:15:14Z

Please, explain. Do you decide to do not implement that?

nir0s · 2019-02-02T18:39:26Z

@sethmlarson @zztalker, I'd actually like to get this in, but I don't currently have the time. If someone is willing to take it and address the issues, I'll merge and release ASAP.

sethmlarson added 5 commits April 19, 2017 09:27

Add get_implementation(), LinuxMintDistribution

5cb2526

Add CloudLinuxDistribution

228caa3

Add DebianDistribution for stretch/sid

e295fef

Fix linter issues

7f6050b

Add specific test for a DebianDistribution branch

0287e62

funkyfuture reviewed Apr 19, 2017

View reviewed changes

sethmlarson mentioned this pull request Apr 19, 2017

Work for v2 (Windows+Mac OS support, Linux inconsistencies) #185

Closed

18 tasks

nir0s reviewed Apr 24, 2017

View reviewed changes

Switch to get_distribution()

0fa8d9d

funkyfuture reviewed May 6, 2017

View reviewed changes

sethmlarson closed this Jul 2, 2018

nir0s reopened this Feb 2, 2019

nir0s closed this Feb 2, 2019

nir0s mentioned this pull request Feb 2, 2019

Prefer the VERSION_CODENAME field of os-release to parsing it from VERSION #230

Merged

		return 'cloudlinux'


		class DebianDistribution(LinuxDistribution):



		class DebianDistribution(LinuxDistribution):
		""" Starting with Debian 9 (stretch) the information inside of



		class CloudLinuxDistribution(LinuxDistribution):
		def id(self):

Conversation

sethmlarson commented Apr 19, 2017

Changes

References

Uh oh!

funkyfuture left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nir0s Apr 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sethmlarson commented Apr 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sethmlarson commented Apr 30, 2017

Uh oh!

sethmlarson commented May 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sethmlarson May 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sethmlarson commented Jun 28, 2017

Uh oh!

nir0s commented Aug 2, 2017

funkyfuture left a comment •

edited

Loading

nir0s Apr 30, 2017 •

edited

Loading

sethmlarson May 9, 2017 •

edited

Loading