shellFor with callCabal2nix puts all the package's src into the store instead of just package.yaml

[expipiplus1]

Describe the bug

haskellPackages.shellFor's dependency on 'input.outPath' causes the whole
source to be put into the store, when just package.yaml would do for
callCabal2Nix.

To Reproduce
Steps to reproduce the behavior:

1. Enter a nix-shell for a package whose description is built using
   callCabal2nix (https://github.com/expipiplus1/vulkan for example)

2. Observe that the whole directory is put into the store, if you're lucky
   you'll get a message 'warning: dumping very large path (> 256 MiB); this may run out of memory'

3. Change nixpkgs thusly

   diff --git a/pkgs/development/haskell-modules/make-package-set.nix b/pkgs/development/haskell-modules/make-package-set.nix
   index 9ba25e09db9..1e7c7d6dfff 100644
   --- a/pkgs/development/haskell-modules/make-package-set.nix
   +++ b/pkgs/development/haskell-modules/make-package-set.nix
   @@ -298,8 +298,7 @@ in package-set { inherit pkgs stdenv callPackage; } self // {
                # If `packages = [ a b ]` and `a` depends on `b`, don't build `b`,
                # because cabal will end up ignoring that built version, assuming
                # new-style commands.
   -            combinedPackages = pkgs.lib.filter
   -              (input: pkgs.lib.all (p: input.outPath or null != p.outPath) selected);
   +            combinedPackages = x: x;

4. Enter the shell again, observing that only 'package.yaml' is put into the store

Expected behavior

I would expect that entering a shell for a package wouldn't need to put all the source (sometimes including build artifacts and all of .git if there's no filter) into the store.

Notify maintainers

• @jmininger who I think made this change.
• @peti for being a Haskell/Nix wiz

[Ericson2314]

@expipiplus1 This is an eval-time add-to-store that's the problem, right? It seems hard to see how the derivation reference more stuff when the reference removed, right?

[cdepillabout]

@expipiplus1 Would you be able to come up with a minimal reproducible example of this?

---

@Ericson2314 I'm having a little trouble understanding what you're asking.

This is an eval-time add-to-store that's the problem, right?

From reading @expipiplus1's issue, it sounds to me like the problem is that the entire contents of the source directory is added to /nix/store, instead of just the .cabal file. (Although without a minimal reproducible example, it is hard to say exactly what @expipiplus1 is asking.)

It doesn't sound to me that being eval-time vs. build-time is particularly the problem, just that the entire source directory was added to the store instead of just the .cabal file.

It seems hard to see how the derivation reference more stuff when the reference removed, right?

Are you referring to these lines in the diff posted above?

-             combinedPackages = pkgs.lib.filter
-              (input: pkgs.lib.all (p: input.outPath or null != p.outPath) selected);
+            combinedPackages = x: x;

This code is pretty complicated, and I'm not that familiar with it so I am hesitant to comment on it, but couldn't the old version be evaluating less stuff, depending on how combinedPackages is evaluated?

The new version of combinedPackages (x: x) effectively doesn't evaluate anything. It just passes the input to the output.

The old version of combinedPackages evaluates outPath from everything in selected, as well as outPath from everything passed to combinedPackages.

While the new version (x: x) is referencing the same amount of things, it evaluates less. My guess is that this is the cause of the issue @expipiplus1 is posting about (although without looking into the whole code for shellFor, it is difficult to see exactly what is going on).

[expipiplus1]

Here's a repro case: https://github.com/expipiplus1/nix-repro-86775

[maralorn]

The simplification suggested looks very suspicious to me. Like it is an improvement in this special use-case but breaks a lot of others.

Maybe there are cases when we cannot be sure that no other files from the repo are needed to build the package?

[expipiplus1]

Ah, I certainly wasn't proposing that change as the correct solution! It was merely to demonstrate that the undesirable behaviour is caused (at least in part) by that function.

[jmininger]

Oh i see. If you change the last line of the shell.nix in your repro repo to just in drv you get the same issue. But that doesn't necessarily mean that shellFor needs to evaluate that part.

[expipiplus1]

Perhaps something simple like an additional argument to callCabal2nix to turn off this behavior would be a good stopgap

[jmininger]

But callCabal2nix requires the src, which means that the src has to get checked in to the nix-store, no?

[expipiplus1]

@jmininger it should only require package.yaml (or foo.cabal)

[jmininger]

@expipiplus1 First of all, does something like this work as a work around?

let myPkgNoSrc = lib.attrsets.filterAttrs (n: _: n != "src") myPackage;
in shellFor { packages = p: [ myPkgNoSrc ]; }

Second, when you refer to "it", do you mean the shellFor function, or the callCabal2nix function? The shellFor function is based on the packages that get passed in.

[expipiplus1]

Second, when you refer to "it", do you mean the shellFor function, or the callCabal2nix function? The shellFor function is based on the packages that get passed in.

shellFor, sorry I wasn't very clear.

@expipiplus1 First of all, does something like this work as a work around?

let myPkgNoSrc = lib.attrsets.filterAttrs (n: _: n != "src") myPackage;
in shellFor { packages = p: [ myPkgNoSrc ]; }

@jmininger I think so, however I like to use just one default.nix file for shells and building, so I'd have to toggle that behavior with isInNixShell (or whatever that is). It also gets complicated when one already has a source filter (I don't think they compose, but I could be wrong on this).

The crux of this issue is that it's wrong for shellFor to depend on anything but the package.yamls (or package-name.cabal) as these are the only files which could possibly influence the output. This misstating of dependencies can sometimes carry a substantial performance penalty moving gigabytes into the store.

I think the correct solution would be to restate combinedPackages to avoid using outPath (which is what pulls in the dependency on src).

[Ericson2314]

Hmm what if as a hack we get rid of the special case if the list only contains one element?

The intent of the filtering is so one can make a sell for multiple packages where one is a dependency of the other, without building any of them, but this problem doesn't arise when there is only 1 package.

[expipiplus1]

@Ericson2314 That's a good idea. I wonder if it's possible to compare packages on the name rather than the outpath.

[expipiplus1]

I've updated my reproducer: https://github.com/expipiplus1/nix-repro-86775

[expipiplus1]

I suspect that this could be fixed by making this line compare name (and version) instead of outpath: https://github.com/nixos/nixpkgs/blob/6586a9cdbe2db167bd36f3cc2df0edff1df43f25/pkgs/development/haskell-modules/make-package-set.nix#L366

It wouldn't be quite the same, but does it make any difference?

[stale]

I marked this as stale due to inactivity. → More info

[expipiplus1]

Still important

…

On Fri, 4 Jun 2021, 11:05 pm stale[bot], ***@***.***> wrote: I marked this as stale due to inactivity. → More info <https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#86775 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGRJXDILMOEA7L4ZJ27FB3TRDTUDANCNFSM4MYX63QQ> .