Fix environment variable handling when running executables #10827
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mpickering
force-pushed
the
wip/duplicate-env-vars
branch
from
b9f0466 to
5a21af6
Compare
sheaf
approved these changes
Mar 11, 2025
mpickering
force-pushed
the
wip/duplicate-env-vars
branch
3 times, most recently
from
30abd04 to
31a5c4d
Compare
|
Let me take the liberty of assuming this is ready for review and marking it so. |
|
We badly need the second review for this one. |
ulysses4ever
approved these changes
Mar 19, 2025
mpickering
force-pushed
the
wip/duplicate-env-vars
branch
from
31a5c4d to
aec71cc
Compare
mergify
bot
added
ready and waiting
This fixes a bug where environment variables were duplicated when running executables.
```
overrideEnv <- fromMaybe [] <$> getEffectiveEnvironment ([("PATH", Just newPath)] ++ envOverrides)
let shellEnv = overrideEnv ++ existingEnv
```
Since getEffectiveEnvironment already calls getEnvironment internally, if any overrides
are passed then the result is a complete environment. Appending it to
the already existing environment results in duplicated environment variables.
The fix:
* Added getFullEnvironment function to handle the common pattern correctly
* Updated code in Bench, Test/ExeV10, Test/LibV09, and Client/Run to use this function
In the future it would be good to generalise `getFullEnvironment`
further so it can also handle the `addLibraryPath` case, which modifies
an environment variable, rather than merely setting or unsetting it.
Fixes #10718
Adds a simple test case that identifies and reports duplicate environment variables in the Cabal environment. For issue (#10718)
Mikolaj
force-pushed
the
wip/duplicate-env-vars
branch
from
aec71cc to
6e54b23
Compare
|
Am I right this should be backported to 3.14? Let me set the label tentatively. |
|
@mergify backport 3.14 |
✅ Backports have been created
|
8 tasks
mergify bot
added a commit
that referenced
this pull request
Mar 31, 2025
…tables (#10887) * Fix environment variable handling when running executables This fixes a bug where environment variables were duplicated when running executables. ``` overrideEnv <- fromMaybe [] <$> getEffectiveEnvironment ([("PATH", Just newPath)] ++ envOverrides) let shellEnv = overrideEnv ++ existingEnv ``` Since getEffectiveEnvironment already calls getEnvironment internally, if any overrides are passed then the result is a complete environment. Appending it to the already existing environment results in duplicated environment variables. The fix: * Added getFullEnvironment function to handle the common pattern correctly * Updated code in Bench, Test/ExeV10, Test/LibV09, and Client/Run to use this function In the future it would be good to generalise `getFullEnvironment` further so it can also handle the `addLibraryPath` case, which modifies an environment variable, rather than merely setting or unsetting it. Fixes #10718 (cherry picked from commit 4375dd5) * Add test for duplicate environment variables when invoking testsuite Adds a simple test case that identifies and reports duplicate environment variables in the Cabal environment. For issue (#10718) (cherry picked from commit 6e54b23) --------- Co-authored-by: Matthew Pickering <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes a bug where environment variables were duplicated when running executables.
Since getEffectiveEnvironment already calls getEnvironment internally, if any overrides
are passed then the result is a complete environment. Appending it to
the already existing environment results in duplicated environment variables.
The fix:
In the future it would be good to generalise
getFullEnvironmentfurther so it can also handle the
addLibraryPathcase, which modifiesan environment variable, rather than merely setting or unsetting it.
Fixes #10718.
Template Α: This PR modifies behaviour or interface
Include the following checklist in your PR:
significance: significantin the changelog file.Template B: This PR does not modify behaviour or interface
E.g. the PR only touches documentation or tests, does refactorings, etc.
Include the following checklist in your PR: