-
Notifications
You must be signed in to change notification settings - Fork 64
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
Distro Inconsistencies Framework #186
Distro Inconsistencies Framework #186
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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()
.
distro.py
Outdated
return props | ||
|
||
|
||
def get_implementation(*args): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@funkyfuture, what do you suggest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking at the docs and the code, i would say this function does get_linux_distribution
or detect_…
?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Sorry, I added you as a reviewer here because you were pinged for comments on the previous PR. |
distro.py
Outdated
return 'cloudlinux' | ||
|
||
|
||
class DebianDistribution(LinuxDistribution): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. I'll open an issue for the version thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
distro.py
Outdated
|
||
|
||
class DebianDistribution(LinuxDistribution): | ||
""" Starting with Debian 9 (stretch) the information inside of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure thing!
distro.py
Outdated
return props | ||
|
||
|
||
def get_implementation(*args): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@funkyfuture, what do you suggest?
Okay I'm working on the comments now! 🚂 |
This should be ready for re-review @nir0s :) |
from that file except `ID_LIKE`. """ | ||
def os_release_attr(self, attribute): | ||
if attribute == 'id_like': | ||
v = super(LinuxMintDistribution, self).os_release_attr(attribute) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line length constraints. :(
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
||
|
||
class CloudLinuxDistribution(LinuxDistribution): | ||
def id(self): |
There was a problem hiding this comment.
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'
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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()
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Would still like these changes to be merged. Ping? |
I deeply apologize. I'll get back to this asap. @SethMichaelLarson Sent from my Google Nexus 5X using FastHub |
No worries! :) |
@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! |
On vacation currently, when I return I'll take a look at this again. |
Please, explain. Do you decide to do not implement that? |
@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. |
These are some of the changes for working towards v2.
Changes
get_implementation()
which will choose whichLinuxDistribution
sub-class to use based on known inconsistencies within Linux distros.LinuxMintDistribution
which will not use most values from/etc/os-release
if the file is not changed from upstream Ubuntu.CloudLinuxDistribution
which will always return an id ofcloudlinux
even if there are only/etc/redhat-release
files available.DebianDistribution
which will detect the new format that Debian is using for codenames within/etc/os-release
PRETTY_NAME
value in the case thatlsb_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
#109 #180 #184
cc: @nir0s @xavfernandez @andy-maier @MartijnBraam @funkyfuture