Skip to content

Fix PATH changes not triggering REPL reconfiguration (#2015) #10817

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

Merged
merged 1 commit into from
May 24, 2025

Conversation

mpickering
Copy link
Collaborator

When a user's PATH environment variable changes between Cabal commands, the environment in the REPL becomes incorrect. This occurs because during initial package configuration, the current PATH is captured via programSearchPathAsPATHVar and then stored in the programOverrideEnv field of a ConfiguredProgram. These ConfiguredPrograms are subsequently serialized into the setup-config file, effectively baking in the PATH environment from the time of configuration.

The issue manifests when PATH is updated after initial configuration. Although the solver re-runs when detecting PATH changes, the dryRunLocalPkg function examines ElaboratedConfiguredPackage and incorrectly determines that nothing has changed that would require reconfiguration. This decision is incorrect because setup-config now contains a stale reference to the original PATH value, which no longer matches the current environment.

To fix this problem, we need to store the ConfiguredPrograms directly in ElaboratedConfiguredPackage. This approach will ensure that when PATH changes, the ConfiguredPrograms will also change, properly triggering reconfiguration. While this solution will cause more recompilations when PATH changes, it guarantees that the correct environment is always used during builds and REPL sessions.

An alternative approach would be to stop baking the PATH variable into the environment of programs, but this would require more substantial changes to how Cabal manages environment variables.

Fixes #2015

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
Copy link
Member

Mikolaj commented Mar 29, 2025

Is this ready for review? If so, could you please add the label?

@mpickering
Copy link
Collaborator Author

Any reviewers? @ulysses4ever ?

1 week without movement, so I am happy to just merge it if no-one wishes to review.

@ulysses4ever
Copy link
Collaborator

Sorry for slow progress here: everyone was busy with the release. I'll try to take a look today.

I don't think it's possible to merge anything without reviews. Unless you're going to change repository settings, which is a big change for the project and should be discussed on a meeting.

Copy link
Collaborator

@ulysses4ever ulysses4ever left a comment

Choose a reason for hiding this comment

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

I'd suggest that you mention in the commit message why adding a new field, seemingly unused anywhere, changes the behaviour (it's because of the Generic business afaiu).

@Mikolaj
Copy link
Member

Mikolaj commented Apr 24, 2025

@mpickering: would you like to respond and then set the merge label maybe?

@mpickering mpickering added the merge me Tell Mergify Bot to merge label May 21, 2025
@mpickering mpickering force-pushed the wip/investigate-recomp branch from 906f0f6 to 9efbc01 Compare May 21, 2025 10:52
@mergify mergify bot added the ready and waiting Mergify is waiting out the cooldown period label May 21, 2025
@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label May 24, 2025
When a user's PATH environment variable changes between Cabal commands,
the environment in the REPL becomes incorrect. This occurs because
during initial package configuration, the current PATH is captured via
`programSearchPathAsPATHVar` and then stored in the `programOverrideEnv`
field of a ConfiguredProgram. These `ConfiguredProgram`s are subsequently
serialized into the setup-config file, effectively baking in the PATH
environment from the time of configuration.

The issue manifests when `PATH` is updated after initial configuration.
Although the solver re-runs when detecting `PATH` changes, the
`dryRunLocalPkg` function examines `ElaboratedConfiguredPackage` and
incorrectly determines that nothing has changed that would require
reconfiguration. This decision is incorrect because `setup-config` now
contains a stale reference to the original `PATH` value, which no longer
matches the current environment.

To fix this problem, we need to store the `ConfiguredProgram`s directly in
`ElaboratedConfiguredPackage`. This approach will ensure that when `PATH`
changes, the `ConfiguredProgram`s will also change, properly triggering
reconfiguration. While this solution will cause more recompilations when
`PATH` changes, it guarantees that the correct environment is always used
during builds and REPL sessions.

An alternative approach would be to stop baking the `PATH` variable into
the environment of programs, but this would require more substantial
changes to how Cabal manages environment variables.

Fixes #2015
@Mikolaj Mikolaj force-pushed the wip/investigate-recomp branch from 9efbc01 to cf4083b Compare May 24, 2025 15:24
@mergify mergify bot merged commit 55aba14 into master May 24, 2025
55 checks passed
@mergify mergify bot deleted the wip/investigate-recomp branch May 24, 2025 17:08
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.

cabal repl: Change in PATH environment variable not reflected
4 participants