Skip to content

Commit

Permalink
Improves romes:todos into more general todos
Browse files Browse the repository at this point in the history
  • Loading branch information
@alt-romes
alt-romes committed Jan 29, 2024
1 parent b9e1d8e commit 24493f7
Show file tree
Hide file tree
Showing 5 changed files with 42 additions and 45 deletions.
5 changes: 2 additions & 3 deletions Cabal/src/Distribution/Simple/Build/Inputs.hs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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?
Expand Down
18 changes: 9 additions & 9 deletions Cabal/src/Distribution/Simple/GHC/Build.hs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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)]

Expand Down
14 changes: 7 additions & 7 deletions Cabal/src/Distribution/Simple/GHC/Build/ExtraSources.hs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 ->
Expand All @@ -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
Expand Down
31 changes: 13 additions & 18 deletions Cabal/src/Distribution/Simple/GHC/Build/Link.hs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down
19 changes: 11 additions & 8 deletions Cabal/src/Distribution/Simple/GHC/Build/Modules.hs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)
Expand All @@ -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
Expand All @@ -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
Expand Down

0 comments on commit 24493f7

Please sign in to comment.