-
-
Notifications
You must be signed in to change notification settings - Fork 14.7k
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
Haskell multiple outputs: bin etc lib (take three) #32629
Conversation
7378681
to
8751bdc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lightly skimmed, untested ACK. Although as a nit the sprinkled getBins seems a bit ugly, but I have no improvements to recommend. 😞
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After 2 failed attempts to get these changes merged, I would like someone to actually use this code before merging it because verifying the new code by means of Hydra only has clearly failed before. I.e. it would be great if someone would confirm, explicitly, "yes, I have used the code in this PR for the following non-trivial use-cases X and Y for the last 2 weeks and I can confirm that it all works as expected".
@@ -10,7 +10,7 @@ | |||
let | |||
inherit (bootPkgs) ghc; | |||
version = "8.2.1"; | |||
|
|||
preReleaseName = "ghc-8.2.1"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this for?
@@ -20,9 +20,9 @@ stdenv.mkDerivation rec { | |||
sed -ie 's|ld |${targetPackages.stdenv.cc.bintools}/bin/ld |g' src/scripts/ldkernel.in | |||
''; | |||
configureFlags = stdenv.lib.optional (!enableIntegerSimple) [ "--enable-gmp" ]; | |||
propagatedNativeBuildInputs = [ alex happy ]; | |||
propagatedNativeBuildInputs = [ (stdenv.lib.getBin alex) (stdenv.lib.getBin happy) ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that a reference like alex
refers to the output by default that's defined first. Is that correct?
If that is so indeed, then we should make sure that the first output defined by Haskell packages is the one containing the binaries so that these references don't need to be wrapped in getBin
all the time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bin
is the first one, this code is just trying to be explicit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that being explicit in this case is not an advantage since it makes the code harder to read without adding any functional benefit. People might be confused by those explicit calls, wondering: "Why is this necessary?" Personally, I'd be in favor of dropping getBin
calls whereever possible. Rather than being explicit, we should document that bin
can be relied upon to be the first output
of all Haskell derivations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't need to be specifically documented for Haskell since it should hold for all of nixpkgs: https://github.com/NixOS/nixpkgs/blob/master/doc/multiple-output.xml#L39
@peti I have compiled https://github.com/input-output-hk/cardano-sl with it (non trivial project, 300+ dependencies) and it builds. Haven't deployed it yet. |
Created jobset to test all packages at https://hydra.nixos.org/jobset/nixpkgs/pr-32629 |
f73e524
to
a0b51de
Compare
@GrahamcOfBorg eval |
According to https://hydra.nixos.org/eval/1418670?compare=trunk#tabs-new there are following outstanding darwin failures: |
|
@vcunat ghcWithPackages installs also |
Hmm, that might be configurable in |
@vcunat fixed :) |
I still need to fix cabal-helper on darwin, but otherwise this is ready for testing. |
@Ericson2314 @shlevy @zimbatm @jb55 @ttuegel @basvandijk @nh2 @nc6 @Boothead @bennofs @Gabriel439 @taktoa @Profpatsch @dezgeg @ryantrinkle @abbradar @rickynils Could I ask you to test this PR with your projects to catch any possible regressions? |
@@ -45,7 +45,7 @@ let | |||
let hlib = haskell.lib; | |||
elmRelease = import ./packages/release.nix { inherit (self) callPackage; }; | |||
elmPkgs' = elmRelease.packages; | |||
elmPkgs = elmPkgs' // { | |||
elmPkgs = lib.mapAttrs (name: value: hlib.disableSharedExecutables value) (elmPkgs' // { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this correct? I'm not a user of the elm packages, but this line looks wrong to me: it only disable shared executables for "outside" users of the elm packages, but if any package in the elmPkgs set depends on another one, it will get the shared executable version (b.c. the fixpoint set isn't changed. the modification is applied after the fixpoint).
For example, if we had this situtation:
elmPkgs = lib.mapAttrs lib.mapAttrs (name: value: hlib.disableSharedExecutables value) ({
elm-exe-a = ... derivation depending on self.elm-exe-b
elm-exe-b = ... some executable ...
})
then elm-exe-a
will get the shared version of elm-exe-b
, but elmPkgs.elm-exe-b
will refer to the non-shared version. Maybe it does not matter in this case though because no elm package depends on executables from another one? Or is it actually meant to be only for the "outside"? (why?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, need to properly override the scope. Couldn't find an easy way to do that without explicitly enumerating all elm packages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, this will do a few more rebuilds but it's a proper override.
wxc = (addBuildDepend super.wxc self.split).override { wxGTK = pkgs.wxGTK30; }; | ||
wxc = (overrideCabal super.wxc (drv: { | ||
buildDepends = (drv.buildDepends or []) ++ [self.split]; | ||
postInstall = "cp -v dist/build/libwxc.so.0.92.3.0 $lib/lib/libwxc.so"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this related to the multiple-outputs changes or something else? Hardcoding the exact version number here also seems a little brittle. And why is the copy needed, shoudn't cabal install take care to install the wxc library?
${optionalString (hasLibOutput && ! enableSeparateDataOutput) '' | ||
# Just like for doc output path in $out potentially landing in | ||
# *.conf, we have to also remove the data directory so that it | ||
# doesn't appear under data-dir field creating a cycle. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks to me like it has potential for silently breaking packages: if a library needs access to data files and enableSeparateDataOutput
is not set, we will brick the library, right? (because references will be replaced with non-existent files).
Can we limit this to only .conf
files, and have the build break in other cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch.
@domenkozar Can you fix conflicts/bring up to date with master and ghc 8.2.2? Then I'll test on our projects. |
a0348e7
to
e41e02a
Compare
@shlevy done |
@GrahamcOfBorg eval |
|
@shlevy that was just a missing |
|
Sadly on darwin, we always create links file in $lib to workaround sierra linker limit. This means we have to always create the lib output even if isLibrary is false. Moreover, binaries in $bin reference $out even if references files don't exist. Probably needs fixing in GHC itself. generic-builder for now just dereferences $bin to $out. This is fragile if a package uses custom setup.
Reduces closure size from 2.7GB to ~190MB (where most is python+nodejs) This optimization is now possible since bin is a separate output but we can build $lib so that other libraries can depend upon any elm component.
Darwin builds always have a lib *output* but may not produce a lib *directory* inside of it when they are not library packages and the executable does not require dylibs for circumventing Sierra linker limits.
It is just uuagc itself but preprocessed by uuagc. Not haddocking should save some space and build time.
The purpose here is to address the failure mode where $lib is not created. Additionally this reduces the number of store paths and nars.
fb50809
to
981458a
Compare
They are many references to this PR fixing open Haskell issues. What's the status of it? |
It's kind of broken until staging is merged, then we can asses it's status. |
New todo:
|
@@ -222,7 +253,15 @@ assert allPkgconfigDepends != [] -> pkgconfig != null; | |||
stdenv.mkDerivation ({ | |||
name = "${pname}-${version}"; | |||
|
|||
outputs = [ "out" ] ++ (optional enableSeparateDataOutput "data") ++ (optional enableSeparateDocOutput "doc"); | |||
outputs = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, no dev
output? I would expect one to contain all the .o
and .hi
files, plus the GHC package .conf
file.
@@ -246,14 +285,14 @@ stdenv.mkDerivation ({ | |||
|
|||
postPatch = optionalString jailbreak '' | |||
echo "Run jailbreak-cabal to lift version restrictions on build inputs." | |||
${jailbreak-cabal}/bin/jailbreak-cabal ${pname}.cabal | |||
${stdenv.lib.getBin jailbreak-cabal}/bin/jailbreak-cabal ${pname}.cabal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not needed to call the getBin
here. The rule of multiple outputs is that ${foo}/bin/bar
will always, always do the right thing.
Can this also split out profiling builds? That would be useful. |
If we scale down ambitions of this PR and just get optional |
Seems like that's the new plan then. |
Addresses failures found in #32112
Fixes #32082
Detailed changelog is in commits, but shortly:
@peti I'd kindly ask you for a jobset including darwin, since I can't build all of the packages myself.
cc @nc6