Skip to content
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

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

Closed
wants to merge 10 commits into from
Closed

Conversation

sethmlarson
Copy link
Contributor

@sethmlarson sethmlarson commented Apr 18, 2017

Been doing a little work for v2 in my own branch. If anything else needs to be added I'll add it to the checklist. Here's what I have so far:

Checklist

  • distro is now a package instead of one file. This means that Python 2.6 can't run it as a module via python -m distro or python -m distro -j but since we're incrementing a major version and this feature isn't used by our most important upstream pip this is probably fine?
  • Split testing up into "CLI", "Linux", "Windows".
  • Changed LinuxDistribution object to inherit from a new object Distribution which is the base for all distribution info objects.
  • Added a new function called get_distribution() which gets the Distribution object for the current OS platform. This is where sub-classes of LinuxDistribution are created if there is an inconsistency between one of the *-release sources (such as LinuxMintDistribution.)
  • ImportError is raised if not on Windows, Linux, or Mac OS platforms.
  • MacOSDistribution needs to be completely finished and needs testing. I don't have my own Mac OS to get real values for this but I'm sure most of the values can be guessed after seeing a few real values of platform.mac_ver().
  • CloudLinuxDistribution needs to be created to deal with inconsistencies for Cloud Linux once Add test cases for CloudLinux 5, 6, and 7 #180 has been merged.
  • DebianDistribution for when debian doesn't use parens for it's codename in "stretch/sid".
  • Go through all comments, docs, and package info and remove all mentions distro of being Linux-only.
  • Resolve all other inconsistencies that I've missed and update tests to reflect.
  • Create an EmptyDistribution object to return when distro.linux_distribution() is called on a non-Linux platform.
  • Enable Mac OS builders on Travis CI (?)
  • Get tests to cover the module 100% and then start enforcing 100% coverage. (?)
  • Add all changes to the CHANGELOG and explain the new major release.

Questions to Resolve

  • Should calling distro.linux_distribution() on a non-Linux platform emit a warning?
  • Should we add distro.windows_distribution() and distro.mac_distribution? I'm in favor of saying "No" to this to encourage users to use distro.distribution() and only keeping distro.linux_distribution() for legacy reasons.
  • Should we raise an ImportError if we're on a system we don't recognize or should we just return an EmptyDistribution object for unknown platforms?
  • Continue to support 1.0.x versions? Cut a v1 branch on GitHub and work from that with master as v2 after this is approved?

References

#177 #178 #180 #184

cc: @nir0s

@nir0s
Copy link
Collaborator

nir0s commented Apr 19, 2017

This is a wonderful effort and I like the implementation. It's very clean. Thank you.

Several comments.

  1. I think we should keep it a single module instead of a package. I think that distro should be as portable as possible so that it can even be copied to servers on the fly to retrieve distribution information. I'm certainly going to be using fabric to deploy distro to servers to retrieve info and making distro a package makes it very hard if not impossible. I don't see why the entire codebase couldn't fit in a single file. It still has a very very small footprint.
  2. I think we have to separate the inconsistency framework and the windows/mac/ support into multiple requests. These are very different things and we need multiple reviewers to make sure everything fits. We also need the ability to merge only some of the code. I'd start from a framework PR for inconsistencies on top of which we could add multi-OS support.
  3. We should still verify that it makes sense to break backward compat with regard to the LinuxDistribution class. What I proposed was just an idea :)

@xavfernandez @andy-maier @MartijnBraam @funkyfuture. Would love your input here.

@sethmlarson
Copy link
Contributor Author

Sure thing, I'll break this whole effort into two requests. :) Thanks for the review!

Copy link
Contributor

@funkyfuture funkyfuture left a comment

Choose a reason for hiding this comment

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

just a few superficial remarks. you may ping me when the code has been merged into one module again,

though imo it's close to being too long for one module (which is at this point rather a matter of gusto and not yet hard to maintain), but essentially agree with @nir0s points.


"""
The ``distro`` package (``distro`` stands for Linux Distribution) provides
information about the Linux distribution it runs on, such as a reliable
Copy link
Contributor

@funkyfuture funkyfuture Apr 19, 2017

Choose a reason for hiding this comment

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

that'd need to be adjusted.

"""
return _distro.info(pretty, best)

if platform.system() == 'Linux':
Copy link
Contributor

Choose a reason for hiding this comment

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

imports should be on the top of the module and be absolute. older Python versions have from __future__ import absolute_imports available.

elif platform.system() == 'Mac OS':
from ._mac_os import get_distribution
else:
from ._distribution import EmptyDistribution
Copy link
Contributor

Choose a reason for hiding this comment

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

rather UnknownDistribution or Unidentified…?

@sethmlarson
Copy link
Contributor Author

sethmlarson commented Apr 19, 2017

@funkyfuture I've split this into another PR #186 for just Linux. I'm keeping this PR here mostly for the check boxes. Thank you for your review! :)

@sethmlarson
Copy link
Contributor Author

I'd still like to get these changes merged. Is this still on the table?

@dsully
Copy link

dsully commented Feb 13, 2018

I'd also like to see this - particularly macOS.

@sethmlarson sethmlarson closed this Jul 2, 2018
@dsully
Copy link

dsully commented Jul 2, 2018

Any reason this was closed?

@sethmlarson
Copy link
Contributor Author

My pull requests for this project have dwelled too long. Anyone is welcome to finish the work out if they'd like. I left the branch un-deleted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants