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

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

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

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:

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
@mpickering mpickering force-pushed the wip/investigate-recomp branch from faff622 to 9fd36c6 Compare March 19, 2025 16:03
@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).

Comment on lines +6 to +12
When modifying your PATH environment variable between Cabal commands, REPL
sessions will now correctly use the updated environment. Previously, if you
changed your PATH after initial configuration, Cabal would continue using the
old PATH settings in REPL sessions without warning.

With this fix, Cabal properly detects PATH changes and reconfigures the REPL
accordingly, ensuring tools and libraries in your current PATH are available.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This text, including the title, feels a little repetitive. I suggest dropping the first sentence in the description and make it into one paragraph:

Suggested change
When modifying your PATH environment variable between Cabal commands, REPL
sessions will now correctly use the updated environment. Previously, if you
changed your PATH after initial configuration, Cabal would continue using the
old PATH settings in REPL sessions without warning.
With this fix, Cabal properly detects PATH changes and reconfigures the REPL
accordingly, ensuring tools and libraries in your current PATH are available.
Previously, if you changed your PATH after initial configuration, Cabal would
continue using the old PATH settings in REPL sessions without warning.
With this fix, Cabal properly detects PATH changes and reconfigures the REPL
accordingly, ensuring tools and libraries in your current PATH are available.

Again, consider that it appears right after the title ("synopsis" field). And that the former third sentence basically repeated the former first one that I suggest to drop.

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.

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