Thanks. Looks good.

Considering that the single-file-output error is a potential way to enforce the correctness of meta.outputsToInstall, we may want to preserve such behavior while proving options to ignore those single-file outputs.

I prepared the implementation to support nixpkgs-review --ignore-single-file-outputs locally. I could push it up if we agree that this should be supported through a flag.

@Mic92, what do you think?

@corngood Thank you for rewiewing!

Do you think it's preferred to have ignoreSingleFileOutputs = true by default (the current implementation), or provide a flag --ignore-single-file-outputs to opt in explicitly?

Do you think it's preferred to have ignoreSingleFileOutputs = true by default (the current implementation), or provide a flag --ignore-single-file-outputs to opt in explicitly?

My gut feeling is that it should be a default, because people often run nixpkgs-review without knowledge of what's actually going to be built, and there are a bunch of things in nixpkgs that will break if they do happen to be built as part of a review. For example, I hit this on every update to dotnet-sdk, because of things in tests.dotnet.

However, you did mention this:

Considering that the single-file-output error is a potential way to enforce the correctness of meta.outputsToInstall

and I don't really understand what you mean by it. Maybe you can give an example?

However, you did mention this:

Considering that the single-file-output error is a potential way to enforce the correctness of meta.outputsToInstall

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'.

but lately I'm getting a lot more file conflicts due to "test.*" being included in attrs.nix

Is it the default behavior now? I thought it would be opt-in.

Is it the default behavior now? I thought it would be opt-in.

It's the top level tests attribute, not e.g. passthru.tests. It seems to be something to do with the github eval. See this review run:

NixOS/nixpkgs#389338 (comment)

• tests.dotnet.final-attrs.check-output
• tests.dotnet.final-attrs.output-matches-const
• tests.dotnet.final-attrs.override-has-no-effect

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 tests.

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 tests.

I looked into this a bit, and I think it's just the difference between local eval using nix-env -qaP and the eval action using release-attrpaths-superset.nix.

I didn't realise they were so drastically different (essentially opt-in vs opt-out).