Skip to content

Commit

Permalink
Revert "cabal-install configureCompiler: configure progdb"
Browse files Browse the repository at this point in the history
This reverts commit 8bdda9c.

In configureCompiler the call to configureAllKnownPrograms was too
eager. When called it selected the version of tools from PATH (such as
alex), and then when a package was configured these tools were passed
using `--with-alex` options to configure, which meant that the
build-tool-depends versions were not used. (See haskell#10692)

Why was this call introduced in the first place? Because
configureCompiler would a different result depending on whether:

* It is run for the first time, the `ProgramDb` will contain
  unconfigured programs.
* It is run subsequently, `ProgramDb` is read from disk, it does not
  contain unconfigured programs.

Reverting this commit rexposes the bug that the serialised ProgramDb
will not contain UnconfiguredPrograms (in the case where reconfiguring
is avoided).

However, there are no code paths which require this logic in
`cabal-install` currently. The configuration phase happens again each
time that `Cabal` is called, with a populated `ProgramDb`. We will
fix this before the next major `cabal-install` release, but it would not
be suitable to backport.

In the future we will fix this properly by refactoring
`configureCompiler` so that `ProgramDb` is not serialised. The general
approach will be to make `configCompilerEx` return a pair of configured
programs (`ghc` and `ghc-pkg`) and expose an additional function from
`Cabal` which uses these two programs to perform the modifications to
the `ProgramDb` which `configCompilerEx` performs.

Also see haskell#2238 and haskell#9840

Fixes haskell#10692
  • Loading branch information
mpickering authored and sheaf committed Jan 15, 2025
1 parent 529bd1f commit fafcb55
Show file tree
Hide file tree
Showing 3 changed files with 53 additions and 8 deletions.
51 changes: 45 additions & 6 deletions cabal-install/src/Distribution/Client/ProjectPlanning.hs
Original file line number Diff line number Diff line change
Expand Up @@ -484,6 +484,7 @@ configureCompiler
let fileMonitorCompiler = newFileMonitor $ distProjectCacheFile "compiler"

progsearchpath <- liftIO $ getSystemSearchPath

rerunIfChanged
verbosity
fileMonitorCompiler
Expand All @@ -499,7 +500,7 @@ configureCompiler
let extraPath = fromNubList packageConfigProgramPathExtra
progdb <- liftIO $ prependProgramSearchPath verbosity extraPath [] defaultProgramDb
let progdb' = userSpecifyPaths (Map.toList (getMapLast packageConfigProgramPaths)) progdb
(comp, plat, progdb'') <-
result@(_, _, progdb'') <-
liftIO $
Cabal.configCompilerEx
hcFlavor
Expand All @@ -516,17 +517,55 @@ configureCompiler
-- programs it cares about, and those are the ones we monitor here.
monitorFiles (programsMonitorFiles progdb'')

-- Configure the unconfigured programs in the program database,
-- as we can't serialise unconfigured programs.
-- See also #2241 and #9840.
finalProgDb <- liftIO $ configureAllKnownPrograms verbosity progdb''
-- Note: There is currently a bug here: we are dropping unconfigured
-- programs from the 'ProgramDb' when we re-use the cache created by
-- 'rerunIfChanged'.
--
-- See Note [Caching the result of configuring the compiler]

return (comp, plat, finalProgDb)
return result
where
hcFlavor = flagToMaybe projectConfigHcFlavor
hcPath = flagToMaybe projectConfigHcPath
hcPkg = flagToMaybe projectConfigHcPkg

{- Note [Caching the result of configuring the compiler]
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
We can't straightforwardly cache anything that contains a 'ProgramDb', because
the 'Binary' instance for 'ProgramDb' discards all unconfigured programs.
See that instance, as well as 'restoreProgramDb', for a few more details.
This means that if we try to cache the result of configuring the compiler (which
contains a 'ProgramDb'):
- On the first run, we will obtain a 'ProgramDb' which may contain several
unconfigured programs. In particular, configuring GHC will add tools such
as `ar` and `ld` as unconfigured programs to the 'ProgramDb', with custom
logic for finding their location based on the location of the GHC binary.
- On subsequent runs, if we use the cache created by 'rerunIfChanged', we will
deserialise the 'ProgramDb' from disk, which means it won't include any
unconfigured programs, which might mean we are unable to find 'ar' or 'ld'.
This is not currently a huge problem because, in the Cabal library, we eagerly
re-run the configureCompiler step (thus recovering any lost information), but
this is wasted work that we should stop doing in Cabal, given that cabal-install
has already figured out all the necessary information about the compiler.
To fix this bug, we can't simply eagerly configure all unconfigured programs,
as was originally attempted, for a couple of reasons:
- it does more work than necessary, by configuring programs that we may not
end up needing,
- it means that we prioritise system executables for built-in build tools
(such as `alex` and `happy`), instead of using the proper version for a
package or package component, as specified by a `build-tool-depends` stanza
or by package-level `extra-prog-path` arguments.
This lead to bug reports #10633 and #10692.
See #9840 for more information about the problems surrounding the lossly
Binary ProgramDb instance.
-}

------------------------------------------------------------------------------

-- * Deciding what to do: making an 'ElaboratedInstallPlan'
Expand Down
6 changes: 4 additions & 2 deletions cabal-testsuite/PackageTests/ExtraProgPath/setup.out
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
# cabal v2-build
Warning: cannot determine version of <ROOT>/pkg-config :
""
Configuration is affected by the following files:
- cabal.project
Warning: cannot determine version of <ROOT>/pkg-config :
""
Warning: cannot determine version of <ROOT>/pkg-config :
""
Resolving dependencies...
Error: [Cabal-7107]
Could not resolve dependencies:
Expand Down
4 changes: 4 additions & 0 deletions changelog.d/pr-10731
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
synopsis: Fix regression where build-tool-depends are not used
packages: cabal-install
prs: #10731
issues: #10633 #10692

0 comments on commit fafcb55

Please sign in to comment.