nix/review_shell.nix: use ignoreSingleFileOutputs whenever available#427
nix/review_shell.nix: use ignoreSingleFileOutputs whenever available#427Mic92 merged 1 commit intoMic92:masterfrom
Conversation
ShamrockLee
force-pushed
the
ignore-file-outputs
branch
from
631c8aa to
0f4ff8d
Compare
|
Thanks. Looks good. |
|
Considering that the single-file-output error is a potential way to enforce the correctness of I prepared the implementation to support @Mic92, what do you think? |
ShamrockLee
force-pushed
the
ignore-file-outputs
branch
from
0f4ff8d to
ec9467b
Compare
corngood
left a comment
corngood
left a comment
There was a problem hiding this comment.
I'm not sure if it's something to do with the change in evaluation methods, but lately I'm getting a lot more file conflicts due to "test.*" being included in attrs.nix, so it would be nice to get this in.
nixpkgs_review/nix/review-shell.nix
Outdated
| ignoreCollisions = true; | ||
| }; | ||
| env = | ||
| local-pkgs.buildEnv { |
There was a problem hiding this comment.
I think this needs parens around the attrset update, so ignoreSingleFileOutputs makes it into buildEnv.
It does seem to work okay once that's changed.
|
@corngood Thank you for rewiewing! Do you think it's preferred to have |
My gut feeling is that it should be a default, because people often run However, you did mention this:
and I don't really understand what you mean by it. Maybe you can give an example? |
$ nix build --impure --expr "let pkgs = import ./. { }; in pkgs.buildEnv { name = ''empty-file-env''; paths = [ (pkgs.emptyFile.overrideAttrs (previousAttrs: { meta = previousAttrs.meta or { } // { outputsToInstall = [ ]; }; })) ]; }"
$ nix build --impure --expr "let pkgs = import ./. { }; in pkgs.buildEnv { name = ''empty-file-env''; paths = [ (pkgs.emptyFile.overrideAttrs (previousAttrs: { meta = removeAttrs previousAttrs.meta or { } [ ''outputsToInstall'' ]; })) ]; }"
error: builder for '/nix/store/2g6w6l2gc2inj96gji5fnaycyjc15rfb-empty-file-env.drv' failed with exit code 255;
last 1 log lines:
> error: The store path /nix/store/ij3gw72f4n5z4dz6nnzl1731p9kmjbwr-empty-file is a file and can't be merged into an environment using pkgs.buildEnv! at /nix/store/qrwjx5798fsp5yxaawrf5aklh1hp2xba-builder.pl line 115.
For full logs, run 'nix log /nix/store/2g6w6l2gc2inj96gji5fnaycyjc15rfb-empty-file-env.drv'. |
Is it the default behavior now? I thought it would be opt-in. |
It's the top level NixOS/nixpkgs#389338 (comment)
etc That one was done with github eval. Below that I posted a darwin review, which used local eval, and it didn't build anything in |
ShamrockLee
changed the title
I looked into this a bit, and I think it's just the difference between local eval using I didn't realise they were so drastically different (essentially opt-in vs opt-out). |
|
@ShamrockLee I've been running this locally (with the params change I mentioned above) for quite a while. What do you think about getting this in? I can make my own PR if you prefer. |
|
Thank you for pinging. This PR slipped under my radar. Considering the default behavior of nixpkgs-review to evaluate I'm adding |
ShamrockLee
force-pushed
the
ignore-file-outputs
branch
2 times, most recently
from
2a78291 to
f186900
Compare
|
@corngood I made |
|
This is enough to reproduce it when run from nixpkgs/master: I needed to get it to find more than 50 attributes, due to With <= 50 attributes it works even with single files when they are passed directly into I tried running this PR branch: But it results in some test failures: |
To clarify, this flag would only have an effect if there are more than 50 attributes in attrs.nix, which would be very confusing for users. Just setting |
ShamrockLee
force-pushed
the
ignore-file-outputs
branch
from
f186900 to
8cc21f6
Compare
ShamrockLee
force-pushed
the
ignore-file-outputs
branch
from
8cc21f6 to
a57be21
Compare
Thank you for finding out this isssue. I fixed the condition by using |
I'm a bit worry about the use of |
ShamrockLee
force-pushed
the
ignore-file-outputs
branch
from
a57be21 to
4ca425c
Compare
|
Just updated to call |
ShamrockLee
force-pushed
the
ignore-file-outputs
branch
from
4ca425c to
4e75f00
Compare
ShamrockLee
force-pushed
the
ignore-file-outputs
branch
from
4e75f00 to
7683bcb
Compare
|
I would like to see this PR merged because it would help me with such PRs as: How can I help get this to mergeable status? |
|
Personally I think we should just set
However the old behaviour was dependent on the number of packages being built relative to that undocumented magic threshold (50), so nobody could really depend on it anyway. At least that's how I understand it. @ShamrockLee what do you think about that? If you'd prefer I can make a separate PR with what I'm suggesting, but it's really just going back to how this PR started. |
|
I changed my mind. It's so hard to get the command-line flag to pass the tests. I'll reset the feature branch to specify |
ShamrockLee
force-pushed
the
ignore-file-outputs
branch
from
7683bcb to
222218a
Compare
WalkthroughRefactors buildEnv usage to conditionally add ignoreSingleFileOutputs when supported, expands mkShell invocation with preferLocalBuild, allowSubstitutes, dontWrapQtApps, and changes packages to use a single Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant Nix as Nix Evaluator
participant Lib as lib
participant Pkgs as local-pkgs
Caller->>Nix: import review-shell.nix (attrs)
Nix->>Lib: check (lib.functionArgs local-pkgs.buildEnv) ? ignoreSingleFileOutputs
alt supports ignoreSingleFileOutputs
Nix->>Pkgs: buildEnv({name, paths=attrs, ignoreCollisions} // {ignoreSingleFileOutputs = true})
else
Nix->>Pkgs: buildEnv({name, paths=attrs, ignoreCollisions})
end
Note over Nix,Pkgs: env constructed
Nix->>Nix: packages = if length(attrs) > 50 then [env] else attrs
Nix->>Caller: mkShell{ preferLocalBuild, allowSubstitutes, dontWrapQtApps, packages }
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
ShamrockLee
force-pushed
the
ignore-file-outputs
branch
from
222218a to
4a6e0eb
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
nixpkgs_review/nix/review-shell.nix (2)
29-38: Tiny readability tweak: bind the predicate onceBind the support check in a let to avoid re-reading functionArgs and improve clarity.
-env = local-pkgs.buildEnv ( - { - name = "env"; - paths = attrs; - ignoreCollisions = true; - } - // lib.optionalAttrs ((lib.functionArgs local-pkgs.buildEnv) ? ignoreSingleFileOutputs) { - ignoreSingleFileOutputs = true; - } -); +env = local-pkgs.buildEnv (let supportsIgnoreSingleFileOutputs = + ((lib.functionArgs local-pkgs.buildEnv) ? ignoreSingleFileOutputs); +in + { + name = "env"; + paths = attrs; + ignoreCollisions = true; + } + // lib.optionalAttrs supportsIgnoreSingleFileOutputs { + ignoreSingleFileOutputs = true; + } +);
40-46: Use local-pkgs.mkShell to guarantee consistent system/config and avoid re-importThis avoids a second nixpkgs import and ensures mkShell matches local-system and the provided config.
-(import nixpkgs-path { }).mkShell { +local-pkgs.mkShell {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
nixpkgs_review/nix/review-shell.nix(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
nixpkgs_review/nix/review-shell.nix (2)
nixpkgs_review/nix.py (3)
build_shell_file_args(370-400)nix_build(303-367)nix_shell(60-95)nixpkgs_review/buildenv.py (1)
Buildenv(23-76)
🔇 Additional comments (1)
nixpkgs_review/nix/review-shell.nix (1)
29-38: ignoreSingleFileOutputs gating is correct; parens fix the pass-through bugMerging the attrsets inside parentheses ensures the flag actually reaches buildEnv; the functionArgs check keeps this backward-compatible. This addresses the earlier review note about missing parens.
ShamrockLee
force-pushed
the
ignore-file-outputs
branch
from
4a6e0eb to
2822be3
Compare
ShamrockLee
force-pushed
the
ignore-file-outputs
branch
from
2822be3 to
e757223
Compare
4 participants
This PR depends on NixOS/nixpkgs#353752 and fixes #408 partially.
Cc: @corngood
Summary by CodeRabbit