Only move code to Simple/GHC/Build*

[philderbeast]

As a start for #9389, I moved gbuild to its own module and did the same for buildOrReplLib. I left the exports from Distribution.Simple.GHC as-is. The three modules I added as other-modules. The motivation for doing this was easy file-diff between implementations for the upcoming deduplication.

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.

[philderbeast]

@mpickering would you have started with a move-only module reshuffle like this for the deduplication?

[mpickering]

I'm not sure I would have done because it introduces churn into the code base. I think I would have started by writing some tests/verifying there are some tests which checked the flags produced are correct. However I can appreciate there's more than one way to skin a cat.

[philderbeast]

I think I would have started by writing some tests/verifying there are some tests which checked the flags produced are correct.

OK thanks for the suggestion @mpickering, I'll do that. These are the local let _ :: GhcOptions bindings aren't they?

cabal/Cabal/src/Distribution/Simple/GHC.hs

Lines 1581 to 1699 in 15b5b05

	baseOpts =
	(componentGhcOptions verbosity lbi bnfo clbi tmpDir)
	`mappend` mempty
	{ ghcOptMode = toFlag GhcModeMake
	, ghcOptInputFiles =
	toNubListR $
	if package pkg_descr == fakePackageId
	then filter isHaskell inputFiles
	else inputFiles
	, ghcOptInputScripts =
	toNubListR $
	if package pkg_descr == fakePackageId
	then filter (not . isHaskell) inputFiles
	else []
	, ghcOptInputModules = toNubListR inputModules
	}
	staticOpts =
	baseOpts
	`mappend` mempty
	{ ghcOptDynLinkMode = toFlag GhcStaticOnly
	, ghcOptHPCDir = hpcdir Hpc.Vanilla
	}
	profOpts =
	baseOpts
	`mappend` mempty
	{ ghcOptProfilingMode = toFlag True
	, ghcOptProfilingAuto =
	Internal.profDetailLevelFlag
	False
	(withProfExeDetail lbi)
	, ghcOptHiSuffix = toFlag "p_hi"
	, ghcOptObjSuffix = toFlag "p_o"
	, ghcOptExtra = hcProfOptions GHC bnfo
	, ghcOptHPCDir = hpcdir Hpc.Prof
	}
	dynOpts =
	baseOpts
	`mappend` mempty
	{ ghcOptDynLinkMode = toFlag GhcDynamicOnly
	, -- TODO: Does it hurt to set -fPIC for executables?
	ghcOptFPic = toFlag True
	, ghcOptHiSuffix = toFlag "dyn_hi"
	, ghcOptObjSuffix = toFlag "dyn_o"
	, ghcOptExtra = hcSharedOptions GHC bnfo
	, ghcOptHPCDir = hpcdir Hpc.Dyn
	}
	dynTooOpts =
	staticOpts
	`mappend` mempty
	{ ghcOptDynLinkMode = toFlag GhcStaticAndDynamic
	, ghcOptDynHiSuffix = toFlag "dyn_hi"
	, ghcOptDynObjSuffix = toFlag "dyn_o"
	, ghcOptHPCDir = hpcdir Hpc.Dyn
	}
	linkerOpts =
	mempty
	{ ghcOptLinkOptions =
	PD.ldOptions bnfo
	++ [ "-static"
	| withFullyStaticExe lbi
	]
	-- Pass extra `ld-options` given
	-- through to GHC's linker.
	++ maybe
	[]
	programOverrideArgs
	(lookupProgram ldProgram (withPrograms lbi))
	, ghcOptLinkLibs =
	if withFullyStaticExe lbi
	then extraLibsStatic bnfo
	else extraLibs bnfo
	, ghcOptLinkLibPath =
	toNubListR $
	if withFullyStaticExe lbi
	then cleanedExtraLibDirsStatic
	else cleanedExtraLibDirs
	, ghcOptLinkFrameworks =
	toNubListR $
	PD.frameworks bnfo
	, ghcOptLinkFrameworkDirs =
	toNubListR $
	PD.extraFrameworkDirs bnfo
	, ghcOptInputFiles =
	toNubListR
	[tmpDir </> x | x <- cLikeObjs ++ cxxObjs ++ jsObjs ++ cmmObjs ++ asmObjs]
	}
	dynLinkerOpts =
	mempty
	{ ghcOptRPaths = rpaths
	, ghcOptInputFiles =
	toNubListR
	[tmpDir </> x | x <- cLikeObjs ++ cxxObjs ++ cmmObjs ++ asmObjs]
	}
	replOpts =
	baseOpts
	{ ghcOptExtra =
	Internal.filterGhciFlags
	(ghcOptExtra baseOpts)
	<> replOptionsFlags replFlags
	, ghcOptInputModules = replNoLoad replFlags (ghcOptInputModules baseOpts)
	, ghcOptInputFiles = replNoLoad replFlags (ghcOptInputFiles baseOpts)
	}
	-- For a normal compile we do separate invocations of ghc for
	-- compiling as for linking. But for repl we have to do just
	-- the one invocation, so that one has to include all the
	-- linker stuff too, like -l flags and any .o files from C
	-- files etc.
	`mappend` linkerOpts
	`mappend` mempty
	{ ghcOptMode = toFlag GhcModeInteractive
	, ghcOptOptimisation = toFlag GhcNoOptimisation
	}
	commonOpts
	| needProfiling = profOpts
	| needDynamic = dynOpts
	| otherwise = staticOpts
	compileOpts
	| useDynToo = dynTooOpts
	| otherwise = commonOpts

[mpickering]

Another idea about how to do this refactoring @philderbeast is to keep both the old and new code paths and compile a lot of packages (when running both and comparing the options are exactly the same).

[Mikolaj]

@philderbeast: does Draft and needs_review mean it's not finished, but you want feedback already? If so, makes sense.

[philderbeast]

@Mikolaj I'd left it in draft by mistake, sorry.

[alt-romes]

Good work! By the time we are able to delete the BuildOrRepl module we'll have finished #9389.

As @mpickering suggested, the next step should be developing a method of validating/testing the existing behaviour against the new (improved) behaviour of the refactored, even more generic, gbuild.

[philderbeast]

Thanks for your approval @alt-romes.

One more time, before attaching the merge labels, I carefully checked that code was only moved. For my final check, I checkout out upstream locally and reordered Cabal/src/Distribution/Simple/GHC.hs for a cleaner comparison with the new modules.

I'm sorry if not sticking with the definition order of Cabal/src/Distribution/Simple/GHC.hs in the new modules made the review harder for you, @geekosaur and @alt-romes.

[alt-romes]

Thanks for your approval @alt-romes.

One more time, before attaching the merge labels, I carefully checked that code was only moved. For my final check, I checkout out upstream locally and reordered Cabal/src/Distribution/Simple/GHC.hs for a cleaner comparison with the new modules.

I'm sorry if not sticking with the definition order of Cabal/src/Distribution/Simple/GHC.hs in the new modules made the review harder for you, @geekosaur and @alt-romes.

Thanks! FWIW I reviewed locally using git diff --color-moved which highlights code that moved differently. This makes it easy to spot code that was altered instead of moved only.

Good work!

[philderbeast]

using git diff --color-moved which highlights code that moved differently

Thanks for the tip @alt-romes.

[Mikolaj]

@philderbeast: do you have a good reason for using this particular merge label? If so, we may want to adjust https://github.com/haskell/cabal/pull/9427/files#diff-eca12c0a30e25b4b46522ebf89465a03ba72a03f540796c979137931d8f92055R250

[philderbeast]

do you have a good reason for using this particular merge label?

Yes @Mikolaj, this label seems to be needed for merging from an organisation, see more on #9377 (comment)

[Mikolaj]

Oh, I see. So that's more or less "the PR branch cannot or should not be modified", so we are fine?

[philderbeast]

I don't know the workings of the label fix for merges from organisations. @ulysses4ever you set this label up for these situations, didn't you? I think it may be that haskell/cabal cannot rebase a pull request from an organisation or that there's some github user interface element that is missing for an organisation to enable this.

[ulysses4ever]

I think it's a mix of reasons, primarily: 1) we use a merge bot; 2) Phil uses a GtHub org, not a personal account, to create PRs.

Technical matter aside, we simply don't maintain the info of who may have a reason to use the no-rebase label. I think, the easiest would be if every contributor who has to use that particular label would spell out the reason on every PR. This will free Mikolaj from proding every PR with the label. I know this is not ideal but I'm afraid I don't see a better solution here.

In this case, perhaps, when setting the label I'd welcome a comment saying "the non-rebase label issused because the PR is opened from a fork owned by a GH organization, as opposed to a GH user, and the merge bot can't update such forks". Or some such... And I'd put it on every PR like that...

[Mikolaj]

I'm not so insufferable --- I will learn in time and stop prodding innocent people. :) But I worry that newcomers tend to look at old PRs and imitate them, e.g., the non-standard labels or even merging directly via the button, because we are doing such exceptional things without enough ceremony, so a sentence of explanation wouldn't hurt.

[philderbeast]

I'm happy to leave a note for the label as a comment on each pull request in future.

[Mikolaj]

This got stuck due to changes CI mandatory tests rule. Rebasing...

[Mikolaj]

@mergify rebase

[mergify]

rebase

❌ Pull request can't be updated with latest base branch changes

Mergify needs the author permission to update the base branch of the pull request.
@cabalism needs to authorize modification on its head branch.

[Mikolaj]

@philderbeast if you could rebase manually and force-push, this should unblock the merge

[Mikolaj]

The MacOs curl network outage strikes again. Let me restart...