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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] nixos-version: Add lsb_release compatibility #23175

Closed
wants to merge 3 commits into from

Conversation

primeos
Copy link
Member

@primeos primeos commented Feb 25, 2017

Motivation for this change

See the discussion at #22729.

BUGS/TODOS:

  • Update the man page
    • The current man page states that --hash or --revision shows the full SHA1 hash of the Git commit (that changed (only if build from a git checkout), now it's the abbreviated commit hash)
  • The default is to print the following message: "No LSB modules are available." which is probably not desirable (changing the default to "nixos-version -r -s" would probably make sense, even tho it breaks full lsb_release compatibility).
    • solved: lsb_release and nixos-version now both keep their default output
  • Rename or alias nixos-version to lsb_relase (lsb_release useless on NixOS聽#22729)
  • nixos-version currently doesn't support parameters like "-rs" (only "-r -s" - I consider that a bug)
    • solved: via enhanced getopt (or something else if someone has a better idea :)
  • Verify lsb_release compatibility (I tried to stick to the specification and the lsb_release binary from Debian (wheezy)).
  • Make sure this doesn't beak too much (fix all references in nixpkgs, add something to the release notes for 17.9, nix-dev notice(?), etc.)

The script is based on this great public-domain Gist from @skx (thanks 馃槃 - I hope it's ok that I mention you).

cc #22729 @grahamc @davidak @edolstra @eduarrrd

Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@mention-bot
Copy link

@primeos, thanks for your PR! By analyzing the history of the files in this pull request, we identified @davidak, @eduarrrd and @grahamc to be potential reviewers.

@skx
Copy link

skx commented Feb 25, 2017

The script is based on this great public-domain Gist from @skx (thanks 馃槃 - I hope it's ok that I mention you).

No objection here; I'm not following NixOS but if that old script was remotely useful then I'm glad I shared it.

@davidak
Copy link
Member

davidak commented Feb 25, 2017

馃憤 all parameters work

Distributor ID: nixos is lowercase while Ubuntu is capitalized
Codename: Gorilla is capitalized while xenial (Ubuntu) is lowercase

otherwise lsb_release compatibility looks also good with Ubuntu

@primeos
Copy link
Member Author

primeos commented Feb 25, 2017

@davidak Fixed the capitalization, thanks 馃槃

Currently this requires #23190 but it could be done without that PR as well (just thought it would make more sense to add VERSION_CODENAME (must be lower-case anyway, according to the specification) to /etc/os-release).

Since the lsb_release specification says: "This specification assigns no meaning to the value of the string, the contents are at the discretion of the distribution provider." we could theoretically do whatever we want, but sticking to the Debian output sounds like a good idea.

@@ -115,16 +115,14 @@ if [[ "$all" = "1" ]] || [[ "$revision" = "1" ]]; then
fi
# TODO: echo "@nixosRevision@"?
# Revision comes from: VERSION="16.09.git.effc189 (Flounder)" -> effc189
Copy link
Member

Choose a reason for hiding this comment

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

you should also update the comment

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed, thanks 馃槃

@nbp nbp mentioned this pull request Feb 26, 2017
4 tasks
@davidak
Copy link
Member

davidak commented Jun 27, 2019

@primeos what's the status of this? this is still needed!

@primeos
Copy link
Member Author

primeos commented Jul 3, 2019

@davidak Sorry for my delayed response. IIRC this was working as expected and I assume the merge conflicts would be easy to resolve - from the technical side it shouldn't be a problem to finish this PR.

However, I do not think that we do actually need this anymore due to /etc/os-release, but forgot to close this PR...

So IMO we should re-evaluate if we do still need this, e.g.:

  • Do most other distributions still install lsb_release by default?
  • Do any major guides, manuals, exams, etc. still depend on it?
  • Are Nixpkgs maintainers/users for or against this change?

And also if we should use the lsb-release package or this shell script.

E.g. http://0pointer.de/blog/projects/os-release.html states the following:

[...] It's [lsb_release] an optional package in many distributions [...]

@davidak
Copy link
Member

davidak commented Jul 3, 2019

@primeos it would be good to have this installed by default, so programs can detect the OS, for example for telemetry collection.

I know 2 where it is needed:

We should at least have it as a package.

@primeos
Copy link
Member Author

primeos commented Jul 3, 2019

@davidak Thanks for the feedback and linking the PRs.

We should at least have it as a package.

Oh, I forgot that the lsb-release package is basically broken... :o

strace -e stat lsb_release -a
[...]
stat("/etc/lsb-release", 0x7ffdfb48b990) = -1 ENOENT (No such file or directory)
stat("/etc/redhat-release", 0x7ffdfb48b780) = -1 ENOENT (No such file or directory)
stat("/etc/debian_version", 0x7ffdfb48b2a0) = -1 ENOENT (No such file or directory)
LSB Version:	n/a
Distributor ID:	n/a
Description:	(none)
Release:	n/a
Codename:	n/a

In that case I would propose that we replace the current lsb-release package with the Bash script from this PR (I doubt that this package is useful on non-NixOS systems and would be surprised if it's still relevant). Then the tools that depend on lsb_release can use this package.

Does that sound like a good idea?

E.g. Debian apparently also uses a custom Python script: https://salsa.debian.org/debian/lsb/blob/debian/master/lsb_release

@davidak
Copy link
Member

davidak commented Jul 3, 2019

I think the most elegant solution would be a universal lsb_release script, that works on any linux distribution by trying out different release files. Then our package works also on other distros. But they probably have their own package.

So the priority should be to fix the problems on NixOS by providing a working lsb_release.

@primeos
Copy link
Member Author

primeos commented Jul 3, 2019

I think the most elegant solution would be a universal lsb_release script, that works on any linux distribution by trying out different release files. Then our package works also on other distros. But they probably have their own package.

Would be easy to implement if they would have the same format, but this is probably not the case and the other files are probably not standardised (but I haven't checked that). It might therefore be better to focus on /etc/os-release for now, but from a non-technical side it would be nice to support other files as well.

So the priority should be to fix the problems on NixOS by providing a working lsb_release.

I've opened #64258 and will close this PR for now.

@primeos primeos closed this Jul 3, 2019
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