feat: take into effect system-features #587
Conversation
sabify
force-pushed
the
system-features
branch
2 times, most recently
from
b4daf60 to
4ce51b3
Compare
sabify
force-pushed
the
system-features
branch
from
4ce51b3 to
25d3307
Compare
WalkthroughUpdates nix.conf generation in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant I as Installer Script
participant R as Remote Host (SSH)
participant N as nix eval
participant FS as Local FS (~/.config/nix/nix.conf)
U->>I: Run with --flake
I->>R: runSshNoTty -> read remote system-features (and gcc arch)
I->>N: nix eval -> compute nixConfContent (features, substituters, trusted-public-keys)
N-->>I: nixConfContent (string or empty)
alt nixConfContent not empty
I->>FS: Write nixConfContent (preserve formatting)
else nixConfContent empty
I-->>U: Skip write
end
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
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. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
…into system-features
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/nixos-anywhere.sh (2)
954-957: Use consistent variable naming convention.The variable
platform_archuses underscores instead of camelCase, which is inconsistent with the rest of the script. While the logic to augmentsystem-featureswithgccarch-<arch>is correct, maintaining consistent naming improves readability.Apply this diff to improve naming consistency:
- platform_arch=$(nix --extra-experimental-features 'nix-command flakes' eval --apply 'x: toString (x.nixpkgs.hostPlatform.gcc.arch or "")' "${flake}#${flakeAttr}") - if [[ -n ${platform_arch} ]]; then - system_features="${system_features} gccarch-${platform_arch}" + platformArch=$(nix --extra-experimental-features 'nix-command flakes' eval --apply 'x: toString (x.nixpkgs.hostPlatform.gcc.arch or "")' "${flake}#${flakeAttr}") + if [[ -n ${platformArch} ]]; then + systemFeatures="${systemFeatures} gccarch-${platformArch}"
962-965: Consider logging when system-features configuration fails.The
|| trueon line 962 silently ignores all errors from the SSH command, which is consistent with the substituters block above (lines 940-944). However, if writing the system-features configuration fails, the user won't be notified, potentially leading to unexpected build behavior.Consider adding a warning message when the configuration fails:
- runSsh sh <<SSH || true + if ! runSsh sh <<SSH; then + echo "Warning: Failed to configure system-features on the installer" >&2 + fi mkdir -p ~/.config/nix -echo "system-features = ${system_features}" >> ~/.config/nix/nix.conf +echo "system-features = ${systemFeatures}" >> ~/.config/nix/nix.conf SSH - fiNote: This also assumes the variable naming changes from the previous comments are applied.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/nixos-anywhere.sh(1 hunks)
🔇 Additional comments (1)
src/nixos-anywhere.sh (1)
959-960: Deduplication logic validated. Validation shows the pipeline yields an empty string for empty input and correctly deduplicates features.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/nixos-anywhere.sh (1)
1051-1070: Variable naming inconsistency remains unaddressed.The variables
system_featuresandplatform_archuse snake_case, which is inconsistent with the rest of the script's camelCase convention (e.g.,sshConnection,flakeAttr,diskoScript).Based on learnings from the previous review, this naming inconsistency was already flagged but not yet resolved.
Apply this diff to use consistent camelCase naming:
- # Get system-features with a specific cpu architecture from the machine and add them to the installer if [[ -n ${flake} ]]; then - system_features=$(nix --extra-experimental-features 'nix-command flakes' eval --apply 'x: toString (x.nix.settings.system-features or "")' "${flake}#${flakeAttr}") - if [[ -z ${system_features} ]]; then - system_features=$(runSshNoTty -o ConnectTimeout=10 nix --extra-experimental-features 'nix-command' config show system-features) + systemFeatures=$(nix --extra-experimental-features 'nix-command flakes' eval --apply 'x: toString (x.nix.settings.system-features or "")' "${flake}#${flakeAttr}") + if [[ -z ${systemFeatures} ]]; then + systemFeatures=$(runSshNoTty -o ConnectTimeout=10 nix --extra-experimental-features 'nix-command' config show system-features) fi - platform_arch=$(nix --extra-experimental-features 'nix-command flakes' eval --apply 'x: toString (x.nixpkgs.hostPlatform.gcc.arch or "")' "${flake}#${flakeAttr}") - if [[ -n ${platform_arch} ]]; then - system_features="${system_features} gccarch-${platform_arch}" + platformArch=$(nix --extra-experimental-features 'nix-command flakes' eval --apply 'x: toString (x.nixpkgs.hostPlatform.gcc.arch or "")' "${flake}#${flakeAttr}") + if [[ -n ${platformArch} ]]; then + systemFeatures="${systemFeatures} gccarch-${platformArch}" fi # deduplicate the features - system_features=$(echo "${system_features}" | tr ' ' '\n' | sort -u | tr '\n' ' ' | sed 's/ $//') + systemFeatures=$(echo "${systemFeatures}" | tr ' ' '\n' | sort -u | tr '\n' ' ' | sed 's/ $//') runSsh sh <<SSH || true mkdir -p ~/.config/nix -echo "system-features = ${system_features}" >> ~/.config/nix/nix.conf +echo "system-features = ${systemFeatures}" >> ~/.config/nix/nix.conf SSH fi
🧹 Nitpick comments (3)
src/nixos-anywhere.sh (3)
1053-1056: Use consistent experimental features flags.Line 1053 uses
'nix-command flakes'but line 1055 uses only'nix-command'. Whileconfig showdoesn't strictly require theflakesfeature, using consistent flags throughout the script improves maintainability and prevents confusion.Apply this diff:
systemFeatures=$(nix --extra-experimental-features 'nix-command flakes' eval --apply 'x: toString (x.nix.settings.system-features or "")' "${flake}#${flakeAttr}") if [[ -z ${systemFeatures} ]]; then - systemFeatures=$(runSshNoTty -o ConnectTimeout=10 nix --extra-experimental-features 'nix-command' config show system-features) + systemFeatures=$(runSshNoTty -o ConnectTimeout=10 nix --extra-experimental-features 'nix-command flakes' config show system-features) fi
1063-1064: Edge case: deduplication fails with empty input.If
systemFeaturesis empty, the deduplication pipeline will still work correctly but thesed 's/ $//'at the end is unnecessary. However, ifsystemFeaturescontains only whitespace, this could produce unexpected results.Consider guarding the deduplication and write operation:
+ if [[ -n ${systemFeatures} ]]; then # deduplicate the features systemFeatures=$(echo "${systemFeatures}" | tr ' ' '\n' | sort -u | tr '\n' ' ' | sed 's/ $//') runSsh sh <<SSH || true mkdir -p ~/.config/nix echo "system-features = ${systemFeatures}" >> ~/.config/nix/nix.conf SSH + fi
1040-1049: Apply similar error handling improvements to substituters block.Similar to the system-features block, the substituters configuration at lines 1044-1048 uses
|| trueto silence all errors. This could hide legitimate SSH or permission failures.Apply consistent error handling:
- runSsh sh <<SSH || true + if ! runSsh sh <<SSH; then + echo "Warning: Failed to configure substituters in installer environment" >&2 + fi mkdir -p ~/.config/nix echo "extra-substituters = ${substituters}" >> ~/.config/nix/nix.conf echo "extra-trusted-public-keys = ${trustedPublicKeys}" >> ~/.config/nix/nix.conf SSH
|
I wanted to avoid evaluating nixos multiple times, so i came up with: 8a49b35 But haven't test it properly yet. |
The applied logic is incomplete. Please look at the following options:
Your proposal addresses the first and second options in which you intentionally define |
|
Okay, could switch the code over to use a single nixos eval to get all this information out of the nixos configuration, similar to my code. It might be also possible to load the nix code from an external file to make it a bit prettier (we have to get useSubstitute injected though - maybe with a |
|
Otherwise nixos-facter might even provide more information about the target cpu. |
Co-authored-by: Jörg Thalheim <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/nixos-anywhere.sh (1)
1040-1040: Use camelCase and avoid suppressing real errors.The variable
system_featuresuses snake_case, which is inconsistent with the rest of the script (e.g.,sshConnection,flakeAttr,nixConfContenton line 1043). Additionally,|| truesuppresses all errors, including legitimate SSH connection failures.Apply this diff:
- system_features=$(runSshNoTty -o ConnectTimeout=10 nix --extra-experimental-features 'nix-command' config show system-features 2>/dev/null || true) + systemFeatures=$(runSshNoTty -o ConnectTimeout=10 nix --extra-experimental-features 'nix-command' config show system-features 2>/dev/null || echo "")Then update line 1054 to reference
systemFeatures:- remoteFeaturesStr = \"${system_features}\"; + remoteFeaturesStr = \"${systemFeatures}\";
| nixConfContent=$(nix --extra-experimental-features 'nix-command flakes' eval --raw --apply " | ||
| config: | ||
| let | ||
| settings = config.nix.settings or {}; | ||
| gccArch = config.nixpkgs.hostPlatform.gcc.arch or null; | ||
| # Check if system-features are defined in configuration | ||
| configFeatures = settings.system-features or null; | ||
| hasConfigFeatures = configFeatures != null && configFeatures != []; | ||
| remoteFeatures = let | ||
| remoteFeaturesStr = \"${system_features}\"; | ||
| # Parse remote features string (space-separated) into list | ||
| remoteFeaturesList = if remoteFeaturesStr != \"\" then | ||
| builtins.filter (x: x != \"\") (builtins.split \" +\" remoteFeaturesStr) | ||
| else []; | ||
| in remoteFeaturesList; | ||
| # Combine base features (config or remote) with platform-specific features | ||
| baseFeatures = if hasConfigFeatures then configFeatures else remoteFeatures; | ||
| # At least one of nix.settings.system-features or nixpkgs.hostPlatform.gcc.arch has been explicitly defined | ||
| allFeatures = if (gccArch != null) || hasConfigFeatures then baseFeatures ++ (if gccArch != null then [\"gccarch-\${gccArch}\"] else []) else []; | ||
| # Deduplicate using listToAttrs trick | ||
| uniqueFeatures = builtins.attrNames (builtins.listToAttrs (map (f: { name = f; value = true; }) allFeatures)); | ||
| substituters = builtins.toString (settings.substituters or []); | ||
| trustedPublicKeys = builtins.toString (settings.trusted-public-keys or []); | ||
| systemFeatures = builtins.toString uniqueFeatures; | ||
| # Helper function for optional config lines | ||
| optionalLine = cond: line: if cond then line + \"\n\" else \"\"; | ||
| useSubstituters = \"${machineSubstituters}\" == \"y\"; | ||
| in | ||
| optionalLine (useSubstituters && substituters != \"\") \"extra-substituters = \${substituters}\" | ||
| + optionalLine (useSubstituters && trustedPublicKeys != \"\") \"extra-trusted-public-keys = \${trustedPublicKeys}\" | ||
| + optionalLine (systemFeatures != \"\") \"system-features = \${systemFeatures}\" | ||
| " "${flake}#${flakeAttr}") |
There was a problem hiding this comment.
Add error handling for nix eval.
The nix eval command (line 1043) has no error handling. If the flake evaluation fails (e.g., syntax error, missing attribute, network issue), the script will continue with an empty or partial nixConfContent, potentially causing silent failures.
Consider adding error handling:
# First, try to evaluate all nix settings from the flake in one go
+ if ! nixConfContent=$(nix --extra-experimental-features 'nix-command flakes' eval --raw --apply "
- nixConfContent=$(nix --extra-experimental-features 'nix-command flakes' eval --raw --apply "
config:
...
- " "${flake}#${flakeAttr}")
+ " "${flake}#${flakeAttr}"); then
+ echo "Warning: Failed to evaluate nix settings from flake" >&2
+ nixConfContent=""
+ fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| nixConfContent=$(nix --extra-experimental-features 'nix-command flakes' eval --raw --apply " | |
| config: | |
| let | |
| settings = config.nix.settings or {}; | |
| gccArch = config.nixpkgs.hostPlatform.gcc.arch or null; | |
| # Check if system-features are defined in configuration | |
| configFeatures = settings.system-features or null; | |
| hasConfigFeatures = configFeatures != null && configFeatures != []; | |
| remoteFeatures = let | |
| remoteFeaturesStr = \"${system_features}\"; | |
| # Parse remote features string (space-separated) into list | |
| remoteFeaturesList = if remoteFeaturesStr != \"\" then | |
| builtins.filter (x: x != \"\") (builtins.split \" +\" remoteFeaturesStr) | |
| else []; | |
| in remoteFeaturesList; | |
| # Combine base features (config or remote) with platform-specific features | |
| baseFeatures = if hasConfigFeatures then configFeatures else remoteFeatures; | |
| # At least one of nix.settings.system-features or nixpkgs.hostPlatform.gcc.arch has been explicitly defined | |
| allFeatures = if (gccArch != null) || hasConfigFeatures then baseFeatures ++ (if gccArch != null then [\"gccarch-\${gccArch}\"] else []) else []; | |
| # Deduplicate using listToAttrs trick | |
| uniqueFeatures = builtins.attrNames (builtins.listToAttrs (map (f: { name = f; value = true; }) allFeatures)); | |
| substituters = builtins.toString (settings.substituters or []); | |
| trustedPublicKeys = builtins.toString (settings.trusted-public-keys or []); | |
| systemFeatures = builtins.toString uniqueFeatures; | |
| # Helper function for optional config lines | |
| optionalLine = cond: line: if cond then line + \"\n\" else \"\"; | |
| useSubstituters = \"${machineSubstituters}\" == \"y\"; | |
| in | |
| optionalLine (useSubstituters && substituters != \"\") \"extra-substituters = \${substituters}\" | |
| + optionalLine (useSubstituters && trustedPublicKeys != \"\") \"extra-trusted-public-keys = \${trustedPublicKeys}\" | |
| + optionalLine (systemFeatures != \"\") \"system-features = \${systemFeatures}\" | |
| " "${flake}#${flakeAttr}") | |
| # First, try to evaluate all nix settings from the flake in one go | |
| if ! nixConfContent=$(nix --extra-experimental-features 'nix-command flakes' eval --raw --apply " | |
| config: | |
| let | |
| settings = config.nix.settings or {}; | |
| gccArch = config.nixpkgs.hostPlatform.gcc.arch or null; | |
| # Check if system-features are defined in configuration | |
| configFeatures = settings.system-features or null; | |
| hasConfigFeatures = configFeatures != null && configFeatures != []; | |
| remoteFeatures = let | |
| remoteFeaturesStr = \"${system_features}\"; | |
| # Parse remote features string (space-separated) into list | |
| remoteFeaturesList = if remoteFeaturesStr != \"\" then | |
| builtins.filter (x: x != \"\") (builtins.split \" +\" remoteFeaturesStr) | |
| else []; | |
| in remoteFeaturesList; | |
| # Combine base features (config or remote) with platform-specific features | |
| baseFeatures = if hasConfigFeatures then configFeatures else remoteFeatures; | |
| # At least one of nix.settings.system-features or nixpkgs.hostPlatform.gcc.arch has been explicitly defined | |
| allFeatures = if (gccArch != null) || hasConfigFeatures then baseFeatures ++ (if gccArch != null then [\"gccarch-\${gccArch}\"] else []) else []; | |
| # Deduplicate using listToAttrs trick | |
| uniqueFeatures = builtins.attrNames (builtins.listToAttrs (map (f: { name = f; value = true; }) allFeatures)); | |
| substituters = builtins.toString (settings.substituters or []); | |
| trustedPublicKeys = builtins.toString (settings.trusted-public-keys or []); | |
| systemFeatures = builtins.toString uniqueFeatures; | |
| # Helper function for optional config lines | |
| optionalLine = cond: line: if cond then line + \"\n\" else \"\"; | |
| useSubstituters = \"${machineSubstituters}\" == \"y\"; | |
| in | |
| optionalLine (useSubstituters && substituters != \"\") \"extra-substituters = \${substituters}\" | |
| optionalLine (useSubstituters && trustedPublicKeys != \"\") \"extra-trusted-public-keys = \${trustedPublicKeys}\" | |
| optionalLine (systemFeatures != \"\") \"system-features = \${systemFeatures}\" | |
| " "${flake}#${flakeAttr}"); then | |
| echo "Warning: Failed to evaluate nix settings from flake" >&2 | |
| nixConfContent="" | |
| fi |
Quote bash variable interpolation and fix builtins.split usage.
Two issues:
-
Shell injection risk: Line 1054 interpolates the bash variable
${system_features}directly into a Nix string without escaping. Ifsystem_featurescontains quotes, backslashes, or newlines, it could break the Nix expression or cause injection. -
Incorrect parsing: Line 1057 uses
builtins.split " +"which returns a list containing both matched strings and null values for the separators. The current code doesn't filter out the nulls, potentially causing downstream issues.
Apply this diff to fix both issues:
# First, try to evaluate all nix settings from the flake in one go
- nixConfContent=$(nix --extra-experimental-features 'nix-command flakes' eval --raw --apply "
+ # Escape the bash variable for safe interpolation into Nix
+ escapedSystemFeatures=$(printf '%s' "$systemFeatures" | sed 's/\\/\\\\/g; s/"/\\"/g')
+ nixConfContent=$(nix --extra-experimental-features 'nix-command flakes' eval --raw --apply "
config:
let
settings = config.nix.settings or {};
gccArch = config.nixpkgs.hostPlatform.gcc.arch or null;
# Check if system-features are defined in configuration
configFeatures = settings.system-features or null;
hasConfigFeatures = configFeatures != null && configFeatures != [];
remoteFeatures = let
- remoteFeaturesStr = \"${system_features}\";
+ remoteFeaturesStr = \"${escapedSystemFeatures}\";
# Parse remote features string (space-separated) into list
remoteFeaturesList = if remoteFeaturesStr != \"\" then
- builtins.filter (x: x != \"\") (builtins.split \" +\" remoteFeaturesStr)
+ builtins.filter (x: builtins.isString x && x != \"\") (builtins.split \" +\" remoteFeaturesStr)
else [];
in remoteFeaturesList;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/nixos-anywhere.sh around lines 1043 to 1080, the Nix eval interpolates
the shell variable ${system_features} directly into the --apply string (risking
shell/Nix injection) and uses builtins.split with a regex that produces
empty/null entries; change the invocation to pass the value safely with --argstr
system_features "${system_features}" (remove the inline \"${system_features}\"
interpolation) and inside the Nix expression set remoteFeaturesStr =
system_features; then replace the current split logic with a space-split plus
explicit empty-string filtering, e.g. remoteFeaturesList = if remoteFeaturesStr
!= "" then builtins.filter (x: x != "") (builtins.split " " remoteFeaturesStr)
else []; this removes injection risk and drops empty entries from the feature
list.
|
@Mic92 I made changes according to your suggested script. Please check it out. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/nixos-anywhere.sh (2)
1083-1088: Critical: Command injection vulnerability in heredoc.Line 1086 uses unquoted
${nixConfContent}within the heredoc. IfnixConfContentcontains backticks,$(), or other special characters, this could cause command injection or shell parsing errors on the remote host.Apply this diff to safely write the content:
# Write to nix.conf if we have any content if [[ -n ${nixConfContent} ]]; then - runSsh sh <<SSH + runSsh sh <<'SSH' mkdir -p ~/.config/nix -echo "${nixConfContent}" >> ~/.config/nix/nix.conf +cat >> ~/.config/nix/nix.conf SSH + printf '%s\n' "$nixConfContent" | runSsh 'cat >> ~/.config/nix/nix.conf' fiNote: Using
<<'SSH'(quoted delimiter) prevents variable expansion. The content is then piped separately usingprintfwhich safely handles special characters.
1043-1080: Critical: Shell injection vulnerability and missing error handling.Three critical issues:
Shell injection risk: Lines 1054 and 1075 interpolate bash variables
${system_features}and${machineSubstituters}directly into the Nix expression string. If these variables contain special characters like quotes, backticks,$(), or newlines, they could break the Nix expression or enable command injection.Incorrect list parsing: Line 1057 uses
builtins.split " +"which returns a list containing both matched strings andnullvalues for separators. While line 1057 filters empty strings, it doesn't filter nulls, which could cause downstream issues.No error handling: The
nix evalcommand has no error handling. If evaluation fails (syntax error, missing attribute, network issue), the script continues with emptynixConfContent, causing silent failures.Apply this diff to fix all three issues:
if [[ -n ${flake} ]]; then systemFeatures=$(runSshNoTty -o ConnectTimeout=10 nix --extra-experimental-features 'nix-command' config show system-features 2>/dev/null || true) # First, try to evaluate all nix settings from the flake in one go - nixConfContent=$(nix --extra-experimental-features 'nix-command flakes' eval --raw --apply " + if ! nixConfContent=$(nix --extra-experimental-features 'nix-command flakes' eval --raw \ + --argstr systemFeatures "${systemFeatures}" \ + --argstr useSubstituters "${machineSubstituters}" \ + --apply " + systemFeatures: useSubstituters: config: let settings = config.nix.settings or {}; gccArch = config.nixpkgs.hostPlatform.gcc.arch or null; # Check if system-features are defined in configuration configFeatures = settings.system-features or null; hasConfigFeatures = configFeatures != null && configFeatures != []; remoteFeatures = let - remoteFeaturesStr = \"${system_features}\"; # Parse remote features string (space-separated) into list - remoteFeaturesList = if remoteFeaturesStr != \"\" then - builtins.filter (x: x != \"\") (builtins.split \" +\" remoteFeaturesStr) + remoteFeaturesList = if systemFeatures != \"\" then + builtins.filter (x: builtins.isString x && x != \"\") (builtins.split \" \" systemFeatures) else []; in remoteFeaturesList; # Combine base features (config or remote) with platform-specific features baseFeatures = if hasConfigFeatures then configFeatures else remoteFeatures; # At least one of nix.settings.system-features or nixpkgs.hostPlatform.gcc.arch has been explicitly defined allFeatures = if (gccArch != null) || hasConfigFeatures then baseFeatures ++ (if gccArch != null then [\"gccarch-\${gccArch}\"] else []) else []; # Deduplicate using listToAttrs trick uniqueFeatures = builtins.attrNames (builtins.listToAttrs (map (f: { name = f; value = true; }) allFeatures)); substituters = builtins.toString (settings.substituters or []); trustedPublicKeys = builtins.toString (settings.trusted-public-keys or []); - systemFeatures = builtins.toString uniqueFeatures; + systemFeaturesStr = builtins.toString uniqueFeatures; # Helper function for optional config lines optionalLine = cond: line: if cond then line + \"\n\" else \"\"; - useSubstituters = \"${machineSubstituters}\" == \"y\"; + useSubs = useSubstituters == \"y\"; in - optionalLine (useSubstituters && substituters != \"\") \"extra-substituters = \${substituters}\" - + optionalLine (useSubstituters && trustedPublicKeys != \"\") \"extra-trusted-public-keys = \${trustedPublicKeys}\" - + optionalLine (systemFeatures != \"\") \"system-features = \${systemFeatures}\" - " "${flake}#${flakeAttr}") + optionalLine (useSubs && substituters != \"\") \"extra-substituters = \${substituters}\" + + optionalLine (useSubs && trustedPublicKeys != \"\") \"extra-trusted-public-keys = \${trustedPublicKeys}\" + + optionalLine (systemFeaturesStr != \"\") \"system-features = \${systemFeaturesStr}\" + " "${flake}#${flakeAttr}"); then + echo "Warning: Failed to evaluate nix settings from flake" >&2 + nixConfContent="" + fi
🧹 Nitpick comments (1)
src/nixos-anywhere.sh (1)
1039-1040: Use consistent variable naming convention.The variable
system_featuresuses snake_case, which is inconsistent with the rest of the script's camelCase convention (e.g.,flakeAttr,sshConnection,nixConfContent).Apply this diff to use camelCase consistently:
if [[ -n ${flake} ]]; then - system_features=$(runSshNoTty -o ConnectTimeout=10 nix --extra-experimental-features 'nix-command' config show system-features 2>/dev/null || true) + systemFeatures=$(runSshNoTty -o ConnectTimeout=10 nix --extra-experimental-features 'nix-command' config show system-features 2>/dev/null || true) # First, try to evaluate all nix settings from the flake in one go nixConfContent=$(nix --extra-experimental-features 'nix-command flakes' eval --raw --apply " config: let settings = config.nix.settings or {}; gccArch = config.nixpkgs.hostPlatform.gcc.arch or null; # Check if system-features are defined in configuration configFeatures = settings.system-features or null; hasConfigFeatures = configFeatures != null && configFeatures != []; remoteFeatures = let - remoteFeaturesStr = \"${system_features}\"; + remoteFeaturesStr = \"${systemFeatures}\";
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/nixos-anywhere.sh (1)
1044-1081: Use --argstr to avoid shell injection risk and add error handling.Two concerns:
Shell injection risk: Lines 1055 and 1076 interpolate bash variables (
${system_features},${machineSubstituters}) directly into the Nix expression string. While the escaping on line 1042 mitigates some risk, using--argstror--argis the safer, idiomatic approach for passing external values into Nix expressions.Missing error handling: The
nix evalcommand (line 1044) has no error handling. If evaluation fails (syntax error, missing attribute, network issue), the script continues with an emptynixConfContent, potentially causing silent failures.Apply this diff to use
--argstrfor safe parameter passing:# Escape the bash variable for safe interpolation into Nix - escapedSystemFeatures="$(printf '%s' "$systemFeatures" | sed 's/\\/\\\\/g; s/"/\\"/g')" # First, try to evaluate all nix settings from the flake in one go - nixConfContent=$(nix --extra-experimental-features 'nix-command flakes' eval --raw --apply " + if ! nixConfContent=$(nix --extra-experimental-features 'nix-command flakes' eval --raw \ + --argstr systemFeatures "$systemFeatures" \ + --argstr machineSubstituters "$machineSubstituters" \ + --apply " + systemFeatures: machineSubstituters: config: let settings = config.nix.settings or {}; @@ -1052,7 +1054,7 @@ hasConfigFeatures = configFeatures != null && configFeatures != []; remoteFeatures = let - remoteFeaturesStr = \"${escapedSystemFeatures}\"; + remoteFeaturesStr = systemFeatures; # Parse remote features string (space-separated) into list remoteFeaturesList = if remoteFeaturesStr != \"\" then builtins.filter (x: builtins.isString x && x != \"\") (builtins.split \" +\" remoteFeaturesStr) @@ -1073,12 +1075,15 @@ # Helper function for optional config lines optionalLine = cond: line: if cond then line + \"\n\" else \"\"; - useSubstituters = \"${machineSubstituters}\" == \"y\"; + useSubstituters = machineSubstituters == \"y\"; in optionalLine (useSubstituters && substituters != \"\") \"extra-substituters = \${substituters}\" + optionalLine (useSubstituters && trustedPublicKeys != \"\") \"extra-trusted-public-keys = \${trustedPublicKeys}\" + optionalLine (systemFeatures != \"\") \"system-features = \${systemFeatures}\" - " "${flake}#${flakeAttr}") + " "${flake}#${flakeAttr}"); then + echo "Warning: Failed to evaluate nix settings from flake" >&2 + nixConfContent="" + fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/nixos-anywhere.sh(1 hunks)
🔇 Additional comments (1)
src/nixos-anywhere.sh (1)
1062-1065: Case 4 (neither gccArch nor system-features defined) may not be handled as intended.According to the PR objectives, when neither
nix.settings.system-featuresnornixpkgs.hostPlatform.gcc.archis defined (case 4), the target machine's defaultsystem-featuresshould be used. However, the current logic on line 1065:allFeatures = if (gccArch != null) || hasConfigFeatures then baseFeatures ++ ... else [];Sets
allFeaturesto an empty list when both are undefined, resulting in nosystem-featuresline being written tonix.conf. This means the installer environment will use its own default features rather than the target machine's features fetched on line 1040.Verify if this behavior is intentional:
If case 4 should preserve the remote features, apply this diff:
# Combine base features (config or remote) with platform-specific features baseFeatures = if hasConfigFeatures then configFeatures else remoteFeatures; - # At least one of nix.settings.system-features or nixpkgs.hostPlatform.gcc.arch has been explicitly defined - allFeatures = if (gccArch != null) || hasConfigFeatures then baseFeatures ++ (if gccArch != null then [\"gccarch-\${gccArch}\"] else []) else []; + # Use base features (from config or remote) and optionally add gccarch + allFeatures = baseFeatures ++ (if gccArch != null then [\"gccarch-\${gccArch}\"] else []);This ensures that:
- Case 1 (both defined): config features + gccarch
- Case 2 (only config): config features
- Case 3 (only gccArch): remote features + gccarch
- Case 4 (neither): remote features
Otherwise, if the current behavior is correct (don't write system-features when neither is defined), please clarify this in a comment.
| if [[ -n ${flake} ]]; then | ||
| system_features=$(runSshNoTty -o ConnectTimeout=10 nix --extra-experimental-features 'nix-command' config show system-features) | ||
| # Escape the bash variable for safe interpolation into Nix | ||
| system_features="$(printf '%s' "$system_features" | sed 's/\\/\\\\/g; s/"/\\"/g')" | ||
| # First, try to evaluate all nix settings from the flake in one go |
There was a problem hiding this comment.
Use consistent variable naming and add error handling for remote SSH command.
Two issues:
-
Naming inconsistency:
system_featuresuses snake_case, while the rest of the script uses camelCase (e.g.,sshConnection,flakeAttr,nixConfContent). -
Missing error handling: Line 1040 will cause the script to abort under
set -euo pipefailif the SSH command fails (e.g., network issue, nix not found on remote).
Apply this diff:
if [[ -n ${flake} ]]; then
- system_features=$(runSshNoTty -o ConnectTimeout=10 nix --extra-experimental-features 'nix-command' config show system-features)
+ systemFeatures=$(runSshNoTty -o ConnectTimeout=10 nix --extra-experimental-features 'nix-command' config show system-features 2>/dev/null || echo "")
# Escape the bash variable for safe interpolation into Nix
- system_features="$(printf '%s' "$system_features" | sed 's/\\/\\\\/g; s/"/\\"/g')"
+ escapedSystemFeatures="$(printf '%s' "$systemFeatures" | sed 's/\\/\\\\/g; s/"/\\"/g')"Then update line 1055 to use the renamed variable:
remoteFeatures = let
- remoteFeaturesStr = \"${system_features}\";
+ remoteFeaturesStr = \"${escapedSystemFeatures}\";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if [[ -n ${flake} ]]; then | |
| system_features=$(runSshNoTty -o ConnectTimeout=10 nix --extra-experimental-features 'nix-command' config show system-features) | |
| # Escape the bash variable for safe interpolation into Nix | |
| system_features="$(printf '%s' "$system_features" | sed 's/\\/\\\\/g; s/"/\\"/g')" | |
| # First, try to evaluate all nix settings from the flake in one go | |
| if [[ -n ${flake} ]]; then | |
| # Fetch remote system-features, but never fail under set -euo pipefail | |
| systemFeatures=$(runSshNoTty -o ConnectTimeout=10 \ | |
| nix --extra-experimental-features 'nix-command' config show system-features \ | |
| 2>/dev/null || echo "") | |
| # Escape the bash variable for safe interpolation into Nix | |
| escapedSystemFeatures="$(printf '%s' "$systemFeatures" \ | |
| | sed 's/\\/\\\\/g; s/"/\\"/g')" | |
| # First, try to evaluate all nix settings from the flake in one go | |
| nixConfContent=$( | |
| nix --extra-experimental-features 'nix-command flakes' eval --raw --apply " | |
| config: | |
| let | |
| settings = config.nix.settings or {}; | |
| gccArch = config.nixpkgs.hostPlatform.gcc.arch or null; | |
| # Check if system-features are defined in configuration | |
| configFeatures = settings.system-features or null; | |
| hasConfig = configFeatures != null && configFeatures != []; | |
| # Fallback to the escaped remote features if none in the flake | |
| remoteFeatures = let | |
| remoteFeaturesStr = \"${escapedSystemFeatures}\"; | |
| remoteFeaturesList = if remoteFeaturesStr != \"\" then | |
| builtins.filter (x: builtins.isString x && x != \"\") | |
| (builtins.split \" +\" remoteFeaturesStr) | |
| else | |
| []; | |
| in remoteFeaturesList; | |
| in | |
| # … rest of the nixConfContent construction … | |
| " "${flake}#${flakeAttr}" | |
| ) |
🤖 Prompt for AI Agents
In src/nixos-anywhere.sh around lines 1039-1043, rename the snake_case variable
system_features to camelCase systemFeatures for consistency with
sshConnection/flakeAttr/nixConfContent, and preserve the escaping step for
quotes/backslashes; additionally, run the remote SSH command with explicit error
handling: capture both its stdout and exit code, and if the SSH command fails
log a clear error including the exit code/output and exit non-zero to avoid an
unhelpful abrupt failure under set -euo pipefail. After renaming, update the
later usage on line 1055 to reference systemFeatures.
| # Write to nix.conf if we have any content | ||
| if [[ -n ${nixConfContent} ]]; then | ||
| runSsh sh <<SSH | ||
| mkdir -p ~/.config/nix | ||
| echo "extra-substituters = ${substituters}" >> ~/.config/nix/nix.conf | ||
| echo "extra-trusted-public-keys = ${trustedPublicKeys}" >> ~/.config/nix/nix.conf | ||
| printf '%s\n' "\$(cat <<'CONTENT' | ||
| ${nixConfContent} | ||
| CONTENT | ||
| )" >> ~/.config/nix/nix.conf | ||
| SSH | ||
| fi |
There was a problem hiding this comment.
Add error handling for nix.conf write operation.
The SSH command that writes to ~/.config/nix/nix.conf has no error handling. Failures (SSH connection issues, permission problems, disk full) will be silently ignored, leaving the system in an inconsistent state.
Apply this diff:
# Write to nix.conf if we have any content
if [[ -n ${nixConfContent} ]]; then
- runSsh sh <<SSH
+ if ! runSsh sh <<SSH; then
+ echo "Warning: Failed to write nix.conf in installer environment" >&2
+ fi
mkdir -p ~/.config/nix
printf '%s\n' "\$(cat <<'CONTENT'
${nixConfContent}
CONTENT
)" >> ~/.config/nix/nix.conf
SSH
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Write to nix.conf if we have any content | |
| if [[ -n ${nixConfContent} ]]; then | |
| runSsh sh <<SSH | |
| mkdir -p ~/.config/nix | |
| echo "extra-substituters = ${substituters}" >> ~/.config/nix/nix.conf | |
| echo "extra-trusted-public-keys = ${trustedPublicKeys}" >> ~/.config/nix/nix.conf | |
| printf '%s\n' "\$(cat <<'CONTENT' | |
| ${nixConfContent} | |
| CONTENT | |
| )" >> ~/.config/nix/nix.conf | |
| SSH | |
| fi | |
| # Write to nix.conf if we have any content | |
| if [[ -n ${nixConfContent} ]]; then | |
| if ! runSsh sh <<SSH; then | |
| echo "Warning: Failed to write nix.conf in installer environment" >&2 | |
| fi | |
| mkdir -p ~/.config/nix | |
| printf '%s\n' "\$(cat <<'CONTENT' | |
| ${nixConfContent} | |
| CONTENT | |
| )" >> ~/.config/nix/nix.conf | |
| SSH | |
| fi |
| runSsh sh <<SSH || true | ||
| if [[ -n ${flake} ]]; then | ||
| system_features=$(runSshNoTty -o ConnectTimeout=10 nix --extra-experimental-features 'nix-command' config show system-features) |
There was a problem hiding this comment.
why is the list of current system-features important? Should this not be overruled by what the user has specified in the machine?
There was a problem hiding this comment.
Sometimes you may not want to explicitly provide system-features and only want to configure gcc.arch, in that case the default target platform feature set needs to append gccarch-${cpu-arch} to it. If you don't do this and only set the gccarch feature, it will override (not append) the whole default features and the build will fail.
2 participants
This is mostly useful when we are building for a specific cpu architecture by specifying
nixpkgs.hostPlatform.gcc.archthat needs its companiongccarch-<arch>assystem-features.Summary by CodeRabbit