fix(install): resolve longstanding bugs with installation process#128
fix(install): resolve longstanding bugs with installation process#128water-sucks merged 3 commits intonix-community:mainfrom
Conversation
WalkthroughBind-mount Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as install CLI
participant FS as filesystem
participant Builder as system builder
participant Channel as channel copy
CLI->>FS: validate flags (root != "", systemClosure?, flakeRef?)
alt systemClosure provided
CLI->>FS: check absolute path
CLI->>FS: os.Stat(systemClosure)
FS-->>CLI: exists / error
CLI-->>CLI: set resultLocation = systemClosure
else no systemClosure
CLI->>Builder: run system build
Builder-->>CLI: return build result path
CLI-->>CLI: set resultLocation = build result
end
CLI->>FS: ensure parent dir for channel mountpoint (MkdirAll)
CLI->>Channel: copy channel -> mountpoint
Channel-->>CLI: success / error
CLI->>FS: create initial generation using resultLocation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cmd/install/install.go (2)
199-202: Clarify the error message to reflect parent directory creation.The implementation correctly creates only the parent directory (as per PR objective #2), but the error message on line 201 references
mountpointChannelDirwhile actually creating its parent directory.Apply this diff to improve clarity:
err := os.MkdirAll(filepath.Dir(mountpointChannelDir), 0o755) if err != nil { - return fmt.Errorf("failed to create %s: %s", mountpointChannelDir, err) + return fmt.Errorf("failed to create parent directory for %s: %s", mountpointChannelDir, err) }
489-489: Typo fix is good, but the placeholder needs attention.The correction from "them new system" to "the new system" is appropriate. However, there's an existing issue in this line: the
{s}placeholder appears to be intended for the mountpoint value but is not actually replaced.Consider replacing the placeholder with the actual mountpoint value:
- manualHint := "you can set the root password manually by executing `nixos enter --root {s}` and then running `passwd` in the shell of the new system" + manualHint := fmt.Sprintf("you can set the root password manually by executing `nixos enter --root %s` and then running `passwd` in the shell of the new system", mountpoint)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/enter/run.go(1 hunks)cmd/install/install.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/install/install.go (2)
internal/configuration/flake.go (1)
FlakeRef(17-23)internal/configuration/configuration.go (2)
SystemBuildOptions(13-26)SystemBuild(75-77)
🔇 Additional comments (5)
cmd/enter/run.go (1)
63-68: LGTM! /sys bind-mount correctly implements UEFI support.The addition of the /sys bind-mount follows the same pattern as existing /dev and /proc mounts and resolves the UEFI bootloader installation issue described in the PR objectives. Mounting /sys before /proc is appropriate and provides access to EFI variables needed by GRUB and systemd-boot.
cmd/install/install.go (4)
52-54: LGTM! Improved validation for --root flag.The change from length check to direct emptiness check is more idiomatic Go, and the absolute path validation is appropriate for the --root filesystem path parameter.
56-68: LGTM! Comprehensive validation for --system flag.The validation correctly implements all necessary checks:
- Absolute path requirement
- Mutual exclusivity with flake references
- File existence verification
This addresses the lack of validation mentioned in the PR objectives and ensures the --system flag is properly constrained.
230-230: LGTM! Style consistency improvement.The inline error assignment matches the pattern used elsewhere in the function.
434-453: LGTM! Conditional build flow correctly implements --system flag functionality.The restructuring properly implements the --system flag usage mentioned in PR objective #1:
- When --system is not provided, the system is built as before
- When --system is provided, the build is skipped and the provided closure is used
- The "Building system..." log is appropriately conditional
The logic is clear and the resultLocation variable is used consistently downstream.
a87fc2d to
767cccc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/install/install.go (1)
56-68: Consider a more descriptive error message for the path existence check.The validation logic correctly ensures the system closure path is absolute, mutually exclusive with the flake reference, and exists. However, the error returned from
os.Staton line 65-67 could be more user-friendly.Consider wrapping the error:
if _, err := os.Stat(opts.SystemClosure); err != nil { - return err + return fmt.Errorf("--system path does not exist or is not accessible: %w", err) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/enter/run.go(1 hunks)cmd/install/install.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/enter/run.go
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/install/install.go (2)
internal/configuration/flake.go (1)
FlakeRef(17-23)internal/configuration/configuration.go (2)
SystemBuildOptions(13-26)SystemBuild(75-77)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build/Test
🔇 Additional comments (4)
cmd/install/install.go (4)
52-54: LGTM!The validation correctly ensures that the root path is absolute when provided.
230-230: LGTM!The error handling standardization improves consistency with the surrounding code.
434-453: Excellent fix for the--systemflag functionality.The restructured control flow correctly implements the conditional logic:
- When
--systemis not provided, the system is built and the result is used.- When
--systemis provided, the specified system closure is used directly, skipping the build.This properly addresses the bug where the
--systemflag was ignored.
489-489: LGTM!Good catch on the grammatical error. The corrected message is now clearer for users.
767cccc to
ed91411
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmd/install/install.go (1)
56-68: Enhance error message with context.The validation correctly checks that the system closure path is absolute, mutually exclusive with FlakeRef, and exists. However, when
os.Statfails on line 65, the raw error is returned without indicating which path was checked.Apply this diff to improve the error message:
if _, err := os.Stat(opts.SystemClosure); err != nil { - return err + return fmt.Errorf("failed to stat system closure at %s: %w", opts.SystemClosure, err) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/enter/run.go(1 hunks)cmd/install/install.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/install/install.go (2)
internal/configuration/flake.go (1)
FlakeRef(17-23)internal/configuration/configuration.go (2)
SystemBuildOptions(13-26)SystemBuild(75-77)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build/Test
🔇 Additional comments (4)
cmd/enter/run.go (1)
63-68: LGTM! /sys bind-mount fixes UEFI bootloader installation.The implementation correctly adds
/sysbind-mounting with appropriate logging and error handling, consistent with the existing/devand/procmounts. This resolves the issue where EFI variables were inaccessible during GRUB and systemd-boot installation on UEFI systems.cmd/install/install.go (3)
198-203: Correct fix for channel directory creation.The change properly creates only the parent directory (
per-user/root) instead of the full channel path, allowing Nix to manage the channel symlinks. The error message now correctly references the directory being created.
435-454: Well-structured conditional build flow.The refactoring correctly implements the
--systemflag functionality: when a system closure is provided, it's used directly; otherwise, the system is built. The "Building system..." log message appropriately appears only when a build occurs, andresultLocationis properly set in both branches.
490-490: Improved hint uses actual mountpoint.Using the
mountpointvariable instead of a hardcoded path makes the hint accurate for all installation scenarios.
There were three bugs with the
nixos installcommand:--systemcommand-line flag was not being used at all (probably a bug from the Zig -> Go port), and was missing some extra validation.$mountpoint/nix/var/nix/profiles/per-user/root/channelsdirectory was getting created directly, when it was supposed to be a symlink created by Nix. This caused Nix to fail when copying the initial channel. This creates only theper-user/rootdirectory for Nix to place the channel links in.nixos entercommand did not bind-mount/sys, which lead to failures with installing GRUB andsystemd-booton UEFI systems since it didn't know where the EFI variables were.Summary by CodeRabbit
Bug Fixes
Improvements