haskellPackages: support template-haskell and test suites when cross compiling#445672
haskellPackages: support template-haskell and test suites when cross compiling#445672alexfmpe wants to merge 4 commits intoNixOS:haskell-updatesfrom
Conversation
d163368 to
2110df8
Compare
| pname = "iserv-proxy"; | ||
| version = "9.3"; | ||
|
|
||
| # https://github.com/stable-haskell/iserv-proxy/pull/1 |
There was a problem hiding this comment.
There was a problem hiding this comment.
Should probably talk to the stable haskell/haskell.nix folks what the state is there, but given that haskell.nix depends on it in some capacity, I'm not worried it will go away suddenly?
73bd537 to
0122493
Compare
4840be1 to
3938e32
Compare
3938e32 to
46474da
Compare
This comment was marked as outdated.
This comment was marked as outdated.
7f57666 to
5a49a58
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Hmm no, I am actually. Here's where we stand Besides the basic machinery added here, there's a bunch of per-config tweaks that will be needed (e.g. env flags for wine). The reason I special cased On my
After looking at #383165, I'd say that it works for Seems to me the concerns on my PR and yours interact in subtle ways so I'd say we ought to get at least one of them merged so they can then be addressed together. Plently of layers on this onion to peel anyway. |
| iserv-proxy = { | ||
| build = buildHaskellPackages.iserv-proxy; | ||
| host = self.iserv-proxy; | ||
| }; |
There was a problem hiding this comment.
You can add iserv-proxy to nativeBuildInputs, so the iserv-proxy in path will be executable. ${emulator} ${iserv-proxy} will use the host one. This is also a little confusing, but more in line with how nixpkgs usually works. Alternatively, you can use __spliced explicitly. (Replying to https://github.com/NixOS/nixpkgs/pull/445672/files#r2426707459 which github doesn't allow me)
There was a problem hiding this comment.
Hm though this does not work with the inline overrides and symlinkJoin too well, ideally we'd add that to the package set probably.
Aren't there some conditionals gating on >= 9.12? |
|
LGTM, but I agree with @wolfgangwalther that the WDYT @wolfgangwalther? |
|
I'm happy with that approach, too. |
I don't think so? Only thing I can find is that the aarch-multiplatform jobset is for ghc912, but that can be changed to ghc910 or probably just haskellPackages? |
|
I just noticed the dependency on |
| libiserv | ||
| network | ||
| ; | ||
| } |
There was a problem hiding this comment.
While not strictly needed, I grabbed this change ahead of time from #451928 since it allows keeping all 'iserv-proxy-closure' overrides in one place. The trigger being the following commit adding splitmix to the bunch but configuration-nix.nix already had a splitmix override
1bc395f to
0f4ffa9
Compare
There was a problem hiding this comment.
I worked through this PR and rebased. Here are some notes on what I did and noticed:
- I rebased the branch on very latest haskell-updates, which I bumped hackage on
todayyesterday. - The last commit was to bump
iserv-proxy- I squashed that into the first commit and also adjusted the version number with the correct unstable date. - At the first commit, I can build
iserv-proxyfor GHC 9.8, 9.10 and 9.12 - which seems reasonable: The default version + one version in both directions. I can't buildiserv-proxyfor GHC 9.4, GHC 9.6 or GHC HEAD, the latter fails somewhere in the dependencies. - The second commit contained changes specifically for GHC 9.6 and libiserv. But
iserv-proxydoes not build with or without these changes for GHC 9.6, so I backed these out and put them into a fixup to look at later, if at all. We can be happy to be able to do this for GHC 9.8+, I think. - The second commit also contained changes to use the external interpreter for test suites. This contained the following line
exec ${if (isCross && crossSupport.canCheck) then "node" else crossSupport.emulator} "$@". This seems wrong, because why should we always usenodewhen in a cross setting - and if not fallback to the cross emulator? I think the condition should rather be onisGhcjsor so? I removed this for now, to reduce scope a bit - and to prevent rebuild of the default non-cross compiler. This can be introduced in a follow up again, to enable testing with the external interpreter - let's focus on TH as the first step. I kept the relevant changes as a fixup commit for future reference. - With the second commit, I am able to build
th-orphans, which was not possible in any cross setting before:- aarch64 -> x86_64 (gnu)
- aarch64 -> x86_64 (musl)
- x86_64 -> aarch64 (gnu)
- (did not test musl for x86 -> aarch64)
- Notably the things that didn't work on the second commit were:
pkgsStatic- which failed witherror: Dynamic loading not supported- I believe this is fixed by the third commit, but the conditions (android || musl) seem wrong. I think this should be conditioned onisStatic.- aarch64-darwin fails to build the compiler. Haven't looked deeper into that, yet.
pkgsCross.aarch64-android-prebuiltfailed with:qemu-aarch64: Could not open '/system/bin/linker64': No such file or directory.
I just pushed my current state. I will let some builds finish and then plan to push the first few commits to haskell-updates already, but not merge the full PR, yet. We can then iterate on the remaining commits, where I still have question marks for me.
| # Compiling the readme throws errors and has no purpose in nixpkgs | ||
| aeson-gadt-th = disableCabalFlag "build-readme" super.aeson-gadt-th; | ||
|
|
There was a problem hiding this comment.
This change doesn't make sense to me. Building the readme is still not required and we have plenty of other packages to build to verify that TH works, right?
There was a problem hiding this comment.
This is one package I know of where the executable uses TH, so I was using it as a sanity check.
Haven't found a case where TH libs work but an executable doesn't so I don't feel strongly about it.
| fi | ||
|
|
||
| exec "$@" | ||
| exec ${if (isCross && crossSupport.canCheck) then "node" else crossSupport.emulator} "$@" |
There was a problem hiding this comment.
This condition seems very wrong - it will use node in a regular cross setting. Shouldn't this have some component of checking for isGhcjs or so?
There was a problem hiding this comment.
Huh yeah stuff got lost in the last rebase
| lib.optionals | ||
| (targetPlatform.isAndroid || (targetPlatform.isMusl && hostPlatform != targetPlatform)) | ||
| [ | ||
| "*.ghc.cabal.configure.opts += --flags=-dynamic-system-linker" |
There was a problem hiding this comment.
The condition here is wrong, according to my experiments. Cross with musl works fine, but once we do isStatic it doesn't. Quite obviously.
I'm not sure whether we should just check isStatic - or whether we should actually check enableShared, which includes isStatic and more.
Given that the error message for android is:
qemu-aarch64: Could not open '/system/bin/linker64': No such file or directory
... I wonder whether we should actually add the isAndroid condition to enableShared to begin with?
There was a problem hiding this comment.
I tried the other cases in enableShared, i.e. windows, iOSPrebuilt, ghcjs - but I can't even get to a sensible error message for the first two and ghcjs works regardless. So I guess we can keep it at isStatic here for now.
0f4ffa9 to
b613cdb
Compare
Fixes error: "iserv-proxy-interpreter: Failed to lookup symbol: __gmpn_cmp" This comes up when building `pkgsStatic.haskell.packages.ghc...`, aka without `.native-bignum` modifier.
These changes were split from the commit before, keeping for future reference for now.
b613cdb to
87ba7fb
Compare
|
OK, I pushed a good chunk of this to haskell-updates already. This should make regular cross (x86 -> aarch64) and pkgsStatic work. Will look into the other / remaining bits here next - still building a ton of stuff right now. |
wolfgangwalther
left a comment
There was a problem hiding this comment.
I tested more stuff again and I don't think any of the remaining bits are ready to merge right now.
How should we proceed - would you like to take these and open separate PRs for them, with more detailed reasoning and showing how they help? Or should we keep them together in this PR?
| ] | ||
| # Inform GHC that we can't load dynamic libraries which forces iserv-proxy to load static libraries. | ||
| ++ lib.optionals targetPlatform.isStatic [ | ||
| ++ lib.optionals (targetPlatform.isAndroid || targetPlatform.isStatic) [ |
There was a problem hiding this comment.
You had the android condition here, but I didn't merge it, yet and instead put it into a separate commit.
I can't make android work with it, I still always get:
qemu-aarch64: Could not open '/system/bin/linker64': No such file or directory
I tried:
NIXPKGS_ALLOW_UNFREE=1 nix-build -A pkgsCross.aarch64-android-prebuilt.haskell.packages.native-bignum.ghc910.th-orphans
And the same for GHC 9.12 - it fails with the same error.
GHC 9.8 fails because it can't apply an android related patch, I doubt you had been testing with that before.
What did you do that made the isAndroid condition here an improvement?
There was a problem hiding this comment.
qemu-aarch64: Could not open '/system/bin/linker64': No such file or directory
That happens with any android build that requires the dynamic linker, unless you run it on an android system
$ NIXPKGS_ALLOW_UNFREE=1 nix-build -A pkgsCross.aarch64-android-prebuilt.hello
/nix/store/f60sv415dxv7am0f06lm956jn2ni4nja-hello-aarch64-unknown-linux-android-2.12.2
$ file result/bin/hello
result/bin/hello: ELF 64-bit LSB pie executable, ARM aarch64, version 1 (SYSV), dynamically linked, interpreter /system/bin/linker64, not stripped
$ ./result-qemu-user/bin/qemu-aarch64 result/bin/hello
qemu-aarch64: Could not open '/system/bin/linker64': No such file or directory
$ NIXPKGS_ALLOW_UNFREE=1 nix-build -A pkgsCross.aarch64-android-prebuilt.pkgsStatic.hello
/nix/store/5jzni4r3gyi0aj7hbx9w39xwn9c523sh-hello-static-aarch64-unknown-linux-musl-2.12.2
$ file result/bin/hello
result/bin/hello: ELF 64-bit LSB executable, ARM aarch64, version 1 (SYSV), statically linked, not stripped
$ ./result-qemu-user/bin/qemu-aarch64 result/bin/hello
Hello, world!
What did you do that made the isAndroid condition here an improvement?
IIRC it made TH work for pkgsCross.aarch64-android-prebuilt.pkgsStatic but that config is a strange creature, as stated in the PR description.
GHC 9.8 fails because it can't apply an android related patch, I doubt you had been testing with that before.
Yeah, I only got android to work on 9.10+. I think something on GHC configure would need backporting for 9.8
There was a problem hiding this comment.
IIRC it made TH work for
pkgsCross.aarch64-android-prebuilt.pkgsStaticbut that config is a strange creature, as stated in the PR description.
Yeah these things (pkgsCross + pkgsStatic) don't compose, so that is not a relevant result. See #380342 and related PRs.
That happens with any android build that requires the dynamic linker, unless you run it on an android system
Right, so this means that iserv-proxy TH will only ever work this way, if we're building with isStatic = true. Which in turn means, that we don't need the isAndroid condition here anymore, because we already condition on isStatic.
| "--with-ffi-libraries=${targetLibs.libffi.out}/lib" | ||
| ] | ||
| ++ lib.optionals (targetPlatform == hostPlatform && !enableNativeBignum) [ | ||
| ++ lib.optionals (!enableNativeBignum) [ |
There was a problem hiding this comment.
I tested this, and I believe this makes pkgsStatic.haskell.packages.ghc98.th-orphans (same for 9.10 and 9.12, i.e. without native-bignum) work - but only on x86_64. It still failed for me on aarch64 with similar errors as before (not exactly the same, but similar).
I believe we should never enable gmp when linking statically, due to licensing issues. I'd say we should even default pkgsStatic.haskell.packages.ghc[...] to the native-bignum variants, without the modifier present.
Thus, I'm not keen on merging this change right now.
Yeah better to open separate PRs for the remaining concerns, I've been sitting on a dozen commits for weeks waiting for the main stuff to be merged anyway as it wasn't feasible to throw more stuff on this one nor open dependent PRs and having cascading rebases. |
Just noted that the upstream iserv-proxy PR now contains https://github.com/stable-haskell/iserv-proxy/pull/1/changes/dc144516464d3b4f4107b1765873473758aae1dc..91ef7ffdeedfb141a4d69dcf9e550abe3e1160c6, which seems to fix something for GHC < 9.7, so it might be worth looking into 9.6 again here. |
|
|
|
Adapted from
https://github.com/input-output-hk/haskell.nix/blob/c0454391b7b026387623289d3deb4ce9b431a528/overlays/linux-cross.nix
Also unblocks meaningful work on cross testing since TH is used in every other suite for test discovery, including QuickCheck'sEDIT: Also run test suites ( pkgsCross.ghcjs ones need #451527 )Moved to #451928Building/running
haskell.packages.ghc912.aeson-gadt-thon x86_64-linux:Note
aarch64-android-prebuilt.pkgsStaticbuilds for the wrong reasons aswhich leads to problems down the line (e.g when trying to add system tools). However, trying
fails when building TH with
iserv-proxy-interpreter: internal error: ASSERTION FAILED: file rts/linker/Elf.c, line 812which is https://gitlab.haskell.org/ghc/ghc/-/blob/ghc-9.12.2-release/rts/linker/Elf.c#L812
Example run
Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.Add a 👍 reaction to pull requests you find important.