New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
cryptsetup, lvm2, systemd: Break cyclic dependency at a different point #97008
Conversation
bebec08
to
885da22
Compare
I'm building this PR locally to test it out; but it's taking a while. This looks better than #97002 to me though. So feel free to close that one already |
This probably means that a change like in f6286de needs to be made again as well. |
I dont fully understand what was done there and why; but I think @flokli has more context. The test suite passed by the way |
@FRidh as you use a separate |
For the installer tests all packages being installed need to be made available in the test, because the installer cannot download them. Instead of building without udev we now build without cryptsetup. Thus, the installer also needs to have offline lvm2 without cryptsetup instead of without udev. |
|
This PR's approach seems to put two different instances of |
I agree it is not ideal; but we can merge it now to unbreak staging and |
I think we should go ahead with this. That way we can have a branch-off. The fix for this we require for release and can be cherry-picked then to the release branch. cc @jonringer edit: |
Well, I haven't looked into correctness of this PR, except for some doubts I have but those should be fixable without a significant rebuild. EDIT: meaning... feel free to use this PR for now, if you think it's good enough (overall the situation is unpleasant for 20.09). I'm rather thinking of a better solution; perhaps a separate libudev derivation. |
Getting udev into a separate derivation is a thing long on my to-do list.
Considering the loss of the `systemd.lib` output in the latest package bump, and things like openssl depending on libudev trough libfido2, this has bumped up my priority list quite a bit - but happy to see any takers doing it before I get to it ;-)
|
@FRidh does this break the luks scenario, or just the tests? If the luks scenario still works, I think we should go forward with the branchoff, I would rather not delay ZHF work for any one particular scenario. We could do a staging-20.09 cycle to integrate the changes after the branchoff. EDIT: |
Currently, nixos-generate-config doesn't detect LUKS volumes (due to how cryptsetup behaves) - resulting in a broken hardware-configuration.nix generated during installation - so this definitely needs to be fixed before we release 20.09, else people will end up generating a broken/unbootable configuration.
|
If I understand correctly, f6286de should simply be reverted now. Before my change, the problem that it solved was that there are two incompatible copies of After my change, there are instead two copies of |
885da22
to
d21cb57
Compare
Updated with that revert, and successfully tested |
Also e02793d then. Please do the reverts as separate commits for clarity. |
d21cb57
to
a7676d7
Compare
@FRidh I had squashed the commits because it doesn’t make sense to do one without the other, and I don’t like leaving behind bad intermediate states for future |
Good remark, I had not thought of the bisecting case. Often these type of PR's are merged instead of rebased so they're grouped together, but that still does not help you (at least not yet). Personally I like |
I'd say two commits that only work together qualifies as a "logical unit". |
The cyclic dependency of systemd → cryptsetup → lvm2 → udev=systemd needs to be broken somewhere. The previous strategy of building cryptsetup with an lvm2 built without udev (NixOS#66856) caused the installer.luksroot test to fail. Instead, build lvm2 with a udev built without cryptsetup. Fixes NixOS#96479. Signed-off-by: Anders Kaseorg <andersk@mit.edu>
a7676d7
to
f4b2c9d
Compare
Yeah, that’s my feeling—a “logical unit” is a minimal change that makes sense independently given its before and after states, not a mechanical step along the historical chain of development between more distant states. Squashed again. |
|
I'm ok to merge. @vcunat I personally don't see how splitting up systemd in libudev and not libudev solves the core problem. We'd still have to have two udevs (one with and one without cryptsetup). It's just that the closure size is potentially a bit smaller, but it doesn't solve the actual cycle as far as I can see? |
@GrahamcOfBorg eval |
EDIT: nevermind, seems to be fine |
Motivation for this change
The cyclic dependency of systemd → cryptsetup → lvm2 → udev=systemd needs to be broken somewhere. The previous strategy of building cryptsetup with an lvm2 built without udev (#66856) caused the installer.luksroot test to fail. Instead, build lvm2 with a udev built without cryptsetup.
Fixes #96479.
Simplified variant of #97002.
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)