Read missing attributes for static Darwin#452
Open
RossComputerGuy wants to merge 1 commit intomainfrom
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors the per-system package attribute generation in ChangesPackage Attribute Matrix Refactoring
Boehm GC Compile Flags
CI Static Build Targets
Unix Syscall Header Include
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
cole-h
previously approved these changes
May 7, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
flake.nix (1)
464-483:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
linuxOnlyguard no longer protects stdenv variants —nix-nswrapperstdenv attrs will be exposed on Darwin.By placing
flatMapAttrs (lib.genAttrs stdenvs …)(line 471) outsidelib.optionalAttrs (linuxOnly → …), every${pkgName}-${stdenvName}and${pkgName}-${stdenvName}-staticentry is now unconditionally produced for every system, bypassing thelinuxOnlygate.For
nix-nswrapper(linuxOnly = true) onaarch64-darwin:
true → falseevaluates tofalse, solib.optionalAttrs false …correctly returns{}for the base and-staticentries (lines 467–469).- But the
flatMapAttrs stdenvsblock at line 471 is no longer inside that guard, sonix-nswrapper-clangStdenv,nix-nswrapper-gccStdenv,nix-nswrapper-clangStdenv-static, etc. are exposed inpackages.aarch64-darwin, and will fail to build (or failnix flake checkevaluation) on Darwin.The intended fix — adding the missing
${pkgName}-${stdenvName}-staticentries — is correct; it just needs to stay inside theoptionalAttrswrapper:🐛 Proposed fix: keep `flatMapAttrs stdenvs` inside `optionalAttrs (linuxOnly → …)`
lib.optionalAttrs (linuxOnly -> nixpkgsFor.${system}.native.stdenv.hostPlatform.isLinux) ( { # These attributes go right into `packages.<system>`. "${pkgName}" = nixpkgsFor.${system}.native.nixComponents2.${pkgName}; "${pkgName}-static" = nixpkgsFor.${system}.native.pkgsStatic.nixComponents2.${pkgName}; } + // flatMapAttrs (lib.genAttrs stdenvs (_: { })) ( + stdenvName: + { }: + { + # These attributes go right into `packages.<system>`. + "${pkgName}-${stdenvName}" = + nixpkgsFor.${system}.nativeForStdenv.${stdenvName}.nixComponents2.${pkgName}; + } + // lib.optionalAttrs supportsCross { + "${pkgName}-${stdenvName}-static" = + nixpkgsFor.${system}.nativeForStdenv.${stdenvName}.pkgsStatic.nixComponents2.${pkgName}; + } + ) ) - // flatMapAttrs (lib.genAttrs stdenvs (_: { })) ( - stdenvName: - { }: - { - # These attributes go right into `packages.<system>`. - "${pkgName}-${stdenvName}" = - nixpkgsFor.${system}.nativeForStdenv.${stdenvName}.nixComponents2.${pkgName}; - } - // lib.optionalAttrs supportsCross { - "${pkgName}-${stdenvName}-static" = - nixpkgsFor.${system}.nativeForStdenv.${stdenvName}.pkgsStatic.nixComponents2.${pkgName}; - } - )With this fix, non-
linuxOnlypackages still expose all stdenv and stdenv-static variants on Darwin (sincefalse → false = true), achieving the PR's goal while keepingnix-nswrapperproperly excluded.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@flake.nix` around lines 464 - 483, The stdenv variant generation (flatMapAttrs / lib.genAttrs over stdenvs producing "${pkgName}-${stdenvName}" and "${pkgName}-${stdenvName}-static") was taken outside the lib.optionalAttrs (linuxOnly -> ...) guard so linuxOnly packages like nix-nswrapper are exposed on Darwin; move the entire flatMapAttrs (lib.genAttrs stdenvs ...) block back inside the surrounding lib.optionalAttrs (linuxOnly -> nixpkgsFor.${system}.native.stdenv.hostPlatform.isLinux) so both the "${pkgName}-${stdenvName}" and corresponding "-static" attributes remain conditional on linuxOnly, keeping the original pkgName/stdenvName/static attribute generation but protected by the linuxOnly check.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@flake.nix`:
- Around line 464-483: The stdenv variant generation (flatMapAttrs /
lib.genAttrs over stdenvs producing "${pkgName}-${stdenvName}" and
"${pkgName}-${stdenvName}-static") was taken outside the lib.optionalAttrs
(linuxOnly -> ...) guard so linuxOnly packages like nix-nswrapper are exposed on
Darwin; move the entire flatMapAttrs (lib.genAttrs stdenvs ...) block back
inside the surrounding lib.optionalAttrs (linuxOnly ->
nixpkgsFor.${system}.native.stdenv.hostPlatform.isLinux) so both the
"${pkgName}-${stdenvName}" and corresponding "-static" attributes remain
conditional on linuxOnly, keeping the original pkgName/stdenvName/static
attribute generation but protected by the linuxOnly check.
RossComputerGuy
force-pushed
the
fix/missing-attributes
branch
from
74ef463 to
c7d2347
Compare
RossComputerGuy
force-pushed
the
fix/missing-attributes
branch
from
c7d2347 to
fbf5a6b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@flake.nix`:
- Around line 464-481: The linux-only guard currently wraps only the base
package attributes because lib.optionalAttrs (linuxOnly ->
nixpkgsFor.${system}.native.stdenv.hostPlatform.isLinux) closes its set early,
so the subsequent flatMapAttrs stdenvs block (producing
"${pkgName}-${stdenvName}" and "${pkgName}-${stdenvName}-static") is merged
unconditionally; update the lib.optionalAttrs invocation so its predicate covers
the entire per-package block (including the flatMapAttrs loop) — e.g., change
the guarded set literal to a guarded expression that encloses both the base
"${pkgName}" / "${pkgName}-static" entries and the flatMapAttrs stdenvs block so
nix-nswrapper and its stdenv variants are only emitted on Linux.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
There was a problem hiding this comment.
♻️ Duplicate comments (1)
flake.nix (1)
464-481:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
linuxOnlystill doesn’t guard stdenv-derived attributesOn Line 464, the guard closes at Line 468, but the
flatMapAttrsmerge on Line 469 is unconditional. That still exposes${pkgName}-${stdenvName}(and-static) forlinuxOnlypackages on Darwin.Proposed minimal fix
- lib.optionalAttrs (linuxOnly -> nixpkgsFor.${system}.native.stdenv.hostPlatform.isLinux) { - # These attributes go right into `packages.<system>`. - "${pkgName}" = nixpkgsFor.${system}.native.nixComponents2.${pkgName}; - "${pkgName}-static" = nixpkgsFor.${system}.native.pkgsStatic.nixComponents2.${pkgName}; - } - // flatMapAttrs (lib.genAttrs stdenvs (_: { })) ( - stdenvName: - { }: - { - # These attributes go right into `packages.<system>`. - "${pkgName}-${stdenvName}" = - nixpkgsFor.${system}.nativeForStdenv.${stdenvName}.nixComponents2.${pkgName}; - } - // lib.optionalAttrs supportsCross { - "${pkgName}-${stdenvName}-static" = - nixpkgsFor.${system}.nativeForStdenv.${stdenvName}.pkgsStatic.nixComponents2.${pkgName}; - } - ) + lib.optionalAttrs (linuxOnly -> nixpkgsFor.${system}.native.stdenv.hostPlatform.isLinux) ( + { + # These attributes go right into `packages.<system>`. + "${pkgName}" = nixpkgsFor.${system}.native.nixComponents2.${pkgName}; + "${pkgName}-static" = nixpkgsFor.${system}.native.pkgsStatic.nixComponents2.${pkgName}; + } + // flatMapAttrs (lib.genAttrs stdenvs (_: { })) ( + stdenvName: + { }: + { + # These attributes go right into `packages.<system>`. + "${pkgName}-${stdenvName}" = + nixpkgsFor.${system}.nativeForStdenv.${stdenvName}.nixComponents2.${pkgName}; + } + // lib.optionalAttrs supportsCross { + "${pkgName}-${stdenvName}-static" = + nixpkgsFor.${system}.nativeForStdenv.${stdenvName}.pkgsStatic.nixComponents2.${pkgName}; + } + ) + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@flake.nix` around lines 464 - 481, The stdenv-derived attributes produced by flatMapAttrs (the "${pkgName}-${stdenvName}" and "${pkgName}-${stdenvName}-static" entries generated via nixpkgsFor.${system}.nativeForStdenv and .pkgsStatic.nixComponents2) are not guarded by the linuxOnly check; move or wrap the flatMapAttrs block inside the same lib.optionalAttrs (linuxOnly -> ...) guard (or add an equivalent guard around the flatMapAttrs result) so those stdenv-derived attributes are only added when linuxOnly is true.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@flake.nix`:
- Around line 464-481: The stdenv-derived attributes produced by flatMapAttrs
(the "${pkgName}-${stdenvName}" and "${pkgName}-${stdenvName}-static" entries
generated via nixpkgsFor.${system}.nativeForStdenv and
.pkgsStatic.nixComponents2) are not guarded by the linuxOnly check; move or wrap
the flatMapAttrs block inside the same lib.optionalAttrs (linuxOnly -> ...)
guard (or add an equivalent guard around the flatMapAttrs result) so those
stdenv-derived attributes are only added when linuxOnly is true.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 447f7709-44bd-403f-a95f-c607a987e03f
📒 Files selected for processing (2)
.github/workflows/build.ymlflake.nix
cole-h
previously approved these changes
May 7, 2026
RossComputerGuy
force-pushed
the
fix/missing-attributes
branch
from
fbf5a6b to
37493f9
Compare
edolstra
reviewed
May 7, 2026
| - uses: DeterminateSystems/determinate-nix-action@main | ||
| - uses: DeterminateSystems/flakehub-cache-action@main | ||
| - run: nix build .#packages.${{ inputs.system }}.nix-cli-static --no-link -L | ||
| - run: nix build .#packages.${{ inputs.system }}.nix-cli-static .#packages.${{ inputs.system }}.nix-cli-libcxxStdenv-static --no-link -L |
Collaborator
There was a problem hiding this comment.
Why do we need 2 static builds? What's the difference?
Member
Author
There was a problem hiding this comment.
Darwin uses libc++, typically Linux uses libstdc++. FlakeBOM needs libc++ on Darwin.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/build.yml:
- Line 84: The workflow is failing because the build command always tries to
build .#packages.${{ inputs.system }}.nix-cli-libcxxStdenv-static (a Darwin-only
stdenv); change the step so it only requests the libcxxStdenv variant when the
target system actually exposes it (e.g., aarch64-darwin). Concretely, update the
run line that builds .#packages.${{ inputs.system }}.nix-cli-static
.#packages.${{ inputs.system }}.nix-cli-libcxxStdenv-static to either (a) build
only .#packages.${{ inputs.system }}.nix-cli-static for Linux systems, or (b)
dynamically assemble the attribute list using the same logic as the flake’s
fixup function (skip missing attributes) so the .nix-cli-libcxxStdenv-static
attribute is omitted on non-Darwin systems.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c8e2c292-cbfc-4b48-a343-bf0eb1d39938
📒 Files selected for processing (3)
.github/workflows/build.ymlflake.nixpackaging/dependencies.nix
✅ Files skipped from review due to trivial changes (1)
- flake.nix
RossComputerGuy
force-pushed
the
fix/missing-attributes
branch
from
37493f9 to
7a1f470
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
flake.nix (1)
464-481:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard scope is still too narrow for
linuxOnlypackages.Line 464 only guards the base attributes; Lines 469–481 still generate stdenv variants unconditionally, so
linuxOnlypackages can still appear on Darwin via${pkgName}-${stdenvName}(and-static).Suggested minimal fix
- lib.optionalAttrs (linuxOnly -> nixpkgsFor.${system}.native.stdenv.hostPlatform.isLinux) { - # These attributes go right into `packages.<system>`. - "${pkgName}" = nixpkgsFor.${system}.native.nixComponents2.${pkgName}; - "${pkgName}-static" = nixpkgsFor.${system}.native.pkgsStatic.nixComponents2.${pkgName}; - } - // flatMapAttrs (lib.genAttrs stdenvs (_: { })) ( - stdenvName: - { }: - { - # These attributes go right into `packages.<system>`. - "${pkgName}-${stdenvName}" = - nixpkgsFor.${system}.nativeForStdenv.${stdenvName}.nixComponents2.${pkgName}; - } - // lib.optionalAttrs supportsCross { - "${pkgName}-${stdenvName}-static" = - nixpkgsFor.${system}.nativeForStdenv.${stdenvName}.pkgsStatic.nixComponents2.${pkgName}; - } - ) + lib.optionalAttrs (linuxOnly -> nixpkgsFor.${system}.native.stdenv.hostPlatform.isLinux) ( + { + # These attributes go right into `packages.<system>`. + "${pkgName}" = nixpkgsFor.${system}.native.nixComponents2.${pkgName}; + "${pkgName}-static" = nixpkgsFor.${system}.native.pkgsStatic.nixComponents2.${pkgName}; + } + // flatMapAttrs (lib.genAttrs stdenvs (_: { })) ( + stdenvName: + { }: + { + # These attributes go right into `packages.<system>`. + "${pkgName}-${stdenvName}" = + nixpkgsFor.${system}.nativeForStdenv.${stdenvName}.nixComponents2.${pkgName}; + } + // lib.optionalAttrs supportsCross { + "${pkgName}-${stdenvName}-static" = + nixpkgsFor.${system}.nativeForStdenv.${stdenvName}.pkgsStatic.nixComponents2.${pkgName}; + } + ) + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@flake.nix` around lines 464 - 481, The linuxOnly guard currently only wraps the base attributes and not the stdenv variants, so stdenv-derived keys like "${pkgName}-${stdenvName}" and their "-static" counterparts are still emitted on non-Linux systems; fix this by moving or duplicating the lib.optionalAttrs (linuxOnly -> ...) guard to also cover the flatMapAttrs/ stdenv mapping block that produces "${pkgName}-${stdenvName}" and "${pkgName}-${stdenvName}-static" (i.e. wrap the flatMapAttrs (stdenvName: { }: { ... }) block or place an equivalent conditional inside it so nixpkgsFor.${system}.nativeForStdenv.${stdenvName} and .pkgsStatic variants are only created when linuxOnly is true).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@flake.nix`:
- Around line 464-481: The linuxOnly guard currently only wraps the base
attributes and not the stdenv variants, so stdenv-derived keys like
"${pkgName}-${stdenvName}" and their "-static" counterparts are still emitted on
non-Linux systems; fix this by moving or duplicating the lib.optionalAttrs
(linuxOnly -> ...) guard to also cover the flatMapAttrs/ stdenv mapping block
that produces "${pkgName}-${stdenvName}" and "${pkgName}-${stdenvName}-static"
(i.e. wrap the flatMapAttrs (stdenvName: { }: { ... }) block or place an
equivalent conditional inside it so
nixpkgsFor.${system}.nativeForStdenv.${stdenvName} and .pkgsStatic variants are
only created when linuxOnly is true).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5ca26225-47dd-4326-8ed5-4516dfe93747
📒 Files selected for processing (4)
.github/workflows/build.ymlflake.nixpackaging/dependencies.nixsrc/libutil/unix/file-descriptor.cc
✅ Files skipped from review due to trivial changes (1)
- src/libutil/unix/file-descriptor.cc
🚧 Files skipped from review as they are similar to previous changes (2)
- packaging/dependencies.nix
- .github/workflows/build.yml
edolstra
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Context
Summary by CodeRabbit