Skip to content

Conversation

@cab404
Copy link
Member

@cab404 cab404 commented Mar 14, 2022

Description of changes

Recent change made ID=nixos checks, used by many programs to patch static nixos binaries and other reasons, obsolete.
This removes ID and ID_LIKE fields from being quoted — as quoting those violates should never be necessary according to os-release file format

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.05 Release Notes (or backporting 21.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Mar 14, 2022
@cab404
Copy link
Member Author

cab404 commented Mar 14, 2022

I can also add lowercase, whitespace and other checks, just in case :D

@cab404 cab404 requested a review from peterhoeg March 14, 2022 04:14
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Mar 14, 2022
@cab404
Copy link
Member Author

cab404 commented Mar 14, 2022

With this patch, ID field is generated correctly

BUG_REPORT_URL="https://github.com/NixOS/nixpkgs/issues"
BUILD_ID="22.05.20220314.dirty"
DOCUMENTATION_URL="https://nixos.org/learn.html"
HOME_URL="https://nixos.org/"
ID=nixos
LOGO="nix-snowflake"
NAME="NixOS"
PRETTY_NAME="NixOS 22.05 (Quokka)"
SUPPORT_URL="https://nixos.org/community.html"
VERSION="22.05 (Quokka)"
VERSION_CODENAME="quokka"
VERSION_ID="22.05"

@peterhoeg
Copy link
Member

peterhoeg commented Mar 14, 2022 via email

@cab404
Copy link
Member Author

cab404 commented Mar 14, 2022

This breaks a lot of existing checks for nixos, as it replaces ID=nixos with ID="nixos".
If checks are broken, they should be fixed as there is nothing wrong with having the fields without spaces quoted as you correctly point out: Assignments that do not include these special characters may be enclosed in quotes too, but this is optional. Having a hardcoded list of fields seems like a suboptimal way to solve it, so in order of preference: 1. fix the places that break because they assume a literal ID=nixos (just out curiosity what exactly breaks?) 2. if the value contains characters outside A–Z, a–z, 0–9, quote it - otherwise don't. And this should be regardless of the field. A hardcoded workaround for 2 specific fields is not the way to go so NAK on this specific solution.

E.g rust-analyzer’s vscode extension (I patched it), Airshipper, probably many more unexpected scripts.
I think that introducing this may break too much many things, and that is generally unnecessary anyway, so 2 lgtm. I see the solution with hardcoding 2 fields as valid as those two fields have different constraints in /etc/os-release documentation.

@peterhoeg
Copy link
Member

peterhoeg commented Mar 14, 2022 via email

@cab404
Copy link
Member Author

cab404 commented Mar 15, 2022

@peterhoeg please, re-review

@cab404 cab404 changed the title nixos/modules/version: preserve ID and ID_LIKE fields from being quoted os-release: preserve fields from being excessively quoted Mar 15, 2022
@cab404 cab404 force-pushed the master branch 2 times, most recently from 99108a2 to a9007d1 Compare March 15, 2022 10:09
@cab404 cab404 requested a review from peterhoeg March 15, 2022 21:00
@balsoft balsoft merged commit 1e49b30 into NixOS:master Mar 16, 2022
@peterhoeg
Copy link
Member

Thank you all!

ben0x539 added a commit to ben0x539/rust that referenced this pull request Mar 23, 2022
Per https://www.freedesktop.org/software/systemd/man/os-release.html,

> Variable assignment values must be enclosed in double or single quotes
> if they include spaces, semicolons or other special characters outside
> of A–Z, a–z, 0–9. (Assignments that do not include these special
> characters may be enclosed in quotes too, but this is optional.)

So, past `ID=nixos`, let's also check for `ID='nixos'` and `ID="nixos"`.

One of these is necessary between NixOS/nixpkgs#162168 and
NixOS/nixpkgs#164068, but this seems more correct either way.
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Apr 5, 2022
bootstrap.py: nixos check in /etc/os-release with quotes

Per https://www.freedesktop.org/software/systemd/man/os-release.html,

> Variable assignment values must be enclosed in double or single quotes
> if they include spaces, semicolons or other special characters outside
> of A–Z, a–z, 0–9. (Assignments that do not include these special
> characters may be enclosed in quotes too, but this is optional.)

So, past `ID=nixos`, let's also check for `ID='nixos'` and `ID="nixos"`.

One of these is necessary between NixOS/nixpkgs#162168 and
NixOS/nixpkgs#164068, but this seems more correct either way.
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Apr 5, 2022
bootstrap.py: nixos check in /etc/os-release with quotes

Per https://www.freedesktop.org/software/systemd/man/os-release.html,

> Variable assignment values must be enclosed in double or single quotes
> if they include spaces, semicolons or other special characters outside
> of A–Z, a–z, 0–9. (Assignments that do not include these special
> characters may be enclosed in quotes too, but this is optional.)

So, past `ID=nixos`, let's also check for `ID='nixos'` and `ID="nixos"`.

One of these is necessary between NixOS/nixpkgs#162168 and
NixOS/nixpkgs#164068, but this seems more correct either way.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants