hadrian: build ghc-iserv-prof in addition to ghc-iserv, as it is required for 10+ tests
ClosedPublic

Authored by alpmestan on Oct 24 2018, 7:34 AM.

Details

Summary

Hadrian didn't give us a chance to build a given executable in vanilla
and profiling, simultaneously, under two different names. This patch implements
support for this in general and applies it to ghc-iserv[-prof].

Test Plan

scc001 fails without this patch, passes with it.

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.Oct 24 2018, 7:34 AM

@alpmestan Thank you, this looks good to me. However, have we lost all CI support with the move from GitHub? Can we fix the CI first?

snowleopard added inline comments.Oct 24 2018, 7:52 AM
hadrian/src/Rules/Program.hs
31–43

I wonder if it makes sense to factor this functionality out and put it next to programName somewhere, perhaps renaming to fromProgramName so it's more obvious that these two functions are inverses.

simonmar added inline comments.
hadrian/src/Packages.hs
139–146

It's pretty horrible to have this in the generic code for building packages. Can't it go in some package-specific place?

156–158

We really shouldn't have these package-specific exceptions here.

hadrian/src/Rules/Program.hs
35

Well, it's not just the testsuite! We actually ship this binary with GHC, it's used when you say ghc -fexternal-interpreter -prof.

BTW, this raises a question: I presume we can produce binary distributions with Hadrian, so can we compare the contents of the Hadrian binary distribution with the old one and see if anything else is missing?

snowleopard added inline comments.Oct 24 2018, 10:07 AM
hadrian/src/Packages.hs
139–146

@simonmar We tried both approaches. We used to have a separate file for each package to describe all their quirks, but it seemed overly heavy and indirect. For example, in this specific case we'd have to create a new module for hpcBin that will only contain this particular tweak to programName, which will just add indirection and some additional boilerplate to pipe things together.

We have 50+ packages, but only several of them really have some irregularities. Creating infrastructure for dealing with these irregularities in some compositional manner seems to be an overkill, but I'm happy to reconsider this. Would you like to create a ticket describing how you would like Hadrian to handle package-specific settings related to program names, command line arguments, etc.?

simonmar added inline comments.
hadrian/src/Packages.hs
139–146

Oh, I recall we discussed this before. Great to hear that you experimented with an alternative approach.

These package-specific exceptions really jar with me. I expect the architecture to consist of

  • Generic pieces that are shared by everything, and
  • Specific pieces that apply to particular directories

Why?

  • If I want to know what magic we have for building a particular package, I should be able to look in one place, rather than grepping the whole build system
  • If I add a new package, I would like to put all the customisation for that package in one place
  • If I delete a package, I would like to remove its customisation by deleting things from one place
  • This is how we did it in the old build system: every directory had a ghc.mk file with the specific build stuff for that directory.

but in general it just feels wrong to do it any other way. I'm a little surprised I'm the only one moaning about it! But if it's just me, I'm sure I can live with it.

As for the specifics, I'm not familiar enough with the pieces yet to propose something. Maybe the next time I'm on a transatlantic flight I'll try to hack around and see what I come up with.

cc @simonpj and @ndmitchell who probably have opinions.

@alpmestan Thank you, this looks good to me. However, have we lost all CI support with the move from GitHub? Can we fix the CI first?

I was wondering about this too. @bgamari What plan would you like us to execute on this front? I suppose another harbormaster build on each and every diff is out of the question for now, because in theory that'd require us to double the number of builds, and we don't have the resources. We could use my phab <-> circle ci bridge on demand, I suppose.

hadrian/src/Rules/Program.hs
31–43

Right, the annoying problem is that this direction requires having our hands on the whole list of packages, so it's a less straightforward functionality than programName.

alpmestan added inline comments.Oct 24 2018, 1:10 PM
hadrian/src/Packages.hs
139–146

I do absolutely agree that it's hard at first to pinpoint where something is done and how it works. I have myself been compiling a list of things that ought to be reorganized eventually and have some ideas for that, but my number 1 priority is to build GHCs that behave just the same as the ones built by make. Once we've got this and some CI that runs all tests with hadrian etc (we've never had this so far), I'm all for going for a potentially big refactoring but we cannot safely and reasonably do this until we see a green testsuite run, IMO.

So if you don't mind, let's not derail this differential, which accomplishes a very well defined goal and by doing so fixes a little over 10 test failures. Shall we therefore open a ticket to discuss the hadrian refactoring?

hadrian/src/Rules/Program.hs
35

Right, users will need it to, but this was just to motivate the need for the profiled variants of the packages.

Regarding your bindist question: our bindists (produced with the binary-dist rule) are still rather preliminary, I'm pretty sure they're missing things compared to the make ones, but to be honest absolutely 0% of our focus over the past few months has been over them, but instead on the compiler rules and the testsuite ones. I would definitely expect some more work on the bindist rules before they're "complete enough".

@snowleopard I just launched a hadrian build on Circle CI: https://circleci.com/gh/ghc/ghc-diffs/513

(see here and here for the hadrian parts of the script)

snowleopard added a comment.EditedOct 25 2018, 5:46 AM

@alpmestan Great! Is there a way to migrate our AppVeyor and Travis scripts too?

Could you also explain me how it works in general: will a Hadrian build be triggered for every Phab differential? Where can reviewers find corresponding build results?

snowleopard added inline comments.Oct 25 2018, 5:52 AM
hadrian/src/Packages.hs
139–146

I agree with @alpmestan: there are a few improvements to the Hadrian codebase that we'd like to make, but they are beyond this specific commit. Right now the priority is to finally make it through the testsuite, from which point we can insist that all GHC commits are validated in Hadrian too, finally getting in sync with GHC development (continuously catching up was/is a nightmare).

@simonmar I'm happy to set up a Skype call (or visit in person) to discuss how we can make package-specific settings easier to discover and modify. I'll be in touch.

@alpmestan Great! Is there a way to migrate our AppVeyor and Travis scripts too?

Could you also explain me how it works in general: will a Hadrian build be triggered for every Phab differential? Where can reviewers find corresponding build results?

Right, so I launched this build manually using a phabricator <-> circle ci bridge that I wrote a few months ago and which is currently running on my own server. There are a bunch of "build plans" configured to go through it, see all the ones named "Circle CI: <something>" at https://phabricator.haskell.org/harbormaster/plan/.

Ben started looking into deploying this very recently, now that the haskell infra started moving to another hosting service. I suspect we will be able to afford running one hadrian build (e.g just the x86_64 linux one?) on all differentials, but not much more for now. I don't know the plan exactly though, @bgamari is in a much better position to talk about the CI plans for hadrian.

bgamari accepted this revision.Oct 28 2018, 12:46 PM
bgamari added inline comments.
hadrian/src/Packages.hs
139–146

For what it's worth, I generally agree with @simonmar. The lack of modularity here could turn into a real maintenance nightmare as these exceptional cases accrue. That being said, we shouldn't hold up these fixes for this refactoring (which will likely be significant).

This revision is now accepted and ready to land.Oct 28 2018, 12:46 PM
This revision was automatically updated to reflect the committed changes.