Nixpkgs: Haskell builds do not always build/source test dependencies

[jkachmar]

Describe the bug
The Haskell build framework does not appear to appropriately source Haskell packages in the nix-shell environment when doCheck = false for the entire Haskell package set _except_ a single packages (in this case, the package being developed.

See this repository for a reproduction; specific areas of interest are as follows:

• the mkDerivation override disabling tests
• the overlay that re-enables tests for the package-under-test

To Reproduce
Steps to reproduce the behavior:

1. Clone this repository
2. Invoke nix-shell
3. Observe the following:
   
   
   
   • hspec is neither built by Nix, nor included in the packages displayed by the shellHook call to ghc-pkg list
   
   
   • calling cabal test from within the shell will attempt to download dependencies from Hackage
   
   
4. Additionally, one can apply the following patch and observe the following:
   
   
   
   • hspec _is_ built by Nix however it is still _not_ displayed by the shellHook call to ghc-pkg list
   
   
   • calling cabal test still attempts to download the package from Hackage, even though we observed that it was compiled by Nix
   
   

patch

diff --git a/shell.nix b/shell.nix
index 31d485e..eaf8bf4 100644
--- a/shell.nix
+++ b/shell.nix
@@ -1,11 +1,10 @@
 let
-  inherit (import ./default.nix {}) haskellPkgs;
+  inherit (import ./default.nix {}) haskellPkgs pkgs;
 in

-haskellPkgs.shellFor {
-  packages = p: [ p.repro ];
-  nativeBuildInputs = [ haskellPkgs.cabal-install ];
-  shellHook = ''
-    ghc-pkg list
-  '';
+pkgs.mkShell {
+  buildInputs = [
+    (haskellPkgs.ghcWithPackages (p: [p.repro]))
+    pkgs.cabal-install
+  ];
 }

Expected behavior
Nix's Haskell build framework should support the following behavior:

• disabling all tests in a package set _while also_ selectively enabling tests for a subset of packages as desired
• appropriately sourcing the transitive dependency tree of all enabled components in a Haskell project within the nix-shell environment's GHC pkgdb

Notify maintainers

ping @cdepillabout, as we spoke about this on Slack.

Metadata

 - system: `"x86_64-darwin"`
 - host os: `Darwin 19.6.0, macOS 10.15.7`
 - multi-user?: `yes`
 - sandbox: `no`
 - version: `nix-env (Nix) 2.4pre20201102_550e11f`
 - channels(jkachmar): `"darwin"`
 - channels(root): `"nixpkgs-21.03pre246624.cfed29bfcb2"`
 - nixpkgs: `/nix/store/s18y38xsw017icfnskgasfxy437ms267-13jxji98gdq08mzcxsk9gvm70cd5fkzr-darwin-stable-src`

NOTE: My personal Nix configuration is fairly involved; at the time of writing I've tested this with:

• this nixpkgs-20.09 checkout
• this nixpkgs-unstable checkout

[cdepillabout]

cc @expipiplus1, who I think touched the shellFor stuff recently and may be able to spot the bug sooner than I'll be able to take a good look at this.

[jkachmar]

I tried an earlier nixpkgs commit (c565d7cc16f5409304b5f3815115d373e3632811, which was some arbitrary 20.03 release from February) along with GHC 8.6.5 (since that's what was in-tree at the time).

ghc-pkg list looks like it still doesn't display the transitive test dependencies and cabal test still tries to download from Hackage, so maybe I'm just misunderstanding how this is all meant to work together.

[expipiplus1]

cc @expipiplus1, who I think touched the shellFor stuff recently and may...

Heart racing

---

Basically the culprit is this line https://github.com/NixOS/nixpkgs/blob/afe87caf9aa94b0fdc61ccd2c31e201afcceb9c3/pkgs/development/haskell-modules/make-package-set.nix#L442

This uses the mkDerivation which you've overridden here: https://github.com/jkachmar/haskell-nix-repro/blob/fbae41b936f9e789bc38783e05dbbcff2dcebe93/default.nix#L27 which forces the tests to be disabled, in this case for the fake package which shellFor has constructed.

[expipiplus1]

A potential fix is having something which respects flags which have been explicitly set in your mkDerivation override, for example flipping the arguments to // :

          args: hprev.mkDerivation ({
            doCheck = false;
            doBenchmark = false;
            doHoogle = false;
            doHaddock = false;
            enableLibraryProfiling = false;
            enableExecutableProfiling = false;
          } // args);

and setting doCheck = true; in the call to shellFor in shell.nix. This is pretty nasty though because the hlib.doCheck is still being ignored. Not to mention that this doesn't stop anyone else from bumping into this.

I think it's quite safe to always pass doCheck to mkDerivation in make-package-set.nix as test dependencies won't make it into cabalDepsForSelected if that package has had tests disabled.

I've not tested this or anything so it could all be wrong!

[expipiplus1]

Looking at similar code for doBenchmark (added by @cdepillabout) doBenchmark has to be specifically requested by the user.

Why isn't doBenchmark always turned on like I suggest doCheck is above, would it make any difference to always enable it?

[cdepillabout]

I sent a PR with an example of getting this to work: https://github.com/jkachmar/haskell-nix-repro/pull/1. I left some commentary there.

[cdepillabout]

@expipiplus1 Thanks for the suggestion with the order of overriding args. That turned out to make the difference. I agree this is pretty tricky.

Why isn't doBenchmark always turned on like I suggest doCheck is above, would it make any difference to always enable it?

This is a good question.

I think it depends on how we want shellFor to be used. If we want shellFor to be able to explicitly ignore deps for benchmarks and tests, then we really need shellFor to have the option to disable/enable doBenchmark and doCheck.

If we want shellFor to be dumb and just take all the transitive deps of its packages, then we could just always set doCheck and doBenchmark to true. But in this case, the user would always have to do something like this mkDerivation override to control whether tests and benchmarks are enabled for all the packages in the Haskell package set.

I don't think there is a "correct" answer here, we just need to think about what would be easier for the most end-users.

[expipiplus1]

But the test dependencies (same for bench depends) are included in the arguments to mkDerivation iff doCheck (or doBench) is true: https://github.com/nixos/nixpkgs/blob/4718ba1ab10d109e2383854c5c56c2c2340c00c1/pkgs/development/haskell-modules/generic-builder.nix#L538-L546

I do see the rationale for disabling this though, but I think there's no harm in enabling these both by default as they do nothing when nothing in the package set requests test or bench depends.

This of course ignores other effects of doCheck and doBench, but I don't think any of those (like enabling the checkPhase) affect the env derivation.

[cdepillabout]

@expipiplus1 I'm sorry, I think we might be talking past each other.

In my previous post (https://github.com/NixOS/nixpkgs/issues/104349#issuecomment-730958996), I was trying to explain our two choices between providing arguments to shellFor to disable/enable tests and benchmarks, or just always having shellFor enable tests and benchmarks on the virtual Haskell package it creates.

I do see the rationale for disabling this though, but I think there's no harm in enabling these both by default as they do nothing when nothing in the package set requests test or bench depends.

I think you're talking here about the following: Assuming we do provide arguments to shellFor to disable/enable tests and benchmarks (currently we only have the argument for benchmarks), do we let the default value be true or false?

I agree with you that defaulting them both to true make sense, for the reasons you give.

I defaulted doBenchmarks to false so that the hash wouldn't change for anyone currently using shellFor. However, I don't feel strongly about this. We could change it to true. Ideally we would also add a Release Note about this.

[expipiplus1]

I agree with that all! Sorry for the communication mishap!

I defaulted doBenchmarks to false so that the hash wouldn't change for anyone currently using shellFor

I had not considered this, makes sense!

[expipiplus1]

defaulting these to true does feel slightly weird because: the action we want to tell mkDerivation isn't "enable the tests and benchmarks" but instead "make sure you include every dependency I give you". Setting doCheck and doBench to true accomplishes this, but only because of the indirect action.

Perhaps an alternative would be to, after zipping the attrs, just pile all the dependencies together in buildDepends? (Again it's not quite the right thing to do though, but maybe a little better?)

[jkachmar]

First off thanks for getting around to this so quickly, and I appreciate all of the elucidation in this thread (and elsewhere)!

Having slept on it, I think what I _really_ want (and what might be more generally useful) is closer to what @expipiplus1 is saying (https://github.com/NixOS/nixpkgs/issues/104349#issuecomment-731154475).

Specifically, it seems like there should be some attributes for the fake package that's used to construct the shell environment which allow us to include these sorts of other dependencies irrespective to doCheck (etc.).

---

It seems reasonable that local developers would want Nix to manage all of their dependencies (i.e. cabal test, cabal bench, and cabal run shouldn't have to access Hackage at all).

However it _also_ seems reasonable that the compilation/inclusion of these dependencies in the nix-shell's exposed pkgdb isn't coupled with whether or not the test/bench suite is actually run when the environment is constructed. It's often very useful to drop into a cabal repl (or even plain ghci) session to muck around with some dependencies that are used within the project, even if I don't intend to build it at all.

---

The counterargument to the behavior described above is that changing mkDerivation is cache-invalidating, and I imagine most people are either pulling from the Nix binary cache or some hosted build cache fed from a CI process (which can probably run test/benchmark suites without issue).

Does that make sense, and do you know if there's a way that the Haskell build framework could be updated to support this kind of exposure?

[expipiplus1]

First off thanks for getting around to this so quickly, and I appreciate all of the elucidation in this thread (and elsewhere)!

Make sure to pass it on down the line :+1:

---

This flag passed to genericBuilder could also disable the building phases, as this derivation is only useful for an interactive shell, and the phases don't make much sense (especially with multiple packages in packages)