fix: environment handling windows (host mode) (#1732)

* fix: environment handling windows (host mode)

* fixup

* fixup

* add more tests

* fixup

* fix setenv

* fixes

* [skip ci] Apply suggestions from code review

Co-authored-by: Jason Song <i@wolfogre.com>

* Update side effects

---------

Co-authored-by: Jason Song <i@wolfogre.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

pkg/container/executions_environment.go

@@ -10,4 +10,6 @@ type ExecutionsEnvironment interface {
 	DefaultPathVariable() string
 	JoinPathVariable(...string) string
 	GetRunnerContext(ctx context.Context) map[string]interface{}
+	// On windows PATH and Path are the same key
+	IsEnvironmentCaseInsensitive() bool
 }

pkg/container/host_environment.go

@@ -419,3 +419,7 @@ func (e *HostEnvironment) ReplaceLogWriter(stdout io.Writer, stderr io.Writer) (
 	e.StdOut = stdout
 	return org, org
 }
+
+func (*HostEnvironment) IsEnvironmentCaseInsensitive() bool {
+	return runtime.GOOS == "windows"
+}

pkg/container/linux_container_environment_extensions.go

@@ -71,3 +71,7 @@ func (*LinuxContainerEnvironmentExtensions) GetRunnerContext(ctx context.Context
 		"tool_cache": "/opt/hostedtoolcache",
 	}
 }
+
+func (*LinuxContainerEnvironmentExtensions) IsEnvironmentCaseInsensitive() bool {
+	return false
+}

pkg/runner/action.go

@@ -319,13 +319,13 @@ func evalDockerArgs(ctx context.Context, step step, action *model.Action, cmd *[
 			inputs[k] = eval.Interpolate(ctx, v)
 		}
 	}
-	mergeIntoMap(step.getEnv(), inputs)
+	mergeIntoMap(step, step.getEnv(), inputs)
 
 	stepEE := rc.NewStepExpressionEvaluator(ctx, step)
 	for i, v := range *cmd {
 		(*cmd)[i] = stepEE.Interpolate(ctx, v)
 	}
-	mergeIntoMap(step.getEnv(), action.Runs.Env)
+	mergeIntoMap(step, step.getEnv(), action.Runs.Env)
 
 	ee := rc.NewStepExpressionEvaluator(ctx, step)
 	for k, v := range *step.getEnv() {

pkg/runner/action_composite.go

@@ -105,13 +105,15 @@ func execAsComposite(step actionStep) common.Executor {
 		rc.Masks = append(rc.Masks, compositeRC.Masks...)
 		rc.ExtraPath = compositeRC.ExtraPath
 		// compositeRC.Env is dirty, contains INPUT_ and merged step env, only rely on compositeRC.GlobalEnv
-		for k, v := range compositeRC.GlobalEnv {
+		mergeIntoMap := mergeIntoMapCaseSensitive
-			rc.Env[k] = v
+		if rc.JobContainer.IsEnvironmentCaseInsensitive() {
-			if rc.GlobalEnv == nil {
+			mergeIntoMap = mergeIntoMapCaseInsensitive
-				rc.GlobalEnv = map[string]string{}
-			}
-			rc.GlobalEnv[k] = v
 		}
+		if rc.GlobalEnv == nil {
+			rc.GlobalEnv = map[string]string{}
+		}
+		mergeIntoMap(rc.GlobalEnv, compositeRC.GlobalEnv)
+		mergeIntoMap(rc.Env, compositeRC.GlobalEnv)
 
 		return err
 	}

pkg/runner/command.go

@@ -87,12 +87,18 @@ func (rc *RunContext) setEnv(ctx context.Context, kvPairs map[string]string, arg
 	if rc.Env == nil {
 		rc.Env = make(map[string]string)
 	}
-	rc.Env[name] = arg
-	// for composite action GITHUB_ENV and set-env passing
 	if rc.GlobalEnv == nil {
 		rc.GlobalEnv = map[string]string{}
 	}
-	rc.GlobalEnv[name] = arg
+	newenv := map[string]string{
+		name: arg,
+	}
+	mergeIntoMap := mergeIntoMapCaseSensitive
+	if rc.JobContainer != nil && rc.JobContainer.IsEnvironmentCaseInsensitive() {
+		mergeIntoMap = mergeIntoMapCaseInsensitive
+	}
+	mergeIntoMap(rc.Env, newenv)
+	mergeIntoMap(rc.GlobalEnv, newenv)
 }
 func (rc *RunContext) setOutput(ctx context.Context, kvPairs map[string]string, arg string) {
 	logger := common.Logger(ctx)

pkg/runner/run_context.go

@@ -298,6 +298,15 @@ func (rc *RunContext) execJobContainer(cmd []string, env map[string]string, user
 func (rc *RunContext) ApplyExtraPath(ctx context.Context, env *map[string]string) {
 	if rc.ExtraPath != nil && len(rc.ExtraPath) > 0 {
 		path := rc.JobContainer.GetPathVariableName()
+		if rc.JobContainer.IsEnvironmentCaseInsensitive() {
+			// On windows system Path and PATH could also be in the map
+			for k := range *env {
+				if strings.EqualFold(path, k) {
+					path = k
+					break
+				}
+			}
+		}
 		if (*env)[path] == "" {
 			cenv := map[string]string{}
 			var cpath string

pkg/runner/step.go

@@ -187,7 +187,7 @@ func setupEnv(ctx context.Context, step step) error {
 
 	mergeEnv(ctx, step)
 	// merge step env last, since it should not be overwritten
-	mergeIntoMap(step.getEnv(), step.getStepModel().GetEnv())
+	mergeIntoMap(step, step.getEnv(), step.getStepModel().GetEnv())
 
 	exprEval := rc.NewExpressionEvaluator(ctx)
 	for k, v := range *step.getEnv() {
@@ -216,9 +216,9 @@ func mergeEnv(ctx context.Context, step step) {
 
 	c := job.Container()
 	if c != nil {
-		mergeIntoMap(env, rc.GetEnv(), c.Env)
+		mergeIntoMap(step, env, rc.GetEnv(), c.Env)
 	} else {
-		mergeIntoMap(env, rc.GetEnv())
+		mergeIntoMap(step, env, rc.GetEnv())
 	}
 
 	rc.withGithubEnv(ctx, step.getGithubContext(ctx), *env)
@@ -258,10 +258,38 @@ func isContinueOnError(ctx context.Context, expr string, step step, stage stepSt
 	return continueOnError, nil
 }
 
-func mergeIntoMap(target *map[string]string, maps ...map[string]string) {
+func mergeIntoMap(step step, target *map[string]string, maps ...map[string]string) {
+	if rc := step.getRunContext(); rc != nil && rc.JobContainer != nil && rc.JobContainer.IsEnvironmentCaseInsensitive() {
+		mergeIntoMapCaseInsensitive(*target, maps...)
+	} else {
+		mergeIntoMapCaseSensitive(*target, maps...)
+	}
+}
+
+func mergeIntoMapCaseSensitive(target map[string]string, maps ...map[string]string) {
 	for _, m := range maps {
 		for k, v := range m {
-			(*target)[k] = v
+			target[k] = v
+		}
+	}
+}
+
+func mergeIntoMapCaseInsensitive(target map[string]string, maps ...map[string]string) {
+	foldKeys := make(map[string]string, len(target))
+	for k := range target {
+		foldKeys[strings.ToLower(k)] = k
+	}
+	toKey := func(s string) string {
+		foldKey := strings.ToLower(s)
+		if k, ok := foldKeys[foldKey]; ok {
+			return k
+		}
+		foldKeys[strings.ToLower(foldKey)] = s
+		return s
+	}
+	for _, m := range maps {
+		for k, v := range m {
+			target[toKey(k)] = v
 		}
 	}
 }

pkg/runner/step_test.go

@@ -63,7 +63,9 @@ func TestMergeIntoMap(t *testing.T) {
 
 	for _, tt := range table {
 		t.Run(tt.name, func(t *testing.T) {
-			mergeIntoMap(&tt.target, tt.maps...)
+			mergeIntoMapCaseSensitive(tt.target, tt.maps...)
+			assert.Equal(t, tt.expected, tt.target)
+			mergeIntoMapCaseInsensitive(tt.target, tt.maps...)
 			assert.Equal(t, tt.expected, tt.target)
 		})
 	}

pkg/runner/testdata/windows-add-env/action.yml

@@ -0,0 +1,7 @@
+runs:
+  using: composite
+  steps:
+  - run: |
+      echo $env:GITHUB_ENV
+      echo "kEy=n/a" > $env:GITHUB_ENV
+    shell: pwsh

pkg/runner/testdata/windows-add-env/push.yml

@@ -25,3 +25,20 @@ jobs:
           echo "Unexpected value for `$env:key2: $env:key2"
           exit 1
         }
+    - run: |
+        echo $env:GITHUB_ENV
+        echo "KEY=test" > $env:GITHUB_ENV
+        echo "Key=expected" > $env:GITHUB_ENV
+    - name: Assert GITHUB_ENV is merged case insensitive
+      run: exit 1
+      if: env.KEY != 'expected' || env.Key != 'expected' || env.key != 'expected'
+    - name: Assert step env is merged case insensitive
+      run: exit 1
+      if: env.KEY != 'n/a' || env.Key != 'n/a' || env.key != 'n/a'
+      env:
+        KeY: 'n/a'
+    - uses: actions/checkout@v3
+    - uses: ./windows-add-env
+    - name: Assert composite env is merged case insensitive
+      run: exit 1
+      if: env.KEY != 'n/a' || env.Key != 'n/a' || env.key != 'n/a'

pkg/runner/testdata/windows-prepend-path/push.yml

@@ -11,6 +11,9 @@ jobs:
         mkdir build
         echo '@echo off' > build/test.cmd
         echo 'echo Hi' >> build/test.cmd
+        mkdir build2
+        echo '@echo off' > build2/test2.cmd
+        echo 'echo test2' >> build2/test2.cmd
     - run: |
         echo '${{ tojson(runner) }}'
         ls
@@ -23,3 +26,9 @@ jobs:
     - run: |
         echo $env:PATH
         test
+    - run: |
+        echo "PATH=$env:PATH;${{ github.workspace }}\build2" > $env:GITHUB_ENV
+    - run: |
+        echo $env:PATH
+        test
+        test2