Merge branch version/0-47-0-RC1 to adopt changes from PR #3553

internal/runners/exec/exec.go

@@ -5,7 +5,6 @@ import (
 	"os"
 	"os/exec"
 	"path/filepath"
-	"runtime"
 	"strconv"
 	"strings"
 
@@ -155,9 +154,6 @@ func (s *Exec) Run(params *Params, args ...string) (rerr error) {
 			return !v
 		}
 		exesOnPath := osutils.FilterExesOnPATH(exeTarget, env["PATH"], filter)
-		if runtime.GOOS == "windows" {
-			exesOnPath = append(exesOnPath, osutils.FilterExesOnPATH(exeTarget, env["Path"], filter)...)
-		}
 
 		if len(exesOnPath) > 0 {
 			exeTarget = exesOnPath[0]

pkg/runtime/internal/envdef/collection.go

@@ -1,9 +1,7 @@
 package envdef
 
 import (
-	"os"
 	"path/filepath"
-	"runtime"
 	"sync"
 
 	"github.com/ActiveState/cli/internal/errs"
@@ -65,29 +63,5 @@ func (c *Collection) Environment(installPath string, inherit bool) (map[string]s
 		}
 	}
 	constants := NewConstants(installPath)
-	env := result.ExpandVariables(constants).GetEnv(inherit)
-	promotePath(env)
-	return env, nil
-}
-
-// promotPath is a temporary fix to ensure that the PATH is interpreted correctly on Windows
-// Should be properly addressed by https://activestatef.atlassian.net/browse/DX-3030
-func promotePath(env map[string]string) {
-	if runtime.GOOS != "windows" {
-		return
-	}
-
-	PATH, exists := env["PATH"]
-	if !exists {
-		return
-	}
-
-	// If Path exists, prepend PATH values to it
-	Path, pathExists := env["Path"]
-	if !pathExists {
-		return
-	}
-
-	env["Path"] = PATH + string(os.PathListSeparator) + Path
-	delete(env, "PATH")
+	return result.ExpandVariables(constants).GetEnv(inherit), nil
 }

pkg/runtime/internal/envdef/environment.go

@@ -6,6 +6,7 @@ import (
 	"maps"
 	"os"
 	"path/filepath"
+	"runtime"
 	"strings"
 
 	"github.com/ActiveState/cli/internal/constants"
@@ -344,35 +345,62 @@ func (ev *EnvironmentVariable) ValueString() string {
 		ev.Separator)
 }
 
-// GetEnvBasedOn returns the environment variable names and values defined by
+// getEnvBasedOn returns the environment variable names and values defined by
 // the EnvironmentDefinition.
 // If an environment variable is configured to inherit from the base
 // environment (`Inherit==true`), the base environment defined by the
 // `envLookup` method is joined with these environment variables.
 // This function is mostly used for testing. Use GetEnv() in production.
-func (ed *EnvironmentDefinition) GetEnvBasedOn(envLookup map[string]string) (map[string]string, error) {
+func (ed *EnvironmentDefinition) getEnvBasedOn(envLookup map[string]string) (map[string]string, error) {
 	res := maps.Clone(envLookup)
 
+	// On Windows, environment variable names are case-insensitive.
+	// For example, it uses "Path", but responds to "PATH" as well.
+	// This causes trouble with our environment merging, which will end up adding "PATH" (with the
+	// correct value) alongside "Path" (with the old value).
+	// In order to remedy this, track the OS-specific environment variable name and if it's
+	// modified/merged, replace it with our version (e.g. "Path" -> "PATH"). We do not use the OS name
+	// because we assume ours is the one that's used elsewhere in the codebase, and Windows will
+	// properly respond to a changed-case name anyway.
+	osEnvNames := map[string]string{}
+	if runtime.GOOS == "windows" {
+		for k := range envLookup {
+			osEnvNames[strings.ToLower(k)] = k
+		}
+	}
+
 	for _, ev := range ed.Env {
 		pev := &ev
+		osName := pev.Name
+		if runtime.GOOS == "windows" {
+			if name, ok := osEnvNames[strings.ToLower(osName)]; ok {
+				osName = name
+			}
+		}
+		osValue, hasOsValue := envLookup[osName]
 		if pev.Inherit {
-			osValue, hasOsValue := envLookup[pev.Name]
 			if hasOsValue {
 				osEv := ev
 				osEv.Values = []string{osValue}
 				var err error
 				pev, err = osEv.Merge(ev)
 				if err != nil {
 					return nil, err
-
 				}
 			}
-		} else if _, hasOsValue := envLookup[pev.Name]; hasOsValue {
+		} else if hasOsValue {
 			res[pev.Name] = "" // unset
 		}
 		// only add environment variable if at least one value is set (This allows us to remove variables from the environment.)
 		if len(ev.Values) > 0 {
 			res[pev.Name] = pev.ValueString()
+			if pev.Name != osName {
+				// On Windows, delete the redundant (case-insensitive) version that our case-sensitive
+				// version could conflict with. (Our version has already processed the value of the
+				// redundant version.)
+				// For example, delete "Path" while preserving our "PATH".
+				delete(res, osName)
+			}
 		}
 	}
 	return res, nil
@@ -388,7 +416,7 @@ func (ed *EnvironmentDefinition) GetEnv(inherit bool) map[string]string {
 	if inherit {
 		lookupEnv = osutils.EnvSliceToMap(os.Environ())
 	}
-	res, err := ed.GetEnvBasedOn(lookupEnv)
+	res, err := ed.getEnvBasedOn(lookupEnv)
 	if err != nil {
 		panic(fmt.Sprintf("Could not inherit OS environment variable: %v", err))
 	}

pkg/runtime/internal/envdef/environment_test.go

@@ -1,4 +1,4 @@
-package envdef_test
+package envdef
 
 import (
 	"encoding/json"
@@ -9,7 +9,6 @@ import (
 
 	"github.com/ActiveState/cli/internal/osutils"
 	"github.com/ActiveState/cli/internal/testhelpers/suite"
-	"github.com/ActiveState/cli/pkg/runtime/internal/envdef"
 	"github.com/stretchr/testify/require"
 
 	"github.com/ActiveState/cli/internal/fileutils"
@@ -21,20 +20,20 @@ type EnvironmentTestSuite struct {
 
 func (suite *EnvironmentTestSuite) TestMergeVariables() {
 
-	ev1 := envdef.EnvironmentVariable{}
+	ev1 := EnvironmentVariable{}
 	err := json.Unmarshal([]byte(`{
 		"env_name": "V",
 		"values": ["a", "b"]
 		}`), &ev1)
 	require.NoError(suite.T(), err)
-	ev2 := envdef.EnvironmentVariable{}
+	ev2 := EnvironmentVariable{}
 	err = json.Unmarshal([]byte(`{
 		"env_name": "V",
 		"values": ["b", "c"]
 		}`), &ev2)
 	require.NoError(suite.T(), err)
 
-	expected := &envdef.EnvironmentVariable{}
+	expected := &EnvironmentVariable{}
 	err = json.Unmarshal([]byte(`{
 		"env_name": "V",
 		"values": ["b", "c", "a"],
@@ -51,22 +50,22 @@ func (suite *EnvironmentTestSuite) TestMergeVariables() {
 }
 
 func (suite *EnvironmentTestSuite) TestMerge() {
-	ed1 := &envdef.EnvironmentDefinition{}
+	ed1 := &EnvironmentDefinition{}
 
 	err := json.Unmarshal([]byte(`{
 			"env": [{"env_name": "V", "values": ["a", "b"]}],
 			"installdir": "abc"
 		}`), ed1)
 	require.NoError(suite.T(), err)
 
-	ed2 := envdef.EnvironmentDefinition{}
+	ed2 := EnvironmentDefinition{}
 	err = json.Unmarshal([]byte(`{
 			"env": [{"env_name": "V", "values": ["c", "d"]}],
 			"installdir": "abc"
 		}`), &ed2)
 	require.NoError(suite.T(), err)
 
-	expected := envdef.EnvironmentDefinition{}
+	expected := EnvironmentDefinition{}
 	err = json.Unmarshal([]byte(`{
 			"env": [{"env_name": "V", "values": ["c", "d", "a", "b"]}],
 			"installdir": "abc"
@@ -80,7 +79,7 @@ func (suite *EnvironmentTestSuite) TestMerge() {
 }
 
 func (suite *EnvironmentTestSuite) TestInheritPath() {
-	ed1 := &envdef.EnvironmentDefinition{}
+	ed1 := &EnvironmentDefinition{}
 
 	err := json.Unmarshal([]byte(`{
 			"env": [{"env_name": "PATH", "values": ["NEWVALUE"]}],
@@ -90,7 +89,7 @@ func (suite *EnvironmentTestSuite) TestInheritPath() {
 		}`), ed1)
 	require.NoError(suite.T(), err)
 
-	env, err := ed1.GetEnvBasedOn(map[string]string{"PATH": "OLDVALUE"})
+	env, err := ed1.getEnvBasedOn(map[string]string{"PATH": "OLDVALUE"})
 	require.NoError(suite.T(), err)
 	suite.True(strings.HasPrefix(env["PATH"], "NEWVALUE"), "%s does not start with NEWVALUE", env["PATH"])
 	suite.True(strings.HasSuffix(env["PATH"], "OLDVALUE"), "%s does not end with OLDVALUE", env["PATH"])
@@ -99,11 +98,11 @@ func (suite *EnvironmentTestSuite) TestInheritPath() {
 func (suite *EnvironmentTestSuite) TestSharedTests() {
 
 	type testCase struct {
-		Name        string                         `json:"name"`
-		Definitions []envdef.EnvironmentDefinition `json:"definitions"`
-		BaseEnv     map[string]string              `json:"base_env"`
-		Expected    map[string]string              `json:"result"`
-		IsError     bool                           `json:"error"`
+		Name        string                  `json:"name"`
+		Definitions []EnvironmentDefinition `json:"definitions"`
+		BaseEnv     map[string]string       `json:"base_env"`
+		Expected    map[string]string       `json:"result"`
+		IsError     bool                    `json:"error"`
 	}
 
 	td, err := os.ReadFile("runtime_test_cases.json")
@@ -126,7 +125,7 @@ func (suite *EnvironmentTestSuite) TestSharedTests() {
 				suite.Assert().NoError(err, "error merging %d-th definition", i)
 			}
 
-			res, err := ed.GetEnvBasedOn(tc.BaseEnv)
+			res, err := ed.getEnvBasedOn(tc.BaseEnv)
 			if tc.IsError {
 				suite.Assert().Error(err)
 				return
@@ -139,7 +138,7 @@ func (suite *EnvironmentTestSuite) TestSharedTests() {
 }
 
 func (suite *EnvironmentTestSuite) TestValueString() {
-	ev1 := envdef.EnvironmentVariable{}
+	ev1 := EnvironmentVariable{}
 	err := json.Unmarshal([]byte(`{
 		"env_name": "V",
 		"values": ["a", "b"]
@@ -151,7 +150,7 @@ func (suite *EnvironmentTestSuite) TestValueString() {
 }
 
 func (suite *EnvironmentTestSuite) TestGetEnv() {
-	ed1 := envdef.EnvironmentDefinition{}
+	ed1 := EnvironmentDefinition{}
 	err := json.Unmarshal([]byte(`{
 			"env": [{"env_name": "V", "values": ["a", "b"]}],
 			"installdir": "abc"
@@ -177,7 +176,7 @@ func (suite *EnvironmentTestSuite) TestFindBinPathFor() {
 	require.NoError(suite.T(), err, "creating temporary directory")
 	defer os.RemoveAll(tmpDir)
 
-	ed1 := envdef.EnvironmentDefinition{}
+	ed1 := EnvironmentDefinition{}
 	err = json.Unmarshal([]byte(`{
 			"env": [{"env_name": "PATH", "values": ["${INSTALLDIR}/bin", "${INSTALLDIR}/bin2"]}],
 			"installdir": "abc"
@@ -187,7 +186,7 @@ func (suite *EnvironmentTestSuite) TestFindBinPathFor() {
 	tmpDir, err = fileutils.GetLongPathName(tmpDir)
 	require.NoError(suite.T(), err)
 
-	constants := envdef.NewConstants(tmpDir)
+	constants := NewConstants(tmpDir)
 	// expand variables
 	ed1.ExpandVariables(constants)
 
@@ -248,7 +247,7 @@ func TestFilterPATH(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			envdef.FilterPATH(tt.args.env, tt.args.excludes...)
+			FilterPATH(tt.args.env, tt.args.excludes...)
 			require.Equal(t, tt.want, tt.args.env["PATH"])
 		})
 	}

test/integration/exec_int_test.go

@@ -178,6 +178,33 @@ func (suite *ExecIntegrationTestSuite) TestExeBatArguments() {
 	cp.ExpectExitCode(0)
 }
 
+func (suite *ExecIntegrationTestSuite) TestExec_PATH_and_Path_on_Windows() {
+	suite.OnlyRunForTags(tagsuite.Exec)
+
+	if runtime.GOOS != "windows" {
+		suite.T().Skip("This test is only for windows")
+	}
+
+	ts := e2e.New(suite.T(), false)
+	defer ts.Close()
+
+	cp := ts.Spawn("checkout", "ActiveState-CLI/small-python", ".")
+	cp.Expect("Checked out", e2e.RuntimeSourcingTimeoutOpt)
+	cp.ExpectExitCode(0)
+
+	cp = ts.Spawn("exec", "where", "python3")
+	cp.Expect(os.TempDir()) // from runtime's defined PATH
+	cp.ExpectExitCode(0)
+
+	cp = ts.Spawn("exec", "where", "notepad")
+	cp.Expect("notepad.exe") // from OS-defined default Path
+	cp.ExpectExitCode(0)
+
+	cp = ts.Spawn("exec", "does-not-exist")
+	cp.Expect("not found") // neither on PATH nor Path
+	cp.ExpectNotExitCode(0)
+}
+
 func TestExecIntegrationTestSuite(t *testing.T) {
 	suite.Run(t, new(ExecIntegrationTestSuite))
 }