Remove internal deps on default ghc and stackage #738

hamishmack · 2020-07-01T12:55:33Z

Adds hpack executable to the nix-tools derivation.

Adds a cabal-hpack test to make sure hpack works with
cabalProject.

Issues warnings when compiler-nix-name is not provided.

Reduces the number of calls to cabalProject (particularly when
checking materialization), by giving internal tools a per-compiler
attribute.

Adds p.tool, p.tools and p.roots where p is a project as an
easy way to match the cabal-nix-name of the project.

Uses happy 1.19.12 when building newer ghc versions.

Updates cabal-install 3.2.0.0 to use the source from github that
is compatible with ghc 8.10.1.

Adds hpack executable to the nix-tools derivation. Adds a `cabal-hpack` test to make sure `hpack` works with `cabalProject`. Issues warnings when compiler-nix-name is not provided. Reduces the number of calls to `cabalProject` (particularly when checking materialization), by giving internal tools a per-compiler attribute. Adds `p.tool`, `p.tools` and `p.roots` where `p` is a project as an easy way to match the `cabal-nix-name` of the project. Uses happy 1.19.12 when building newer ghc versions. Updates cabal-install 3.2.0.0 to use the source from github that is compatible with ghc 8.10.1.

michaelpj

This feels more difficult than I was expecting. I think the key is probably the issue with nix-tools. I hadn't thought we'd need to compile nix-tools with the user's compiler: it doesn't link against their stuff, so it can probably be compiled with anything.

The key thing is really call-cabal-to-nix: I think everything else can get away with a fixed version of the compiler (as a bonus, it's easier to cache).

michaelpj · 2020-07-01T15:36:26Z

default.nix

        then [(
          final: prev: {
            haskell-nix = prev.haskell-nix // {
-              inherit defaultCompilerNixName;
+              userCompilerNixName = args.defaultCompilerNixName;


I'd be tempted to just drop it and make this an error...

We need something to make the tests work though. I quite like the idea of replacing this with something like:

haskell-nix = prev.haskell-nix // { withGhc = compiler-nix-name: { cabalProject = args: final.haskell-nix.cabalProject ({ inherit compiler-nix-name; } // args); hackage-package = args: final.haskell-nix.hackage-package ({ inherit compiler-nix-name; } // args); ... }; };

Why do the tests need to set defaultNixName? Shouldn't they all be setting explicit compiler-nix-names for their individual tests?

Yes I'm going to update all the tests so they do that instead of using defaultCompilerNixName. I was hoping there was a way to avoid that, but @angerman pointed out it is nicer to be explicit here. It makes the tests more useful as examples.

Test suite no longer uses defaultCompilerNixName.

I thought the point of that change was so we could then get rid of the userCompilerNixName thing? If we no longer need it for tests then let's just delete it.

The only other reason I can see to keep it is backwards compatibility, but I buy Rodney's argument that that's something of a losing battle anyway and we should just keep a changelog.

michaelpj · 2020-07-01T15:45:31Z

overlays/bootstrap.nix

+      let
+        # This compiler-nix-name will only be used to build nix-tools and cabal-install
+        # when checking materialization of alex, happy and hscolour.
+        compiler-nix-name = "ghc865";


Do we need to build nix-tools and cabal-install with the user's GHC? Both of them should be fairly compiler-agnostic, the most relevant thing there will probably be what version of cabal they use.

I think we could plausibly have an internalCompilerNixName for building such things. Then we wouldn't have to have a whole set of nix-tools expressions, wouldn't have to pass the compiler version into a bunch of the call-cabal-to-nix stuff.

Do we need to build nix-tools and cabal-install with the user's GHC?

I don't think we need to, but it has at least two nice advantages:

Less overhead on macOS where dynamic linking pulls in the compiler used for nix-tools and cabal-install.

Tests that haskell.nix can making it easy to compile stuff with different ghcs.

The main thing is that we're spending quite a lot of complexity on threading compiler versions through when we don't actually need them.

And I believe we can compile things with different GHCs - we have lots of tests for that!

Did we resolve this? I think this was my main gripe. If you want to do it this way it's fine, I just think it's causing a lot of suffering for a comparatively minor gain.

(Not to mention, if we used a fixed GHC for nix-tools we'd get better caching)

Its only really a problem for stack projects (for cabal projects we always have a compiler-nix-name ahead of time). For stack projects (and cabal projects) we use config.compiler.nix-name once it is available (for shellFor { tools = ... } and p.tool. That only leaves nix-tools for use in the stackProject stuff. For that I have added internalDefaultCompilerNixName. In theory someone could add an overlay to set it, but it is only used to selecting the nix-tools build to use for stack projects (so hopefully no one will need to). It is currently set to "ghc883".

I'm don't think it's a problem for users, I think it's a problem for us! At the moment AFAICT a significant fraction of the compiler name threading in this PR is just to get nix-tools... which is a set of binaries that don't have to be built with the user's compiler! So why are we doing that?

To put it another way: if we think of the nix-tools binaries as part of the haskell.nix interface, it's perfectly fine for them to be built with an internalCompilerNixName, so long as that doesn't leak out.

AFAICT a significant fraction of the compiler name threading in this PR is just to get nix-tools

We have to have compiler-nix-name already in cabalProject (to know what GHC to compile the project with). The threading in the PR is mostly for that.

Okay, I'm not sure how much of it we'd get rid of (at least various things like the updater scripts now need compiler names where they didn't before), but I do just think it's simpler. There is certainly a lot of indexing into this set of nix-tools derivations that would be more straightforward if there were only one!

I guess I also still don't see what the advantage is. Just the dynamic linking thing?

There is certainly a lot of indexing into this set of nix-tools derivations that would be more straightforward if there were only one!

I'll replace internalDefaultCompilerNixName with internal-nix-tools and also use it in the scripts.

I guess I also still don't see what the advantage is. Just the dynamic linking thing?

Yes that is my main concern. On macOS it is likely build or download a whole extra GHC. If the user tries to solve that by passing in your own nix-tools to cabalProject but then everything in the cache that depends on nix-tools does not match.

Okay: I think this discussion deserves a note in the code explaining the reasoning, since it's a bit non-obvious in the end.

overlays/eval-on-current.nix

overlays/haskell.nix

test/default.nix

overlays/tools.nix

lib/call-stack-to-nix.nix

lib/call-cabal-project-to-nix.nix

# Conflicts: # lib/call-stack-to-nix.nix # lib/stack-cache-generator.nix # overlays/haskell.nix

builder/default.nix

michaelpj · 2020-07-06T16:19:09Z

lib/cabal-licenses.nix

@@ -12,6 +12,7 @@ lib: with lib.licenses;
  "AGPL-3.0-only"     = agpl3;
  "AGPL-3.0-or-later" = agpl3Plus;
  "Apache-2.0"        = asl20;
+  "GPL-2.0-or-later AND BSD-3-Clause" = [gpl2Plus bsd3];


michaelpj · 2020-07-06T16:20:44Z

overlays/haskell.nix

+          # Niv likes to have a nixpkgs so we are using
+          # that to replace nixpkgs-default. This is
+          # here for backwards compatibility.
+          nixpkgs-default = sources.nixpkgs;


Just kill it IMO

michaelpj · 2020-07-06T16:22:15Z

overlays/haskell.nix

-          let inherit (final.haskell-nix) defaultCompilerNixName;
-          in final.recurseIntoAttrs ({
+        haskellNixRoots' = final.haskell-nix.roots' (final.haskell-nix.defaultCompilerNixNameWithWarning (default:
+            "Please consider replacing `haskellNixRoots'` with `p.roots'` on your project "


If we follow the "Rodney" deprecation-strategy then we could just get rid of the warnings and note the change in the changelog.

michaelpj · 2020-07-06T16:25:11Z

overlays/bootstrap.nix

@@ -432,7 +458,8 @@ in {
                  "unix" "xhtml"
                ];
            }];
-          } // args)).nix-tools.components.exes;
+          } // args);
+        exes = project.nix-tools.components.exes // project.hpack.components.exes;


Hm, I wonder if we should instead consider hpack to be part of cabal-install? It's a little surprising to put it in here - I certainly wouldn't expect to find it there.

I guess the reason not to have it as it's own tool is just that it would be a pain to do the plumbing?

I put it there because nix-tools actually depends on hpack, so it was already part of hsPkgs. This also means the version used is governed by the constraints in nix-tools.cabal (rather than something in nix).

michaelpj

Mostly still the same argument ;)

michaelpj

Looking good now! Mainly now I think it's just worth taking some time to write down any of the non-obvious choices that we made and why.

michaelpj · 2020-07-07T10:31:57Z

nix-tools/regenerate.nix

@@ -36,6 +36,7 @@ let
  };
 in
  let plan-to-nix = (haskell-nix.cabalProject {
+    compiler-nix-name = "ghc865";


Is this right? Should this be passed in? I'm not really sure

michaelpj · 2020-07-07T10:33:59Z

overlays/bootstrap.nix

+    cabal-install-unchecked = final.lib.mapAttrs (compiler-nix-name: _:
+      final.haskell-nix.cabal-install-tool { inherit compiler-nix-name; checkMaterialization = false; }) final.haskell-nix.compiler;
+    # Use this where we still have not good way to choose GHC version
+    internal-cabal-install = final.haskell-nix.cabal-install.ghc883;


Is it important that we use the same version for internal-cabal-install and internal-nix-tools? Should we pull out internalToolsCompilerVersion? Also this could do with a note saying when/if it's safe to update the compiler version used here. (TBH, might be overkill but I think maybe we should annotate every reference to a specific compiler version with a note saying when/if we can change it!)

michaelpj · 2020-07-07T10:35:34Z

overlays/bootstrap.nix

+      let
+        # This compiler-nix-name will only be used to build nix-tools and cabal-install
+        # when checking materialization of alex, happy and hscolour.
+        compiler-nix-name = "ghc865";


Okay: I think this discussion deserves a note in the code explaining the reasoning, since it's a bit non-obvious in the end.

michaelpj · 2020-07-07T10:35:59Z

overlays/bootstrap.nix

+      let
+        # This compiler-nix-name will only be used to build nix-tools and cabal-install
+        # when checking materialization of alex, happy and hscolour.
+        compiler-nix-name = "ghc865";


Does this version have to match any of the other versions? When can we upgrade it?

michaelpj · 2020-07-07T10:36:51Z

overlays/haskell.nix

@@ -252,22 +252,22 @@ final: prev: {
        };

        update-index-state-hashes = import ../scripts/update-index-state-hashes.nix {
-            inherit (final.haskell-nix) indexStateHashesPath nix-tools;
+            inherit (final.haskell-nix) indexStateHashesPath internal-nix-tools;


Extreme nit but I'd probably leave the parameter called nix-tools and do nix-tools = internal-nix-tools.

michaelpj · 2020-07-07T10:39:03Z

overlays/haskell.nix

        };

        # given a source location call `cabal-to-nix` (from nix-tools) on it
        # to produce the nix representation of it.
        callCabalToNix = { name, src, cabal-file ? "${name}.cabal" }:
            final.buildPackages.pkgs.runCommand "${name}.nix" {
-                nativeBuildInputs = [ final.buildPackages.haskell-nix.nix-tools ];
+                nativeBuildInputs = [ final.buildPackages.haskell-nix.internal-nix-tools ];


Worth a note about why we're using the internal one here.

michaelpj · 2020-07-07T10:40:32Z

scripts/update-external.nix

@@ -1,5 +1,5 @@
 { stdenv, writeScript, glibc, coreutils, git, openssh
-, nix-tools, cabal-install, nix-prefetch-git
+, internal-nix-tools, internal-cabal-install, nix-prefetch-git


Other end of my nit above. I guess my reasoning is that this Nix file "doesn't care" about the nix-tools/cabal-install, it's the caller that decides it's going to be the internal one.

And likewise below.

michaelpj · 2020-07-07T10:41:19Z

test/default.nix

 , nixpkgsArgs ? haskellNix.nixpkgsArgs
 , ifdLevel ? 1000
+, compiler-nix-name ? "ghc865"


Does this need to be the same as any of the other versions? When can we upgrade it?

michaelpj · 2020-07-07T10:41:57Z

build.nix

 , nixpkgsArgs ? haskellNix.nixpkgsArgs
 , pkgs ? import nixpkgs nixpkgsArgs
 , ifdLevel ? 1000
+, compiler-nix-name ? "ghc865"


Does this need to be the same as any of the other versions? When can we upgrade it?

michaelpj · 2020-07-08T09:49:03Z

overlays/bootstrap.nix

+    # For cabal projects we match the versions used to the compiler
+    # selected for the project to avoid the chance of a dependency
+    # another GHC version (particularly useful on macOS where
+    # executables are dynamically linked to GHC itself).


... which means that if you use a tool built with a different GHC you will get that GHC itself in your closure.

michaelpj · 2020-07-08T09:51:06Z

👍

michaelpj · 2020-07-08T09:51:36Z

BTW great work with this generally :)

Changes to the interface of haskell.nix (from the changelog.md file): * Removed `sources.nixpkgs-default`, use `sources.nixpkgs` instead. * Removed `./nixpkgs` directory, use `(import ./. {}).sources` or `./nix/sources.nix` instead. * Removes V1 interface for details on how to fix old code see: input-output-hk#709 * Removed defaultCompilerNixName. * cabalProject, cabalProject', hackage-project and hackage-package now require a `compiler-nix-name` argument. * `haskell-nix.tool` and `.tools` now require a `compiler-nix-name` argument. New functions `p.tool` and `p.tools` (where p is a project) do not. Like `shellFor { tools = ... }` they will use the compiler nix name from the project (including stack projects where it is derived from the resolver). * `haskell-nix.alex` and `haskell-nix.happy` have been removed. Use `p.tool "alex" "3.2.5"` or `shellFor { tools = { alex = "3.2.5"; } }`. * `haskell-nix.nix-tools` -> `haskell-nix.nix-tools.ghc883` (it includes the hpack exe now). * `haskell-nix.cabal-install` -> `p.tool "cabal" "3.2.0.0"` or `shellFor { tools = { cabal = "3.2.0.0"; } }` * `haskell-nix.haskellNixRoots` -> `haskell-nix.roots ghc883` or `p.roots` Other changes: Adds hpack executable to the nix-tools derivations. Adds a `cabal-hpack` test to make sure `hpack` works with `cabalProject`. Reduces the number of calls to `cabalProject` (particularly when checking materialization), by giving internal tools a per-compiler attribute. Uses happy 1.19.12 when building newer ghc versions. Updates cabal-install 3.2.0.0 to use the source from github that is compatible with ghc 8.10.1. Updates the docs for callCabalProjectToNix. Adds a license mapping to fix a common warning.

hamishmack added 4 commits July 2, 2020 00:52

Merge remote-tracking branch 'origin/master' into hkm/avoid-defaults

fe86dcf

ifdLevel 1

ee0cbc4

Set "builtin": false in sources.json

e22fff7

michaelpj reviewed Jul 1, 2020

View reviewed changes

hamishmack added 11 commits July 2, 2020 04:14

Use nixpkgs instead of nixpkgs-default for niv

f0a1490

Use nixpkgs instead of nixpkgs-default for niv

eeff15e

ifdLevel 2

30df20a

ifdLevel 3

482a1a1

Update tests to make compiler-nix-name explicit

b367bf2

Fix for cabal-install on ghc 8.10.1

52793cf

Fix build.nix

70f7775

Fix test

dd704bd

Add missing compiler-nix-name in ci.nix

cf4eeff

Better warnings

46fd628

Merge master into hkm/avoid-defaults

275e162

# Conflicts: # lib/call-stack-to-nix.nix # lib/stack-cache-generator.nix # overlays/haskell.nix

hamishmack force-pushed the hkm/avoid-defaults branch from f605217 to 275e162 Compare July 6, 2020 03:23

hamishmack added 7 commits July 6, 2020 16:08

Better warnings

caa940f

Remove redundant tests

ba4e38e

Add default for ./test/test.sh on buildkite

d41b1c8

Add cabal-nix-name default for buildkite tests

926766a

Add compiler-nix-name to script functions

da181b4

Fix extra-hackage buildkite test

b30a9a1

Check the materialization

de8cbb7

hamishmack mentioned this pull request Jul 6, 2020

Overriding GHC doesn't apply to tools #749

Closed

TravisWhitaker mentioned this pull request Jul 6, 2020

Make it easier to cache eval-time dependencies. #728

Closed

michaelpj reviewed Jul 6, 2020

View reviewed changes

builder/default.nix Show resolved Hide resolved

hamishmack added 3 commits July 6, 2020 22:29

Fix materialization

10055a1

Fix eval time warnings

74935bc

Fix hydra eval (maybe)

bc2e42c

michaelpj reviewed Jul 6, 2020

View reviewed changes

hamishmack added 4 commits July 7, 2020 15:35

Remove deprecated code

757b9a4

Remove ./nixpkgs

dfc49e2

Remove ./nixpkgs

855089e

Reenable call-stack-to-nix ifdInput test

c1bf1a3

hamishmack requested a review from michaelpj July 7, 2020 08:56

Merge remote-tracking branch 'origin/master' into hkm/avoid-defaults

18311c4

michaelpj approved these changes Jul 7, 2020

View reviewed changes

hamishmack added 5 commits July 8, 2020 20:50

Documentation and explicit test compiler-nix-names

4ec19da

Add missing \

c0b401b

Add compiler-nix-name for maintainer scripts test

7c1453c

Move compiler-nix-name to the right command

cb3d1c0

Merge remote-tracking branch 'origin/master' into hkm/avoid-defaults

1be5755

michaelpj reviewed Jul 8, 2020

View reviewed changes

hamishmack added 4 commits July 8, 2020 21:56

Fix test

b9345d2

Expand comment

4cebce4

Fix docs for callCabalProjectToNix

21fb517

Set date

eba7f1e

hamishmack merged commit 4cac8bd into master Jul 8, 2020

iohk-bors bot deleted the hkm/avoid-defaults branch July 8, 2020 10:54

hamishmack mentioned this pull request Jul 8, 2020

Get rid of the default GHC and default haskellPackages #729

Closed

This was referenced Aug 11, 2020

"Materialized nix used for dummy-data-ghc-8.6.4 is missing" #811

Closed

Use internal nix-tools and cabal-install in call-cabal-projet-to-nix #820

Closed

Remove internal deps on default ghc and stackage #738

Remove internal deps on default ghc and stackage #738

Conversation

hamishmack commented Jul 1, 2020

michaelpj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelpj left a comment

Choose a reason for hiding this comment

michaelpj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelpj commented Jul 8, 2020

michaelpj commented Jul 8, 2020