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

Add support for OpenBSD, NetBSD, and FreeBSD #207

Merged
merged 15 commits into from
Jan 21, 2018
Merged

Add support for OpenBSD, NetBSD, and FreeBSD #207

merged 15 commits into from
Jan 21, 2018

Conversation

sethmlarson
Copy link
Contributor

I had to use uname for *BSD as it doesn't supply lsb-release, os-release, or dist-release info the way we expect so instead I collapse uname output into lsb-release properties. Fixes #191

@sethmlarson
Copy link
Contributor Author

Seems to be failing the codecov/patch coverage check, can this be ignored @nir0s?

Copy link

@woodsb02 woodsb02 left a comment

Choose a reason for hiding this comment

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

Thanks for your efforts Seth!

@@ -0,0 +1,4 @@
#!/bin/bash

Choose a reason for hiding this comment

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

Can this be #!/bin/sh instead?
Note that the BSDs do not have bash installed by default. This comment should apply to all shell scripts that are not using any bash specific features.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely!

@@ -0,0 +1,4 @@
#!/bin/bash

Choose a reason for hiding this comment

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

#!/bin/sh

@@ -0,0 +1,4 @@
#!/bin/bash

Choose a reason for hiding this comment

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

#!/bin/sh

distro.py Outdated
@@ -932,9 +932,16 @@ def _lsb_release_info(self):
cmd = ('lsb_release', '-a')
stdout = subprocess.check_output(cmd, stderr=devnull)
except OSError: # Command not found
return {}
try:

Choose a reason for hiding this comment

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

Maybe the name of this function should be changed from _lsb_release_info, given that it is not using lsb information? Or create a separate _uname_info function that is used if the OS is not Linux?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I think I'm going to do that. Definitely felt weird tagging along on lsb_release's prop names.

distro.py Outdated
@@ -960,6 +967,18 @@ def _parse_lsb_release_content(lines):
props.update({k.replace(' ', '_').lower(): v.strip()})
return props

@staticmethod
def _parse_uname_content(lines):

Choose a reason for hiding this comment

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

2 extra pieces of information could be added for FreeBSD:

$ uname -rs
FreeBSD 11.1-RELEASE-p4
  1. On FreeBSD the distro.codename() function could output either "RELEASE", "STABLE" or "CURRENT" which are described here: https://www.freebsd.org/doc/handbook/current-stable.html

  2. On FreeBSD the distro.build_number() output could be the kernel patch level. FreeBSD releases get regular security/errata patches which get added to the uname output if they affect the kernel. In the above output, the kernel is at patch level "p4". Note that the userland can at times be patched separate from the kernel, leading to userland patchlevel and kernel patchlevel being different. Note that uname only provides kernel details. The way to show userland patch level on FreeBSD is "freebsd-version -u", whilst kernel patch level is shown on uname or exactly the same in "freebsd-version -k".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. In my VM I didn't see the -p4, only the -RELEASE. The thing is that's not really the "codename"? Codename typically is the name of the release like "Trusty" for Ubuntu 14.04. I'm not sure if RELEASE, STABLE and CURRENT fit that bill enough to use them for distro.codename()?

  2. Could the p be dropped from the p4, does it have meaning? I'm pretty sure no other distro.version() call will result in anything but integers and dots so it would break distro.version_parts()'s contract? Not 100% certain here.

@woodsb02 woodsb02 mentioned this pull request Dec 31, 2017
@sethmlarson
Copy link
Contributor Author

Thanks @woodsb02 for the review, I'll take a look at the comments and expand FreeBSD support. :)

@sethmlarson
Copy link
Contributor Author

sethmlarson commented Dec 31, 2017

So I wasn't able to get any additional information from freebsd-version command compared to uname so I think we'll just stick with that for now? All other comments have been addressed though. :)

@sethmlarson
Copy link
Contributor Author

Pinging @nir0s for a review, this patch should be ready to ship. :)

@nir0s
Copy link
Collaborator

nir0s commented Jan 8, 2018

Thanks! I'll review.. But regardless of that, it would be great to add the coverage.

@sethmlarson
Copy link
Contributor Author

I'll see what I can do!

@nir0s
Copy link
Collaborator

nir0s commented Jan 9, 2018

So, I think that this PR means it's not longer a "Linux OS Distribution Information API" :)

The PR should include changes to the README, docs and code removing the "Linux" specificity. Given that we want to propose your addition for Windows and OSX support, I guess we can call distro an "OS Platform Information API".

@nir0s
Copy link
Collaborator

nir0s commented Jan 9, 2018

@woodsb02, would you also take a look please?

@sethmlarson
Copy link
Contributor Author

I'll comb through everything and try to remove as much as I can. :)

@nir0s nir0s merged commit 16fd311 into python-distro:master Jan 21, 2018
@sethmlarson
Copy link
Contributor Author

Thanks @nir0s! :)

@woodsb02
Copy link

Indeed, thanks for this!

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.

4 participants