nixos-rebuild-ng: fix switch inside a path symlinked to nix store#417191
nixos-rebuild-ng: fix switch inside a path symlinked to nix store#417191thiagokokada wants to merge 2 commits intoNixOS:masterfrom
Conversation
github-actions
bot
added
10.rebuild-darwin: 1-10
|
Other options here:
|
|
I don't think it is that difficult to be honest. From what I understand it is just the I gave it a try here: #417203.
I want to avoid depending more in the NixOS implementation details to be honest.
This probably still needs to be done either way. |
|
Honestly, "does the symlink I got back have a |
Do we always have a bootspec though? And if you want to implement this, sure, I find no issue. I am not really familiar with the bootspec to know what to do here. |
PR NixOS#375493 was introduced to fix an issue of different behavior between `nixos-rebuild repl` and `nixos-rebuild switch` by forcing usage of `git+file://` protocol when evaluating the Flake. This sadly reintroduced an older issue from the original `nixos-rebuild` that is caused by a pretty nasty bug in `nix`: - NixOS#144811 Let's do the same fix we did for `nixos-rebuild` and just stopping normalizing the Flake (NixOS#153515). This will bring back the original issues this code is supposed to fix, but I argue that a difference between `nixos-rebuild repl` and `nixos-rebuild switch` is better than having a broken system.
thiagokokada
force-pushed
the
revert-375493
branch
from
c66d710 to
96bc121
Compare
|
IMO, I would much prefer to revert this change first to avoid causing more issues and I am still not completely clear the benefits of this change (I understand that it has some difference in behavior from Also reverting this change matches the behavior of old Just checking for the bootspec files or anything else will still cause the bad behavior of |
I guess not. There is an internal option to disable it, so there is at least some degree of expectation that it may not exist: It sure looks like
I had the idea this was fixing something a little more pervasive. Sounds like I misunderstood by a bit.
Fair point. Though I do think if the error message mentions it may be caused by running from the nix store it's not much of an issue. |
This is why I want the input of @Mic92 before merging this PR, but AFAIK it is only the difference between |
thiagokokada
force-pushed
the
revert-375493
branch
from
63d4c67 to
616d7ff
Compare
| flake_str: str, | ||
| hostname_fn: Callable[[], str | None] = lambda: None, | ||
| ) -> Self: | ||
| def parse(cls, flake_str: str, target_host: Remote | None = None) -> Self: |
There was a problem hiding this comment.
For reviewers: sorry for adding a refactor for a supposedly revertion PR only but this code was really bothering me.
I understand why I wrote it that way (I wanted to make hostname computation to be lazy since it can be somewhat expensive, especially in the --target-host case), but I ended up creating a code that can only be summed up as "it was probably wrote by a drunk person", and I don't even drink alcohol.
If reviewing the changes is difficult, please review by commit, it should be easier to understand what is going on.
|
Replaced by #418243. |
2 participants
PR #375493 was introduced to fix an issue of different behavior between
nixos-rebuild replandnixos-rebuild switchby forcing usage ofgit+file://protocol when evaluating the Flake. This sadly reintroduced an older issue from the originalnixos-rebuildthat is caused by a pretty nasty bug innix:Let's do the same fix we did for
nixos-rebuildand just stopping normalizing the Flake (#153515). This will bring back the original issues this code is supposed to fix, but I argue that a difference betweennixos-rebuild replandnixos-rebuild switchis better than having a broken system.Fix: #144811.
Before
After
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.