Skip to content

Commit

Permalink
Handle case-insensitive env var names on Windows.
Browse files Browse the repository at this point in the history 
We read the case-insensitive version as needed, but replace it with our case-sensitive version (e.g. "Path" -> "PATH").
  • Loading branch information
@mitchell-as
mitchell-as committed Oct 22, 2024
1 parent 4bbfd14 commit 7653324
Show file tree
Hide file tree
Showing 2 changed files with 52 additions and 27 deletions.
38 changes: 32 additions & 6 deletions pkg/runtime/internal/envdef/environment.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"maps"
"os"
"path/filepath"
"runtime"
"strings"

"github.com/ActiveState/cli/internal/constants"
Expand Down Expand Up @@ -344,35 +345,60 @@ 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 case-insensitive version. Our case-sensitive version has already
// processed the value of the case-insensitive version.
delete(res, osName)
}
}
}
return res, nil
Expand All @@ -388,7 +414,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))
}
Expand Down
41 changes: 20 additions & 21 deletions pkg/runtime/internal/envdef/environment_test.go
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package envdef_test
package envdef

import (
"encoding/json"
Expand All @@ -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"
Expand All @@ -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"],
Expand All @@ -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"
Expand All @@ -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"]}],
Expand All @@ -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"])
Expand All @@ -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")
Expand All @@ -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
Expand All @@ -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"]
Expand All @@ -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"
Expand All @@ -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"
Expand All @@ -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)

Expand Down Expand Up @@ -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"])
})
}
Expand Down

0 comments on commit 7653324

Please sign in to comment.