ghc, haskell-infra: Improve cross-compilation #40642
Conversation
|
|
||
| , version ? "8.5.20180118" | ||
| , ghcRevision ? "e1d4140be4d2a1508015093b69e1ef53516e1eb6" |
There was a problem hiding this comment.
Is it really necessary to parameterize all aspects of the src attribute as function arguments? I would assume that few people actually mess with these settings, and those who want to can easily override the src attribute as a whole.
There was a problem hiding this comment.
Right! that should better be handed like this once we have the generic expression. Will drop.
| , description ? "" | ||
| , doCheck ? !isCross && (stdenv.lib.versionOlder "7.4" ghc.version) | ||
| , doBenchmark ? false | ||
| , doVerbose ? false |
There was a problem hiding this comment.
In the presence of buildFlags this attribute seems redundant?
There was a problem hiding this comment.
I agree that we can drop this in favor or passing -v3 to the buildFlags.
| (optionalString (versionOlder "8.4" ghc.version) (enableFeature enableStaticLibraries "static")) | ||
| # --enable-static does not work on windows. This is a bug in GHC. | ||
| # --enable-static will pass -staticlib to ghc, which only works for mach-o and elf. | ||
| (optionalString (!hostPlatform.isWindows && versionOlder "8.4" ghc.version) (enableFeature enableStaticLibraries "static")) |
There was a problem hiding this comment.
If someone requests static builds for a given platform, then it feels like poor style to just ignore the setting and to silently build shared libraries instead. I'd prefer to see this particular limitation expressed as an assertion instead.
There was a problem hiding this comment.
I'll amend the PR, to
enableStaticLibraries ? !hostPlatform.isWindows
and a
assert (enableStaticLibraries && hostPlatform.isWindows) "can't use `--enable-static` on windows."
The reason why this does not work is not that we can't built static objects, we can, but we can't use
-staticlib on ghc on windows. -staticlib rolls all dependencies into a combined archive. While this
would work on windows if we used gnu ar and MRI script, GHC can't rely on gnu ar, and as such has
a quck archive concatenation module for gnu and bsd archives only.
There was a problem hiding this comment.
On Nix we do indeed provide GNU ar always, so it would be nice to optionally configure GHC to expect GNU binutils just as one can configure GCC for the same reasons.
There was a problem hiding this comment.
I'd rather prefer some runtime flag over a build time flag, or even some runtime support int he form of trying to use an MRI script and falling back to the backup solution.
| , version ? "8.4.2" | ||
| , ghcFlavour ? "" |
There was a problem hiding this comment.
Can you please add a small comment here explaining that "" means "use ghc's default choice"?
peti
changed the title
|
@peti, thanks for the quick review, I'll make some amendments later today. |
|
|
||
| # What flavour to build. An empty string indicates no | ||
| # specific flavour and falls back to ghc default values. | ||
| , ghcFlavour ? "" |
There was a problem hiding this comment.
, ghcFlavour = stdenv.lib.optionalString (targetPlatform != hostPlatform) "perf-cross"
There was a problem hiding this comment.
Can we also backport this change to the earlier GHCs for consistency?
There was a problem hiding this comment.
We can. But I'd argue against it. I'd rather see us align the 8.{4,6}.y and head into a common form, and then graft previous version onto the common.nix. Otherwise we are going to end up backporting this stuff every time again across all expressions.
| @@ -38,11 +46,14 @@ let | |||
| "${targetPlatform.config}-"; | |||
|
|
|||
| buildMK = '' | |||
| BuildFlavour = ${if targetPlatform != hostPlatform then ghcCrossFlavour else ghcFlavour} | |||
There was a problem hiding this comment.
Simple ${ghcFlavour} per the above
| @@ -165,7 +176,11 @@ stdenv.mkDerivation rec { | |||
| # zsh and other shells are smart about `{ghc}` but bash isn't, and doesn't | |||
| # treat that as a unary `{x,y,z,..}` repetition. | |||
| postInstall = '' | |||
| paxmark m $out/lib/${name}/bin/${if targetPlatform != hostPlatform then "ghc" else "{ghc,haddock}"} | |||
| for f in $out/lib/${name}/bin/${if targetPlatform != hostPlatform then "ghc" else "{ghc,haddock}"}; do | |||
There was a problem hiding this comment.
Can we just do $out/lib/${name}/bin/* then?
| @@ -38,11 +46,14 @@ let | |||
| "${targetPlatform.config}-"; | |||
|
|
|||
| buildMK = '' | |||
| BuildFlavour = ${if targetPlatform != hostPlatform then ghcCrossFlavour else ghcFlavour} | |||
| ifneq \"\$(BuildFlavour)\" \"\" | |||
There was a problem hiding this comment.
Nix doesn't require " to be escaped here, to be clear. Does make?
| @@ -24,7 +24,15 @@ | |||
| # platform). Static libs are always built. | |||
| enableShared ? !targetPlatform.useAndroidPrebuilt | |||
There was a problem hiding this comment.
You wish to allow dynamic linking here but not 8.2?
There was a problem hiding this comment.
I don't follow? That line wasn't even changed?
| @@ -68,6 +69,10 @@ in | |||
|
|
|||
| assert editedCabalFile != null -> revision != null; | |||
|
|
|||
| # --enable-static does not work on windows. This is a bug in GHC. | |||
| # --enable-static will pass -staticlib to ghc, which only works for mach-o and elf. | |||
| assert hostPlatform.isWindows -> enableStaticLibraries = false; | |||
| @@ -54,10 +68,11 @@ let | |||
| EXTRA_CC_OPTS += -std=gnu99 | |||
| ''; | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
blank line wasn't added in 8.4.1. In general it's nice to diff the different GHCs so they don't unnecessarily drift apart. We really should deduplicate soon...
| @@ -268,6 +278,28 @@ stdenv.mkDerivation ({ | |||
| for f in "$packageConfDir/"*.conf; do | |||
| sed -i "s,dynamic-library-dirs: .*,dynamic-library-dirs: $dynamicLinksDir," $f | |||
| done | |||
| '') + (optionalString (isCross && setupHaskellDepends != []) '' | |||
There was a problem hiding this comment.
Can you abstract this part out like https://github.com/NixOS/nixpkgs/pull/39735/files#diff-d65852670e35e11b4bd12ed1c634abce ? Fine to wait on separate derivations until #39735 to avoid controversy though :)
There was a problem hiding this comment.
I thought about it, but I kept the Setup.hs changes out of this diff on purpose.
|
I'm going to rebase onto #40442 after I merge it to staging. |
Ericson2314
force-pushed
the
feature/clean-ghc
branch
from
008d6f0
to
934d825
Compare
…Inputs This is because they are just for Setup.hs, so they are just used at build time and completely isolated from the normal components' dependencies. This was previous implemented in 8a8f040, but reverted in e69c7f5 because it broken setup-depends non-cross in haskell shell environments (custom Setup.hs in cross shell environments has never worked). This version adds a special native exception to avoid that breakage.
…taticlib The reason why this does not work is not that we can't built static objects, we can, but we can't use `-staticlib` on GHC on windows. `-staticlib` rolls all dependencies into a combined archive. While this would work on windows if we used gnu ar and MRI script, GHC can't rely on GNU ar, and as such has a quick archive concatenation module for GNU and BSD archives only.
Shell glob works even as the exact set of executable (filenames) varries beween configuations.
nixpkgs#37012 and nixpkgs#37707 introduces the setup-hooks for libiconv, which inject `-liconv` into the `NIX_LDFLAGS`. This breaks horribly on windows where the linker end up having no idea how to linke `-liconv`. The configure.ac file specifically ignores libiconv on windows.
Ericson2314
force-pushed
the
feature/clean-ghc
branch
from
934d825
to
4b48094
Compare
Ericson2314
force-pushed
the
feature/clean-ghc
branch
from
4b48094
to
934d825
Compare
Ericson2314
changed the title
Ericson2314
changed the title
Ericson2314
force-pushed
the
feature/clean-ghc
branch
from
934d825
to
4b48094
Compare
GrahamcOfBorg
added
2.status: merge conflict
and removed
2.status: merge conflict
labels
|
@peti I wanted to land this somewhere pronto so as to enable more work, but if you could also merge this into master via your normal |
|
It's not that easy, unfortunately, because the patch does not apply to current versions of |
|
I'll prepare another PR then. Thanks for checking. |
ghc, haskell infra: #40642 direct to master
…master Revert "ghc, haskell infra: NixOS#40642 direct to master"
Motivation for this change
This adds some minor changes to the GHC expressions, and tries to align the head and 8.4.2 expression. Ideally we'd have one common one among all ghc expressions I think.
This should allow building quick flavors, and building cross compilers to windows.
Things done
build-use-sandboxinnix.confon non-NixOS)nix-shell -p nox --run "nox-review wip"./result/bin/)