Navigation Menu

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

ghc:8.10.2Binary bootstrap for 8.8 on aarch64 (NixOS#97407) #101214

Merged
merged 1 commit into from Oct 24, 2020

Conversation

lostnet
Copy link
Contributor

@lostnet lostnet commented Oct 20, 2020

Motivation for this change

This is a collection of workarounds for aarch64 to bootstrap ghc8.8.4(default) and ghc8.10.2 using a ghc8.10.2Binary. With these changes these versions of ghc should work as they do on other architectures to the extent that they work on aarch64 (these versions of ghc are considered experimental on Aarch64, and in particular can have race conditions. etc, with parallel builds, discussion of these problems is in issue #97407.)

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 some sample packages, this is the bootstrap for all of the standard ghc collection on aarch64. I tested on a small machine(rock64) and @vcunat tested on aarch64.nixos.community which is faster and more likely to surface parallel build problems as they may occur on hydra.)
  • 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)
    (ghc-8.10.2-binary is very large ~2.9gb on aarch64; large libraries sensative to stripping.)
  • Ensured that relevant documentation is up to date
    (avoided any changes to other platforms and workaround specific bugs without changing default versions on aarch64, etc)
  • Fits CONTRIBUTING.md.

Copy link
Member

@cdepillabout cdepillabout left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a collection of workarounds for aarch64 to bootstrap ghc8.8.4(default) and ghc8.10.2 using a ghc8.10.2Binary. With these changes these versions of ghc should work as they do on other architectures to the extent that they work on aarch64 (these versions of ghc are considered experimental on Aarch64, and in particular can have race conditions. etc, with parallel builds, discussion of these problems is in issue #97407.)

I didn't read through all of #97407, but could you be more explicit in what the purpose of this PR is?

For instance, without this PR, is it not possible to build ghc8.8.4 and ghc8.10.2 on aarch64? (If so, then we should probably prioritize getting this PR in.)

these versions of ghc are considered experimental on Aarch64, and in particular can have race conditions. etc, with parallel builds

Does "these versions of ghc" refer to ghc-8.8.4 and ghc-8.10.2? Or are you specifically referring to ghc8_10_2Binary?

If they are experimental (and can have race conditions with parallel builds), then maybe we will have more problems than it is worth having Hydra build and distribute them?

Comment on lines 92 to 97
'' + stdenv.lib.optionalString stdenv.hostPlatform.isAarch64 ''
find . -name package.conf.in \
-exec sed -i "s@FFI_LIB_DIR@FFI_LIB_DIR ${numactl.out}/lib@g" {} \;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a short comment as to why this is needed?

Comment on lines 135 to 155
# Keep rpath as small as possible on aarch64 for patchelf#244
''
(cd $out/lib; ln -s ${ncurses6.out}/lib/libtinfo.so.6)
(cd $out/lib; ln -s ${gmp.out}/lib/libgmp.so.10)
(cd $out/lib; ln -s ${numactl.out}/lib/libnuma.so.1)
for p in $(find "$out/lib" -type f -name "*\.so*"); do
(cd $out/lib; ln -s $p)
done

for p in $(find "$out/lib" -type f -executable); do
if isELF "$p"; then
echo "Patchelfing $p"
patchelf --set-rpath "\$ORIGIN:\$ORIGIN/../.." $p
fi
done
''
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a little more explanation about what is going on here, and why this is needed?

I imagine you want to modify the rpaths as little as possible, so you link the various libraries into a single directory and set the rpaths to that directory.

It would be nice to have a little bit more of an explanation so that someone doesn't come along and remove this section (not understanding that it was necessary).

description = "The Glasgow Haskell Compiler";
license = stdenv.lib.licenses.bsd3;
platforms = ["x86_64-linux" "armv7l-linux" "aarch64-linux" "i686-linux" "x86_64-darwin"];
maintainers = with stdenv.lib.maintainers; [ lostnet ];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Comment on lines +91 to +96
bootPkgs = if stdenv.isAarch64 then
packages.ghc8102Binary
else
packages.ghc865Binary;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might want to add a comment here as well explaining why this is necessary (so that someone doesn't accidentally try to change it in the future).

@vcunat
Copy link
Member

vcunat commented Oct 21, 2020

For instance, without this PR, is it not possible to build ghc8.8.4 and ghc8.10.2 on aarch64? (If so, then we should probably prioritize getting this PR in.)

@cdepillabout: yes, ghc on aarch64 is completely broken now, at least on Hydra.nixos.org (and thus no binaries from cache.nixos.org). That holds for 20.09 and master (20.03 seems OK). Example: https://hydra.nixos.org/eval/1620539?filter=ghc

@lostnet
Copy link
Contributor Author

lostnet commented Oct 21, 2020

Does "these versions of ghc" refer to ghc-8.8.4 and ghc-8.10.2? Or are you specifically referring to ghc8_10_2Binary?

This refers to ghc-aarch of any current form. I think a bug fixed in 8.10.1 is the cause of the parallelbuilds segv being demonstrated with build time SEGVs in 8.6.5Binary (and non-Binary for me) which is why I trust that binary to back bootstrap over trying to bring a 8.8Binary in.

If they are experimental (and can have race conditions with parallel builds), then maybe we will have more problems than it is worth having Hydra build and distribute them?

I think it is worth having some official ghc release on aarch64, there are plenty of compilers that have bugs but are better than nothing. For 8.8, I hope it has less problems than 8.6.5, but I don't think it will be easy to quantify without some usage. The objection to moving the default to 8.10.2 is stackage LTS.

I will try to address the comments with code comments. I think the libnuma fix is an example of bit rot caused by no bootstrap, when libnuma was added to fix the 8.10.2Binary, resulting binaries were not tested due to the segv and rts now needs to link libnuma on aarch64 binaries.

@cdepillabout
Copy link
Member

Thanks for clarifying things!

It sounds like this is a pretty important change, so let's merge it in after @lostnet adds a little more documentation.

@lostnet
Copy link
Contributor Author

lostnet commented Oct 21, 2020

Thanks @cdepillabout! I've added the comments that hopefully clarify/warn about the several different aarch64 quirks and verified it still doesn't change the x86_64 hashes and has the same aarch64 hashes from the initial push.

@cdepillabout
Copy link
Member

LGTM

@lostnet Thanks for taking the time to figure this out and documenting everything.

I'll merge this in, but please try to keep an eye on Hydra the next couple days and see if it ends up being able to build Haskell packages on aarch64 again!

@vcunat
Copy link
Member

vcunat commented Oct 24, 2020

Not really merged yet, for some reason?

I'd suggest merging into the staging-next branch instead of master, as aarch64 is quite loaded on Hydra and it should save one haskellPackages rebuild.

@cdepillabout cdepillabout changed the base branch from master to staging-next October 24, 2020 08:32
@cdepillabout
Copy link
Member

@vcunat Thanks for the message. Looks like I had just forgotten to actually merge this in!

I changed the base branch to staging-next as per your suggestion.

@cdepillabout cdepillabout merged commit 81881f2 into NixOS:staging-next Oct 24, 2020
@domenkozar
Copy link
Member

This should also go to 20.09-staging

@vcunat
Copy link
Member

vcunat commented Oct 28, 2020

I think it can go directly in 20.09, but I'd wait for staging-next results first. aarch64 is now a bit overloaded on Hydra, too, so doing both at once will just make both late.

@lostnet
Copy link
Contributor Author

lostnet commented Oct 28, 2020

@vcunat it looks to me like a cherry pick with the commits that create/modify 8.10.2-binary.nix should be fine on 20.09, so you probably wont need an external PR? I've been watching staging-next on hydra, but it looks like it could be another day or two at its current pace..

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

Successfully merging this pull request may close these issues.

None yet

4 participants