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

Jump to bottom

lsb-release: Fix/replace with a custom Bash script #64258

Merged
merged 7 commits into from Jul 30, 2019
Merged

lsb-release: Fix/replace with a custom Bash script #64258

merged 7 commits into from Jul 30, 2019

Conversation

primeos
Copy link
Member

@primeos primeos commented Jul 3, 2019 •
edited

Motivation for this change

See #22729 and #23175.

TODOs:

  • Choose a license (I used MIT because Nixpkgs uses it, but we could also use something else)
  • Replace cat and getopt with references to the Nix store
  • Determine if we want to fallback to other files than /etc/os-release
    • Could be nice for non-NixOS systems, but probably problematic to implement as the format of the
      other files is most likely not standardized.
    • We'll skip this for now.
  • Verify the output (LGTM, but I might have missed something)
    • Thanks @davidak for checking it as well and posting additional examples
  • Verify that the script is secure and robust
    • Note: Some variables are unnecessary quoted (e.g. the ones which are either 0 or 1) and the variables sourced from /etc/os-releases are not quoted (but that file should always be owned by root).
    • Should be ok, all important variables are quoted. And this script doesn't run with any elevated privileges.
  • ...
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@primeos primeos changed the title lsb-release: Fix the package by replacing it with a custom Bash script lsb-release: Replace the package with a custom Bash script Jul 3, 2019
@primeos primeos changed the title lsb-release: Replace the package with a custom Bash script lsb-release: Fix/replace with a custom Bash script Jul 3, 2019
@primeos primeos mentioned this pull request Jul 3, 2019
Closed
12 tasks
davidak
davidak requested changes Jul 3, 2019
View reviewed changes
pkgs/os-specific/linux/lsb-release/lsb_release.sh Outdated Show resolved Hide resolved
pkgs/os-specific/linux/lsb-release/lsb_release.sh Outdated Show resolved Hide resolved
@davidak
Copy link
Member

davidak commented Jul 3, 2019 •
edited

Re: TODOS

Choose a license (I used MIT because Nixpkgs uses it, but we could also use something else)

For inclusion in this repo MIT is best.

Determine if we want to fallback to other files than /etc/os-release

Can be done later. Should not delay this PR.

Also, just support the standard lines.

Verify the output (LGTM, but I might have missed something)

This script has an additional newline at the end of the output.

Except that identical to debian and ubuntu. 👍

[davidak@nixos:~]$ /nix/store/slm41d7vfqc6i429d0hn5341li3pi0m6-lsb_release/bin/lsb_release -a
No LSB modules are available.
Distributor ID:	NixOS
Description:	NixOS 19.03.172866.4649b6ef4b5 (Koi)
Release:	19.03.172866.4649b6ef4b5
Codename:	koi


root@d646ab96b21c:/# lsb_release -a
No LSB modules are available.
Distributor ID:	Debian
Description:	Debian GNU/Linux 9.5 (stretch)
Release:	9.5
Codename:	stretch

root@ab88485573f0:/# lsb_release -a
No LSB modules are available.
Distributor ID:	Ubuntu
Description:	Ubuntu 18.04.2 LTS
Release:	18.04
Codename:	bionic

[root@5205a23e9de4 /]# lsb_release -a
LSB Version:	:core-4.1-amd64:core-4.1-noarch:cxx-4.1-amd64:cxx-4.1-noarch:desktop-4.1-amd64:desktop-4.1-noarch:languages-4.1-amd64:languages-4.1-noarch:printing-4.1-amd64:printing-4.1-noarch
Distributor ID:	Fedora
Description:	Fedora release 30 (Thirty)
Release:	30
Codename:	Thirty

[root@ba72babf3e28 /]# lsb_release -a
LSB Version:	1.4
Distributor ID:	Arch
Description:	Arch Linux
Release:	rolling
Codename:	n/a

@primeos @davidak
Update pkgs/os-specific/linux/lsb-release/lsb_release.sh
f92d52b 
Co-Authored-By: David Kleuker <davidak@users.noreply.github.com>
@primeos
Copy link
Member Author

primeos commented Jul 3, 2019

This script has an additional newline at the end of the output.

That also tricked me but this is due to the configuration of nix-shell, which prints an additional newline after each command (if you pipe to vim, xxd, etc. there's only one newline like on Debian, etc.).
Not sure where this is configured.

But I just noticed that the script from Debian prints No LSB modules are available. to stderr, I'll change that in the next commit.

Thanks for pasting the outputs on other operating systems, that's really helpful :) (I only compared it to the output on Debian stretch)

@davidak
Copy link
Member

davidak commented Jul 3, 2019

I think the newline is no problem. It's only different.

Thanks for pasting the outputs on other operating systems, that's really helpful :) (I only compared it to the output on Debian stretch)

I just used docker to quickly spin up an instance. Perfectly for such tests :)

@primeos
Copy link
Member Author

primeos commented Jul 3, 2019

I think the newline is no problem. It's only different.

What I meant is that this is caused by the shell, not by the program (lsb_release):

$ lsb_release -a
No LSB modules are available.
Distributor ID:	NixOS
Description:	NixOS 19.03.git.50e5e3a (Koi)
Release:	19.03.git.50e5e3a
Codename:	koi

$ PS1='\[\033[1;32m\][nix-shell:\w]\$\[\033[0m\] '
$ lsb_release -a
No LSB modules are available.
Distributor ID:	NixOS
Description:	NixOS 19.03.git.50e5e3a (Koi)
Release:	19.03.git.50e5e3a
Codename:	koi
$

The prompt is configured to print an additional newline (via PS1, I did initially miss that, when I had a look).

So the output should actually be identical now (apart from the actual values of course).

davidak
davidak approved these changes Jul 3, 2019
View reviewed changes
Copy link
Member

@davidak davidak left a comment

Choose a reason for hiding this comment

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

Code looks sane and script works as expected. Thanks!

@davidak
Copy link
Member

davidak commented Jul 10, 2019

@primeos is this ready for merging or do you want to change anything before?

@primeos
lsb-release: Quote the variables from /etc/os-release
ada8e0f 
Seems like a good idea as some variables could contain special
characters (e.g. "*"), especially on other operating systems.
@primeos
Copy link
Member Author

primeos commented Jul 15, 2019

@primeos is this ready for merging or do you want to change anything before?

@davidak Sorry for the delay, I ran out of time (the time of this PR was a bit suboptimal).
I just checked the TODOs and it should be ready to go :)

cc @dtzWill @rardiol (previous contributors to lsb-release): Any objections? If not I would merge this PR in a few days.

@nixos-discourse
Copy link

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/38

@primeos
lsb-release: Fix the output when /etc/os-release is missing
fa61c79 
This also fixes the short output.

The previous behaviour broke e.g. the build of radiotray-ng:
Error: /etc/os-release is not present, aborting.
Error: /etc/os-release is not present, aborting.
Error: /etc/os-release is not present, aborting.
CMake Error at package/CMakeLists.txt:10 (string):
  string no output variable specified

Building version: 0.2.6 for   -- DEB packaging...
CMake Error at package/CMakeLists.txt:56 (if):
  if given arguments:

    "STREQUAL" "16.04" "OR" "STREQUAL" "stretch"

  Unknown arguments specified

-- Configuring incomplete, errors occurred!
@primeos
Copy link
Member Author

primeos commented Jul 30, 2019

If not I would merge this PR in a few days.

Unfortunately I was pretty busy and lost track of this PR...
I just ran the rebuilds and unfortunately this broke the build for radiotray-ng.
Turns out that the output wasn't backwards compatible if /etc/os-release is missing (e.g. in the Nix build sandbox).

I did try my best to make the output as backwards compatible as possible (at least without /etc/os-release as the current binary doesn't work correctly outside of the sandbox and is therefore a bad example).
If the rebuilds succeed I'll merge this PR - let's hope there won't be any problematic regressions... (but in that case we should still be able to modify the script accordingly).

Unfortunately the code isn't really nice anymore but I guess it'll have to do for now... (we can still refactor it later if it works as intended). Also: We might want to change the default values if we're in a Nix build sandbox to increase the usefulness of the new script (currently it only helps at run-time).

@primeos primeos merged commit 63f9340 into NixOS:master Jul 30, 2019
@davidak davidak mentioned this pull request Aug 14, 2019
Closed
@Artturin Artturin mentioned this pull request Jan 10, 2023
Merged
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidak davidak davidak approved these changes

Assignees
No one assigned
Labels
10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@primeos @davidak @nixos-discourse