Skip to content
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

cabal-install: Be less eager to configure external programs #10731

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

Conversation

mpickering
Copy link
Collaborator

@mpickering mpickering commented Jan 9, 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:

@Mikolaj Mikolaj requested a review from jasagredo January 9, 2025 12:57
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.

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 haskell#2241 and haskell#9840

Fixes haskell#10692
@ulysses4ever
Copy link
Collaborator

Hey @mpickering! Any chance you could take a brief look at #2015? I believe it's closely related to the issue here. In particular, cabal repl runs with $PATH effective at the time of configuring, not at the time of running cabal repl. So, if you run cabal build on clean checkout, then change $PATH, then run cabal repl, inside the repl you won't see the change. This keeps blowing people's heads...

I tried to look into it, and it seems to stem, as you say, from storing the compiler's configuration on disk, and configuration includes $PATH. But I don't know what'd be the least intrusive way to fix it. E.g. should we just reconfigure from scratch inside cabal repl? Or should we treat $PATH in a special way, and maybe try to invalidate only it on every cabal repl run.

The code for cabal repl had a big update from your multi-repl patch. That's another reason I thought you may be a great candidate for guiding or, indeed, solving #2015.

@mpickering
Copy link
Collaborator Author

@ulysses4ever I've just been assigned to look at these two regressions, but I will comment on the issue about how I would begin with that problem.

synopsis: Fix regression where build-tool-depends are not used
packages: cabal-install
prs: #10731

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
issues: #10692

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe you need to add a alex.exe + alex.shim for this to also work on Windows?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean, the test passes but it probably won't use the hacky alex substitute?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it's not supposed to use the scripts/alex version. If it does, then any error that we get is good.

Copy link
Collaborator

@jasagredo jasagredo Jan 9, 2025

Choose a reason for hiding this comment

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

What I mean is that on Windows the scripts/alex file won't be considered as an executable in PATH, and therefore we are not testing that this works as expected, i.e. that the PATH executables are ignored.

We are indeed seeing that it uses the build-tool-depends one, but we don't have any conflict to fix there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[3.14] Initial configuration of programs does not use build-tool-depends
3 participants