Conversation
039ccd4 to
62c6b8d
Compare
| ; | ||
| # nixos-rebuild is wrapped to add the repository HEAD commit id to the added boot menu entry. | ||
| # Note the derivation produced by this nixos-rebuild wrapper then differs from the one produced from the repository itself since it injects the commit id via builtins.getEnv. | ||
| nixos-rebuild-wrapped = pkgs.writeShellApplication { |
There was a problem hiding this comment.
it should be moved into a proper overlay directory and overlaid in the ambient pkgs so that every other module can refer to it, including the upgrade manual command
i also think it should shadow nixos-rebuild directly and re-expose nixos-rebuild unwrapped as an attribute on it as it's usual etiquette in nixpkgs
There was a problem hiding this comment.
In the overlay, i then added nixos-rebuild (wrapped with git rev-parse) and nixos-rebuild-unwrapped (original version).
I built the auto-upgrade systemd unit to confirm the nixos-rebuild tool is the wrapped one.
For convenience, it allows to get the commit id from the boot menu, but this modifies the derivation produced by nixos-rebuild since the commit id is injected via the builtins.getEnv.
62c6b8d to
120f20e
Compare
|
@rlahfa-dinum I also renamed |
rlahfa-dinum
added
status: awaiting-maintainers
| git | ||
| ]; | ||
| text = '' | ||
| revision=$(git rev-parse HEAD) |
There was a problem hiding this comment.
What happens if this is run not in a Git repo? Can we have minimal fallback/error-handling here?
| # | ||
| # SPDX-License-Identifier: MIT | ||
|
|
||
| # nixos-rebuild is wrapped to add the repository HEAD commit id to the added boot menu entry. |
There was a problem hiding this comment.
This should be called git-aware-nixos-rebuild or something.
There was a problem hiding this comment.
I finally called it nixos-rebuild-git-aware.
Actually, i'm a bit confused because in your previous comments, I had understood that you wanted to override nixos-rebuild while exposing the original one via the attribute nixos-rebuild-unwrapped.
In this fixup commit, I do not modify the attribute nixos-rebuild but just add an attribute nixos-rebuild-git-aware. This is much simpler.
There was a problem hiding this comment.
I wrote:
it should be moved into a proper overlay directory and overlaid in the ambient pkgs so that every other module can refer to it, including the upgrade manual command
i also think it should shadow nixos-rebuild directly and re-expose nixos-rebuild unwrapped as an attribute on it as it's usual etiquette in nixpkgs
this doesn't say anything about the name of the attribute? shadowing meant that it should shadow the nixos-rebuild binary in $PATH, but you can have the attribute named nixos-rebuild-git-aware producing a bin/nixos-rebuild and having a nixos-rebuild-git-aware.unwrapped attribute to get you access to the git-unaware attribute?
|
|
||
| super: self: (import ./default.nix { inherit (super) callPackage; }) | ||
| final: prev: | ||
| (import ./default.nix { |
There was a problem hiding this comment.
Let's avoid passing all the arguments one by one and reuse dependency injection via callPackage otherwise the pattern is self defeating.
There was a problem hiding this comment.
I don't think this is still relevant with my last commit.
| { callPackage, nixos-rebuild }: { | ||
| mkPlasmaLookAndFeelPackage = callPackage ./plasma/mk-look-and-feel-package.nix { }; | ||
| plasma-portail-tray-icon = callPackage ./plasma/portail-tray-icon { }; | ||
| nixos-rebuild = callPackage ./nixos-rebuild { inherit nixos-rebuild; }; |
There was a problem hiding this comment.
You should define git-aware-nixos-rebuild on its own here while letting callPackage polyfill it.
There was a problem hiding this comment.
I don't think this is still relevant with my last commit.
| final: prev: | ||
| (import ./default.nix { | ||
| inherit (final) callPackage; | ||
| nixos-rebuild = prev.nixos-rebuild; |
There was a problem hiding this comment.
You should define a new overlay on its own, not in pkgs/default here, that makes use of a prev.callPackage pattern.
There was a problem hiding this comment.
I don't think this is still relevant with my last commit.
| # SPDX-License-Identifier: MIT | ||
|
|
||
| # nixos-rebuild is wrapped to add the repository HEAD commit id to the added boot menu entry. | ||
| # Note the derivation produced by this nixos-rebuild wrapper then differs from the one produced from the repository itself since it injects the commit id via builtins.getEnv. |
There was a problem hiding this comment.
Can you add a link to the nixos-rebuild wrapper produced by the repository itself and the place where it makes use of builtins.getEnv ?
There was a problem hiding this comment.
Can you add a link to the nixos-rebuild wrapper produced by the repository itself
What do you mean?
(This file contains the wrapper itself.)
I added a link to the nixpkgs code using getEnv (maybeEnv actually, which is a wrapper around getEnv).
There was a problem hiding this comment.
Note the derivation produced by this nixos-rebuild wrapper then differs from the one produced from the repository itself since it injects the commit id via builtins.getEnv.
This comment seems to say there's a different piece of code to look at. Otherwise, can you try to rework the comment? I think I have trouble understanding it after rereading a couple of review passes already.
rlahfa-dinum
left a comment
rlahfa-dinum
left a comment
There was a problem hiding this comment.
Minor points but I think this is going to cause confusion otherwise if we do not nip this in the bud.
rlahfa-dinum
added
status: awaiting-author
rlahfa-dinum
left a comment
rlahfa-dinum
left a comment
There was a problem hiding this comment.
Looks good to me, I think this should be tested real quick. Let's wait on someone from the team who has time to test this branch.
rlahfa-dinum
added
the
status: awaiting smoke testing
2 participants
For convenience, it allows to get the commit id from the boot menu, but this modifies the derivation produced by nixos-rebuild since the commit id is injected via the builtins.getEnv.