-
-
Notifications
You must be signed in to change notification settings - Fork 14k
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 infra: Fix cross compilation to work with new system #26799
Conversation
@Ericson2314, thanks for your PR! By analyzing the history of the files in this pull request, we identified @peti, @kosmikus and @pikajude to be potential reviewers. |
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.
In addition to in-line comments, any way we can reduce duplication here?
# | ||
# We generally want the latest llvm package set, which would normally be | ||
# `llvmPackages` on most platforms. But on Darwin, the default is the version | ||
# released with OSX, so we force 3.9, which is the correct version at this |
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.
nixpkgs convention is to override versions like this at the callPackage site, not here.
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.
Now overridden in pkgs/top-level/haskell-packags,nix
.
|
||
# If enabled GHC will be build with the GPL-free but slower integer-simple | ||
, ncurses, gmp, libffi | ||
, __targetPackages |
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.
Can you document this and other non-package non-obvious arguments?
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 is technically something bound in all-packages, rather than a ghc-specific "configuration argument", but sure. Do note that in #26805 I add __targetInputs
and __propagatedTargetInputs
so there is no need for this parameter.
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.
Now documented in manual
# library instead of the faster but GPLed integer-gmp library. | ||
, enableIntegerSimple ? false, gmp | ||
enableIntegerSimple ? targetPlatform != hostPlatform |
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.
Why? Is it hard to build cross-gmp?
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.
Honestly, I probably skipped it at first issue before, and never returned. Good catch.
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.
, # If enabled, use -fPIC when compiling static libs. | ||
enableRelocatedStaticLibs ? targetPlatform != hostPlatform | ||
|
||
, # TODO: Make false by default |
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.
Why is this a todo instead of just... doing it? 😁
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.
Not sure what this is anymore, but there's no more TODO 😁
, # Whether to build dynamic libs for the standard library (on the target | ||
# platform). Static libs are always built. | ||
dynamic ? let triple = targetPlatform.config; | ||
# On iOS, dynamic linking is not supported |
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.
Not supported by ghc or by the target? Because iOS does allow dynamic linking these days.
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 forget 😲!
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.
Done now.
"--with-iconv-libraries=${__targetPackages.libiconv}/lib" | ||
] ++ stdenv.lib.optionals (targetPlatform != hostPlatform) [ | ||
|
||
# TODO: next rebuild make these unconditional |
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.
Just open this against staging and do it now IMO
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.
Done.
@Ericson2314 Your call if you want to delay this until that is in, no rush on my end |
preConfigure = commonPreConfigure; | ||
#v p dyn | ||
preConfigure = stdenv.lib.optionalString (targetPlatform != hostPlatform)'' | ||
sed 's|#BuildFlavour = quick-cross|BuildFlavour = perf-cross|' mk/build.mk.sample > mk/build.mk |
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 whole section of writing to mk/build.mk
can be done with just one I/O operation using sed just as a stream editor, and the HEREDOC feature of bash. I know we all have fast I/O devices, but it would make the code shorter too.
preConfigure = commonPreConfigure; | ||
#v p dyn | ||
preConfigure = stdenv.lib.optionalString (targetPlatform != hostPlatform)'' | ||
sed 's|#BuildFlavour = quick-cross|BuildFlavour = perf-cross|' mk/build.mk.sample > mk/build.mk |
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.
Idem
59249b9
to
23eea08
Compare
8dcbb41
to
fff31cb
Compare
9a9fbb4
to
33b7086
Compare
These changes will affect the final derivation
Rather than just sticking it on the PATH
That way the next commit can apply a similar diff to each GHC.
GHC currently handles this stuff in a quite non-standard way, basically taking prog var `FOO` to mean `FOO_FROM_TARGET`. It's because it (wrongly) thinks from stage 2's perspective.
There's no reason to wait for non-binary native to *build* cross ghc, but we want a nix-built GHC for Setup.hs or things won't work.
Hello World with ghcHEAD. ghc822 to come after some patches. Android will be turned into real Android...later.
Cabal2nix expects a --compiler flag that contains a Cabal Compiler description. We used to use the compiler's derivation name for this, but this breaks when cross-compiling due to the target suffix. Instead we add an explicit haskellCompilerName attribute to Haskell compiler derivations.
33b7086
to
bf68790
Compare
@peti just rebased to fix a conflict. Do the changes look good to you? Builds passed decently before; hope to merge imminently! |
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.
Looks good to me.
Yay!!!!! |
Woo, congratulations! 🎉
…On Mon, 22 Jan 2018, 20:33 John Ericson, ***@***.***> wrote:
Yay!!!!!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#26799 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AA0U3J2NaUtikAyMBdNyqcBLoh-GxMe7ks5tNPCDgaJpZM4OD-Qa>
.
|
/me wonders what the purpose of that |
Eh, no real reason. I guess I was diffing things maybe I dunno. |
Motivation for this change
This is a re-doing of the Haskell support for cross compilation to fit in with my other cross refactoring. As a cheat sheet, with the new system:
haskell.packages.ghc{HEAD,821}.ghc
, orbuildPackages.haskell.compiler.ghc{HEAD,821}.ghc
haskell.packages.ghc{HEAD,821}.fooBarBaz
There's other PRs that need to be merged first, but I am opening this now so people can get a flavor for what's to come.
Fixes #21874
Things to do
Optional changes
Get rid ofActually, the--with
flags.--with
flags are needed for GHC's own dynamic linker, long story short.Add tests (for both Linux and Darwin) in
pkgs/top-level/release-cross.nix
. Linux should work right now, and darwin Later. Done for GHC 8.2.2 and GHC HEAD from x86_64 linux to 32-bit and 64-bit arm.Update to a newer GHC commit with @bgamari's and @angerman's fixes (thanks!) so this actually builds. GHC HEAD is good to go, but GHC 822 needs one more patch to land to I have a more permanent URL: https://phabricator.haskell.org/D4287
Misc simple things
configurePlatforms
to control passing of--build
,--host
, and--target
.hackage2nix
.targetPrefix
is bound on GHC, so we don't need to recompute it all over the place.crossPrefix
orprefix
, from earlier versions, are goneCC @expipiplus1 @dtzWill @dhess