Skip to content

Be more careful in handling unconfigured programs in the program database (backport #9967) #10349

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

Closed
wants to merge 4 commits into from

Conversation

mergify[bot]
Copy link
Contributor

@mergify mergify bot commented Sep 14, 2024

This PR contains several commits that address problems with how we were handling unconfigured programs in the program database:

  • We should configure unconfigured programs before serialising them (using the Binary ProgramDb instance) in
    Distribution.Client.ProjectPlanning.configureCompiler, as otherwise we can accidentally discard information.
    Not doing so can lead to situations in which we can build a package just fine, but if we re-start a build we will fail,
    because the program database on disk stored after configuring doesn't match the program database in memory.
  • As a follow-on from the above, configureRequiredProgram needs to gracefully handle an already-configured program.
    Not doing so means we would fall back to the simpleProgram treatment, which can cause Cabal to fail to configure a program such as hsc2hs which has custom logic for parsing its version number.
  • When compiling a Setup executable, we would pass a program database for use of the stripExe command. However,
    the strip program might not be configured in this program database; so we make sure to configure it (in the correct
    environment).

Template Α: This PR modifies behaviour or interface.

TODO:

We should configure unconfigured programs before serialising them
(using the Binary ProgramDb instance) in
Distribution.Client.ProjectPlanning.configureCompiler, as otherwise
we can accidentally discard information.

This isn't currently a live bug, because in practice we end up
re-running configuration steps when interfacing with the Cabal library.
However, in the bright future in which we avoid re-configuring things
when building a Cabal package that we already determined when invoking
cabal-install, this starts causing problems.

(cherry picked from commit 8bdda9c)

# Conflicts:
#	cabal-testsuite/PackageTests/ExtraProgPath/setup.out
In configureRequiredProgram, we would unconditionally configure the
program, even if it was configured already. To explain why this is
problematic:

  - The programs we try to configure in this function are those that
    arise from a "build-tool-depends" field of a package.
  - If the program is not in the "unconfigured" state, we would
    unconditionally treat it as a simpleProgram.
    This means that if such a program is builtin but is not a
    simpleProgram, we might fail to configure it properly.

This problem arises when we configure programs more eagerly: when, in
the past, we might have gone looking up an unconfigured program and
successfully configured it, now we might find the program is already
configured.

One example in which this would cause an issue is when we have

   build-tool-depends: hsc2hs

If we end up re-configuring hsc2hs in configureRequiredProgram, we would
treat it as a simple program, which would cause us to be unable to
determine its version.

(cherry picked from commit c6a26d4)
This commit configures the unconfigured programs in the program database
when we are building a custom Setup script. This ensures that we can
find the "strip" program which might otherwise still be unconfigured.

(cherry picked from commit 0a0cc19)

# Conflicts:
#	cabal-install/src/Distribution/Client/SetupWrapper.hs
@mergify mergify bot added the conflicts label Sep 14, 2024
Copy link
Contributor Author

mergify bot commented Sep 14, 2024

Cherry-pick of 8bdda9c has failed:

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

You are currently cherry-picking commit 8bdda9c0b.
  (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

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

Cherry-pick of 0a0cc19 has failed:

On branch mergify/bp/3.12/pr-9967
Your branch is ahead of 'origin/3.12' by 2 commits.
  (use "git push" to publish your local commits)

You are currently cherry-picking commit 0a0cc19f1.
  (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)

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   cabal-install/src/Distribution/Client/SetupWrapper.hs

no changes added to commit (use "git add" and/or "git commit -a")

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

There was some hacky logic in place to give a "haskell-suite" compiler
a dummy location so that we would attempt to configure it.

This logic is no longer necessary since 'configureRequiredProgram' was
changed to accomodate the situation in which 'lookupKnownProgram' fails.
In fact, it is downright harmful, as this dummy location will trigger
errors when we attempt to configure all known programs, which is
a necessary step before attempting to serialise a program database.

(cherry picked from commit 6ac9dfb)
@geekosaur geekosaur force-pushed the mergify/bp/3.12/pr-9967 branch from 4860905 to 11f391e Compare September 14, 2024 01:05
@geekosaur
Copy link
Collaborator

Turns out there's an API change that also breaks the build.

@geekosaur geekosaur closed this Sep 14, 2024
@mergify mergify bot deleted the mergify/bp/3.12/pr-9967 branch September 14, 2024 01:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants