Hadrian: support extra libraries + OSX rpath
AbandonedPublic

Authored by harpocrates on Dec 3 2018, 11:55 PM.

Details

Summary

This fixes some of the issues that surfaced when trying to build
dynamic GHC on OSX. Unfortunately, due some other libffi
issues, this doesn't completely fix dynamic builds on OSX.

  • Use 'extra-libraries' from .cabal files instead of hardcoding which packages need which extra libs. Also add support for 'extra-lib-dirs'.
  • Make sure Hadrian looks in the right places to support both plain '<pkg>.buildinfo' and '<pkg>.buildinfo.in' files.
  • Make the '-rpath' support more robust across OS's (it previously didn't work on OSX and possibly windows either).
harpocrates created this revision.Dec 3 2018, 11:55 PM
harpocrates updated this revision to Diff 18993.Dec 4 2018, 1:30 AM
  • guard 'ghci's 'ffi' extra-lib behind a flag
alpmestan requested changes to this revision.Dec 4 2018, 1:37 AM

Very nice!

I just have two questions/requests for now. I'll spin up some Linux builds to make sure this didn't break anything there.

hadrian/src/Context.hs
67

And we need both this one and the "normal" distDir?

It would be nice to have tiny teeny notes about when each one is used.

hadrian/src/Settings/Builders/Ghc.hs
72

We get rid of gmpLibs options here but I don't think they make it back under another form. In particular, they are not covered by extra-libraries yet apparently, since I don't see any such field in integer-gmp.cabal for example: https://git.haskell.org/ghc.git/blob/HEAD:/libraries/integer-gmp/integer-gmp.cabal

Am I missing something?

This revision now requires changes to proceed.Dec 4 2018, 1:37 AM
harpocrates updated this revision to Diff 18995.Dec 4 2018, 2:16 AM
  • Rename 'cabalDistDir' into 'distDir'
harpocrates marked 2 inline comments as done.Dec 4 2018, 2:17 AM
harpocrates added inline comments.
hadrian/src/Context.hs
67

Nope, we don't need both. Let's stick with the distDir name. :)

hadrian/src/Settings/Builders/Ghc.hs
72

Yeah, a bunch of extra-libraries are tucked away in buildinfo.in files. See https://git.haskell.org/ghc.git/blob/HEAD:/libraries/integer-gmp/integer-gmp.buildinfo.in for the integer-gmp case.

harpocrates updated this revision to Diff 18996.Dec 4 2018, 3:00 AM
harpocrates marked 2 inline comments as done.
  • guard 'ghci's 'ffi' extra-lib behind a flag
  • Rename 'cabalDistDir' into 'distDir'
alpmestan added inline comments.Dec 4 2018, 4:02 AM
hadrian/src/Settings/Builders/Ghc.hs
72

Ha! Thanks, I missed that.

angerman requested changes to this revision.Dec 4 2018, 7:41 AM

The ghci ffi guard breaks the build. Without it, this builds for me on macOS in ~40min for the default build.

libraries/ghci/ghci.cabal.in
24–25 ↗(On Diff #18996)

This defaults to False on macOS

73–75 ↗(On Diff #18996)

if we want to guard this for windows only (though that's kind of a weird guard), wouldn't

if !os(windows):

work?

This revision now requires changes to proceed.Dec 4 2018, 7:41 AM
DavidEichmann added inline comments.Dec 4 2018, 7:42 AM
hadrian/src/Oracles/Setting.hs
172

I think windows doesn't support 'rpath'.

  • Address review comments
harpocrates marked 3 inline comments as done.Dec 4 2018, 10:36 AM
harpocrates added inline comments.
hadrian/src/Oracles/Setting.hs
172

Quite right. Also looks like freebsd does (I'm going off Cabal and GHC's logic for this).

libraries/ghci/ghci.cabal.in
73–75 ↗(On Diff #18996)

I put this back to what it was before, so it'll work on mac. Looking at the code, seems to me the ffi symbols are needed on all platforms. I'll just wait for someone with a windows machine to report.

Tamar, could you please have a look wrt. to libffi and windows?

Phyx added a comment.Dec 7 2018, 1:16 AM

Sure, I will take a look after work today

Phyx added a comment.Dec 8 2018, 2:52 AM

Just some small comments, I am waiting on the build to finish.

hadrian/src/Oracles/Setting.hs
172

It does support it when lazy binding, but since GHC doesn't currently support dynamic way on Windows this is fine. but i'd appreciate a TODO so I can find it later.

hadrian/src/Settings/Builders/Ghc.hs
62

Same here, keep the TODO for Windows.

Phyx added a comment.Dec 8 2018, 7:55 AM

I don't know if it's related, but I tried twice but the build seems to fail with

Up to date
Up to date
copyFile: does not exist (The system cannot find the file specified.)
shakeArgsWith   0.002s    0%
Function shake  1.009s    1%
Database read   0.663s    1%
With database   0.040s    0%
Running rules  53.702s   96%  =========================
Total          55.416s  100%
Error when running Shake build system:
  at src/Rules.hs:(34,19)-(47,17):
  at src/Rules.hs:47:5-17:
* Depends on: _build/stage1/lib/package.conf.d/rts-1.0.conf
* Raised the exception:
ExitFailure 1
harpocrates planned changes to this revision.Dec 11 2018, 6:00 PM

I tried this out in a Windows VM and it indeed doesn't work (the extra-libraries: ffi in libraries/ghci/ghci.cabal.in is what is messing it up - take that out and Windows builds, but Mac doesn't). I've obviously not fully grasped the complexity of the libffi situation. It would be great if there were a note somewhere summarizing what is supposed to be happening.

That said, I'm out of time for now, so I'll just bump this out of the review queue for now.

  • Add some TODOs
harpocrates retitled this revision from Hadrian: support extra libraries + dynamic GHC to Hadrian: support extra libraries + OSX rpath.Dec 17 2018, 10:54 AM
harpocrates edited the summary of this revision. (Show Details)
harpocrates edited the test plan for this revision. (Show Details)

Unless someone has a good reason otherwise, I think we should merge this as is.

I've updated the summary to emphasize that this does not completely fix dynamic GHC on OSX, but it does fix a handful of other problems which are important in their own right. Besides, it looks like the libffi issue is being tackled in a handful of other patches.

harpocrates marked 7 inline comments as done.Dec 17 2018, 10:59 AM

Keep in mind that the dynamic ghc build has been disabled in D5430 due to the failing hadrian build.

Friendly ping!

Is there anything that is blocking this from being merged?

Is the patch on gitlab too? (for CI)

Is the patch on gitlab too? (for CI)

It is now: https://gitlab.haskell.org/ghc/ghc/merge_requests/86.