haskell generic-builder: Always use separate pkg db for custom setup #41939

Ericson2314 · 2018-06-13T17:53:25Z

Motivation for this change

This decreases complexity and ensures setup dependencies are properly specified with setup-depends as they should be. Testing will say if this is a reasonable change.

Things done

Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
Built on platform(s)
- NixOS
- macOS
- other Linux distributions
Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
Tested execution of all binary files (usually in ./result/bin/)
Fits CONTRIBUTING.md.

CC @ElvishJerricco

Ericson2314 · 2018-06-13T17:54:45Z

Second eval of https://hydra.nixos.org/jobset/nixpkgs/ericson2314-haskell-updates will test this.

matthewbauer · 2018-06-13T17:55:17Z

Let's wait on this for a little bit if that's okay. It looks like it could break things if we're not careful.

kirelagin · 2018-06-18T06:57:17Z

I came here to open a pull request with exactly the same changes as a proposed fix to #39646, so I’ll say instead that from my experience this works pretty well and I think this needs to be merged.

This decreases complexity and ensures setup dependencies are properly specified with `setup-depends` as they should be. Testing will say if this is a reasonable change.

Ericson2314 · 2018-06-18T18:12:51Z

My old eval had some mysterious failure on some GHCs. https://hydra.nixos.org/eval/1464372 is another attempt.

kirelagin · 2018-06-18T18:46:09Z

@Ericson2314 Oh, I saw something similar. Cabal clearly has missing setup depends, and some other packages might as well. See e.g. https://github.com/NixOS/nixpkgs/pull/39735/files#diff-8f392bd0e4fd42a66d9e0cd4901ec287R1055.

Ericson2314 · 2018-06-18T20:30:26Z

@kirelagin Thanks. I took the opportunity to rebase that PR of yours while I was at it.

kirelagin · 2018-06-18T22:04:26Z

Oh, cool, thank you!
(Your latest eval is starting to look good btw 🙂.)

Ericson2314 · 2018-06-19T02:25:04Z

Indeed with 600 builds left and no new fails. I hereby declare it good enough. Most people are asleep now anyways, and I have another thing to fix.

peti

It does not feel right to have that code in Nixpkgs. IMHO, it belongs into cabal2nix at https://github.com/NixOS/cabal2nix/blob/master/src/Distribution/Nixpkgs/Haskell/FromCabal/PostProcess.hs#L35.

Also, I don't understand why this change is applied to all Cabal_* attributes. As far as I know, Cabal_1_* does need depend on parsec, right`? So the addition should be unnecessary there. Furthermore, both of these libraries are core libraries in ghc 8.4.x and beyond, so these additions shouldn't be necessary for that compiler either.

Ericson2314 · 2018-06-20T11:08:53Z

@peti I've never understood the criterion for which goes in configuration-common.nix vs PostProcess.hs, so I have no problem if you want it there instead and can assist.

Furthermore, both of these libraries are core libraries in ghc 8.4.x and beyond, so these additions shouldn't be necessary for that compiler either.

Well, that would be adding a null dep then? Seems harmless enough. OTOH the effect of letting the wired-in-ness of libraries to pollute everything is quite messy.

peti · 2018-06-22T09:51:26Z

I've never understood the criterion for which goes in configuration-common.nix vs PostProcess.hs.

Suppose build A is broken due to a missing dependency. If you fix that issue via configuration-common.nix, then A builds in that particular branch of Nixpkgs from that revision on forward. Users who use cabal2nix to set up their own overlays or developments builds, however, will most likely still have the same issue. Users of other branches of Nixpkgs still have the same issue, too, unless you think of cherry-picking the fix everywhere else (which hardly anyone ever does).

Now, contrast to the fix being added to cabal2nix: That fixes the build of A in master the next time our automated hackag2nix process runs and the results are merged from haskell-updates, which happens every couple of days, on average. So the change arrives in master slower than it would if you'd make it in configuration-common.nix. On the other hand, that change will fix all cabal2nix-generated builds for everyone inside of or outside of Nixpkgs. Users of callHackage benefit from it. Every now and then I re-run hackage2nix manually in release branches, and then A will be fixed in those branches, too. So while cabal2nix takes a bit of a detour delivering the fixes to master, it delivers the fix to a lot more destinations than configuration-common.nix does.

Arguably, almost all overrides in configuration-common.nix should really be moved into cabal2nix. We should not fix broken build instructions from cabal2nix through some other mechanism -- instead, we should fix cabal2nix.

The reason we're not doing that is that it's more work, Nix contributors feel more comfortable hacking Nixpkgs than they feel hacking Haskell code in cabal2nix, and last but not least human nature favors instant rewards (fast fixes) over long-term goals (proper fixes that take longer to manifest). So now we have a shit-a-ton of overrides distributed all over the package set that patch up a broken generator, rather than just having a proper generator.

Since that's the reality we live in, I use the following rule of thumb to distinguish between those changes that should go into cabal2nix and those that probably should but it's fine if they don't: I ask myself how likely is it that someone would benefit from having that fix in cabal2nix? If the patch affects very fundamental packages like Cabal, hspec, QuickCheck, async, etc., then we should really be generating proper Nix expressions for those. If the patch affects some crazy ass package that I have never heard about and that seems to have no reverse dependencies on Hackage either, then I think it's fine to take a short cut for convenience because few people will ever build that with Nix outside of Nixpkgs anyway.

A second criteria is whether the overrides in configuration-common.nix are (a) expensive to maintain or (b) expensive at run-time. This particular change, for example, traverses the entire Haskell package set to find the names of 4 packages, 3 of which don't need the override to begin with. That code is, basically, a funky way of saying Cabal_2_2_0_1 without committing to that version number, and this feels wasteful to me. Adding the hook

  ("Cabal >2.2", over (setupDepends . haskell) (Set.union (Set.fromList [bind "self.mtl", bind "self.parsec"])))

to https://github.com/NixOS/cabal2nix/blob/master/src/Distribution/Nixpkgs/Haskell/FromCabal/PostProcess.hs#L35 accomplishes the same thing without run-time cost, so that seems like the better solution to me.

Both of these libraries are core libraries in ghc 8.4.x and beyond, so these additions shouldn't be necessary for that compiler either.

Well, that would be adding a null dep then? Seems harmless enough.

I worry about communicating intend to people who may be reading that code. The intention of that code is to fix Cabal >2.2 builds with ghc <8.4 compilers. But that's not what it's saying at all. The next person who looks that that code 1 year from now will have a hard time figuring out what is going on. I'd much prefer the code to do exactly what it intends to do instead of doing something far more general that just happens to achieve the actual result, too. Well, to be honest, most of all I'd prefer that change to go into cabal2nix.

NixOS/nixpkgs#41939 haskell/cabal#5391

peti · 2018-06-22T10:14:01Z

NixOS/cabal2nix@7ccbd66
haskell/cabal#5391

Ericson2314 · 2018-06-22T13:53:31Z

Thank you for that very thorough explanation @peti!

This is now handled by cabal2nix: - NixOS/cabal2nix@7ccbd66. - NixOS#41939

matthewbauer · 2018-07-02T20:06:14Z

pkgs/development/haskell-modules/generic-builder.nix

@@ -284,14 +276,13 @@ stdenv.mkDerivation ({
  # dependencies for the build machine.
  #
  # pkgs* arrays defined in stdenv/setup.hs
-  + (optionalString useSeparateSetupDb ''
+  + ''


@Ericson2314 I think this is causing the ARGMAX issue lately - this gives us way more --extra-*-dirs than we used to have. Can we be smarter at avoiding duplicates here? buildPkgDb will add to arg length much more than ordinary -L stuff.

In a later PR, isn't it changed so that --extra-* flags are not made for the setup.hs package db?

https://github.com/nixos/nixpkgs/blob/master/pkgs/development/haskell-modules/generic-builder.nix#L269-L289

Ok but the separate setup db thing could still be an issue. Here is the line that is hitting argmax:

ghc -package-db=/var/folders/l4/v2k_52x55r3_bf4cty3g4bg00000gn/T//setup-package.conf.d -j4 -threaded --make -o Setup -odir /var/folders/l4/v2k_52x55r3_bf4cty3g4bg00000gn/T/ -hidir /var/folders/l4/v2k_52x55r3_bf4cty3g4bg00000gn/T/ Setup.hs

GHC must not know how to deal with ARGMAX correctly in package-db.

OK huh that's really weird because I'd suspect that package db is super small. I think the real issue is having ghc as two different types of deps on native causes the setup hook to be run twice, which then without strict deps effectively causes the env hooks to be run twice more than they were without strict deps already.

NixOS/nixpkgs#41939 haskell/cabal#5391

Ericson2314 requested a review from matthewbauer June 13, 2018 17:53

Ericson2314 requested a review from peti as a code owner June 13, 2018 17:53

GrahamcOfBorg added 6.topic: haskell 10.rebuild-darwin: 501+ 10.rebuild-linux: 501+ labels Jun 13, 2018

haskell generic-builder: Always use separate pkg db for custom setup

f8ec07e

This decreases complexity and ensures setup dependencies are properly specified with `setup-depends` as they should be. Testing will say if this is a reasonable change.

Ericson2314 force-pushed the haskell-always-setup-separately branch from 75908aa to f8ec07e Compare June 18, 2018 18:07

haskell: Add setup depends for Cabal

6d5751b

Ericson2314 merged commit b6dfa31 into NixOS:master Jun 19, 2018

Ericson2314 deleted the haskell-always-setup-separately branch June 19, 2018 02:25

stepcut mentioned this pull request Jun 19, 2018

haskell: Setup.hs should not be compiled with the same pkgdb as the package itself #39646

Closed

peti reviewed Jun 20, 2018

View reviewed changes

peti added a commit to NixOS/cabal2nix that referenced this pull request Jun 22, 2018

PostProcess.hs: Cabal 2.2.0.1 has undeclared setup dependencies

7ccbd66

NixOS/nixpkgs#41939 haskell/cabal#5391

peti added a commit to peti/nixpkgs that referenced this pull request Jun 22, 2018

haskell-Cabal: drop obsolete override to fix Setup dependencies

1486fee

This is now handled by cabal2nix: - NixOS/cabal2nix@7ccbd66. - NixOS#41939

matthewbauer reviewed Jul 2, 2018

View reviewed changes

nh2 pushed a commit to nh2/cabal2nix that referenced this pull request Aug 22, 2018

PostProcess.hs: Cabal 2.2.0.1 has undeclared setup dependencies

4080fbc

NixOS/nixpkgs#41939 haskell/cabal#5391

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

haskell generic-builder: Always use separate pkg db for custom setup #41939

haskell generic-builder: Always use separate pkg db for custom setup #41939

Ericson2314 commented Jun 13, 2018

Ericson2314 commented Jun 13, 2018

matthewbauer commented Jun 13, 2018

kirelagin commented Jun 18, 2018 •

edited

Ericson2314 commented Jun 18, 2018

kirelagin commented Jun 18, 2018

Ericson2314 commented Jun 18, 2018

kirelagin commented Jun 18, 2018

Ericson2314 commented Jun 19, 2018 •

edited

peti left a comment •

edited

Ericson2314 commented Jun 20, 2018 •

edited

peti commented Jun 22, 2018 •

edited

peti commented Jun 22, 2018

Ericson2314 commented Jun 22, 2018

matthewbauer Jul 2, 2018

Ericson2314 Jul 3, 2018

Ericson2314 Jul 3, 2018

matthewbauer Jul 3, 2018

Ericson2314 Jul 5, 2018 •

edited

haskell generic-builder: Always use separate pkg db for custom setup #41939

haskell generic-builder: Always use separate pkg db for custom setup #41939

Conversation

Ericson2314 commented Jun 13, 2018

Motivation for this change

Things done

Ericson2314 commented Jun 13, 2018

matthewbauer commented Jun 13, 2018

kirelagin commented Jun 18, 2018 • edited

Ericson2314 commented Jun 18, 2018

kirelagin commented Jun 18, 2018

Ericson2314 commented Jun 18, 2018

kirelagin commented Jun 18, 2018

Ericson2314 commented Jun 19, 2018 • edited

peti left a comment • edited

Choose a reason for hiding this comment

Ericson2314 commented Jun 20, 2018 • edited

peti commented Jun 22, 2018 • edited

peti commented Jun 22, 2018

Ericson2314 commented Jun 22, 2018

matthewbauer Jul 2, 2018

Choose a reason for hiding this comment

Ericson2314 Jul 3, 2018

Choose a reason for hiding this comment

Ericson2314 Jul 3, 2018

Choose a reason for hiding this comment

matthewbauer Jul 3, 2018

Choose a reason for hiding this comment

Ericson2314 Jul 5, 2018 • edited

Choose a reason for hiding this comment

kirelagin commented Jun 18, 2018 •

edited

Ericson2314 commented Jun 19, 2018 •

edited

peti left a comment •

edited

Ericson2314 commented Jun 20, 2018 •

edited

peti commented Jun 22, 2018 •

edited

Ericson2314 Jul 5, 2018 •

edited