Skip to content

Commit a8cf469

Browse files
mergify[bot]mpickering
mergify[bot]
and
mpickering
authored
Backport #10827: Fix environment variable handling when running executables (#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]>
1 parent 93144da commit a8cf469

File tree

11 files changed

+77
-21
lines changed

11 files changed

+77
-21
lines changed

‎Cabal/src/Distribution/Simple/Bench.hs

+1-5
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@ module Distribution.Simple.Bench
2323
import Distribution.Compat.Prelude
2424
import Prelude ()
2525

26-
import Distribution.Compat.Environment
2726
import qualified Distribution.PackageDescription as PD
2827
import Distribution.Pretty
2928
import Distribution.Simple.Build (addInternalBuildToolsFixed)
@@ -92,15 +91,12 @@ bench args pkg_descr lbi flags = do
9291
dieWithException verbosity $
9392
NoBenchMarkProgram cmd
9493

95-
existingEnv <- getEnvironment
96-
9794
-- Compute the appropriate environment for running the benchmark
9895
let progDb = LBI.withPrograms lbiForBench
9996
pathVar = progSearchPath progDb
10097
envOverrides = progOverrideEnv progDb
10198
newPath <- programSearchPathAsPATHVar pathVar
102-
overrideEnv <- fromMaybe [] <$> getEffectiveEnvironment ([("PATH", Just newPath)] ++ envOverrides)
103-
let shellEnv = overrideEnv ++ existingEnv
99+
shellEnv <- getFullEnvironment ([("PATH", Just newPath)] ++ envOverrides)
104100

105101
-- Add (DY)LD_LIBRARY_PATH if needed
106102
shellEnv' <-

‎Cabal/src/Distribution/Simple/Program/Run.hs

+18
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ module Distribution.Simple.Program.Run
3030
, getProgramInvocationOutputAndErrors
3131
, getProgramInvocationLBSAndErrors
3232
, getEffectiveEnvironment
33+
, getFullEnvironment
3334
) where
3435

3536
import Distribution.Compat.Prelude
@@ -236,6 +237,12 @@ getProgramInvocationIODataAndErrors
236237
-- | Return the current environment extended with the given overrides.
237238
-- If an entry is specified twice in @overrides@, the second entry takes
238239
-- precedence.
240+
--
241+
-- getEffectiveEnvironment returns 'Nothing' when there are no overrides.
242+
-- It returns an argument that is suitable to pass directly to 'CreateProcess' to
243+
-- override the environment.
244+
-- If you need the full environment to manipulate further, even when there are no overrides,
245+
-- then call 'getFullEnvironment'.
239246
getEffectiveEnvironment
240247
:: [(String, Maybe String)]
241248
-> IO (Maybe [(String, String)])
@@ -247,6 +254,17 @@ getEffectiveEnvironment overrides =
247254
update (var, Nothing) = Map.delete var
248255
update (var, Just val) = Map.insert var val
249256

257+
-- | Like 'getEffectiveEnvironment', but when no overrides are specified,
258+
-- returns the full environment instead of 'Nothing'.
259+
getFullEnvironment
260+
:: [(String, Maybe String)]
261+
-> IO [(String, String)]
262+
getFullEnvironment overrides = do
263+
menv <- getEffectiveEnvironment overrides
264+
case menv of
265+
Just env -> return env
266+
Nothing -> getEnvironment
267+
250268
-- | Like the unix xargs program. Useful for when we've got very long command
251269
-- lines that might overflow an OS limit on command line length and so you
252270
-- need to invoke a command multiple times to get all the args in.

‎Cabal/src/Distribution/Simple/Test/ExeV10.hs

+7-5
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@ module Distribution.Simple.Test.ExeV10
88
import Distribution.Compat.Prelude
99
import Prelude ()
1010

11-
import Distribution.Compat.Environment
1211
import qualified Distribution.PackageDescription as PD
1312
import Distribution.Simple.BuildPaths
1413
import Distribution.Simple.Compiler
@@ -64,8 +63,6 @@ runTest pkg_descr lbi clbi hpcMarkupInfo flags suite = do
6463
way = guessWay lbi
6564
tixDir_ = i $ tixDir distPref way
6665

67-
existingEnv <- getEnvironment
68-
6966
let cmd =
7067
i (LBI.buildDir lbi)
7168
</> testName'
@@ -92,13 +89,18 @@ runTest pkg_descr lbi clbi hpcMarkupInfo flags suite = do
9289
pathVar = progSearchPath progDb
9390
envOverrides = progOverrideEnv progDb
9491
newPath <- programSearchPathAsPATHVar pathVar
95-
overrideEnv <- fromMaybe [] <$> getEffectiveEnvironment ([("PATH", Just newPath)] ++ envOverrides)
9692
let opts =
9793
map
9894
(testOption pkg_descr lbi suite)
9995
(testOptions flags)
10096
tixFile = packageRoot (testCommonFlags flags) </> getSymbolicPath (tixFilePath distPref way (testName'))
101-
shellEnv = [("HPCTIXFILE", tixFile) | isCoverageEnabled] ++ overrideEnv ++ existingEnv
97+
98+
shellEnv <-
99+
getFullEnvironment
100+
( [("PATH", Just newPath)]
101+
++ [("HPCTIXFILE", Just tixFile) | isCoverageEnabled]
102+
++ envOverrides
103+
)
102104

103105
-- Add (DY)LD_LIBRARY_PATH if needed
104106
shellEnv' <-

‎Cabal/src/Distribution/Simple/Test/LibV09.hs

+7-7
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@ import Distribution.Compat.Prelude
1717
import Distribution.Types.UnqualComponentName
1818
import Prelude ()
1919

20-
import Distribution.Compat.Environment
2120
import Distribution.Compat.Internal.TempFile
2221
import Distribution.Compat.Process (proc)
2322
import Distribution.ModuleName
@@ -70,7 +69,6 @@ runTest pkg_descr lbi clbi hpcMarkupInfo flags suite = do
7069
way = guessWay lbi
7170

7271
let mbWorkDir = LBI.mbWorkDirLBI lbi
73-
existingEnv <- getEnvironment
7472

7573
let cmd =
7674
interpretSymbolicPath mbWorkDir (LBI.buildDir lbi)
@@ -100,15 +98,17 @@ runTest pkg_descr lbi clbi hpcMarkupInfo flags suite = do
10098
pathVar = progSearchPath progDb
10199
envOverrides = progOverrideEnv progDb
102100
newPath <- programSearchPathAsPATHVar pathVar
103-
overrideEnv <- fromMaybe [] <$> getEffectiveEnvironment ([("PATH", Just newPath)] ++ envOverrides)
104101

105102
-- Run test executable
106103
let opts = map (testOption pkg_descr lbi suite) $ testOptions flags
107104
tixFile = i $ tixFilePath distPref way testName'
108-
shellEnv =
109-
[("HPCTIXFILE", tixFile) | isCoverageEnabled]
110-
++ overrideEnv
111-
++ existingEnv
105+
106+
shellEnv <-
107+
getFullEnvironment
108+
( [("PATH", Just newPath)]
109+
++ [("HPCTIXFILE", Just tixFile) | isCoverageEnabled]
110+
++ envOverrides
111+
)
112112
-- Add (DY)LD_LIBRARY_PATH if needed
113113
shellEnv' <-
114114
if LBI.withDynExe lbi

‎cabal-install/src/Distribution/Client/Run.hs

+1-4
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,6 @@ import Distribution.Types.UnqualComponentName
6060
import qualified Distribution.Simple.GHCJS as GHCJS
6161

6262
import Distribution.Client.Errors
63-
import Distribution.Compat.Environment (getEnvironment)
6463
import Distribution.Utils.Path
6564

6665
-- | Return the executable to run and any extra arguments that should be
@@ -181,13 +180,11 @@ run verbosity lbi exe exeArgs = do
181180
return (p, [])
182181

183182
-- Compute the appropriate environment for running the executable
184-
existingEnv <- getEnvironment
185183
let progDb = withPrograms lbiForExe
186184
pathVar = progSearchPath progDb
187185
envOverrides = progOverrideEnv progDb
188186
newPath <- programSearchPathAsPATHVar pathVar
189-
overrideEnv <- fromMaybe [] <$> getEffectiveEnvironment ([("PATH", Just newPath)] ++ envOverrides)
190-
let env = overrideEnv ++ existingEnv
187+
env <- getFullEnvironment ([("PATH", Just newPath)] ++ envOverrides)
191188

192189
-- Add (DY)LD_LIBRARY_PATH if needed
193190
env' <-

‎cabal-testsuite/PackageTests/DuplicateEnvVars/cabal.out

Whitespace-only changes.

‎cabal-testsuite/PackageTests/DuplicateEnvVars/cabal.project

+1
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
packages: p

‎cabal-testsuite/PackageTests/DuplicateEnvVars/cabal.test.hs

+5
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
import Test.Cabal.Prelude
2+
3+
main = cabalTest $ recordMode DoNotRecord $ do
4+
res <- cabal' "test" ["all"]
5+
assertOutputContains "No duplicate environment variables found" res

‎cabal-testsuite/PackageTests/DuplicateEnvVars/p/Main.hs

+16
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
module Main where
2+
3+
import Data.List (group, sort)
4+
import System.Environment (getEnvironment)
5+
6+
main = do
7+
env <- getEnvironment
8+
let sortedEnv = sort env
9+
duplicates = filter (\g -> length g > 1) $ group $ map fst sortedEnv
10+
11+
if null duplicates
12+
then putStrLn "No duplicate environment variables found."
13+
else do
14+
putStrLn "Found duplicate environment variables:"
15+
mapM_ (\d -> putStrLn $ " - " ++ head d) duplicates
16+
fail "Test failed due to duplicate environment variables"

‎cabal-testsuite/PackageTests/DuplicateEnvVars/p/p.cabal

+10
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
cabal-version: 3.0
2+
name: p
3+
version: 0.1.0.0
4+
build-type: Simple
5+
6+
test-suite env-test
7+
default-language: Haskell2010
8+
type: exitcode-stdio-1.0
9+
main-is: Main.hs
10+
build-depends: base

‎changelog.d/pr-10827.md

+11
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
---
2+
synopsis: "Fix duplicate environment variables in test and benchmark runs"
3+
packages: [Cabal, cabal-install]
4+
prs: 10827
5+
issues: 10718
6+
---
7+
8+
Cabal no longer creates duplicate environment variables when running test
9+
suites, benchmarks, or internal executables. Previously, when setting up the
10+
environment for these processes, Cabal would append the overridden environment
11+
to the existing environment, creating duplicates of the same variable.

0 commit comments

Comments
 (0)