Skip to content

configureCompiler: separate compiler vs ProgramDb #10993

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sheaf
Copy link
Collaborator

@sheaf sheaf commented Jun 17, 2025

This commit splits up the logic in configureCompiler into two parts:

  1. Configuring the compiler proper, e.g. finding the location and version of GHC.
  2. Creating a program database of attendant programs such as ghc-pkg, haddock, and build tools such as ar, ld. This is done using information about the compiler, such as its location on the filesystem and toolchain information from its settings file.

The main goal is to avoid an issue in which we get the wrong program database from the cache we get from recompilation checking. However, it's not straightforward to trigger the bug, so I don't have a test at the moment. In practice, I expect this change to be invisible to users.

See Note [Caching the result of configuring the compiler] for an explanation of the problem and the design of the solution.


Template B: This PR does not modify behaviour or interface

E.g. the PR only touches documentation or tests, does refactorings, etc.

Include the following checklist in your PR:

@sheaf sheaf marked this pull request as draft June 17, 2025 10:30
Comment on lines +527 to +532
-- Now, **outside** of the caching logic of 'rerunIfChanged', add on
-- auxiliary unconfigured programs to the ProgramDb (e.g. hc-pkg, haddock, ar, ld...).
--
-- See Note [Caching the result of configuring the compiler]
finalProgDb <- liftIO $ Cabal.configCompilerProgDb verbosity hc hcProgDb hcPkg
return (hc, plat, finalProgDb)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the main change: we move out the logic for finding ar, ld etc outside of the recompilation logic. This is because they will typically remain as unconfigured programs until they are needed, which means they would be silently dropped when reloading from cache due to the lossiness of the Binary ProgramDb instance when unconfigured programs are involved.

Copy link
Collaborator

@mpickering mpickering left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change makes a lot of sense to me.

This commit splits up the logic in configureCompiler into two parts:

  1. Configuring the compiler proper, e.g. finding the location
     and version of GHC.
  2. Creating a program database of attendant programs such as ghc-pkg,
     haddock, and build tools such as ar, ld.
     This is done using information about the compiler, such as its
     location on the filesystem and toolchain information from its
     settings file.
@sheaf sheaf force-pushed the wip/separateProgDb branch from d68cb50 to ecf8eb7 Compare June 18, 2025 09:31
@sheaf sheaf marked this pull request as ready for review June 18, 2025 09:33
@sheaf
Copy link
Collaborator Author

sheaf commented Jun 18, 2025

I will not be providing a test for this change, as I think the prior hacky solution was "good enough" to solve the issue with recompilation-checking. I discussed the design with @mpickering and I think the new approach makes a lot more sense than what was there before.

@sheaf sheaf added the merge me Tell Mergify Bot to merge label Jun 18, 2025
@mergify mergify bot added ready and waiting Mergify is waiting out the cooldown period merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days labels Jun 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days merge me Tell Mergify Bot to merge ready and waiting Mergify is waiting out the cooldown period
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants