Refactor the core component building logic: gbuild vs buildOrReplLib

[alt-romes]

1. Refactors the duplicated buildExtraSources function from gbuild and
   buildOrReplLib into a standalone monadic computation in the context of
   building a component. This refactor allows
   us to share the code for building an extra source amongst the two
   functions.

2. Creates a new module Distribution.Simple.GHC.Build.Modules which, in the
   same spirit as ...GHC.Build.ExtraModules, defines an action which builds
   all the Haskell modules of the component being built.

   This function clarifies and re-implements the logic of building Haskell
   modules in the different possible ways, while accounting for
   Template Haskell special "way requirements", which was previously
   duplicated in a non-obvious manner in gbuild and buildOrReplLib.

   The Note [Building Haskell modules accounting for TH] in that module
   explains the big picture, and the implementation is re-done in light of
   it.

3. Re-work the linker invocations, focusing on preserving existing
   behaviour before simplifying or fixing bugs any further.

Template Β: This PR does not modify cabal behaviour (documentation, tests, refactoring, etc.)

Include the following checklist in your PR:

• Patches conform to the coding conventions.

[alt-romes]

@mergify refresh

[mergify]

refresh

✅ Pull request refreshed

[philderbeast]

Thanks for the sneak peek "live preview" of this today @alt-romes. Should it be converted to draft or is it ready for review? It would be good to have people familiar with GHC development involved in the review.

[andreabedini]

TBH I am not sure of the value of BuildM, at least in the way it is used here. It looks like that, most of the times, the environment it carries is immediately extracted:

buildHaskellModules numJobs ghcProg pkg_descr buildTargetDir wantedWays = do
  verbosity <- buildVerbosity
  component <- buildComponent
  clbi <- buildCLBI
  lbi <- buildLBI
...

This is just off the top of my head. I need to have a close look.

[andreabedini]

Depending on how you read the policy, we might not need to support GHC 8.4 anymore. Last 8.4 is 8.4.4, which was released on 14/10/2018, more than 5 years ago.

[philderbeast]

Leaving TODO comments with this refactor is an easy way to leave a marker for later intent. Right now it would not be so easy to raise an issue pointing at code. Existing lines of code will likely be replaced and we'll not have a permalink to the new lines until they're merged, will we?

[mpickering]

I have debugged with @alt-romes why there are CI failures on 7.6.3 and he is going to investigate how to fix this.

[alt-romes]

Full CI is passing!

I've furthermore tested the changes of the refactor against master by compiling head.hackage with both and diffing the GHC invocations: Nothing is different!

This is ready to be merged, except for the ROMES:TODO: that we still must decide what to do with.

[andreabedini]

This looks really good now, thank you.

I think most of the ROMES:TODO comments can be rephrased as general observations about the code, I think questions would be ok too.

I've furthermore tested the changes of the refactor against master by compiling head.hackage with both and diffing the GHC invocations: Nothing is different!

Fantastic. I know that the CLC uses https://github.com/Bodigrim/clc-stackage#how-to to assess changes. Maybe we should standarise a procedure to assess changes. If you have any notes to share about how you did the test, free free to share them here ☺️

[andreabedini]

Who is wanting dynamic linking (I assume)? Is it that some kind extra sources need to force dynamic linking? Actually it seem to only affect ghcOptFPic, I am not sure. Can you explain in the comment the role of this.
Can we also use a more descriptive rather than a Bool?

[alt-romes]

I think this is about JS sources not requiring particularly to be built in the Dynamic way, even if the GHC is dynamic, because they behave differently from other object files like those from C. It may be as well because GHCJS could not even support dynamic-related flags such as -fPIC.

Really, this was the way it was done before: all extra sources except for GHCJS do "wantDyn = True", and JS sources (for the GHCJS compiler) "wantDyn = False". So I'm just preserving this behaviour.

I could investigate, but I think it would be better not mess around with GHCJS bits, especially because GHC's JS backend behaves somewhat differently to GHCJS and might result in a more uniform story here.

I will improve the comment.

[alt-romes]

Thanks @andreabedini. I've addressed your review, and cleared the ROMES:TODOs comments for more general comments or todos.

[alt-romes]

@andreabedini, regarding the testing strategy:

I didn't do anything super high-tech. Here are a few steps:

Have a GHC wrapper called ghc-new which logs all invocations:

#!/bin/sh

echo "ghc $@" >> "/Users/romes/ghc-dev/cabal-9389/ghc-log-new"

exec "ghc" ${1+"$@"}

And another one called ghc-old, which logs to a different file.

Then, I ran the cabal testuite with two different cabal versions, passing -w ghc-new to one, and -w ghc-old to the other.
Finally, vimdiff ghc-log-new ghc-log-old.

Then I cloned the head.hackage repository twice and built all packages in it with the two different cabals, each logging to a different file. And vimdiff again.

[andreabedini]

Awesome work 👍

[mpickering]

Looks good to me, given the robust testing strategy. It's impossible to review this patch as-is due to the large changes but I am happy to merge if it results in the same output for the tested packages.

[alt-romes]

@mpickering thank you, that makes sense.

I'll proceed with merging. If you think we should wait feel free to remove the merge label.