haskellPackages.{ghcWithPackages, ghcWithHoogle}: make overrideable #146770
Conversation
sternenseemann
requested review from
cdepillabout,
expipiplus1 and
maralorn
as code owners
github-actions
bot
added
the
6.topic: haskell
sternenseemann
added
the
9.needs: port to stable
sternenseemann
force-pushed
the
ghc-with-packages-overrideable
branch
from
a6d26d6 to
cffdd83
Compare
ofborg
bot
added
10.rebuild-darwin: 0
It's been quite a while since any version below that has been in use in nixpkgs, so this check is almost certainly safe to remove.
Overrideable functions are possible by wrapping them as functors in an attribute set first and appending the overrideScope and override attributes later.
sternenseemann
force-pushed
the
ghc-with-packages-overrideable
branch
from
cffdd83 to
10c9142
Compare
|
A bit more detailed writeup concerning the how and why of this change, as asked for by @cdepillabout: Basically, this allows to use To allow for this the following changes were made:
|
|
I won`t pretend I understand everything of this, but the change seems reasonable and wrappers still seem to be working. |
|
@sternenseemann Thanks for writing up this explanation! I'm going to try to do a review of this. Sorry for taking a long time to get to this review? Here's one thing that I don't think I can use GitHub's UI to comment on: diff --git a/pkgs/development/haskell-modules/configuration-common.nix b/pkgs/development/haskell-modules/configuration-common.nix
index 87d1cef5857..802038d4fcb 100644
--- a/pkgs/development/haskell-modules/configuration-common.nix
+++ b/pkgs/development/haskell-modules/configuration-common.nix
@@ -44,6 +44,7 @@ self: super: {
# enable using a local hoogle with extra packagages in the database
# nix-shell -p "haskellPackages.hoogleLocal { packages = with haskellPackages; [ mtl lens ]; }"
# $ hoogle server
+ # this is broken?
hoogleLocal = { packages ? [] }: self.callPackage ./hoogle.nix { inherit packages; };
# Needs older QuickCheck versionIt looks like this I've never heard of this edit: Ah, looks like |
There was a problem hiding this comment.
In what situation is it critical that ensureAttrs is used here? (I'm mostly just curious...)
There was a problem hiding this comment.
When drv allArgs returns a function.
There was a problem hiding this comment.
Ah sorry, I can see that, but what I'm trying to ask is what sort of call to callPackage would fail if ensureAttrs was not used here?
For instance, I don't think ensureAttrs is necessary for a normal call to callPackage that looks like:
callPackage
( { mkDerivation, lens, aeson }:
mkDerivation {
name = "my-haskell-package";
...
}
)
{}What would a call to callPackage look like where drv allArgs returns function? What would be the use of this?
There was a problem hiding this comment.
callPackage ./hoogle.nix { } and callPackage ./with-packages-wrapper.nix { } both need it.
There was a problem hiding this comment.
I guess we should have documented this feature, since it appears to differ from the top-level callPackage:
Here's an example showing what I mean:
with import <nixpkgs> {};
let
myPkgExpression = { runCommand, lens }: prefix:
runCommand "get-lens-version" {} ''
echo ${prefix} lens version ${lens.version} >> $out
'';
myPkg = callPackage myPkg { lens = haskellPackages.lens; };
myPkgWithArg = myPkg "my prefix";
myPkgWithOverride = myPkg.override { lens = haskellPackages.lens_5_0_1; };
myPkgWithArgWithOverride = myPkgWithArg.override { lens = haskellPackages.lens_5_0_1; };
myPkgWithOverrideWithArg = myPkgWithOverride "my prefix";
in
myPkgWithArgWithOverride
# myPkgWithOverrideWithArgmyPkgExpression (and myPkg) define a package using callPackage similar to our new hoogle.nix and with-packages-wrapper.nix.
myPkgWithOverride and myPkgWithArgWithOverride then try to override the lens version, both before and after passing the actual "my prefix" argument.
With both of these, I get the following error:
error: infinite recursion encountered
at /nix/store/g65b6lh3akdka0z9iji75yl07sb9n2s6-nixos-21.11.334247.573095944e7/nixos/lib/trivial.nix:364:19:
363| */
364| isFunction = f: builtins.isFunction f ||
| ^
365| (f ? __functor && isFunction (f.__functor f));
I guess we should have explicitly documented this in haskellPackages.callPackage so that users see that haskellPackages.callPackage is a little more flexible than the top-level callPackage.
There was a problem hiding this comment.
I'd rather say that this is a potential bug in top level callPackage or a pitfall — in principle the behavior of top level callPackage is the same: cnovert a function to a functor set in order to be able to add the override attribute. Haven't tried debugging this particular case though.
cdepillabout
left a comment
cdepillabout
left a comment
There was a problem hiding this comment.
LGTM. Thanks!
Make
ghcWithPackages.override { withLLVM = true; } (p: [ ... ])work
I confirmed this with the following steps:
$ nix-shell -I nixpkgs=./. -p 'haskellPackages.ghcWithPackages.override { useLLVM = true; } (p: [ p.mtl p.conduit ])'
$ ... # create a file example.hs
$ ghc -v -fllvm example.hsLooking at the output of ghc -v, I could see that LLVM was successfully being used.
sternenseemann
force-pushed
the
ghc-with-packages-overrideable
branch
from
10c9142 to
076451b
Compare
|
@cdepillabout added documentation as asked and cleaned up the |
sternenseemann
force-pushed
the
ghc-with-packages-overrideable
branch
from
076451b to
90a0d42
Compare
This is achieved by passing the entire package set to the respective
wrappers and passing the select function as a second attribute. Together
with the new support for callPackage-ing functions this allows for
things like `ghcWithPackages.override { useLLVM = true; } (p: [ … ])`.
To make this possible for `ghcWithHoogle` as well, we need to make the
wrapper a bit more bespoke and inline the hoogle feature as well. The
hoogle wrapper, however, can remain separate and is exposed as
`hoogleWithPackages` additionally, as it can also serve standalone use.
`hoogleLocal` is kept for backwards compatibility (including the old,
suboptimal API), but will inform users about the better alternative via
a warning.
sternenseemann
force-pushed
the
ghc-with-packages-overrideable
branch
from
90a0d42 to
ac72c1a
Compare
ofborg
bot
added
10.rebuild-darwin: 1-10
ofborg
bot
added
10.rebuild-linux: 1
|
@sternenseemann I think this looks good. You want to merge this in? |
|
Overriding was already possible before this PR with the slightly different (haskellPackages.ghcWithPackages (ps: with ps; [mtl])).override { withLLVM = true; }Now this is broken. Sigh... @sternenseemann please update https://haskell4nix.readthedocs.io/frequently-asked-questions.html#how-to-get-an-environment-with-ghc-and-llvm as well (on Github at https://github.com/NixOS/cabal2nix/tree/master/doc) |
|
Oh, hugh. To be honest, the previous override seems much more plausible for me. Did you simply not know that possibility, or are there arguments for the one over the other? |
Ah, very annoying, this did not occur to me at all, sorry! I'll amend the documentation and changelog as well. I also think we can add a thing that causes the old scheme to generate a nicer error message. Not sure if I had done the same had I known this, but with the deed being done I do think it's an improvement, since it's overall a bit clearer and more consistent with the rest of nixpkgs and allows for overriding |
This comment has been minimized.
This comment has been minimized.
4 participants
Motivation for this change
Make
ghcWithPackages.override { withLLVM = true; } (p: [ ... ])work.Things done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)nixos/doc/manual/md-to-db.shto update generated release notes