Improve the overall nix-darwin experience #136
Conversation
WalkthroughConsolidates flake inputs under a single Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Host as "macOS host"
participant Module as "determinateNix module"
participant Files as "/etc/nix & registry"
participant Launchd as "launchd"
participant VM as "Linux builder VM"
participant Nix as "Nix daemon"
User->>Host: Apply configuration
Host->>Module: Evaluate `determinateNix.enable`
alt determinateNix enabled
Module->>Files: Render `nix/custom.conf`, registry, machines, determinate/config.json
Module->>Launchd: Install/enable linux-builder service & SSH fragments
Launchd->>VM: Start VM builder (if `nixosVmBasedLinuxBuilder.enable`)
Module->>Nix: Wire env vars & distributed-build machines, override Determinate Nix settings
else disabled
Module->>Nix: Keep nix-darwin defaults
end
sequenceDiagram
autonumber
participant Flake as "flake.nix"
participant DevShell as "devShells"
participant Formatter as "self.formatter.${system}"
Flake->>Formatter: Define per-system `nixfmt` via forEachSupportedSystem
DevShell->>Formatter: Reference `self.formatter.${system}` in devShell packages
Formatter-->>DevShell: Provide system-specific `nixfmt` derivation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (1)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (5)
modules/nix-darwin/default.nix (4)
343-356: Doc mismatch: protocol can’t be null here.The option type is types.str, but the docs suggest using null. Remove the null mention to avoid confusion.
The protocol used for communicating with the build machine. Use `ssh-ng` if your remote builder and your local Nix version support that improved protocol. - - Use `null` when trying to change the special localhost builder without a - protocol which is for example used by hydra.
331-333: Doc path typo: reference the correct option path.It should reference nixosVmBasedLinuxBuilder, not linux-builder.
- {option}`determinateNix.linux-builder.config.virtualisation.cores` to configure + {option}`determinateNix.nixosVmBasedLinuxBuilder.config.virtualisation.cores` to configure
404-405: Doc path mismatch.The referenced option path should be determinateNix.buildMachines.*.systems.
- This sets the corresponding `nix.buildMachines.*.systems` option. + This sets the corresponding `determinateNix.buildMachines.*.systems` option.
589-595: Redundant assertion.This assertion is inside mkIf (cfg.enable), so it always holds. It adds noise without protection.
- assertions = [ - { - assertion = cfg.enable; - message = ''`determinateNix.nixosVmBasedLinuxBuilder.enable` requires `determinateNix.enable`''; - } - ];README.md (1)
67-67: Clarity nit: “apply the determinate nix-darwin module”.Add the determinate input (like in the later example), otherwise readers can’t actually “apply” the module.
I recommend adding this line to the inputs in the first example (outside this hunk):
inputs.determinate.url = "https://flakehub.com/f/DeterminateSystems/determinate/*";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
flake.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
README.md(3 hunks)flake.nix(2 hunks)modules/nix-darwin/default.nix(2 hunks)tests/flake.nix(2 hunks)
🧰 Additional context used
🪛 Gitleaks (8.28.0)
modules/nix-darwin/default.nix
[high] 648-648: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (5)
modules/nix-darwin/default.nix (1)
641-658: Gitleaks false positive: embedded public host key.The base64 value in publicHostKey is an SSH host public key (non-secret). Confirm this is intentional and generic. If so, consider a comment to suppress secret scanners.
If you want, I can add a brief comment like “Public host key, safe to commit” above this block to avoid future noise.
flake.nix (2)
70-70: Use per-system formatter from self — nice.Switching devShells to self.formatter.${system} keeps things consistent across systems.
79-79: Confirm nixfmt availability — ensure pinned nixpkgs ≥ 24.11Nixpkgs 24.11+ exposes pkgs.nixfmt on Linux and Darwin (replacing nixfmt-rfc-style); if your flake is pinned older than 24.11, update the pin or fall back to the older nixfmt-rfc-style attribute.
tests/flake.nix (2)
13-16: Good switch to }@inputs pattern.Aligns with the README intent and resolves scope for inputs.* references.
50-62: darwin config and customSettings look correct.
- determinateNix.enable = true
- customSettings keys match the module
- system.stateVersion is set
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
modules/nix-darwin/config/config.nix (1)
55-58: Optional: also suppress empty-list renderings (without blocking explicit empty strings)If desired, treat only empty lists as non-renderable while keeping explicit empty strings (so users can still clear options intentionally).
Apply this diff:
- # Filter out null attributes so that you don't end up with lines like `my-attr =`. - nonNullAttrs = lib.filterAttrs (_: v: v != null) attrs; + # Filter out nulls, and (optionally) also drop empty lists to avoid `key =` lines from []. + # Keeps explicit empty strings so users can still clear options intentionally. + nonNullAttrs = lib.filterAttrs (_: v: + v != null && + !(builtins.isList v && v == []) + ) attrs;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
modules/nix-darwin/config/config.nix(1 hunks)
🔇 Additional comments (1)
modules/nix-darwin/config/config.nix (1)
52-58: Good fix: drop null attrs to avoid straykey =linesThis neatly prevents accidental blank-value lines while preserving intentionally empty strings (useful for clearing nix.conf list options). Looks correct and side‑effect free.
Minor check: Is it intentional to still allow
key =when the value is an empty list (which renders to an empty string)? If you also want to suppress lines produced by empty lists, we can filter those too (see optional suggestion below).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
modules/nix-darwin/default.nix (4)
387-399: Align protocol type with docs (allow null) or drop null from docs.The description mentions null but the type is types.str.
Option A (allow null):
- protocol = mkOption { - type = types.str; + protocol = mkOption { + type = types.nullOr (types.enum [ "ssh" "ssh-ng" ]); default = "ssh-ng"; defaultText = literalExpression ''"ssh-ng"''; example = literalExpression ''"ssh"''; description = '' The protocol used for communicating with the build machine. Use `ssh-ng` if your remote builder and your local Nix version support that improved protocol. - Use `null` when trying to change the special localhost builder without a - protocol which is for example used by hydra. + Set to null only when targeting the special localhost builder without a protocol (e.g., Hydra). ''; };Option B (keep types.str): remove the “Use null …” lines from the description.
676-684: Harden SSH first-connection UX for the local builder.Without a baked-in publicHostKey, first connect can prompt. Use accept-new to avoid interactivity while keeping protections.
environment.etc."ssh/ssh_config.d/100-linux-builder.conf".text = '' Host linux-builder User builder Hostname localhost HostKeyAlias linux-builder Port 31022 IdentityFile /etc/nix/builder_ed25519 + StrictHostKeyChecking accept-new '';Verify the target macOS OpenSSH supports accept-new (it’s widely available on modern macOS).
12-29: Import lib.unique for feature list de-dup.You concat supportedFeatures and mandatoryFeatures later; de-dup for cleaner output.
inherit (lib) all concatMapStrings concatStringsSep filterAttrs hasAttr literalExpression mapAttrsToList mkDefault mkEnableOption mkForce mkIf mkMerge mkOption mkRenamedOptionModule optionalString + unique types ;
773-777: De-duplicate combined features in machines file.- res = (machine.supportedFeatures ++ machine.mandatoryFeatures); + res = unique (machine.supportedFeatures ++ machine.mandatoryFeatures);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
modules/nix-darwin/default.nix(2 hunks)
🧰 Additional context used
🪛 Gitleaks (8.28.0)
modules/nix-darwin/default.nix
[high] 692-692: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ 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: test-modules (macos-latest)
🔇 Additional comments (7)
modules/nix-darwin/default.nix (7)
691-694: Hard-coded publicHostKey: confirm provenance or derive at runtime.Gitleaks flags this as a “Generic API Key.” It’s a public host key (not secret), but a static value may be wrong for users. Consider deriving it (or using StrictHostKeyChecking accept-new) instead of committing a specific key. Please confirm this value matches the key created by your linux-builder on first run.
598-601: Provide textual default for trusted-users.- inherit (managedDefault "determinateNix.trusted-users" [ ]) default defaultText; + inherit (managedDefault "determinateNix.trusted-users" [ ] (literalExpression "[]")) default defaultText;
625-629: Do not rm -rf the builder working directory when the builder is disabled.This is destructive and surprising; toggling the flag wipes user state.
Remove this block:
- (mkIf (!nixosVmBasedLinuxBuilderCfg.enable) { - system.activationScripts.preActivation.text = '' - rm -rf ${nixosVmBasedLinuxBuilderCfg.workingDirectory} - ''; - })
532-533: Provide textual default for registry.- inherit (managedDefault "determinateNix.registry" { }) default defaultText; + inherit (managedDefault "determinateNix.registry" { } (literalExpression "{ }")) default defaultText;
291-294: Provide textual default for distributedBuilds.Follow-up to managedDefault fix: pass a literal expression for defaultText.
- inherit (managedDefault "determinateNix.distributedBuilds" false) default defaultText; + inherit (managedDefault "determinateNix.distributedBuilds" false (literalExpression "false")) default defaultText;
300-306: Provide textual default for envVars.- inherit (managedDefault "determinateNix.envVars" { }) default defaultText; + inherit (managedDefault "determinateNix.envVars" { } (literalExpression "{ }")) default defaultText;
62-72: Fix managedDefault: defaultText must be a string.defaultText is currently set to the raw default value (can be non-string). mkOption.defaultText requires a string/literal expression.
Apply this diff:
- managedDefault = name: default: { + managedDefault = name: default: defaultText: { default = if cfg.enable then default else throw '' ${name}: accessed when `determinateNix.enable` is off; this is a bug in nix-darwin or a third-party module ''; - defaultText = default; + defaultText = defaultText; };
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (12)
modules/nix-darwin/default.nix (12)
64-74: defaultText must be a string, not the raw default value.This issue was flagged in a previous review. The
defaultTextfield expects a string (typically vialiteralExpression), but line 73 assigns the rawdefaultvalue (which can be a bool, attrset, etc.). This breaks documentation generation and evaluation.Apply this diff:
- managedDefault = name: default: { + managedDefault = name: default: defaultText: { default = if cfg.enable then default else throw '' ${name}: accessed when `determinateNix.enable` is off; this is a bug in nix-darwin or a third-party module ''; - defaultText = default; + defaultText = defaultText; };Then update all call sites to pass a proper
defaultTextstring argument (e.g.,literalExpression "false"for booleans,literalExpression "{ }"for empty attrsets).
118-134: Usetypes.nullOrfor nullable enum instead of including null in the enum.This issue was flagged in a previous review. The type definition includes
nullas an enum member (line 120), which is incorrect. Usetypes.nullOr (types.enum [...])to properly allow null values.Apply this diff:
protocol = mkOption { - type = types.enum [ - null - "ssh" - "ssh-ng" - ]; + type = types.nullOr (types.enum [ "ssh" "ssh-ng" ]); default = "ssh"; example = "ssh-ng";
240-240: Pass proper textual default to managedDefault.Following the managedDefault signature fix, this call needs to provide a
defaultTextargument.Apply this diff after fixing
managedDefault:- inherit (managedDefault "determinateNix.buildMachines" [ ]) default defaultText; + inherit (managedDefault "determinateNix.buildMachines" [ ] (literalExpression "[]")) default defaultText;
294-301: Pass proper textual default to managedDefault.Following the managedDefault signature fix, provide a textual default.
Apply this diff:
- inherit (managedDefault "determinateNix.distributedBuilds" false) default defaultText; + inherit (managedDefault "determinateNix.distributedBuilds" false (literalExpression "false")) default defaultText;
303-309: Pass proper textual default to managedDefault.Following the managedDefault signature fix, provide a textual default.
Apply this diff:
- inherit (managedDefault "determinateNix.envVars" { }) default defaultText; + inherit (managedDefault "determinateNix.envVars" { } (literalExpression "{ }")) default defaultText;
390-403: Type/docs mismatch: null mentioned but not allowed.This issue was flagged in a previous review. The type is
types.strbut the description references usingnull. Either tighten to an enum and remove null guidance, or make the type nullable.Apply this diff to tighten the type and remove null guidance:
protocol = mkOption { - type = types.str; + type = types.enum [ "ssh" "ssh-ng" ]; default = "ssh-ng"; defaultText = literalExpression ''"ssh-ng"''; example = literalExpression ''"ssh"''; description = '' The protocol used for communicating with the build machine. Use `ssh-ng` if your remote builder and your local Nix version support that improved protocol. - - Use `null` when trying to change the special localhost builder without a - protocol which is for example used by hydra. ''; };
451-451: Doc reference should use determinateNix.buildMachines, not nix.buildMachines.This issue was flagged in a previous review. The documentation incorrectly references the old
nix.buildMachinespath.Apply this diff:
- This sets the corresponding `nix.buildMachines.*.systems` option. + This sets the corresponding `determinateNix.buildMachines.*.systems` option.
535-535: Pass proper textual default to managedDefault.Following the managedDefault signature fix, provide a textual default.
Apply this diff:
- inherit (managedDefault "determinateNix.registry" { }) default defaultText; + inherit (managedDefault "determinateNix.registry" { } (literalExpression "{ }")) default defaultText;
555-555: Correct option path to nix.optimise.automatic.This issue was flagged in a previous review. The documentation references a non-existent
determinateNix.optimise.automaticoption.Apply this diff:
- {option}`determinateNix.optimise.automatic` to run {command}`nix-store --optimise` + {option}`nix.optimise.automatic` to run {command}`nix-store --optimise`
601-618: Pass proper textual default to managedDefault.Following the managedDefault signature fix, provide a textual default.
Apply this diff:
- inherit (managedDefault "determinateNix.trusted-users" [ ]) default defaultText; + inherit (managedDefault "determinateNix.trusted-users" [ ] (literalExpression "[]")) default defaultText;
633-633: Fix typo: BUilder → Builder.This issue was flagged in a previous review. Line 633 contains "nixosVmBasedLinuxBUilder" with an incorrect uppercase U.
Apply this diff:
- Setting `determinateNix.nixosVmBasedLinuxBUilder.enable = true` requires you to set `determinateNix.enable = true` as well. + Setting `determinateNix.nixosVmBasedLinuxBuilder.enable = true` requires you to set `determinateNix.enable = true` as well.
732-736: Use lib.optional instead of mkIf in warnings list.This issue was flagged in a previous review. The
warningsattribute expects a list of strings, butmkIfreturns an attrset. This will cause a type error.Apply this diff:
- warnings = [ - (mkIf ( - !cfg.distributedBuilds && cfg.buildMachines != [ ] - ) "`determinateNix.distributedBuilds` is not enabled, thus build machines aren't configured.") - ]; + warnings = + lib.optional + (!cfg.distributedBuilds && cfg.buildMachines != [ ]) + "`determinateNix.distributedBuilds` is not enabled, thus build machines aren't configured.";Note: Ensure
libis in scope (it's already imported at the top of the file).
🧹 Nitpick comments (1)
modules/nix-darwin/default.nix (1)
766-800: Consider extracting machine formatting logic into a helper function.The machines file generation logic is correct but spans 35 lines with nested conditionals. Extracting the per-machine formatting into a named helper function would improve readability and testability.
Example structure:
let formatMachine = machine: concatStringsSep " " [ # protocol, user, hostname "${optionalString (machine.protocol != null) "${machine.protocol}://"}${ optionalString (machine.sshUser != null) "${machine.sshUser}@" }${machine.hostName}" # system or systems (if machine.system != null then machine.system else if machine.systems != [ ] then concatStringsSep "," machine.systems else "-") # ... rest of fields ]; in text = concatMapStrings (m: formatMachine m + "\n") cfg.buildMachines;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
modules/nix-darwin/default.nix(2 hunks)
🧰 Additional context used
🪛 Gitleaks (8.28.0)
modules/nix-darwin/default.nix
[high] 698-698: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (3)
modules/nix-darwin/default.nix (3)
82-85: LGTM: Clean option renaming.The
mkRenamedOptionModulecorrectly provides backward compatibility for thedeterminate-nix→determinateNixtransition.
251-292: LGTM: Well-structured Determinate Nixd configuration options.The
determinateNixdoption group provides a clean interface for configuring authentication, builder state, and garbage collection. The default forbuilder.state(line 271) correctly disables the native builder when the VM-based builder is enabled, preventing conflicts.
54-62: LGTM: Appropriate restrictions on user-configurable options.The
disallowedOptionslist correctly prevents users from configuring settings that Determinate Nix manages automatically, including the newly addedexternal-buildersandextra-nix-path.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
modules/nix-darwin/default.nix (6)
66-76: defaultText must be a string, not the raw default value.This issue has already been flagged in previous reviews. The
managedDefaultfunction assignsdefaultText = defaultbutdefaultTextmust be a string representation, typically vialiteralExpression.
399-412: Type/docs mismatch for protocol option.This was already flagged in previous reviews. The option type is
types.strbut the description mentions usingnull, creating an inconsistency.
564-566: Documentation references non-existent option path.This was already flagged in previous reviews. The documentation references
determinateNix.optimise.automaticwhich doesn't exist in this module. Should likely referencenix.optimise.automatic.
660-661: Environment variable should be guarded against missing attribute.This was already flagged in previous reviews. The code should handle the case where
NIX_SSL_CERT_FILEmight not be defined inconfig.environment.variables.
741-745: Critical: mkIf returns attrset, not string.This was already flagged in previous reviews. The
warningslist expects strings, butmkIfreturns an attrset. Uselib.optionalinstead.
450-450: Documentation references incorrect option namespace.This appears to be the same issue flagged in previous reviews about referencing
nix.buildMachinesinstead ofdeterminateNix.buildMachines, but the line reference shows it's about systems rather than the earlier issue.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
modules/nix-darwin/default.nix(2 hunks)
🧰 Additional context used
🪛 Gitleaks (8.28.0)
modules/nix-darwin/default.nix
[high] 707-707: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ 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: test-modules (macos-latest)
🔇 Additional comments (6)
modules/nix-darwin/default.nix (6)
707-707: Static analysis false positive: This is a public SSH host key.The Gitleaks warning is a false positive. This base64-encoded string is a public SSH host key for the linux-builder VM, not a secret API key. Public keys are safe to include in configuration.
800-834: LGTM: Build machines file generation handles edge cases correctly.The machines file generation properly:
- Constructs protocol prefixes conditionally
- Handles null protocol (no prefix)
- Falls back from
systemtosystemsto "-"- Merges supported and mandatory features
- Uses "-" placeholder for null values
This aligns with the expected format for Nix's
build-remote.pl.
771-796: Well-designed null filtering for Determinate Nixd configuration.The implementation correctly:
- Filters out null values using
optionalAttrs- Only generates the config file when non-null settings exist
- Uses
recursiveUpdateto merge the conditional attribute setsThis ensures the generated JSON config is clean and only contains explicitly configured values.
647-658: LGTM: Safe migration and directory creation logic.The preActivation script:
- Safely checks for the old directory before migrating
- Only migrates if the old directory exists and new doesn't
- Ensures the working directory exists
This handles the migration path cleanly without data loss.
667-676: LGTM: TMPDIR handling prevents certificate cleanup issues.Using
/run/org.nixos.linux-builderinstead of/tmpprevents macOS from automatically cleaning up certificates after 3 days of inactivity. The script properly:
- Sets up the custom TMPDIR
- Creates it fresh on each run
- Cleans it up on exit
- Conditionally removes ephemeral VM disk
748-748: Verify disabling of nix-darwin’s Nix management
Upstream nix-darwin modules aren’t vendored here—confirm that settingnix.enable = mkForce false;in
modules/nix-darwin/default.nixfully prevents all built-in nix-darwin Nix configuration.
| "external-builders" | ||
| "extra-nix-path" |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the disallowedOptions list matches Determinate Nix's managed settings.
The list was updated to use external-builders (instead of builders) and added extra-nix-path. Confirm these changes align with the current version of Determinate Nix's managed configuration options.
🌐 Web query:
What configuration options does Determinate Nix automatically manage as of 2025? Specifically, verify if Determinate Nix manages "external-builders" or "builders", and whether "extra-nix-path" is managed.
💡 Result:
Short answer:
-
Determinate Nix writes and manages /etc/nix/nix.conf and asks you not to edit generated values; custom settings must go in /etc/nix/nix.custom.conf. [1][2]
-
extra-nix-path (the nix-path / --extra-nix-path mechanism) is part of Nix’s configurable nix-path settings and command-line flags; you can supply custom Nix configuration via /etc/nix/nix.custom.conf when using Determinate. So extra-nix-path can be set as custom configuration (or via the CLI). [2][3]
-
external-builders / builders: Determinate documents a “native Linux builder” feature for macOS and describes Determinate Nixd handling remote-build/daemon responsibilities, but the public docs do not show that Determinate simply exposes/overwrites the Nix “external-builders” config key directly; remote/external-build behaviour is provided/handled by Determinate Nixd and Determinate’s distribution rather than by recommending edits to the generated nix.conf. In short: Determinate provides and manages builder functionality (native Linux builder and Nixd-managed remote builds) but does not instruct users to directly edit an external-builders entry in /etc/nix/nix.conf — custom changes belong in nix.custom.conf. [1][2][4]
Sources:
- Determinate Nix — Configuring Determinate Nix (writes /etc/nix/nix.conf; use /etc/nix/nix.custom.conf for custom values). [1]
- Determinate Nix docs — Use Determinate with nix-darwin / customSettings examples showing nix.custom.conf usage. [2]
- Nix manual (nix.conf) — shows nix-path / extra-nix-path/--extra-nix-path behavior. [3]
- Determinate Nix docs — Native Linux builder / Determinate Nixd and remote build discussion. [1][4]
If you’d like, I can open the exact nix.conf fields Determinate writes on a current install (or fetch specific doc lines) and quote them.
Remove unmanaged keys from disallowedOptions
modules/nix-darwin/default.nix lines 59–60: remove "external-builders" and "extra-nix-path" from disallowedOptions — Determinate Nix does not auto-manage these settings in its generated /etc/nix/nix.conf.
🤖 Prompt for AI Agents
In modules/nix-darwin/default.nix around lines 59–60, the disallowedOptions list
currently includes "external-builders" and "extra-nix-path"; remove those two
entries from the disallowedOptions array so Determinate Nix does not incorrectly
block settings that it does not auto-manage in /etc/nix/nix.conf.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (12)
modules/nix-darwin/default.nix (12)
45-55: Fix defaultText typing in managedDefault.The
defaultTextfield must be a string (typically vialiteralExpression). Passing the rawdefaultvalue (which can be a bool or attrset) is incorrect and will break documentation generation.Apply this diff:
- managedDefault = name: default: { + managedDefault = name: default: defaultText: { default = if cfg.enable then default else throw '' ${name}: accessed when `determinateNix.enable` is off; this is a bug in nix-darwin or a third-party module ''; - defaultText = default; + defaultText = defaultText; };Based on past review comments.
38-39: Remove unmanaged keys from disallowedOptions.Determinate Nix does not auto-manage
external-buildersorextra-nix-pathin its generated/etc/nix/nix.conf. These entries should be removed from thedisallowedOptionslist.Apply this diff:
disallowedOptions = [ "always-allow-substitutes" "bash-prompt-prefix" - "external-builders" - "extra-nix-path" "netrc-file" "ssl-cert-file" "upgrade-nix-store-path-url" ];Based on past review comments.
222-222: Pass a proper textual default for buildMachines.Following the
managedDefaultfix, provide a string expression fordefaultText.- inherit (managedDefault "determinateNix.buildMachines" [ ]) default defaultText; + inherit (managedDefault "determinateNix.buildMachines" [ ] (literalExpression "[]")) default defaultText;Based on past review comments.
284-284: Pass a proper textual default for distributedBuilds.Following the
managedDefaultfix, provide a string expression fordefaultText.- inherit (managedDefault "determinateNix.distributedBuilds" false) default defaultText; + inherit (managedDefault "determinateNix.distributedBuilds" false (literalExpression "false")) default defaultText;Based on past review comments.
295-295: Pass a proper textual default for envVars.Following the
managedDefaultfix,defaultTextshould be a string.- inherit (managedDefault "determinateNix.envVars" { }) default defaultText; + inherit (managedDefault "determinateNix.envVars" { } (literalExpression "{ }")) default defaultText;Based on past review comments.
378-391: Protocol type/docs mismatch (null mentioned but not allowed).The option type is
types.strbut the description references usingnull. Either allow null or drop the null guidance.Apply this diff:
protocol = mkOption { - type = types.str; + type = types.enum [ "ssh" "ssh-ng" ]; default = "ssh-ng"; defaultText = lib.literalExpression ''"ssh-ng"''; example = lib.literalExpression ''"ssh"''; description = '' The protocol used for communicating with the build machine. Use `ssh-ng` if your remote builder and your local Nix version support that improved protocol. - - Use `null` when trying to change the special localhost builder without a - protocol which is for example used by hydra. ''; };Based on past review comments.
523-523: Pass a proper textual default for registry.Following the
managedDefaultfix.- inherit (managedDefault "determinateNix.registry" { }) default defaultText; + inherit (managedDefault "determinateNix.registry" { } (literalExpression "{ }")) default defaultText;Based on past review comments.
591-591: Pass a proper textual default for trusted-users.Following the
managedDefaultfix.- inherit (managedDefault "determinateNix.trusted-users" [ ]) default defaultText; + inherit (managedDefault "determinateNix.trusted-users" [ ] (literalExpression "[]")) default defaultText;Based on past review comments.
622-622: Typo in assertion message (BUilder).- Setting `determinateNix.nixosVmBasedLinuxBUilder.enable = true` requires you to set `determinateNix.enable = true` as well. + Setting `determinateNix.nixosVmBasedLinuxBuilder.enable = true` requires you to set `determinateNix.enable = true` as well.Based on past review comments.
641-641: Guard NIX_SSL_CERT_FILE inheritance to avoid missing-attr eval errors.Direct assignment assumes the variable exists. Use
optionalAttrsto make it conditional.launchd.daemons.linux-builder = { - environment.NIX_SSL_CERT_FILE = "${pkgs.cacert}/etc/ssl/certs/ca-bundle.crt"; + environment = lib.optionalAttrs (config.environment.variables ? NIX_SSL_CERT_FILE) { + NIX_SSL_CERT_FILE = config.environment.variables.NIX_SSL_CERT_FILE; + };Based on past review comments.
723-727: mkIf used on a list element yields the wrong type.The
warningsattribute expects a list of strings.mkIfreturns an attrset. Uselib.optionalinstead.Apply this diff:
- warnings = [ - (lib.mkIf ( - !cfg.distributedBuilds && cfg.buildMachines != [ ] - ) "`determinateNix.distributedBuilds` is not enabled, thus build machines aren't configured.") - ]; + warnings = + lib.optional + (!cfg.distributedBuilds && cfg.buildMachines != [ ]) + "`determinateNix.distributedBuilds` is not enabled, thus build machines aren't configured.";Based on past review comments.
827-827: Disable distributed builds by setting builders to an empty string.The nix.conf
buildersoption is a string (semicolon/newline-separated list or @/file). Settingbuilders = nullis incorrect; use an empty string to disable distributed builds.Apply this diff:
(lib.mkIf (!cfg.distributedBuilds) { builders = null; }) + (lib.mkIf (!cfg.distributedBuilds) { builders = ""; })Based on past review comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
modules/nix-darwin/default.nix(2 hunks)
🧰 Additional context used
🪛 Gitleaks (8.28.0)
modules/nix-darwin/default.nix
[high] 687-687: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ 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: test-modules (macos-latest)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
modules/nix-darwin/default.nix (6)
551-553: Incorrect option path reference (re-introduced issue).A past review noted that line 551 references
determinateNix.optimise.automatic, but this option does not exist in this module. The correct path isnix.optimise.automatic.Apply this diff:
- {option}`determinateNix.optimise.automatic` to run {command}`nix-store --optimise` + {option}`nix.optimise.automatic` to run {command}`nix-store --optimise`Based on past review comments.
38-39: Remove unmanaged keys from disallowedOptions (re-introduced issue).A past review with web search verification confirmed that Determinate Nix does not auto-manage
"external-builders"or"extra-nix-path"in its generated/etc/nix/nix.conf. These entries should not be indisallowedOptions. This issue was flagged previously but has reappeared.Apply this diff:
disallowedOptions = [ "always-allow-substitutes" "bash-prompt-prefix" - "external-builders" - "extra-nix-path" "netrc-file" "ssl-cert-file" "upgrade-nix-store-path-url" ];Based on past review comments.
715-717: Don't wipe the working directory when the builder is disabled (re-introduced critical issue).A past review noted this was addressed in commits 81cf3a0 to eb9c2e1, but this destructive
rm -rfblock has reappeared. This will delete the working directory on every activation when the Linux builder is disabled, which is surprising and data-loss-prone if users toggle the builder or reuse the directory later.Apply this diff to remove the destructive block:
- # Nixpkgs Linux builder disabled - (lib.mkIf (!nixosVmBasedLinuxBuilderCfg.enable) { - system.activationScripts.preActivation.text = '' - rm -rf ${nixosVmBasedLinuxBuilderCfg.workingDirectory} - ''; - })Based on past review comments.
386-399: Protocol type/docs mismatch (re-introduced issue).A past review flagged that the
protocoloption declarestype = types.strbut the description (lines 396-397) mentions usingnull, creating a type/docs mismatch. The correct approach is to either tighten to an enum or usenullOr.Apply this diff to tighten to enum and remove null guidance:
protocol = lib.mkOption { - type = types.str; + type = types.enum [ "ssh" "ssh-ng" ]; default = "ssh-ng"; defaultText = lib.literalExpression ''"ssh-ng"''; example = lib.literalExpression ''"ssh"''; description = '' The protocol used for communicating with the build machine. Use `ssh-ng` if your remote builder and your local Nix version support that improved protocol. - - Use `null` when trying to change the special localhost builder without a - protocol which is for example used by hydra. ''; };Based on past review comments.
45-55: Fix defaultText typing in managedDefault (re-introduced issue).A past review noted that
defaultTextmust be a string (often vialiteralExpression), not the raw default value. This issue was marked as addressed in commit f900e8a but has reappeared. Line 54 still assignsdefaultText = default;, which will break docs/eval whendefaultis a bool or attrset.Apply this diff:
- managedDefault = name: default: { + managedDefault = name: default: defaultText: { default = if cfg.enable then default else throw '' ${name}: accessed when `determinateNix.enable` is off; this is a bug in nix-darwin or a third-party module ''; - defaultText = default; + defaultText = defaultText; };Then update all call sites to pass a proper textual default as the third argument (e.g.,
literalExpression "false"for booleans,literalExpression "{ }"for empty attrsets).Based on past review comments.
731-735: mkIf used on a list element yields the wrong type (re-introduced critical issue).A past review noted that
warningsexpects a list of strings, butmkIfreturns an attrset. This type mismatch will cause evaluation errors.Apply this diff:
- warnings = [ - (lib.mkIf ( - !cfg.distributedBuilds && cfg.buildMachines != [ ] - ) "`determinateNix.distributedBuilds` is not enabled, thus build machines aren't configured.") - ]; + warnings = + lib.optional + (!cfg.distributedBuilds && cfg.buildMachines != [ ]) + "`determinateNix.distributedBuilds` is not enabled, thus build machines aren't configured.";Based on past review comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
modules/nix-darwin/default.nix(2 hunks)
🧰 Additional context used
🪛 Gitleaks (8.28.0)
modules/nix-darwin/default.nix
[high] 695-695: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ 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: test-modules (macos-latest)
🔇 Additional comments (1)
modules/nix-darwin/default.nix (1)
649-649: Remove unnecessary guard on NIX_SSL_CERT_FILE: no references toconfig.environment.variables.NIX_SSL_CERT_FILEin the codebase; direct assignment is safe.
lucperkins
dismissed
edolstra’s stale review
The contents of the PR have changed pretty dramatically since initial approval. Best to re-assess.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
modules/nix-darwin/default.nix (4)
718-722: Fix incorrect use of mkIf in warnings list (re-raised critical issue).
warningsexpects a list of strings, butmkIfreturns an attrset. Uselib.optionalinstead.Apply this diff:
- warnings = [ - (lib.mkIf ( - !cfg.distributedBuilds && cfg.buildMachines != [ ] - ) "`determinateNix.distributedBuilds` is not enabled, thus build machines aren't configured.") - ]; + warnings = lib.optional + (!cfg.distributedBuilds && cfg.buildMachines != [ ]) + "`determinateNix.distributedBuilds` is not enabled, thus build machines aren't configured.";Based on past review comments.
826-834: Explicitly disable distributed builds when distributedBuilds is false (re-raised critical issue).Per past review with web search verification: the nix.conf
buildersoption must be explicitly set to an empty string to disable distributed builds. Currently, the code doesn't setbuildersat all whendistributedBuildsis false.Apply this diff:
determinateNix.customSettings = lib.mkMerge [ (lib.mkIf (cfg.registry != { }) { flake-registry = "/etc/${registryFile}"; }) + (lib.mkIf (!cfg.distributedBuilds) { builders = ""; }) (lib.mkIf nixosVmBasedLinuxBuilderCfg.enable {Based on past review comments.
37-38: Remove unmanaged keys from disallowedOptions (re-raised critical issue).Per past review with web search verification: Determinate Nix does not auto-manage
external-buildersorextra-nix-pathin its generated/etc/nix/nix.conf. These should be removed from the disallowed list.Apply this diff:
disallowedOptions = [ "bash-prompt-prefix" - "external-builders" - "extra-nix-path" "netrc-file" "ssl-cert-file" "upgrade-nix-store-path-url" ];Based on past review comments.
391-404: Fix type/docs mismatch: protocol cannot be null with current type.The type is
types.strbut the description mentions usingnull. Either tighten to an enum and remove null guidance, or make the type nullable.Option 1 (recommended): Remove null guidance and restrict to enum:
protocol = lib.mkOption { - type = types.str; + type = types.enum [ "ssh" "ssh-ng" ]; default = "ssh-ng"; defaultText = lib.literalExpression ''"ssh-ng"''; example = lib.literalExpression ''"ssh"''; description = '' The protocol used for communicating with the build machine. Use `ssh-ng` if your remote builder and your local Nix version support that improved protocol. - - Use `null` when trying to change the special localhost builder without a - protocol which is for example used by hydra. ''; };Option 2: Allow null in type:
protocol = lib.mkOption { - type = types.str; + type = types.nullOr (types.enum [ "ssh" "ssh-ng" ]); default = "ssh-ng";Based on past review comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
modules/nix-darwin/default.nix(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: Build and Publish
modules/nix-darwin/default.nix
[error] 590-590: undefined variable 'literalExpression' (in configuration code at 590:71)
🪛 Gitleaks (8.28.0)
modules/nix-darwin/default.nix
[high] 689-689: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
…rove-the-nix-darwin-solution-for-better-user-experience
This PR overhauls our nix-darwin support pretty significantly:
determinateNix.enable = trueis set, nix-darwin's built-in Nix configuration is forcibly disabled./etc/nix/nix.custom.conf, it now directly supports the nix-darwin-ified configuration options rather than converting any providing attribute set into the proper conf format.garbageCollector.strategyand so on).Summary by CodeRabbit
New Features
Documentation
Refactor
Chores
Tests
Bug Fixes