haskellPackages.shellFor: Fix ghc calls #46453

infinisil · 2018-09-09T23:02:33Z

Motivation for this change

This cost me a couple hours to figure out. The original error was

Got error while processing diagnostics: <command line>: cannot satisfy -package-id hnix-0.5.2-5hMz8sQY4ky2OCdjsvbD9J

from ghc-mod via HIE. After a very long time of debugging, I finally figured out that the reason was that shellFor doesn't run any shellHook that sets up the correct NIX_GHC* env vars to have it find stuff. This PR just does a dirty fast fix by copying part of the logic in generic-builder.nix, please don't merge it like this.

Ping @peti @ryantm @basvandijk @domenkozar (Ping the people I annoyed for hours in #haskell-ide-engine because of this: @alanz @DanielG)

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/)
Determined the impact on package closure size (by running nix path-info -S before and after)
Fits CONTRIBUTING.md.

teto · 2018-09-10T05:44:38Z

I think I had a similar problem domenkozar/hie-nix#27. even though I didn't use shellFor

basvandijk · 2018-09-10T07:01:46Z

Great work!

Now I wonder, can't we use shellFor to implement env in the generic-builder.nix?

Not that that's a prerequisite for merging this PR. I think we should merge this now and see how to DRY it later.

infinisil · 2018-09-10T13:29:33Z

@basvandijk Hmm, I usually don't like introducing bad code into nixpkgs. I'll see if I can refactor it in the next couple days. If I can't, then we should probably merge this, as it clearly fixes a bug and potentially lots of frustration. Also this might need backporting

Also pinging @ElvishJerricco who introduced the function originally in #36393

ElvishJerricco · 2018-09-10T15:57:56Z

This is not the only place this logic is duplicated in the haskell infra, IIRC. We really need a generic solution to this...

infinisil · 2018-09-17T15:55:50Z

@basvandijk @peti @ElvishJerricco Please take another look, I cleaned up most of the duplication

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

ElvishJerricco

I like the idea of defining haskellDrv.env in terms of shellFor!

ElvishJerricco · 2018-09-17T17:43:54Z

pkgs/development/haskell-modules/make-package-set.nix

+    haskellDrv = callPackageWithScope defaultScope drv args;
+    in haskellDrv // {
+      env = self.shellFor { packages = p: [ haskellDrv ]; };
+    };


I think this will break if you try to use (overrideCabal haskellDrv ...).env. It's probably easiest to either declare this in passthru in generic-builder.nix, or to use overrideCabal here to add it to passthru. I'd prefer the former.

The problem with the former is that shellFor doesn't work with a "raw" derivation, because it depends on .overrideCabal being available, which is only added with callPackage.

I'll test if this really breaks it.

Ah. Then yea adding with passthru and overrideCabal is important. As is, any use of overrideCabal will wipe out this env.

@infinisil Actually, I dunno if there's a way to fix this at all. We always want to refer to the final version of the derivation. But overrideCabal doesn't give you that. So even if we manage to preserve env via overrideCabal, it'll be the wrong env as soon as anyone does overrideCabal again; i.e. if they add a dependency, it won't be added to env.

So maybe we need to invert the control here: put the source of truth in generic-builder and define shellFor in terms of that.

Actually, how about putting the extractBuildInputs thing as a generic-builder passthru, define it in terms of that. I think that's the only thing that depends on overrideCabal.

@infinisil Yep, sounds good. Eliminating overrideCabal entirely from the equation for extracting build inputs sounds like a really good thing.

ElvishJerricco · 2018-09-17T17:45:31Z

pkgs/development/haskell-modules/make-package-set.nix

+        # If `packages = [ a b ]` and `a` depends on `b`, don't build `b`, such
+        # that `a` will take the `b` dependency from the cabal.project file.
+        # This allows changing `b` while in the shell and have `a` use the
+        # changes directly.


cabal new-build is happy to ignore any installed b actually. The real reason for this is to avoid building b with Nix at all, so that entering the nix shell doesn't require rebuilding b every time it changes when you're just going to ignore that version of b.

Ahh good point, will adjust that message

ElvishJerricco · 2018-09-17T17:46:27Z

pkgs/development/haskell-modules/make-package-set.nix

        mkDrvArgs = builtins.removeAttrs args ["packages" "withHoogle"];
      in pkgs.stdenv.mkDerivation (mkDrvArgs // {
-        name = "ghc-shell-for-packages";
-        nativeBuildInputs = [(withPackages (_: haskellInputs))] ++ mkDrvArgs.nativeBuildInputs or [];
+        name = "interactive-${pkgs.lib.concatMapStringsSep "-" (p: p.name) selected}-environment";


Hm this naming could be a bit much. I think it'll be common for selected to have quite a few packages, e.g. 10+

Yeah, I think maybe just "interactive-${name}-environment" if it's a single package and "interactive-multi-package-environment" or so if it's more than one would be better.

I'd prefer just having a generic name, but allowing mkDrvArgs to override the name however the user likes; but it doesn't matter very much

pkgs/development/haskell-modules/make-package-set.nix

ElvishJerricco · 2018-09-17T17:49:13Z

pkgs/development/haskell-modules/make-package-set.nix

+          ${if ghc.isHaLVM or false
+              then ''export NIX_${ghcCommandCaps}_LIBDIR="${ghcEnv}/lib/HaLVM-${ghc.version}"''
+              else ''export NIX_${ghcCommandCaps}_LIBDIR="${ghcEnv}/lib/${ghcCommand}-${ghc.version}"''}
+          ${mkDrvArgs.shellHook or ""}


I like adding these environment variables, but I'm pretty sure it needn't be done in a shellHook. Maybe I'm being picky, but I think it seems nicer to just set them as mkDerivation arguments to me :P

Oh, that's a good point, I just copied it from before, but that sounds much better, will change.

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

infinisil · 2018-09-17T19:43:21Z

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

-// optionalAttrs (hardeningDisable != []) { inherit hardeningDisable; }
-// optionalAttrs (stdenv.buildPlatform.libc == "glibc"){ LOCALE_ARCHIVE = "${glibcLocales}/lib/locale/locale-archive"; }
-)
+  }


Recommended to hide whitespace changes (Click on Diff settings above).

I don't see any other way than this to add .env to passthru. Because it itself depends on the thing.

infinisil · 2018-09-17T19:45:14Z

pkgs/development/haskell-modules/lib.nix

-        _getBuildInputs = extractBuildInputs p.compiler args;
-      };
-    }))._getBuildInputs;
+  getBuildInputs = p: p.getBuildInputs;


This change very slightly changes the results of this function, namely that setupHaskellDepends is not in haskellBuildInputs anymore. It is in the derivations nativeBuildInputs though, and therefore included in nix-shells.

ElvishJerricco · 2018-09-17T20:45:03Z

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

-
-assert allPkgconfigDepends != [] -> pkgconfig != null;
-
-stdenv.mkDerivation ({


Is there any way to avoid re-indenting everything? Makes it harder to review and kills git blame.

Maybe we can just do fix (drv: stdenv.mkDerivation ({ and add a paren at the end?

You could add -w to git diff or git blame :)

I guess the github blame's won't work with this though. I could of course just not indent it, but then it's kinda ugly

Or fix, yeah that sounds better

ElvishJerricco · 2018-09-17T20:47:01Z

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

+in
+  drv // {
+    env = buildHaskellPackages.shellFor {
+      packages = p: [ drv ];


Is this a duplicate of line 432? We should only have the passthru one.

Oh yeah good point, will remove

ElvishJerricco · 2018-09-17T20:53:45Z

pkgs/development/haskell-modules/make-package-set.nix

+        "NIX_${ghcCommandCaps}_DOCDIR" = "${ghcEnv}/share/doc/ghc/html";
+        "NIX_${ghcCommandCaps}_LIBDIR" = if ghc.isHaLVM or false
+          then ''export NIX_${ghcCommandCaps}_LIBDIR="${ghcEnv}/lib/HaLVM-${ghc.version}"''
+          else ''export NIX_${ghcCommandCaps}_LIBDIR="${ghcEnv}/lib/${ghcCommand}-${ghc.version}"'';


ElvishJerricco · 2018-09-17T20:57:46Z

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

+        };
+
+        env = buildHaskellPackages.shellFor {
+          packages = p: [ drv ];


I don't think buildHaskellPackages is right.

Hmm right, I guess it doesn't matter much though, because the p is ignored anyways, so no dependencies come from that set anyways. It was the easiest way to get access to shellFor.

@infinisil Does it control where GHC comes from? This will get the GHC for targeting the build platform.

Ah you're right, will have to do something about this then

infinisil · 2018-09-17T21:24:30Z

Cleaned up all the commits, very tidy now

ElvishJerricco · 2018-09-17T21:33:58Z

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

-            else ''export NIX_${ghcCommandCaps}_LIBDIR="${ghcEnv}/lib/${ghcCommand}-${ghc.version}"''}
-        ${shellHook}
-      '';
+    env = buildHaskellPackages.shellFor {


@infinisil In the case of cross compilation, this will give you a shell with a native GHC rather than a cross compiling GHC, won't it?

Maybe it'd be better to have a haskellLib function for shellFor that takes GHC and a list of packages as arguments. Then haskellPackages.shellFor can be a convenience for plugging in GHC automatically and letting packages be a function for convenient access to the package set as an argument.

infinisil · 2018-09-17T21:37:18Z

pkgs/development/haskell-modules/make-package-set.nix

@@ -46,6 +46,7 @@ let
    inherit buildHaskellPackages;
    inherit (self) ghc;
    inherit (buildHaskellPackages) jailbreak-cabal;
+    inherit (extensible-self) shellFor;


Passing it like this now

infinisil · 2018-09-17T21:42:09Z

Rebased, fixed conflicts, and now using self.shellFor instead of extensible-self, works better.

ElvishJerricco · 2018-09-27T19:32:08Z

@peti If I understand correctly, ghcWithPackages is unaffected by this PR. env always was built on top of that. Now env is built on shellFor, which is still built on ghcWithPackages.

infinisil · 2018-09-30T00:40:52Z

I have tested this myself successfully on my nixbot consisting of two subprojects, and just running nix-shell -E 'with import ~/src/nixpkgs {}; haskellPackages.xmonad.env.overrideAttrs (old: { buildInputs = old.buildInputs or [] ++ [ haskellPackages.cabal-install ]; })' and running cabal build in xmonad's master revision.

Unrelated: I think cabal-install should be included as a buildInput/nativeBuildInput in all .env's, it's annoying to always have to override it.

- Now correctly sets NIX_GHC* env vars

infinisil · 2018-10-02T19:14:44Z

@peti Rebased to fix conflicts, didn't make any changes. Would like to get this merged.

ElvishJerricco · 2018-10-10T12:24:58Z

Is there a chance we could backport this to 18.09? Who would be able to make that decision?

Backport #46453

infinisil requested review from basvandijk, peti and ryantm as code owners September 9, 2018 23:02

GrahamcOfBorg added 6.topic: haskell 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Sep 9, 2018

basvandijk approved these changes Sep 10, 2018

View reviewed changes

infinisil force-pushed the fix/haskell/shellFor branch from e6dd03d to b6b7236 Compare September 17, 2018 15:54

infinisil changed the title ~~[HACK] haskellPackages.shellFor: Fix ghc calls~~ haskellPackages.shellFor: Fix ghc calls Sep 17, 2018

infinisil commented Sep 17, 2018

View reviewed changes

pkgs/development/haskell-modules/generic-builder.nix Show resolved Hide resolved

ElvishJerricco requested changes Sep 17, 2018

View reviewed changes

infinisil commented Sep 17, 2018

View reviewed changes

ElvishJerricco reviewed Sep 17, 2018

View reviewed changes

infinisil force-pushed the fix/haskell/shellFor branch from d968c56 to c16f1bc Compare September 17, 2018 21:23

infinisil requested a review from FRidh as a code owner September 17, 2018 21:23

infinisil force-pushed the fix/haskell/shellFor branch from c16f1bc to 8f1def5 Compare September 17, 2018 21:23

ElvishJerricco reviewed Sep 17, 2018

View reviewed changes

infinisil commented Sep 17, 2018

View reviewed changes

infinisil force-pushed the fix/haskell/shellFor branch from 594f7dd to 3d251e1 Compare September 17, 2018 21:41

ElvishJerricco mentioned this pull request Sep 28, 2018

Cannot pre-build shells with nix-build reflex-frp/reflex-platform#373

Closed

infinisil force-pushed the fix/haskell/shellFor branch from 3d251e1 to 7d7215d Compare September 29, 2018 18:58

infinisil changed the base branch from master to haskell-updates September 29, 2018 18:58

GrahamcOfBorg added 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Sep 29, 2018

peti force-pushed the haskell-updates branch 3 times, most recently from c77ce53 to a4badb2 Compare October 2, 2018 19:09

infinisil added 3 commits October 2, 2018 21:12

haskell.lib.getBuildInputs: Use generic builder passthru to implement

2c4f1fd

haskellPackages.shellFor: Clean and fixup

98e1937

- Now correctly sets NIX_GHC* env vars

haskellPackages.*.env: Use shellFor

51267d3

infinisil force-pushed the fix/haskell/shellFor branch from 7d7215d to 51267d3 Compare October 2, 2018 19:13

peti merged this pull request into NixOS:haskell-updates Oct 2, 2018

GrahamcOfBorg added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux and removed 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 labels Oct 2, 2018

ElvishJerricco mentioned this pull request Oct 10, 2018

Backport #46453 #48155

Merged

basvandijk added a commit that referenced this pull request Oct 27, 2018

Merge pull request #48155 from ElvishJerricco/fix/haskell/shellFor/18.09

cbcd5aa

Backport #46453

teto mentioned this pull request Nov 19, 2018

Have ghcWithPackages propagate NIX_GHC_LIBDIR/ NIX_GHC_DOCDIR #50694

Closed

infinisil mentioned this pull request Nov 27, 2018

Haskell mkDerivation env attribute sucks in the source #51079

Closed

infinisil mentioned this pull request Mar 7, 2019

Dynamic libraries are not linked in ghci when using nix-shell --run cabal new-repl #56860

Closed

Ericson2314 mentioned this pull request Dec 23, 2019

Upstream work-on-multi into Nixpkgs's shellFor reflex-frp/reflex-platform#586

Open


		assert allPkgconfigDepends != [] -> pkgconfig != null;

		stdenv.mkDerivation ({

haskellPackages.shellFor: Fix ghc calls #46453

haskellPackages.shellFor: Fix ghc calls #46453

Conversation

infinisil commented Sep 9, 2018

Motivation for this change

Things done

teto commented Sep 10, 2018

basvandijk commented Sep 10, 2018 • edited Loading

infinisil commented Sep 10, 2018

ElvishJerricco commented Sep 10, 2018

infinisil commented Sep 17, 2018

ElvishJerricco 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

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

infinisil commented Sep 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

infinisil commented Sep 17, 2018 • edited Loading

ElvishJerricco commented Sep 27, 2018

infinisil commented Sep 30, 2018

infinisil commented Oct 2, 2018

ElvishJerricco commented Oct 10, 2018

basvandijk commented Sep 10, 2018 •

edited

Loading

infinisil commented Sep 17, 2018 •

edited

Loading