Hadrian: improve bindist rule
ClosedPublic

Authored by alpmestan on Thu, Nov 22, 11:10 AM.

Details

Summary

As outlined in Trac #15925, hadrian bindists had not made a clear choice with
respect to relocatable GHCs and wrapper scripts. This commit implements
the policy described in the ticket. That is:

  • the bindists ship {bin, lib} as they are, modulo the addition of haddock from stage2/bin
  • we now _always_ generate wrapper scripts for all the programs that are in the bindist's bin/ directory

The idea being that anyone on Linux/Windows/OS X can just unpack
the binary distribution anywhere and start using bin/ghc, while the
installation process systematicaly generates wrapper scripts.

Test Plan

hadrian/build.sh binary-dist ; cd _build/bindist/ghc-X.Y.Z-arch/; configure --prefix=/tmp/foo && make install

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
alpmestan created this revision.Thu, Nov 22, 11:10 AM
alpmestan edited the test plan for this revision. (Show Details)Thu, Nov 22, 11:19 AM
bgamari requested changes to this revision.Thu, Nov 22, 1:44 PM

Thanks for doing this!

Can we add a Note describing the policy regarding wrappers? It would also be nice to explicitly state the reason *why* the wrappers are necessary and how the compiler works without wrappers.

This revision now requires changes to proceed.Thu, Nov 22, 1:44 PM

Like @bgamari, I'd like to have a note about wrappers somewhere in the code, as otherwise it's always a matter of googling for hours in order to find out more information about them.

hadrian/src/Rules/BinaryDist.hs
122

At this point, we know that haddock has necessarily been built, to generate the docs.

This comment is misleading/unnecessary. The copyFile haddockPath below will require the Haddock executable to be built, which is a far more reliable guarantee than making assumptions about the code above. Let's just delete this comment, starting straight with "We copy the Haddock binary...".

224

Oh, this looks awkward :) Maybe just exceed the line limit in this case, or move "wrappers" to the next line?

Like @bgamari, I'd like to have a note about wrappers somewhere in the code, as otherwise it's always a matter of googling for hours in order to find out more information about them.

Yes, it is a great suggestion, it's in the works -- I'll push a note tonight.

hadrian/src/Rules/BinaryDist.hs
122

OK, will change.

224

arc complains when the lines exceed the limit, and I either have to enter a good explanation or abort. =)

OK, I'll put it on the next line.

alpmestan updated this revision to Diff 18845.Thu, Nov 22, 4:58 PM
  • add notes about the bindists in general and the story for wrapper scripts
alpmestan marked 4 inline comments as done.Thu, Nov 22, 4:59 PM

Done. I added one note about the way we generate bindists, and one note about wrapper scripts / relocatable GHCs, i.e about the way we consume them. Hopefully things are much clearer now.

alpmestan updated this revision to Diff 18846.EditedThu, Nov 22, 5:00 PM
  • missing ~ in note
snowleopard accepted this revision.Thu, Nov 22, 7:37 PM

@alpmestan Many thanks, this is a very helpful note.

angerman accepted this revision.Thu, Nov 22, 8:17 PM
angerman added inline comments.
hadrian/src/Rules/BinaryDist.hs
68

Are we sure about FreeBSD? or *BSD for that?

alpmestan marked an inline comment as done.Thu, Nov 22, 8:41 PM
alpmestan added inline comments.
hadrian/src/Rules/BinaryDist.hs
68

It should be the case, since we landed https://phabricator.haskell.org/D5335.

alpmestan marked 2 inline comments as done.Thu, Nov 22, 8:41 PM
snowleopard retitled this revision from hadrian: improve bindist rule to Hadrian: improve bindist rule.Thu, Nov 22, 8:43 PM

Let me capitalise the title, since Hadrian is a name :-)

bgamari accepted this revision.Sat, Nov 24, 11:24 AM

Thanks for the comment, @alpmestan!

This revision is now accepted and ready to land.Sat, Nov 24, 11:24 AM
This revision was automatically updated to reflect the committed changes.