Skip to content

Conversation

@thiagokokada
Copy link
Contributor

@thiagokokada thiagokokada commented Nov 6, 2024

The current state of nixos-rebuild is dare: it is one of the most critical piece of code we have in NixOS, but it has tons of issues:

  • The code is written in Bash, and while this by itself is not necessary bad, it means that it is difficult to do refactorings due to the lack of tooling for the language
  • The code itself is a hacky mess. Changing even one line of code can cause issues that affects dozens of people
  • Lack of proper testing (we do have some integration tests, but no unit tests and coverage is probably pitiful)
  • The code predates some of the improvements nix had over the years, e.g.: it builds Flakes inside a temporary directory and read the resulting symlink since the code seems to predate --print-out-paths flag

Given all of those above, improvements in the nixos-rebuild are difficult to do. A full rewrite is probably the easier way to improve the situation since this can be done in a separate package that will not break anyone. So this is an attempt of the rewrite.

The language of choice here is Python. I am open to other options here, and I mostly choose Python since it is the language I am most comfortable here, but I am open for other options. Still, I think Python is a good choice because:

  • It is the language of choice for many critical things inside nixpkgs, like the NixOSTestVM and systemd-boot activation scripts
  • It is a language with great tooling, e.g.: mypy for type checking, ruff for linting, pytest for unit testing
  • It is a scripting language that fits well with the scope of this project
  • Python's standard library is great and it means we will probably need zero external dependencies for this project. For example, nixos-rebuild currently depends in jq for JSON parsing, while Python has json in standard library

I am aware about the current switch-to-configuration-ng rewrite, however I am not sure what is the scope of that project vs nixos-rebuild. If the idea is just a drop-in replacement than both of those rewrites are ortogonal, since nixos-rebuild also includes some extra logic for e.g.: profile management. If the idea is to migrate more and more logic to switch-to-configuration-ng and eventually drop nixos-rebuild, I am happy to close this PR.

Objectives and Non-objectives

  • Be as much of a drop-in replacement as possible
  • Fix obvious bugs
  • Obvious improvements that are non-breaking
  • Do not change logic even if this would be an improvement

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • 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/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 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
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added the 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS label Nov 6, 2024
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/nixos-rebuild-ng-a-nixos-rebuild-rewrite/55606/1

@github-actions github-actions bot removed the 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS label Nov 6, 2024
@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. labels Nov 6, 2024
@thiagokokada thiagokokada force-pushed the nixos-rebuild-python branch 16 times, most recently from 8866519 to 5ff4130 Compare November 6, 2024 22:53
@thiagokokada thiagokokada force-pushed the nixos-rebuild-python branch 3 times, most recently from 3f23d12 to c7d41e2 Compare November 6, 2024 23:51
@Scrumplex
Copy link
Member

I think this looks good for an early adopter release

@thiagokokada thiagokokada force-pushed the nixos-rebuild-python branch 2 times, most recently from bb2e3ef to e19ac27 Compare November 15, 2024 10:57
@thiagokokada
Copy link
Contributor Author

CC @Sigmanificient since you made some good reviews about my Python code.

@thiagokokada thiagokokada force-pushed the nixos-rebuild-python branch 7 times, most recently from f7c00d1 to c4264e5 Compare November 15, 2024 17:07
@thiagokokada
Copy link
Contributor Author

@ofborg test nixos-rebuild-ng

@thiagokokada
Copy link
Contributor Author

@ofborg build nixos-rebuild-ng

@lucasew
Copy link
Contributor

lucasew commented Nov 15, 2024

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 354029


x86_64-linux

✅ 2 packages built:
  • nixos-rebuild-ng
  • nixos-rebuild-ng.dist

@thiagokokada
Copy link
Contributor Author

thiagokokada commented Nov 15, 2024

For fun:

$ hyperfine -i "nixos-rebuild list-generations" "./result/bin/nixos-rebuild-ng list-generations"
Benchmark 1: nixos-rebuild list-generations
  Time (mean ± σ):     510.0 ms ±  13.2 ms    [User: 182.7 ms, System: 361.7 ms]
  Range (min … max):   492.9 ms … 532.5 ms    10 runs

  Warning: Ignoring non-zero exit code.

Benchmark 2: ./result/bin/nixos-rebuild-ng list-generations
  Time (mean ± σ):     108.4 ms ±   5.3 ms    [User: 81.6 ms, System: 26.0 ms]
  Range (min … max):    98.5 ms … 119.0 ms    27 runs

Summary
  ./result/bin/nixos-rebuild-ng list-generations ran
    4.71 ± 0.26 times faster than nixos-rebuild list-generations

BTW, I either need to run nixos-rebuild with sudo or ignore the fact that it exists with error (even if it outputs the result), because it fails with find: ‘/nix/var/nix/profiles/per-container’: Permission denied. This is fixed in nixos-rebuild-ng.

@lucasew
Copy link
Contributor

lucasew commented Nov 16, 2024

Nice!

It's using the manpage of nixos-rebuild now. Maybe we should gather more feedback around this --help behaviour but I tested here and it works as expected.

LGTM

@thiagokokada
Copy link
Contributor Author

thiagokokada commented Nov 16, 2024

We had the branch-off for release-24.11 already so I am going to merge this.

There is a failure for x86_64-darwin right now but this doesn't matter too much since one of the first changes I will do in the next PR is to reduce the build closure, that should fix this issue.

@thiagokokada thiagokokada merged commit 1b68d68 into NixOS:master Nov 16, 2024
26 of 27 checks passed
@thiagokokada thiagokokada deleted the nixos-rebuild-python branch November 16, 2024 10:47
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/nixos-rebuild-ng-a-nixos-rebuild-rewrite/55606/17

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2.status: wait for branch‐off Waiting for the next Nixpkgs branch‐off 6.topic: continuous integration Affects continuous integration (CI) in Nixpkgs, including Ofborg and GitHub Actions 6.topic: policy discussion Discuss policies to work in and around Nixpkgs 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. 11.by: package-maintainer This PR was created by a maintainer of all the package it changes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants