Fix hadrian default build cannot find -lffi.
Changes PlannedPublic

Authored by DavidEichmann on Dec 7 2018, 5:17 PM.

Details

Reviewers
bgamari
alpmestan
Trac Issues
#15837
Summary

Support building dynamic libffi.
Add special case to install libffi.so (this is a temporary workaround)

Test Plan

Ensure build environment does NOT have a system libffi installed (you may want to use a nix environment).
Then hadrian/build.sh -c --flavour=default

DavidEichmann created this revision.Dec 7 2018, 5:17 PM

This fixes the build on CircleCI: https://gitlab.staging.haskell.org/DavidEichmann/ghc/-/jobs/904. Using Alp's nix environment I was able to recreate the build failure by building the default flavour. In the same environment with this patch build succeeds and I have run the tests in the perf and default flavours. I still see the drop in test failures the we expect from dynamically linking ghc.

bgamari added inline comments.
hadrian/src/Hadrian/Haskell/Cabal/Parse.hs
171

I'll admit, I am very worried about how many package-specific hacks have been sneaking into what should be generic code. As @simonmar pointed out earlier, in many ways this is a regression from the make build system where at least all package-specific logic is contained in a single file.

@davide, can you create two tickets?

  • a ticket reminding us to remove this hack, and
  • a ticket where we can discuss the potential refactoring of Hadrian to keep package-specific hacks better isolated
DavidEichmann marked an inline comment as done.Dec 9 2018, 7:46 AM

You're absolutely right @bgamari, this situation is not ideal, but I do fully intend to continue work on this. The Hadrian build is broken and the correct solution seems non-trivial, hence I saw this hacky solution as somewhat justified.

I've opened the 2 tickets as requested: Trac #16021 and Trac #16020

I think there's a bit of work to do to make this work nicely under all circumstances, but that should otherwise do the trick.

hadrian/src/Hadrian/Haskell/Cabal/Parse.hs
164

Shouldn't this only happen when we build the RTS in at least one dynamic enabled way? Otherwise when we build with the quickest flavour (which doesn't compile anything in a dyn way), we'll be copying those libffi shared libraries around but they'll never be used. Another essential condition is probably that platformSupportsSharedLibs holds.

168

I suppose we don't want to hardcode .so so that this will all work flawlessly on other systems. All the ones that support shared libraries anyway (see platformSupportsSharedLibs, which I think you're already familiar with).

So we can either "guess" the right extension by looking at the OS, or we can just say getDirectoryFiles buildPath' ["libffi.so*", "libffi.dylib*", "libffi.dll*"] or something along those lines, and catch all those cases at once.

hadrian/src/Packages.hs
196

Do we really want to make this context unconditionally dynamic, don't we need it in the vanilla way sometimes too? Perhaps it'd be better to just use libffiContext { way = dynamic } in those use sites?

Relatedly, does this mean we should be iterating over Context Stage1 libffi w | w <- [vanilla, dynamic] ] instead of considering just vanilla (before this patch) or just dynamic (with this patch) ?

After speaking with Alp, we've decided it is safer to simply revert the dynamic linking changes: D5430. This should fix the Hadrian build while a hack free solution can be found.

DavidEichmann planned changes to this revision.Dec 10 2018, 11:00 AM

Funny enough, I just came accross this in the make build system rts/ghc.mk:127:

# This is a little hacky. We don't know the SO version, so we only
# depend on libffi.so, but copy libffi.so*
rts/dist/build/lib$(LIBFFI_NAME)$(soext): libffi/build/inst/lib/lib$(LIBFFI_NAME)$(soext)
	cp libffi/build/inst/lib/lib$(LIBFFI_NAME)$(soext)* rts/dist/build
This comment was removed by DavidEichmann.