nixos-rebuild-ng: support using nix-output-monitor by philiptaron · Pull Request #455383 · NixOS/nixpkgs

philiptaron · 2025-10-24T22:23:45Z

This PR introduces the NixOS option system.rebuild.enableNom, which causes nixos-rebuild-ng to use nix-output-manager to display a beautiful tree of dependencies being built.

Its first commit is from #454982 which should be merged first.

Recommend reviewing commit-by-commit. Most of the commits prior to the last one are just refactoring and could be jettisoned if objected to, but ... I like them. 🙂

I tested this out on my system and it works great.

Things done

I'm interested in using `nom` for `nixos-rebuild`, which wants to do this. In order to avoid the patch breaking on upgrade without someone noticing, it's applied unilaterally, just with do-nothing replacements.

This enables removal of the `with` statements in the derivation.

It's a little strange to patch something that's in Nixpkgs, but it works great.

This provides the place to customize the Nix being executed without needing to use `propagatedBuildInputs`.

…han a patch

`nom` doesn't accept `--extra-experimental-features` before `nom build`; since `nix` accepts it anywhere in the command line, put it after the build. The tests also need to be updated for the `NIX_OR_NOM` constant.

This uses `nix-output-monitor` to produce a much better looking `nixos-rebuild` experience.

thiagokokada

Did just a very high look at this, but there is a bunch of controversial changes:

It seems that now this always run the reexec code, while the original version this is conditionally enabled on purpose (we disable this if the user runs nixos-rebuild-ng standalone, i.e., not as part of NixOS, because reexec can do the wrong thing in those cases)
There is some undocumented changes in behavior, like printing help instead of the manpage
Hardcoding binary names to be patched later. This seems bad because we will need to update the patch every time we need to add a new executable
Not sure why we need to split the package in 3 files? I like the split between tests and the main package, but anything else seems unnecessary

In general this PR just seems to be trying to many things at once. To get it to a mergeable state I would recommend trying to focus in either introducing nix-output-monitor or refactoring the code. But even them I would prefer for us to talk before doing bigger refactoring, there is a reason a few things are the way they are.

philiptaron · 2025-10-24T23:02:10Z

But even them I would prefer for us to talk before doing bigger refactoring, there is a reason a few things are the way they are.

Yes, let's definitely do that. How about here? I'm also available on Matrix, but for these sorts of discussions I prefer GitHub.

I'll respond in separate threads to the other issues, and I'm always glad to split into multiple PRs.

thiagokokada · 2025-10-24T23:11:16Z

Yes, let's definitely do that. How about here? I'm also available on Matrix, but for these sorts of discussions I prefer GitHub.

Since I think this will need some back and forth I think Matrix will work better. But as a general feedback:

Avoid patching code that is already in nixpkgs tree, this is why the constants.py file is that way and unless there is a better solution I would prefer to keep it as-is
I prefer to use the Python wrapper to set the PATH for external programs instead of setting the full path for each program
Either implement a new feature or refactor, don't do both in the same PR

I think this should be enough for now, but if you want I can do a proper review tomorrow.

philiptaron · 2025-10-24T23:40:01Z

Is there a matrix room for these sorts of discussions? One reason I favor GitHub is due to the ease with which we can scope the discussion but still have it in public. The other is that my favorite matrix client just got ejected from Nixpkgs due to a dependency on libsoup, so I am currently adapting (poorly) to other clients.

thiagokokada · 2025-10-24T23:13:22Z

nixos/modules/installer/tools/tools.nix

  };

+  options.system.rebuild.enableNom = lib.mkEnableOption "" // {
+    default = false;


I think this is the default already.

Sounds good, I'll remove.

thiagokokada · 2025-10-24T23:14:00Z

pkgs/by-name/ni/nix-output-monitor/package.nix

I can't really review the nix-output-monitor changes so you better ping the maintainer for that.

But do we really need those patches? When we wrap the Python programs we add everything from nativeBuildInputs to the PATH. So I assume just adding nix-output-monitor to nixos-rebuild-ng derivation should be sufficient here.

That's true. I preferred to avoid the wrapper in favor of embedding the Nix store path in the file, but I'll respect your choice in the matter.

thiagokokada · 2025-10-25T15:13:05Z

pkgs/by-name/ni/nixos-rebuild-ng/python.nix

+  pytestFlags = [ "-vv" ];
+
+  makeWrapperArgs =
+    lib.optional (!withReexec) "--set NIXOS_REBUILD_REEXEC_ENV 1"


I know this looks a clever way to avoid having this as a build constant, but the reason we had this is because we can run nixos-rebuild-ng without building it (by running nix-shell -A nixos-rebuild-ng.devShell && python -m nixos_rebuild) and this is nice for hacking new features.

Now if you do the same this will try to re-exec instead so you can really hack new things, unless you remember to pass --no-reexec flag.

thiagokokada · 2025-10-25T15:15:48Z

pkgs/by-name/ni/nixos-rebuild-ng/0001-replacements.patch

I am against us patching files for code that is inside the nixpkgs street since this makes adding new features unnecessary complicated: we need to add the code first, commit, modify the code, create a patch and commit the path. This is why the constants.py file was the way it was.

I'll return to the constants/--replace-fail route.

thiagokokada · 2025-10-25T15:16:31Z

pkgs/by-name/ni/nixos-rebuild-ng/0002-help-runs-man.patch

Same here, let's avoid needing to patch code that is inside nixpkgs.

Sounds good. I'll return to the constants route.

thiagokokada

BTW, does this support --target-host and --build-host ? I don't think it is necessarily a blocker to not support them, but if we don't support we need to at least document this somewhere.

philiptaron added 10 commits October 24, 2025 14:34

nix-output-monitor: allow pinning a specific Nix version

a6ede7f

I'm interested in using `nom` for `nixos-rebuild`, which wants to do this. In order to avoid the patch breaking on upgrade without someone noticing, it's applied unilaterally, just with do-nothing replacements.

nixos-rebuild-ng: use callPackage from python3Packages

7154574

This enables removal of the `with` statements in the derivation.

nixos-rebuild-ng.tests.linters: fix build

2f3f59b

nixos-rebuild-ng: extract linters passthru to separate file

effe701

nixos-rebuild-ng: replace postPatch with a real patch

25492df

It's a little strange to patch something that's in Nixpkgs, but it works great.

nixos-rebuild-ng: extract constants for every Nix command

c39ea91

This provides the place to customize the Nix being executed without needing to use `propagatedBuildInputs`.

nixos-rebuild-ng: implement withShellFiles directly

525891d

nixos-rebuild-ng: implement withReexec using makeWrapperArgs rather t…

079cf93

…han a patch

nixos-rebuild-ng: prepare for using nom build

e9be87f

`nom` doesn't accept `--extra-experimental-features` before `nom build`; since `nix` accepts it anywhere in the command line, put it after the build. The tests also need to be updated for the `NIX_OR_NOM` constant.

nixos: introduce system.rebuild.enableNom option

d0e571d

This uses `nix-output-monitor` to produce a much better looking `nixos-rebuild` experience.

philiptaron requested a review from thiagokokada October 24, 2025 22:23

philiptaron mentioned this pull request Oct 24, 2025

nix-output-monitor: allow pinning a specific Nix version #454982

Open

16 tasks

nix-owners bot requested a review from maralorn October 24, 2025 22:29

thiagokokada requested changes Oct 24, 2025

View reviewed changes

philiptaron marked this pull request as draft October 24, 2025 22:56

thiagokokada requested changes Oct 25, 2025

View reviewed changes

thiagokokada reviewed Oct 25, 2025

View reviewed changes

nixpkgs-ci bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 7, 2025

Uh oh!

Conversation

philiptaron commented Oct 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Things done

Uh oh!

thiagokokada left a comment

Choose a reason for hiding this comment

Uh oh!

philiptaron commented Oct 24, 2025

Uh oh!

thiagokokada commented Oct 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

philiptaron commented Oct 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thiagokokada left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

philiptaron commented Oct 24, 2025 •

edited

Loading

thiagokokada commented Oct 24, 2025 •

edited

Loading