Skip to content

Conversation

@mergify
Copy link
Contributor

@mergify mergify bot commented Mar 28, 2025

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 #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.

A surgical way to fix this is to avoid configuring the programs, and manually adding back the builtinPrograms to the ProgramDb, so the ProgramDb returned by configureCompiler always contains all the unconfigured programs.

The testcase is not so easy to write because

  • The bug only surfaces when the build-tool you are depending on is known (ie alex, happy etc)
  • But then it is tricky to write a test, as we can't depend on the known tools or bundle the source for them.
  • So we create a fake "alex", which cabal then invokes on a fake ".x" file. This is maybe a bit fragile if the way cabal invokes alex changes in future, but then the test can be modified as well.

Also see #2241 and #9840

Fixes #10692

Please read Github PR Conventions and then fill in one of these two templates.


Template Α: This PR modifies behaviour or interface

Include the following checklist in your PR:


This is an automatic backport of pull request #10731 done by [Mergify](https://mergify.com).

@mergify mergify bot added the conflicts label Mar 28, 2025
@mergify
Copy link
Contributor Author

mergify bot commented Mar 28, 2025

Cherry-pick of 1c64bb8 has failed:

On branch mergify/bp/3.14/pr-10731
Your branch is up to date with 'origin/3.14'.

You are currently cherry-picking commit 1c64bb8c1.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	modified:   cabal-install/src/Distribution/Client/ProjectPlanning.hs
	new file:   changelog.d/pr-10731

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   cabal-testsuite/PackageTests/ExtraProgPath/setup.out

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

@Mikolaj Mikolaj added squash+merge me Tell Mergify Bot to squash-merge and removed conflicts labels Mar 28, 2025
@mergify
Copy link
Contributor Author

mergify bot commented Mar 28, 2025

This pull request has been removed from the queue for the following reason: checks failed.

The merge conditions cannot be satisfied due to failing checks:

You may have to fix your CI before adding the pull request to the queue again.
If you update this pull request, to fix the CI, it will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue instead, you can requeue the pull request, without updating it, by posting a @mergifyio requeue comment.

@Mikolaj
Copy link
Member

Mikolaj commented Mar 28, 2025

@mergify dequeue

@mergify
Copy link
Contributor Author

mergify bot commented Mar 28, 2025

dequeue

☑️ The pull request is not queued

@Mikolaj
Copy link
Member

Mikolaj commented Mar 28, 2025

@mergify rebase

mpickering and others added 4 commits March 28, 2025 23:47
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 #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 #2238 and #9840

Fixes #10692

(cherry picked from commit 1c64bb8)

# Conflicts:
#	cabal-testsuite/PackageTests/ExtraProgPath/setup.out
The testcase is not so easy to write because

* The bug only surfaces when the build-tool you are depending on is
  known (ie alex, happy etc)
* But then it is tricky to write a test, as we can't depend on the known
  tools or bundle the source for them.
* So we create a fake "alex", which cabal then invokes on a fake ".x"
  file. This is maybe a bit fragile if the way cabal invokes alex
  changes in future, but then the test can be modified as well.

Ticket #10692

(cherry picked from commit 24f8395)
Whilst fixing #10692, I realised there was also this bug where
extra-prog-path would not be honoured for specific packages.

The idea behind extra-prog-path is that each local package can use a
different version of a preprocessor if desired.

(cherry picked from commit 2c19bf3)
@mergify
Copy link
Contributor Author

mergify bot commented Mar 28, 2025

rebase

✅ Branch has been successfully rebased

@Mikolaj Mikolaj force-pushed the mergify/bp/3.14/pr-10731 branch from d7a781e to 95f19ac Compare March 28, 2025 23:47
@mergify mergify bot merged commit d714099 into 3.14 Mar 29, 2025
58 checks passed
@mergify mergify bot deleted the mergify/bp/3.14/pr-10731 branch March 29, 2025 06:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport squash+merge me Tell Mergify Bot to squash-merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants