Improve the overall nix-darwin experience#136
Conversation
📝 WalkthroughWalkthroughConsolidates flake inputs into a single Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Admin
participant Flake as "flake.nix"
participant Inputs as "inputs (single map)"
participant DevShell as "devShells"
participant Formatter as "self.formatter.${system}"
Admin->>Flake: Evaluate flake
Flake->>Inputs: Expose consolidated inputs map
DevShell->>Formatter: Reference self.formatter.${system}
Formatter-->>DevShell: Provide per-system nixfmt derivation
sequenceDiagram
autonumber
actor User
participant Host as "macOS host"
participant Module as "modules/nix-darwin/default.nix"
participant Files as "/etc/nix & registry"
participant Launchd as "launchd"
participant VM as "Linux builder VM"
participant Nix as "Nix environment"
User->>Host: Apply configuration
Host->>Module: Evaluate determinateNix.enable
alt determinateNix enabled
Module->>Files: Render nix/custom.conf, registry.json, determinate/config.json, nix/machines
Module->>Launchd: Install/enable linux-builder service & SSH config fragments
Launchd->>VM: Start VM builder (if nixosVmBasedLinuxBuilder.enable)
Module->>Nix: Wire env vars and Determinate Nix overrides
else disabled
Module->>Nix: Retain nix-darwin defaults
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 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
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
modules/nix-darwin/default.nix (2)
814-822: Set builders to empty string when distributedBuilds is disabled (re-introduced issue).Past review comments with web search verification confirmed that 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 to explicitly disable builders when not using distributed builds:
determinateNix.customSettings = lib.mkMerge [ (lib.mkIf (cfg.registry != { }) { flake-registry = "/etc/${registryFile}"; }) + (lib.mkIf (!cfg.distributedBuilds) { builders = ""; }) (lib.mkIf nixosVmBasedLinuxBuilderCfg.enable { # To enable fetching the cached NixOS VM trusted-public-keys = [ "cache.nixos.org-1:6NCHdD59X431o0gWypbMrAURkbJ16ZPMQFGspcDShjY=" ]; trusted-users = [ "root" ]; substituters = lib.mkAfter [ "https://cache.nixos.org/" ]; }) ];Based on past review comments.
37-38: Remove unmanaged keys from disallowedOptions (re-introduced issue).Based on past review comments with web search verification, Determinate Nix does not auto-manage
external-buildersorextra-nix-pathin its generated/etc/nix/nix.conf. Including these indisallowedOptionsincorrectly prevents users from configuring settings that Determinate Nix doesn't manage.Apply this diff to remove the unmanaged entries:
disallowedOptions = [ "bash-prompt-prefix" - "external-builders" - "extra-nix-path" "netrc-file" "ssl-cert-file" "upgrade-nix-store-path-url" ];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] 677-677: 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)
…rove-the-nix-darwin-solution-for-better-user-experience
…rove-the-nix-darwin-solution-for-better-user-experience
…rove-the-nix-darwin-solution-for-better-user-experience
…rove-the-nix-darwin-solution-for-better-user-experience
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @modules/nix-darwin/default.nix:
- Around line 394-407: The option `protocol` in `nixosVmBasedLinuxBuilder`
currently uses `lib.mkOption` with type `types.str` but the description mentions
using `null`; change the option type to allow null (e.g. `types.nullOr
types.str`) to match `buildMachines.protocol`, keep the default `"ssh-ng"` (or
set default to `null` if you prefer that behaviour), and ensure
`defaultText`/`example` remain consistent with the new type and description.
- Around line 74-218: The buildMachines submodule currently allows entries with
system = null and systems = [], resulting in malformed /etc/nix/machines; add a
validation/assertion inside the buildMachines submodule to require that each
machine entry has either system set (non-null) or systems is a non-empty list.
Implement this by adding a validate (or assertion) over the list value in the
buildMachines option that iterates each entry and ensures (machine.system !=
null) || (builtins.length machine.systems > 0), and return a clear error message
referencing the machine hostName when the check fails.
- Around line 618-629: The assertion guarding that enabling
determinateNix.nixosVmBasedLinuxBuilder requires determinateNix.enable is
currently nested inside lib.mkIf cfg.enable, so when cfg.enable is false the
assertion never runs and invalid combinations are silently allowed; move the
assertion block that references determinateNix.enable out of the lib.mkIf
cfg.enable gating (but keep it conditional on
nixosVmBasedLinuxBuilderCfg.enable) so the assertion always executes if
nixosVmBasedLinuxBuilderCfg.enable is true regardless of cfg.enable, i.e.
extract the assertions array (the entry with assertion =
config.determinateNix.enable and its message) and place it in a lib.mkIf
nixosVmBasedLinuxBuilderCfg.enable { ... } outside the cfg.enable wrapper.
🧹 Nitpick comments (2)
modules/nix-darwin/default.nix (2)
220-282:explicitlySetAttrsis a misnomer (defaults are always written todeterminate/config.json).
builder.memoryBytesandbuilder.cpuCountdefault to non-null values, soexplicitlySetAttrs != { }will almost always be true and the file will be emitted even when the user didn’t configure anything. If that’s intended, rename/comment accordingly; if not intended, make those defaultsnulland rely on Nixd defaults.Also applies to: 747-791
631-642: Harden shell snippets: quote paths and considerset -euo pipefail.
workingDirectoryis user-configurable and interpolated unquoted into shell (mv,mkdir,rm). Quoting prevents surprising breakage on spaces and reduces foot-guns.Proposed fix (quoting)
system.activationScripts.preActivation.text = let directory = nixosVmBasedLinuxBuilderCfg.workingDirectory; in '' # Migrate if using the old working directory - if [ -e /var/lib/darwin-builder ] && [ ! -e ${directory} ]; then - mv /var/lib/darwin-builder ${directory} + if [ -e /var/lib/darwin-builder ] && [ ! -e "${directory}" ]; then + mv /var/lib/darwin-builder "${directory}" fi - mkdir -p ${directory} + mkdir -p "${directory}" '';Also applies to: 651-660
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
modules/nix-darwin/default.nix
🧰 Additional context used
🪛 Gitleaks (8.30.0)
modules/nix-darwin/default.nix
[high] 691-691: 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 (2)
modules/nix-darwin/default.nix (2)
57-72: Potentially surprising default:determinateNix.enable = truedisables nix-darwin’snix.*handling by default.If this module is imported unconditionally, this flips behavior for users who didn’t opt in. If it’s only imported by Determinate’s installer/module wiring, then it’s fine—worth double-checking how
modules/nix-darwin/default.nixis brought in.
686-706: This is a public SSH key and is safe to commit. Gitleaks is not enforced in this repository's CI, so no action is required.Likely an incorrect or invalid review comment.
| # Local build machines specified in `/etc/nix/machines`. | ||
| buildMachines = lib.mkOption { | ||
| type = types.listOf ( | ||
| types.submodule { | ||
| options = { | ||
| hostName = lib.mkOption { | ||
| type = types.str; | ||
| example = "nixbuilder.example.org"; | ||
| description = '' | ||
| The hostname of the build machine. | ||
| ''; | ||
| }; | ||
| protocol = lib.mkOption { | ||
| type = types.nullOr ( | ||
| types.enum [ | ||
| "ssh" | ||
| "ssh-ng" | ||
| ] | ||
| ); | ||
| default = "ssh"; | ||
| example = "ssh-ng"; | ||
| 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. | ||
| ''; | ||
| }; | ||
| system = lib.mkOption { | ||
| type = types.nullOr types.str; | ||
| default = null; | ||
| example = "x86_64-linux"; | ||
| description = '' | ||
| The system type the build machine can execute derivations on. | ||
| Either this attribute or {var}`systems` must be | ||
| present, where {var}`system` takes precedence if | ||
| both are set. | ||
| ''; | ||
| }; | ||
| systems = lib.mkOption { | ||
| type = types.listOf types.str; | ||
| default = [ ]; | ||
| example = [ | ||
| "x86_64-linux" | ||
| "aarch64-linux" | ||
| ]; | ||
| description = '' | ||
| The system types the build machine can execute derivations on. | ||
| Either this attribute or {var}`system` must be | ||
| present, where {var}`system` takes precedence if | ||
| both are set. | ||
| ''; | ||
| }; | ||
| sshUser = lib.mkOption { | ||
| type = types.nullOr types.str; | ||
| default = null; | ||
| example = "builder"; | ||
| description = '' | ||
| The username to log in as on the remote host. This user must be | ||
| able to log in and run nix commands non-interactively. It must | ||
| also be privileged to build derivations, so must be included in | ||
| {option}`determinateNix.customSettings.trusted-users`. | ||
| ''; | ||
| }; | ||
| sshKey = lib.mkOption { | ||
| type = types.nullOr types.str; | ||
| default = null; | ||
| example = "/root/.ssh/id_buildhost_builduser"; | ||
| description = '' | ||
| The path to the SSH private key with which to authenticate on | ||
| the build machine. The private key must not have a passphrase. | ||
| If null, the building user (root on NixOS machines) must have an | ||
| appropriate ssh configuration to log in non-interactively. | ||
|
|
||
| Note that for security reasons, this path must point to a file | ||
| in the local filesystem, *not* to the nix store. | ||
| ''; | ||
| }; | ||
| maxJobs = lib.mkOption { | ||
| type = types.int; | ||
| default = 1; | ||
| description = '' | ||
| The number of concurrent jobs the build machine supports. The | ||
| build machine will enforce its own limits, but this allows hydra | ||
| to schedule better since there is no work-stealing between build | ||
| machines. | ||
| ''; | ||
| }; | ||
| speedFactor = lib.mkOption { | ||
| type = types.int; | ||
| default = 1; | ||
| description = '' | ||
| The relative speed of this builder. This is an arbitrary integer | ||
| that indicates the speed of this builder, relative to other | ||
| builders. Higher is faster. | ||
| ''; | ||
| }; | ||
| mandatoryFeatures = lib.mkOption { | ||
| type = types.listOf types.str; | ||
| default = [ ]; | ||
| example = [ "big-parallel" ]; | ||
| description = '' | ||
| A list of features mandatory for this builder. The builder will | ||
| be ignored for derivations that don't require all features in | ||
| this list. All mandatory features are automatically included in | ||
| {var}`supportedFeatures`. | ||
| ''; | ||
| }; | ||
| supportedFeatures = lib.mkOption { | ||
| type = types.listOf types.str; | ||
| default = [ ]; | ||
| example = [ | ||
| "kvm" | ||
| "big-parallel" | ||
| ]; | ||
| description = '' | ||
| A list of features supported by this builder. The builder will | ||
| be ignored for derivations that require features not in this | ||
| list. | ||
| ''; | ||
| }; | ||
| publicHostKey = lib.mkOption { | ||
| type = types.nullOr types.str; | ||
| default = null; | ||
| description = '' | ||
| The (base64-encoded) public host key of this builder. The field | ||
| is calculated via {command}`base64 -w0 /etc/ssh/ssh_host_type_key.pub`. | ||
| If null, SSH will use its regular known-hosts file when connecting. | ||
| ''; | ||
| }; | ||
| }; | ||
| } | ||
| ); | ||
| default = [ ]; | ||
| defaultText = lib.literalExpression "[ ]"; | ||
| description = '' | ||
| This option lists the machines to be used if distributed builds are | ||
| enabled (see {option}`determinateNix.distributedBuilds`). | ||
| Nix will perform derivations on those machines via SSH by copying the | ||
| inputs to the Nix store on the remote machine, starting the build, | ||
| then copying the output back to the local Nix store. | ||
| ''; | ||
| }; |
There was a problem hiding this comment.
Enforce system or systems for buildMachines (currently allows invalid entries).
Right now, a machine with system = null and systems = [] produces - in /etc/nix/machines (Line 807), which is likely to confuse users and/or Nix’s builder selection. Consider adding a submodule assertion.
Proposed fix (add assertion inside the buildMachines submodule)
types.submodule {
+ config = {
+ assertions = [
+ {
+ assertion = config.system != null || config.systems != [ ];
+ message = "determinateNix.buildMachines: either `system` or `systems` must be set for each machine.";
+ }
+ ];
+ };
options = {
hostName = lib.mkOption {
type = types.str;🤖 Prompt for AI Agents
In @modules/nix-darwin/default.nix around lines 74 - 218, The buildMachines
submodule currently allows entries with system = null and systems = [],
resulting in malformed /etc/nix/machines; add a validation/assertion inside the
buildMachines submodule to require that each machine entry has either system set
(non-null) or systems is a non-empty list. Implement this by adding a validate
(or assertion) over the list value in the buildMachines option that iterates
each entry and ensures (machine.system != null) || (builtins.length
machine.systems > 0), and return a clear error message referencing the machine
hostName when the check fails.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @modules/nix-darwin/default.nix:
- Around line 651-660: Validate and restrict the hostName option so it cannot
contain characters that break attribute names or filesystem paths (e.g., allow
only [A-Za-z0-9_-] and reject spaces/special chars) by adding a check to the
hostName option definition (the hostName option around the existing option
block) that uses a built-in assertion or a custom validation function; update or
add an error message describing the allowed pattern. Alternatively (or
additionally), quote all dynamic attribute usages and shell insertions that
currently interpolate hostName: replace unquoted
launchd.daemons.${nixosVmBasedLinuxBuilderCfg.hostName} with
launchd.daemons."${nixosVmBasedLinuxBuilderCfg.hostName}" and ensure paths like
/run/org.nixos.${nixosVmBasedLinuxBuilderCfg.hostName},
${nixosVmBasedLinuxBuilderCfg.workingDirectory}/${nixosVmBasedLinuxBuilderCfg.package.nixosConfig.networking.hostName}.qcow2,
and any TMPDIR interpolations are properly shell-quoted or sanitized to prevent
injection/failure. Ensure the validation and quoting changes address hostName
usage in the option definition and in places referencing
nixosVmBasedLinuxBuilderCfg.hostName, launchd.daemons, TMPDIR, workingDirectory,
and package.nixosConfig.networking.hostName.
🧹 Nitpick comments (1)
modules/nix-darwin/default.nix (1)
631-642: Verify migration path and directory permissions.The preActivation script migrates from the old
/var/lib/darwin-builderdirectory to a configurable working directory. While the logic is sound, consider these aspects:
- The migration performs an
mvoperation that could fail if permissions are incorrect or if the source directory is in use.- No verification that the migration succeeded before proceeding.
- The
mkdir -pat line 641 will succeed even if themvfailed, potentially masking issues.🛡️ Suggested improvement for robustness
'' # Migrate if using the old working directory if [ -e /var/lib/darwin-builder ] && [ ! -e ${directory} ]; then - mv /var/lib/darwin-builder ${directory} + if ! mv /var/lib/darwin-builder ${directory}; then + echo "Warning: Failed to migrate /var/lib/darwin-builder to ${directory}" >&2 + fi fi mkdir -p ${directory} '';
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
modules/nix-darwin/default.nix
🧰 Additional context used
🪛 Gitleaks (8.30.0)
modules/nix-darwin/default.nix
[high] 691-691: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (6)
modules/nix-darwin/default.nix (6)
691-691: Static analysis false positive: This is a legitimate public SSH host key, not a secret.The Gitleaks warning about line 691 is a false positive. This is a base64-encoded SSH public host key (indicated by the
c3NoLWVkMjU1MTkprefix which decodes to "ssh-ed25519"), not a private key or API secret. Public SSH host keys are meant to be shared and are used for host verification, not authentication.The key appears to be the public host key for the NixOS VM builder, which is appropriately used here to enable SSH host key verification without relying on SSH's known_hosts mechanism.
793-827: Well-structured machines file generation with proper fallback handling.The code correctly implements the Nix distributed builds machine file format with appropriate handling of optional fields using "-" as a placeholder. The logic properly:
- Combines protocol, SSH user, and hostname into the URI format (lines 797-799)
- Handles the mutual exclusivity of
systemvssystems(lines 801-807)- Merges mandatory features into supported features (lines 812-816)
- Outputs mandatory features separately (lines 818-822)
747-789: Robust configuration file generation with null-value filtering.The implementation correctly filters out null values before generating the Determinate Nixd configuration file:
- Uses
optionalAttrs (v != null)to exclude null values (line 759)- Only generates the config file when there are actual settings to write (line 787)
- Maintains proper nested structure using
setAttrByPath(line 759)This prevents writing unnecessary or invalid configuration entries.
712-717: Strong validation prevents misconfiguration.The assertion correctly prevents users from setting Nix options that Determinate Nix manages internally, such as
external-builders,netrc-file, andssl-cert-file. This ensures that Determinate Nix's internal configuration management isn't bypassed, which could lead to conflicts or unexpected behavior.
720-722: Helpful warning for common misconfiguration.The warning appropriately alerts users when they've defined build machines but haven't enabled distributed builds. This catches a common configuration mistake where the machines would be ignored, potentially causing confusion about why remote building isn't working.
684-706: Proper integration disables native builder when VM builder is enabled.When the NixOS VM-based Linux builder is enabled, this code correctly:
- Configures the build machine entry with all necessary parameters (lines 686-701)
- Disables Determinate Nix's native Linux builder to avoid conflicts (line 703)
- Enables distributed builds and substitute usage (lines 704-705)
This ensures that the two builder implementations don't interfere with each other.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @modules/nix-darwin/default.nix:
- Around line 104-128: Add a validation assertion in the module config (e.g., in
the assertions array near the config section) that iterates over
cfg.buildMachines and requires each entry to set either system (not null) or
systems (non-empty list); implement using lib.all with a predicate like m.system
!= null || m.systems != [] and provide a clear message such as "Each entry in
determinateNix.buildMachines must specify either `system` or `systems`." to fail
early on misconfiguration.
🧹 Nitpick comments (1)
modules/nix-darwin/default.nix (1)
34-42: Clarify the relationship between disallowed options and module-managed configuration.The
external-buildersoption is disallowed incustomSettings, yet this module generates/etc/nix/machines(the external builders configuration). While this is likely intentional—Determinate Nix manages builders automatically viabuildMachines—the relationship could be clearer to prevent user confusion.📝 Consider enhancing the documentation
Add a comment explaining why these options are disallowed:
# Settings that Determinate Nix handles for you +# Note: external-builders is managed via determinateNix.buildMachines +# and other options are automatically configured by Determinate Nixd disallowedOptions = [ "bash-prompt-prefix" "external-builders"
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
modules/nix-darwin/default.nix
🧰 Additional context used
🪛 Gitleaks (8.30.0)
modules/nix-darwin/default.nix
[high] 686-686: 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 (10)
modules/nix-darwin/default.nix (10)
51-72: LGTM!The option renaming is properly handled with
mkRenamedOptionModulefor backwards compatibility, and the documentation clearly explains the module's behavior regarding configuration management.
220-282: LGTM!The Determinate Nixd configuration options are well-structured with appropriate types, sensible defaults (8GB memory, 1 CPU), and clear security documentation about netrc file paths.
303-466: LGTM! Well-structured with appropriate lazy evaluation.The NixOS VM-based Linux builder configuration is comprehensive and well-documented. The defaults for
maxJobs(line 375) andsystems(line 442) correctly reference the finalizedpackage.nixosConfig, which works due to Nix's lazy evaluation. The distinction from Determinate Nix's native Linux builder is clearly communicated.
468-545: LGTM!The flake registry configuration is well-designed. The automatic transformation of
flakeattributes to path-based references with metadata filtering (line 531) is correct, and the documentation appropriately recommends using the registry for CLI commands rather than flake references in code.
547-614: LGTM!The custom settings configuration appropriately uses a freeform type for extensibility while defining specific common options with proper types and documentation. The
cores = 0special value and sandbox behavior are clearly documented.
686-686: Static analysis false positive: This is a public SSH host key, not a secret.The static analysis tool flagged this as a "Generic API Key," but this is actually a base64-encoded public SSH host key for the NixOS VM builder. Public SSH host keys are not sensitive and are meant to be shared for host verification. This matches the
publicHostKeyfield definition at lines 197-205.
621-702: LGTM! Robust VM builder integration.The NixOS VM-based Linux builder configuration is well-implemented:
- Proper assertion ensuring
determinateNix.enableis set (line 624)- Sophisticated TMPDIR handling to prevent macOS auto-cleanup (lines 646-650)
- Correct ephemeral mode support (lines 651-653)
- Proper SSH configuration and identity file wiring (lines 670-677)
- Correctly disables native Linux builder when VM builder is enabled (line 698)
705-717: LGTM! Good defensive checks.The assertions properly validate that disallowed options aren't set in
customSettings, and the warning helpfully alerts users whenbuildMachinesis configured butdistributedBuildsisn't enabled.
720-720: No breaking dependencies detected with nix.enable force-disable.The search found no nix-darwin modules that depend on
nix.*options being enabled. Theconfig/config.nixfile is a stateless utility for generating Nix configuration, andmigration.nixalready handles and expectsnix.enable = false. The force-disable is intentional and documented (line 64) for exclusive Determinate Nix control, with no adverse impact on other nix-darwin modules.
788-822: The machines file format is correct and matches Nix's build-remote.pl specification.The code generates all 8 required fields in the correct order: remote store URI, system types, SSH key, max jobs, speed factor, supported features, mandatory features, and public host key. The developers have already documented awareness of the format requirement (line 786-787).
Minor refinement: In the mandatory features section, use the local
resvariable in the concatenation instead ofmachine.mandatoryFeaturesto be consistent with how supported features are handled above.
| # Only include non-null attributes in the config file | ||
| explicitlySetAttrs = | ||
| let | ||
| fragmentFor = | ||
| path: | ||
| let | ||
| v = lib.getAttrFromPath path dnixd; | ||
| in | ||
| lib.optionalAttrs (v != null) (lib.setAttrByPath path v); | ||
| in | ||
| builtins.foldl' lib.recursiveUpdate { } ( | ||
| # Keep this list up to date with the structure of the Determinate Nixd config file | ||
| map fragmentFor [ | ||
| [ | ||
| "authentication" | ||
| "additionalNetrcSources" | ||
| ] | ||
| [ | ||
| "builder" | ||
| "state" | ||
| ] | ||
| [ | ||
| "builder" | ||
| "memoryBytes" | ||
| ] | ||
| [ | ||
| "builder" | ||
| "cpuCount" | ||
| ] | ||
| [ | ||
| "garbageCollector" | ||
| "strategy" | ||
| ] | ||
| ] | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Refactor: The manual path list maintenance is fragile and error-prone.
The explicitlySetAttrs implementation requires manually maintaining a hardcoded list of paths (lines 758-779) that must stay synchronized with the determinateNixd option structure. If new configuration options are added to determinateNixd without updating this list, they will be silently ignored in the generated config file, leading to subtle bugs.
♻️ Consider a more maintainable approach
Instead of manually listing paths, you could recursively filter out null values:
explicitlySetAttrs =
let
removeNulls = lib.filterAttrsRecursive (n: v: v != null);
in
removeNulls cfg.determinateNixd;This automatically handles any new options added to the determinateNixd structure without code changes. However, verify this approach works correctly with the expected JSON schema for Determinate Nixd's config file.
colemickens
left a comment
colemickens
left a comment
There was a problem hiding this comment.
Tested these instructions: https://gist.github.com/lucperkins/3756e86858ae33ab8087d61d7b61ce6e
With: bf20d5a of determinate (this PR).
It seems to be working just great!
lucperkins
deleted the
lucperkins/cf-170-improve-the-nix-darwin-solution-for-better-user-experience
branch
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
README.md (1)
69-69: Minor style improvement: remove redundant phrasing.The phrase "macOS system" is slightly redundant since "macOS" already implies operating system. Consider simplifying to just "macOS".
✨ Suggested fix
-If you use [nix-darwin] to provide Nix-based configuration for your macOS system, you need to disable nix-darwin's built-in Nix configuration mechanisms by applying the `determinate` nix-darwin module and setting `determinateNix.enable = true`; if not, Determinate Nix **does not work properly**. +If you use [nix-darwin] to provide Nix-based configuration for macOS, you need to disable nix-darwin's built-in Nix configuration mechanisms by applying the `determinate` nix-darwin module and setting `determinateNix.enable = true`; if not, Determinate Nix **does not work properly**.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md
🧰 Additional context used
🪛 LanguageTool
README.md
[style] ~69-~69: This phrase is redundant (‘OS’ stands for ‘operating system’). Use simply “macOS”.
Context: ...rovide Nix-based configuration for your macOS system, you need to disable nix-darwin's built...
(ACRONYM_TAUTOLOGY)
⏰ 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 (4)
README.md (4)
50-65: Clear documentation of consolidated inputs pattern for NixOS.The NixOS example properly demonstrates the new consolidated inputs structure with
inputs.determinate.nixosModules.default. This aligns well with the PR objective of streamlining the flake configuration.
74-102: Well-structured nix-darwin example with clear requirements.The example clearly shows the new requirement to apply
inputs.determinate.darwinModules.defaultand setdeterminateNix.enable = trueto disable nix-darwin's built-in Nix configuration. The consolidated inputs pattern is consistent with the NixOS example.
109-143: Good documentation of customSettings usage.The example demonstrates how to use the
customSettingsattribute to provide nix configuration options (flake-registry, sandbox) that are written to/etc/nix/nix.custom.conf. This clearly communicates the new capability of accepting nix-darwin-ified configuration directly instead of requiring conversion.
146-162: New determinateNixd configuration section is comprehensive.The documentation introduces the
determinateNixdattribute for configuring Determinate Nixd via nix-darwin, with concrete examples showinggarbageCollector.strategyandauthentication.additionalNetrcSources. This aligns with PR objective #4 and the link todnixd-configdocumentation is properly defined at line 170.
|
holy shit it merged |
4 participants
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
✏️ Tip: You can customize this high-level summary in your review settings.