Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cabal2nix: split into a lightweight version and a wrapper #81125

Merged
merged 1 commit into from Mar 7, 2020

Conversation

@madjar
Copy link
Member

madjar commented Feb 26, 2020

Current, the cabal2nix derivation contains both the executable, and a wrapper
that adds nix and nix-prefetch-scripts, which are required for some
features.

However, when calling callCabal2nix to create a derivation from a cabal file
at evaluation time,
these features are not actually used, but the huge closure of
nix-prefetch-scripts (which includes multiple vcs, as well as python and perl)
still needs to be fetched.

This commit splits cabal2nix into a lightweight version that is a standalone
static binary (cabal2nix-unwrapped), and a wrapper that includes the proper
dependencies in the path for full usage of the command line
utility (cabal2nix).

This commit also switches to the default ghc, to reduce the likelyhood of
building a different ghc when calling callCabal2nix.

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
@madjar madjar requested review from cdepillabout and Infinisil as code owners Feb 26, 2020
@madjar
Copy link
Member Author

madjar commented Feb 26, 2020

@peti Do you know why cabal2nix was using ghc 8.8.1 in the first place?

@ofborg ofborg bot added the 6.topic: haskell label Feb 26, 2020
@madjar madjar force-pushed the madjar:split-cabal2nix branch from b049c69 to d5fe57e Feb 26, 2020
@madjar
Copy link
Member Author

madjar commented Feb 26, 2020

Note for reviewers: I'm not sure what are the best practices for writing such a wrapper, so suggestions are definitely welcome there.

Copy link
Member

cdepillabout left a comment

Can you make this target the haskell-updates branch instead of master?

This seems reasonable to me except for the following three things:

  • There was probably a reason @peti set cabal2nix to use ghc881 instead of ghc865. That said, haskell-updates is switching to ghc882, so maybe this doesn't matter.
  • cabal2nix-unwrapped probably shouldn't be exposed at the top level.
  • symlinkJoin is probably not the right thing to use here, since you're not joining a bunch of derivations. Although, I dunno, maybe its easier than using the normal stdenv.mkDerivation.
@madjar madjar changed the base branch from master to haskell-updates Feb 27, 2020
@madjar madjar force-pushed the madjar:split-cabal2nix branch 3 times, most recently from 776f42d to bb71f8c Feb 27, 2020
@peti peti force-pushed the NixOS:haskell-updates branch from 05357fc to 83c4b73 Feb 28, 2020
@madjar
Copy link
Member Author

madjar commented Feb 28, 2020

@cdepillabout Thanks for the review! Regarding your points:

  • I agree that it was a bit rash to change the ghc along with this change. But I also agree that aligning it to the default version of haskell-updates is probably for the best.
  • I feel too that it's weird to have cabal2nix-unwrapped living at the top-level, but I dont really know where else to put it (apart from inlining it both in cabal2nix and in haskellSrc2nix, which doesn't sound great). Maybe it can be available as cabal2nix.unwrapped, but that might make overriding much harder. Do you have a suggestion?
    EDIT: grep '\-unwrapped' pkgs/top-level/all-packages.nix |wc -l (58) suggest that it's an existing pattern to expose the unwrapped version at the top level.
  • For symlinkJoin, I followed https://nixos.wiki/wiki/Nix_Cookbook#Wrapping_packages, but it shouldn't be hard to rewrite it as a plain mkDerivation. I'll do that now.
@madjar madjar force-pushed the madjar:split-cabal2nix branch from bb71f8c to a26545b Feb 28, 2020
@cdepillabout
Copy link
Member

cdepillabout commented Feb 28, 2020

I feel too that it's weird to have cabal2nix-unwrapped living at the top-level, but I dont really know where else to put it (apart from inlining it both in cabal2nix and in haskellSrc2nix, which doesn't sound great)

Hmm, in make-package-set.nix, can you just get the normal unwrapped version from buildHaskellPackages instead of buildPackages?

edit: Ah, I see that would end up pulling a cabal2nix built with a version of ghc depending on the haskell package set.

Yeah, I agree that we don't have many good solutions here.

I think I am okay with having cabal2nix-unwrapped on the top-level like you have here currently.

I need to try running this once or twice, but I think it should be pretty good as-is.

@madjar
Copy link
Member Author

madjar commented Feb 28, 2020

Not really: we want make-package-set.nix to use the justStaticExecutables version to avoid the gigantic closure of haskell libraries. From there, I thought that is would be nice to make sure the actual cabal2nix binaries are shared between make-package-set.nix and the top-level cabal2nixm which implies using both justStaticExecutables and generateOptparseApplicativeCompletion, and maybe having a way to share these.

On the other hand, we can decide we don't care about the (binary) duplication, and just use justStaticExecutables buildHaskellPackages.cabal2nix in make-package-set.nix. However, this would mean using a different cabal2nix for each version of ghc, which may not be what we want.

In the end, if the duplication of binary is acceptable, then changing make-package-set.nix to use justStaticExecutables buildPackages.haskellPackages.cabal2nix and leaving the top-level unchanged might be a good solution. What do you think?

@cdepillabout
Copy link
Member

cdepillabout commented Feb 28, 2020

@madjar Sorry, I ninja-edited my last comment while you were typing, I think.

I think this PR looks fine as you have it. Give me a day or so to actually play around with this and make sure it is working.

@madjar
Copy link
Member Author

madjar commented Feb 28, 2020

@peti peti force-pushed the NixOS:haskell-updates branch 6 times, most recently from 518e64c to e81ec2f Feb 28, 2020
@madjar
Copy link
Member Author

madjar commented Mar 2, 2020

On second thought, I'm happy with this version that uses mkDerivation.

If you're satisfied with this, give me a sign. I'll make sure to rebase on top of a working commit, and merge.

@cdepillabout
Copy link
Member

cdepillabout commented Mar 3, 2020

@madjar This looks good to me.

I'd like to get @peti to have a quick look at this just in case.

When @peti signs off on it, I'm happy merging this in.


For @peti: the commit in question is a26545b.

mkdir -p $out/bin
ln -s ${cabal2nix-unwrapped}/share $out
ln -s ${cabal2nix-unwrapped}/bin/hackage2nix $out/bin
makeWrapper ${cabal2nix-unwrapped}/bin/cabal2nix $out/bin/cabal2nix \
--prefix PATH ":" "${nix}/bin" \
--prefix PATH ":" "${nix-prefetch-scripts}/bin"

This comment has been minimized.

Copy link
@Infinisil

Infinisil Mar 6, 2020

Member

I recommend this instead:

  --prefix PATH : "${lib.makeBinPath [ nix nix-prefetch-scripts ]}"

makeBinPath takes care of appending /bin and using the bin output if necessary

This comment has been minimized.

Copy link
@madjar

madjar Mar 6, 2020

Author Member

Oh, that's cool! I'll do that right away.

@madjar madjar force-pushed the madjar:split-cabal2nix branch from a26545b to 42dd19b Mar 6, 2020
@madjar
Copy link
Member Author

madjar commented Mar 6, 2020

Rebased on top of haskell-updates, and applied @Infinisil's recommendation.

@Infinisil Infinisil force-pushed the madjar:split-cabal2nix branch from 42dd19b to 4a0fc1a Mar 6, 2020
@Infinisil
Copy link
Member

Infinisil commented Mar 6, 2020

I hope you don't mind, I just pushed a change so it doesn't need a whitelist of paths to be copied from the unwrapped derivation. Makes it a bit more succinct. The symlinkJoin approach is described here: https://nixos.wiki/wiki/Nix_Cookbook#Wrapping_packages

@madjar
Copy link
Member Author

madjar commented Mar 6, 2020

@Infinisil No worries! I think I used the whitelist to avoid copying position, but now that I look again, it seems to be overridden anyway.

@peti peti force-pushed the NixOS:haskell-updates branch 6 times, most recently from 0cad7b0 to 6e2198b Mar 6, 2020
Current, the `cabal2nix` derivation contains both the executable, and a wrapper
that adds `nix` and `nix-prefetch-scripts`, which are required for some
features.

However, when calling `callCabal2nix` to create a derivation from a cabal file
at evaluation time,
these features are not actually used, but the huge closure of
`nix-prefetch-scripts` (which includes multiple vcs, as well as python and perl)
still needs to be fetched.

This commit splits cabal2nix into a lightweight version that is a standalone
static binary (`cabal2nix-unwrapped`), and a wrapper that includes the proper
dependencies in the path for full usage of the command line
utility (`cabal2nix`).

This commit also switches to the default ghc, to reduce the likelyhood of
building a different ghc when calling `callCabal2nix`.
@cdepillabout cdepillabout force-pushed the madjar:split-cabal2nix branch from 4a0fc1a to 9f46e2b Mar 7, 2020
@cdepillabout
Copy link
Member

cdepillabout commented Mar 7, 2020

@GrahamcOfBorg build cabal2nix

@GrahamcOfBorg build cabal2nix-unwrapped


@madjar The haskell-updates branch recently updated, so I rebased this PR again.

I've also tested this with the command:

$ nix-build -E 'with import ./. {}; haskellPackages.callHackageDirect { pkg = "pretty-simple"; ver = "3.2.2.0"; sha256 = "197fmg7j97x6fibzshmqc5h6mbrk7fl3ss5kqy8x8qyd23ai3rpc"; } {}'

As far as I could tell, it looks like it didn't have to pull in a bunch of language runtimes, so it worked as expected.

@cdepillabout cdepillabout merged commit 2167fc9 into NixOS:haskell-updates Mar 7, 2020
17 of 19 checks passed
17 of 19 checks passed
cabal2nix on x86_64-linux
Details
cabal2nix-unwrapped on x86_64-linux
Details
cabal2nix on aarch64-linux No attempt
Details
cabal2nix-unwrapped on aarch64-linux No attempt
Details
Evaluation Performance Report Evaluator Performance Report
Details
cabal2nix on x86_64-darwin Success
Details
cabal2nix-unwrapped on x86_64-darwin Success
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@cdepillabout
Copy link
Member

cdepillabout commented Mar 7, 2020

@madjar Thanks for putting this together!

@madjar madjar deleted the madjar:split-cabal2nix branch Mar 9, 2020
@madjar
Copy link
Member Author

madjar commented Mar 9, 2020

My pleasure!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.