From 24493f7b2a82ce9c002cc6ab6cf6d5ef2475af1b Mon Sep 17 00:00:00 2001 From: Rodrigo Mesquita Date: Wed, 24 Jan 2024 09:19:03 +0000 Subject: [PATCH] Improves romes:todos into more general todos --- Cabal/src/Distribution/Simple/Build/Inputs.hs | 5 ++- Cabal/src/Distribution/Simple/GHC/Build.hs | 18 +++++------ .../Simple/GHC/Build/ExtraSources.hs | 14 ++++----- .../src/Distribution/Simple/GHC/Build/Link.hs | 31 ++++++++----------- .../Distribution/Simple/GHC/Build/Modules.hs | 19 +++++++----- 5 files changed, 42 insertions(+), 45 deletions(-) diff --git a/Cabal/src/Distribution/Simple/Build/Inputs.hs b/Cabal/src/Distribution/Simple/Build/Inputs.hs index a2c4a66bff0..48b3b60a12b 100644 --- a/Cabal/src/Distribution/Simple/Build/Inputs.hs +++ b/Cabal/src/Distribution/Simple/Build/Inputs.hs @@ -32,9 +32,8 @@ import Distribution.Types.LocalBuildInfo import Distribution.Types.TargetInfo import Distribution.Verbosity --- | The information required for a build computation (@'BuildM'@) --- which is available right before building each component, i.e. the pre-build --- component inputs. +-- | The information required for a build computation which is available right +-- before building each component, i.e. the pre-build component inputs. data PreBuildComponentInputs = PreBuildComponentInputs { buildingWhat :: BuildingWhat -- ^ What kind of build are we doing? diff --git a/Cabal/src/Distribution/Simple/GHC/Build.hs b/Cabal/src/Distribution/Simple/GHC/Build.hs index 4f50309ad39..cc50e3bdb3c 100644 --- a/Cabal/src/Distribution/Simple/GHC/Build.hs +++ b/Cabal/src/Distribution/Simple/GHC/Build.hs @@ -91,11 +91,11 @@ build numJobs pkg_descr pbci = do -- See Note [Build Target Dir vs Target Dir] as well _targetDir <- liftIO $ makeRelativeToCurrentDirectory targetDir_absolute buildTargetDir <- - -- ROMES:TODO: To preserve the previous behaviour, we don't use relative - -- dirs for executables. Historically, this isn't needed to reduce the CLI - -- limit (unlike for libraries) because we link executables with the module - -- names instead of passing the path to object file -- that's something - -- else we can now fix after the refactor lands. + -- To preserve the previous behaviour, we don't use relative dirs for + -- executables. Historically, this isn't needed to reduce the CLI limit + -- (unlike for libraries) because we link executables with the module names + -- instead of passing the path to object file -- that's something else we + -- can now fix after the refactor lands. if isLib then liftIO $ makeRelativeToCurrentDirectory buildTargetDir_absolute else return buildTargetDir_absolute @@ -105,8 +105,8 @@ build numJobs pkg_descr pbci = do -- Determine in which ways we want to build the component let wantVanilla = if isLib then withVanillaLib lbi else False - -- ROMES:TODO: Arguably, wantStatic should be "withFullyStaticExe lbi" for - -- executables, but it was not before the refactor. + -- Arguably, wantStatic should be "withFullyStaticExe lbi" for executables, + -- but it was not before the refactor. wantStatic = if isLib then withStaticLib lbi else not (wantDynamic || wantProf) wantDynamic = case component of CLib{} -> withSharedLib lbi @@ -125,8 +125,8 @@ build numJobs pkg_descr pbci = do -- otherwise, we take just one. (if isLib then id else take 1) $ [ProfWay | wantProf] - -- ROMES:TODO: I don't see why we shouldn't build with dynamic - -- indefinite components (being instantiated?). + -- I don't see why we shouldn't build with dynamic + -- indefinite components. <> [DynWay | wantDynamic && not (componentIsIndefinite clbi)] <> [StaticWay | wantStatic || wantVanilla || not (wantDynamic || wantProf)] diff --git a/Cabal/src/Distribution/Simple/GHC/Build/ExtraSources.hs b/Cabal/src/Distribution/Simple/GHC/Build/ExtraSources.hs index 12dac910b75..07ad6ac31d8 100644 --- a/Cabal/src/Distribution/Simple/GHC/Build/ExtraSources.hs +++ b/Cabal/src/Distribution/Simple/GHC/Build/ExtraSources.hs @@ -152,8 +152,10 @@ buildExtraSources description componentSourceGhcOptions wantDyn viewSources ghcP comp = compiler lbi platform = hostPlatform lbi - -- ROMES:TODO: Instead of keeping this logic here, we really just want to - -- receive as an input the `neededWays` and build accordingly + -- Instead of keeping this logic here, we really just want to + -- receive as an input the `neededWays` from GHC/Build.build and build + -- accordingly, since we've already determined the extra needed ways + -- needed for e.g. template haskell. Although we'd have to account for 'wantDyn'. isGhcDynamic = isDynamic comp doingTH = usesTemplateHaskellOrQQ bi forceSharedLib = doingTH && isGhcDynamic @@ -193,14 +195,12 @@ buildExtraSources description componentSourceGhcOptions wantDyn viewSources ghcP needsRecomp <- checkNeedsRecompilation sourceFile opts when needsRecomp $ runGhcProg opts + -- TODO: This whole section can be streamlined to the + -- wantedWays+neededWays logic used in Build/Modules.hs createDirectoryIfMissingVerbose verbosity True odir case targetComponent targetInfo of -- For libraries, we compile extra objects in the three ways: vanilla, shared, and profiled. -- We suffix shared objects with .dyn_o and profiled ones with .p_o. - -- - -- ROMES:TODO: Should we use those suffixes for extra sources for - -- executables too? We use those suffixes for haskell objects for - -- executables ... (see gbuild) CLib _lib -- Unless for repl, in which case we only need the vanilla way | BuildRepl _ <- buildingWhat -> @@ -216,7 +216,7 @@ buildExtraSources description componentSourceGhcOptions wantDyn viewSources ghcP -- For foreign libraries, we determine with which options to build the -- objects (vanilla vs shared vs profiled) CFLib flib - | withProfExe lbi -> -- ROMES:TODO: doesn't sound right "ProfExe" for FLib... + | withProfExe lbi -> -- It doesn't sound right to query "ProfExe" for a foreign library... compileIfNeeded profSrcOpts | withDynFLib flib && wantDyn -> compileIfNeeded sharedSrcOpts diff --git a/Cabal/src/Distribution/Simple/GHC/Build/Link.hs b/Cabal/src/Distribution/Simple/GHC/Build/Link.hs index 04295340b82..ab80a152268 100644 --- a/Cabal/src/Distribution/Simple/GHC/Build/Link.hs +++ b/Cabal/src/Distribution/Simple/GHC/Build/Link.hs @@ -82,17 +82,16 @@ linkOrLoadComponent ghcProg pkg_descr extraSources (buildTargetDir, targetDir) ( cleanedExtraLibDirsStatic <- liftIO $ filterM doesDirectoryExist (extraLibDirsStatic bi) let - -- ROMES:TODO: If we fix the order to C++ then C, then we cannot keep this matching the - -- previous behaviour, because it would report C objects first, then C++ - -- objects. Delete this comment after acknowledge. extraSourcesObjs = map (`replaceExtension` objExtension) extraSources + -- TODO: Shouldn't we use withStaticLib for libraries and something else + -- for foreign libs in the three cases where we use `withFullyStaticExe` below? linkerOpts rpaths = mempty { ghcOptLinkOptions = PD.ldOptions bi ++ [ "-static" - | withFullyStaticExe lbi -- ROMES:TODO: wb withStaticLib and flibs?? + | withFullyStaticExe lbi ] -- Pass extra `ld-options` given -- through to GHC's linker. @@ -101,15 +100,11 @@ linkOrLoadComponent ghcProg pkg_descr extraSources (buildTargetDir, targetDir) ( programOverrideArgs (lookupProgram ldProgram (withPrograms lbi)) , ghcOptLinkLibs = - -- ROMES:TODO: Looks wrong, why would we only check for fully - -- static exec, when we could be building libs or foreign libs? - -- We used to use this predicate for libraries too... if withFullyStaticExe lbi then extraLibsStatic bi else extraLibs bi , ghcOptLinkLibPath = toNubListR $ - -- ROMES:TODO: what about withStaticLib and flibs?? if withFullyStaticExe lbi then cleanedExtraLibDirsStatic else cleanedExtraLibDirs @@ -127,8 +122,7 @@ linkOrLoadComponent ghcProg pkg_descr extraSources (buildTargetDir, targetDir) ( replOpts = staticOpts { -- Repl options use Static as the base, but doesn't need to pass -static. - -- ROMES:TODO: It could, but it didn't use to before the - -- refactor. I think it would be more uniform to just pass the flag. + -- However, it maybe should, for uniformity. ghcOptDynLinkMode = NoFlag , ghcOptExtra = Internal.filterGhciFlags @@ -143,7 +137,7 @@ linkOrLoadComponent ghcProg pkg_descr extraSources (buildTargetDir, targetDir) ( -- linker stuff too, like -l flags and any .o files from C -- files etc. -- - -- ROMES:TODO: The repl doesn't use the runtime paths from linkerOpts + -- TODO: The repl doesn't use the runtime paths from linkerOpts -- (ghcOptRPaths), which looks like a bug. After the refactor we -- can fix this. `mappend` linkerOpts mempty @@ -252,16 +246,18 @@ linkLibrary buildTargetDir cleanedExtraLibDirs pkg_descr verbosity runGhcProg li ] ] - -- ROMES:TODO: I'm fairly certain that, just like the executable, we can keep just the + -- I'm fairly certain that, just like the executable, we can keep just the -- module input list, and point to the right sources dir (as is already -- done), and GHC will pick up the right suffix (p_ for profile, dyn_ when - -- -shared...). - -- That would mean linking the lib would be just like the executable, and we could more easily merge the two. + -- -shared...). The downside to doing this is that GHC would have to + -- reconstruct the module graph again. + -- That would mean linking the lib would be just like the executable, and + -- we could more easily merge the two. -- -- Right now, instead, we pass the path to each object file. ghcBaseLinkArgs = mempty - { -- ROMES:TODO: This basically duplicates componentGhcOptions. + { -- TODO: This basically duplicates componentGhcOptions. -- I think we want to do the same as we do for executables: re-use the -- base options, and link by module names, not object paths. ghcOptExtra = hcStaticOptions GHC libBi @@ -321,7 +317,8 @@ linkLibrary buildTargetDir cleanedExtraLibDirs pkg_descr verbosity runGhcProg li , ghcOptInputFiles = toNubListR staticObjectFiles , ghcOptOutputFile = toFlag staticLibFilePath , ghcOptLinkLibs = extraLibs libBi - , ghcOptLinkLibPath = toNubListR $ cleanedExtraLibDirs -- ROMES:TODO: why not extra dirs *static*??? + , -- TODO: Shouldn't this use cleanedExtraLibDirsStatic instead? + ghcOptLinkLibPath = toNubListR $ cleanedExtraLibDirs } staticObjectFiles <- getObjFiles StaticWay @@ -477,8 +474,6 @@ linkFLib flib bi lbi linkerOpts (wantedWays, buildOpts) targetDir runGhcProg = d let buildName = flibBuildName lbi flib -- There should not be more than one wanted way when building an flib assert (Set.size wantedWays == 1) $ - -- ROMES:TODO: Using the "wantedWay" is a bit senseless here, we likely - -- just want to use the Way of each ForeignLib type. forM_ wantedWays $ \way -> do runGhcProg (linkOpts way){ghcOptOutputFile = toFlag (targetDir buildName)} renameFile (targetDir buildName) (targetDir flibTargetName lbi flib) diff --git a/Cabal/src/Distribution/Simple/GHC/Build/Modules.hs b/Cabal/src/Distribution/Simple/GHC/Build/Modules.hs index 72f548fc55c..0a6c408ee4b 100644 --- a/Cabal/src/Distribution/Simple/GHC/Build/Modules.hs +++ b/Cabal/src/Distribution/Simple/GHC/Build/Modules.hs @@ -151,9 +151,10 @@ buildHaskellModules numJobs ghcProg pkg_descr buildTargetDir wantedWays pbci = d (Internal.componentGhcOptions verbosity lbi bi clbi buildTargetDir) `mappend` mempty { ghcOptMode = toFlag GhcModeMake - , -- romes:TODO previously we didn't pass -no-link when building libs, - -- but I also think that could result in a bug (e.g. if a lib module - -- is called Main and exports main). So we really want nolink when building libs too. + , -- Previously we didn't pass -no-link when building libs, + -- but I think that could result in a bug (e.g. if a lib module is + -- called Main and exports main). So we really want nolink when + -- building libs too (TODO). ghcOptNoLink = if isLib then NoFlag else toFlag True , ghcOptNumJobs = numJobs , ghcOptInputModules = toNubListR inputModules @@ -176,8 +177,9 @@ buildHaskellModules numJobs ghcProg pkg_descr buildTargetDir wantedWays pbci = d optSuffixFlag "" _ = NoFlag optSuffixFlag pre x = toFlag (pre ++ x) - -- ROMES:TODO: For libs we don't pass -static when building static, leaving it implicit. - -- We should just always pass -static, but we don't want to change behaviour when doing the refactor. + -- For libs we don't pass -static when building static, leaving it + -- implicit. We should just always pass -static, but we don't want to + -- change behaviour when doing the refactor. staticOpts = (baseOpts StaticWay){ghcOptDynLinkMode = if isLib then NoFlag else toFlag GhcStaticOnly} dynOpts = (baseOpts DynWay) @@ -201,8 +203,8 @@ buildHaskellModules numJobs ghcProg pkg_descr buildTargetDir wantedWays pbci = d , ghcOptDynHiSuffix = toFlag (buildWayPrefix DynWay ++ "hi") , ghcOptDynObjSuffix = toFlag (buildWayPrefix DynWay ++ "o") , ghcOptHPCDir = hpcdir Hpc.Dyn - -- ROMES:TODO: We don't pass hcSharedOpts to dyntoo? - -- (baseOtps StaticWay -> hcStaticOptions, not hcSharedOpts) + -- Should we pass hcSharedOpts in the -dynamic-too ghc invocation? + -- (Note that `baseOtps StaticWay = hcStaticOptions`, not hcSharedOpts) } -- Determines how to build for each way, also serves as the base options @@ -221,7 +223,8 @@ buildHaskellModules numJobs ghcProg pkg_descr buildTargetDir wantedWays pbci = d neededWays = wantedWays <> Set.fromList - -- ROMES:TODO: You also don't need this if you are using an external interpreter!! + -- TODO: You also don't need to build the GHC way when doing TH if + -- you are using an external interpreter!! [defaultGhcWay | doingTH && defaultGhcWay `Set.notMember` wantedWays] -- If we need both static and dynamic, use dynamic-too instead of