Fixing how distro deals with inconsistent *-release files.

[sethmlarson]

So this PR is a minimal (and IMO rather hacky) solution to the two cases where distro has trouble with getting the right information from *-release files. The two related issues are #78 for Linux Mint and #152 for Debian stretch/sid when lsb-release isn't installed. I don't think this solution should be merged, might be a good place for some discussion about the proper direction.

If I were to be able to make more changes it might be nice to have distro.LinuxDistribution() do all it's parsing, assigns values, and then return a class that does minimal logic and spits out those values when they're queried. Could still keep same interface including os_release_attr, lsb_release_attr, etc. Would allow for a cleaner layout of where special cases happen rather than have each special case occur within the id(), codename() and version() functions. I think this would be a much better route to go, especially for when future inconsistencies are found.

Thoughts?

[nir0s]

This is cool and certainly will allow us to solve some inconsistencies but will also somewhat pollute the code as we find more and more inconsistent implementations because it changes the default implementation in the base class.

My idea is to write a class per inconsistent distribution which will inherit LinuxDistribution and implement only the inconsistent behavior while using the default class's parsing methods.

So, for instance, in Linux Mint's use case, the method under LinuxDistrubtion which returns the ordering of the files, would return something else. There will be a single method under LinuxDistribution which would call the relevant class's implementation based on the different inconsistencies which would replace the base class's attribute with the new implementation.

Any main method in the base classe (id(), version(), etc...) will call that method and if there is an inconsistency, that attribute will be replaced and returned. If there is no inconsistency, it will return the original value.

Should I provide an example, or is that clear?

Also, instead, it's possible to create a function which would identify all inconsistencies and return a class (inheriting LinuxDistribution) which would implement the relevant inconsistency handling and return the rest as is. This might be an even nicer solution.

[nir0s]

An example:

replace:

_distro = LinuxDistribution()

with:

class Centos7(LinuxDistribution):
    def codename(self):
        return 'Ridiculous'

    def version(self, pretty=False, best=False):
        return '7.9'


def _get_implementation():
    original = LinuxDistribution()
    if original.id() == 'centos' and original.version() == '7':
        return Centos7()

    return LinuxDistribution()


_distro = _get_implementation()

$ distro -j
{
    "codename": "Ridiculous", 
    "id": "centos", 
    "like": "rhel fedora", 
    "version": "7.9", 
    "version_parts": {
        "build_number": "", 
        "major": "7", 
        "minor": "9"
    }
}

I think it's a very clean solution and wonderfully easy for developers to support. It also allows you to use whatever method you want from the base class to parse files or change specific implementations.

WDYT?

[sethmlarson]

I agree completely on the solution. This PR was more to be exploratory and to spark discussion. :) My only concern is that now developers can't just call LinuxDistribution to get the proper object to describe their system like they could in the past.

[nir0s]

Why not?

…

On Apr 2, 2017 5:43 PM, "Seth M. Larson" ***@***.***> wrote: I agree completely on the solution. This PR was more to be exploratory and to spark discussion. :) My only concern is that now developers can't just call LinuxDistribution to get the proper object to describe their system like they could in the past. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#178 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB6QhMX8y-GEZWZ8MUnJo1sC1tST-Q60ks5rr7QJgaJpZM4MwouJ> .

[sethmlarson]

If they do on a system with inconsistencies they won't receive Centos7Distribution.

[nir0s]

Oh you mean if their distribution has inconsistencies. True.

…

On Apr 2, 2017 5:48 PM, "Nir Cohen" ***@***.***> wrote: Why not? On Apr 2, 2017 5:43 PM, "Seth M. Larson" ***@***.***> wrote: > I agree completely on the solution. This PR was more to be exploratory > and to spark discussion. :) My only concern is that now developers can't > just call LinuxDistribution to get the proper object to describe their > system like they could in the past. > > — > You are receiving this because you commented. > Reply to this email directly, view it on GitHub > <#178 (comment)>, or mute > the thread > <https://github.com/notifications/unsubscribe-auth/AB6QhMX8y-GEZWZ8MUnJo1sC1tST-Q60ks5rr7QJgaJpZM4MwouJ> > . >

[sethmlarson]

So I currently see two ways of dealing with it, I'm sure there's more but here's the two I could think of. One more transparent but also breaks functionality in the case that anyone is subclassing the LinuxDistribution type. I don't know why anyone would, but they could do it in theory. The other requires all code using LinuxDistribution to be rewritten.

We could rename LinuxDistribution to _LinuxDistribution and then make a function LinuxDistribution() that does exactly what _get_implementation() does. This would break things if anyone is subclassing but also would make it so no code besides that needs to change. Also a little deceptive because it looks like a class but it's actually a function but I've had to do this in other places like urllib3 to fix issues like post-import sys.path changing.

We could make _get_implementation a public method? Maybe rename it to get_distribution()? But then all code that calls LinuxDistribution directly needs to be rewritten to use the new helper.

As crazy as it sounds I think I'm more in favor of option number 1, even though it would necessitate a major version bump...

[sethmlarson]

And if you think about it option 1 lends itself well to if and when we decide to implement Windows and Mac OS functionality. Have a single function Distribution() that determines what object you actually get.

[nir0s]

While I agree that it would be "safer" to fork the class to a function, it would also be misleading and non-future-proof. While distro isn't that much widely used, I'd rather release a major version breaking the previous one and explicitly stating that it breaks stuff in BIG RED :)

Using _get_implementation, I think we're being very explicit. I think that making it a public method which returns an object (not necessarily LinuxDisribution obviously), would be safer.

We can say something like:

By default, you can still use distro.id(), etc..
If you want to get the relevant object for your distribution, simply do:

dist = distro.get_implementation() instead of dist = distro.LinuxDistrubtion().

Yes, it breaks backwards, but I'd release distro 2.0 for that to be so clean.

Also, I don't think it neglects the idea of Windows and OSx. Users will still use distro.id(), etc.. by default which gets its information from the relevant class provided by (the now public) get_implementation (or get_distribution, if you like the name better, I really don't mind).

[sethmlarson]

The name is fine with me! Sounds good. :) Question is do we throw Windows and Mac compatibility into v2 as well so we don't have two major releases?

[nir0s]

I think so. Makes sense.. But maybe we should get some more feedback about win and osx support?

[sethmlarson]

@nir0s I'd love some feedback. :) Do we have an opinion from any upstream like pip?

[nir0s]

@dstufft? @xavfernandez? @andy-maier? Your input?

[sethmlarson]

Just an FYI to reviewers first reading this PR: This PR isn't what needs review. Just need feedback on adding Windows/OSX and the proposed method of mitigating Linux distro inconsistencies by @nir0s :) Thanks!

[sethmlarson]

Since we're talking about parts here as well as in another issue I'll reference it: #177

[xavfernandez]

Concerning pip, the only place we are using distro is here: https://github.com/pypa/pip/blob/3cdd162/pip/download.py#L94-L107 , to construct a user-agent with accurate information about the user's system.
So concerning Windows/OSX, it might be interesting to have more information than we currently provide but it's not clear what it could be.
Concerning the idea to write a class per inconsistent distribution which will inherit LinuxDistribution it seems quite clean 👍

[nir0s]

I guess we can start working on the proposed solution and keep discussing additional OS related stuff in the dedicated issue.

[sethmlarson]

That sounds good to me, I'll start a little work on a v2 branch and we can take it from there.