From 1f41910cc3a512b44a371a65b622cae93941b25e Mon Sep 17 00:00:00 2001 From: Mike Drakos Date: Thu, 17 Oct 2024 14:41:33 -0700 Subject: [PATCH 1/6] Fix env ordering --- internal/runners/exec/exec.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/internal/runners/exec/exec.go b/internal/runners/exec/exec.go index 038d1354a9..4c0143f923 100644 --- a/internal/runners/exec/exec.go +++ b/internal/runners/exec/exec.go @@ -6,6 +6,7 @@ import ( "os/exec" "path/filepath" "runtime" + "sort" "strconv" "strings" @@ -24,7 +25,7 @@ import ( "github.com/ActiveState/cli/internal/output" "github.com/ActiveState/cli/internal/primer" "github.com/ActiveState/cli/internal/runbits/rationalize" - "github.com/ActiveState/cli/internal/runbits/runtime" + runtime_runbit "github.com/ActiveState/cli/internal/runbits/runtime" "github.com/ActiveState/cli/internal/runbits/runtime/trigger" "github.com/ActiveState/cli/internal/subshell" "github.com/ActiveState/cli/internal/virtualenvironment" @@ -184,7 +185,12 @@ func (s *Exec) Run(params *Params, args ...string) (rerr error) { } } - _, _, err = osutils.ExecuteAndPipeStd(exeTarget, args[1:], osutils.EnvMapToSlice(env)) + // Sort the env so our PATH environment variable is interpreted first + envs := osutils.EnvMapToSlice(env) + sort.Slice(envs, func(i, j int) bool { + return envs[i] > envs[j] + }) + _, _, err = osutils.ExecuteAndPipeStd(exeTarget, args[1:], envs) if eerr, ok := err.(*exec.ExitError); ok { return errs.Silence(errs.WrapExitCode(eerr, eerr.ExitCode())) } From 8bde15e13cc095f70ffb7fcd210555a5e2aa2bcb Mon Sep 17 00:00:00 2001 From: Mike Drakos Date: Thu, 17 Oct 2024 15:21:44 -0700 Subject: [PATCH 2/6] Increase timeout --- test/integration/exec_int_test.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/test/integration/exec_int_test.go b/test/integration/exec_int_test.go index f3cfff58d7..acd8cd8345 100644 --- a/test/integration/exec_int_test.go +++ b/test/integration/exec_int_test.go @@ -8,7 +8,6 @@ import ( "runtime" "strings" "testing" - "time" "github.com/ActiveState/cli/internal/constants" "github.com/ActiveState/cli/internal/environment" @@ -16,7 +15,6 @@ import ( "github.com/ActiveState/cli/internal/testhelpers/e2e" "github.com/ActiveState/cli/internal/testhelpers/suite" "github.com/ActiveState/cli/internal/testhelpers/tagsuite" - "github.com/ActiveState/termtest" ) type ExecIntegrationTestSuite struct { @@ -176,7 +174,7 @@ func (suite *ExecIntegrationTestSuite) TestExeBatArguments() { inputs := []string{"aa", "hello world", "&whoami", "imnot|apipe", "%NotAppData%", "^NotEscaped", "(NotAGroup)"} outputs := `"` + strings.Join(inputs, `" "`) + `"` cp = ts.SpawnWithOpts(e2e.OptArgs(append([]string{"exec", reportBat, "--"}, inputs...)...)) - cp.Expect(outputs, termtest.OptExpectTimeout(5*time.Second)) + cp.Expect(outputs, e2e.RuntimeBuildSourcingTimeoutOpt) cp.ExpectExitCode(0) } From aa2462492b98767e5ed55d520a6e30ecee5217fe Mon Sep 17 00:00:00 2001 From: Mike Drakos Date: Fri, 18 Oct 2024 11:34:51 -0700 Subject: [PATCH 3/6] Only swap path environment variables --- internal/runners/exec/exec.go | 48 ++++++++++++++++++++++++++++++----- 1 file changed, 41 insertions(+), 7 deletions(-) diff --git a/internal/runners/exec/exec.go b/internal/runners/exec/exec.go index 4c0143f923..411c9301b3 100644 --- a/internal/runners/exec/exec.go +++ b/internal/runners/exec/exec.go @@ -6,7 +6,6 @@ import ( "os/exec" "path/filepath" "runtime" - "sort" "strconv" "strings" @@ -185,12 +184,7 @@ func (s *Exec) Run(params *Params, args ...string) (rerr error) { } } - // Sort the env so our PATH environment variable is interpreted first - envs := osutils.EnvMapToSlice(env) - sort.Slice(envs, func(i, j int) bool { - return envs[i] > envs[j] - }) - _, _, err = osutils.ExecuteAndPipeStd(exeTarget, args[1:], envs) + _, _, err = osutils.ExecuteAndPipeStd(exeTarget, args[1:], sortPaths(osutils.EnvMapToSlice(env))) if eerr, ok := err.(*exec.ExitError); ok { return errs.Silence(errs.WrapExitCode(eerr, eerr.ExitCode())) } @@ -201,6 +195,46 @@ func (s *Exec) Run(params *Params, args ...string) (rerr error) { return nil } +// sortPaths the env so our PATH environment variable is interpreted first +func sortPaths(env []string) []string { + if runtime.GOOS != "windows" { + return env + } + + const ( + windowsPathPrefix = "Path=" + unixPathPrefix = "PATH=" + ) + + var windowsPathIndex, unixPathIndex = -1, -1 + for i, e := range env { + switch { + case strings.HasPrefix(e, windowsPathPrefix): + windowsPathIndex = i + case strings.HasPrefix(e, unixPathPrefix): + unixPathIndex = i + } + if windowsPathIndex != -1 && unixPathIndex != -1 { + break + } + } + + if windowsPathIndex == -1 || unixPathIndex == -1 { + return env + } + + // Ensure Unix PATH is after Windows PATH + if windowsPathIndex > unixPathIndex { + env[windowsPathIndex], env[unixPathIndex] = env[unixPathIndex], env[windowsPathIndex] + } + + for _, e := range env { + fmt.Println(e) + } + + return env +} + func projectFromRuntimeDir(cfg projectfile.ConfigGetter, runtimeDir string) string { projects := projectfile.GetProjectMapping(cfg) for _, paths := range projects { From 1fe51072a1fa78d6223621053b9c8720f8b5988c Mon Sep 17 00:00:00 2001 From: Mike Drakos Date: Fri, 18 Oct 2024 14:36:36 -0700 Subject: [PATCH 4/6] Promote path --- internal/runners/exec/exec.go | 42 +---------------------------------- pkg/runtime/runtime.go | 24 ++++++++++++++++++++ 2 files changed, 25 insertions(+), 41 deletions(-) diff --git a/internal/runners/exec/exec.go b/internal/runners/exec/exec.go index 411c9301b3..112d39482e 100644 --- a/internal/runners/exec/exec.go +++ b/internal/runners/exec/exec.go @@ -184,7 +184,7 @@ func (s *Exec) Run(params *Params, args ...string) (rerr error) { } } - _, _, err = osutils.ExecuteAndPipeStd(exeTarget, args[1:], sortPaths(osutils.EnvMapToSlice(env))) + _, _, err = osutils.ExecuteAndPipeStd(exeTarget, args[1:], osutils.EnvMapToSlice(env)) if eerr, ok := err.(*exec.ExitError); ok { return errs.Silence(errs.WrapExitCode(eerr, eerr.ExitCode())) } @@ -195,46 +195,6 @@ func (s *Exec) Run(params *Params, args ...string) (rerr error) { return nil } -// sortPaths the env so our PATH environment variable is interpreted first -func sortPaths(env []string) []string { - if runtime.GOOS != "windows" { - return env - } - - const ( - windowsPathPrefix = "Path=" - unixPathPrefix = "PATH=" - ) - - var windowsPathIndex, unixPathIndex = -1, -1 - for i, e := range env { - switch { - case strings.HasPrefix(e, windowsPathPrefix): - windowsPathIndex = i - case strings.HasPrefix(e, unixPathPrefix): - unixPathIndex = i - } - if windowsPathIndex != -1 && unixPathIndex != -1 { - break - } - } - - if windowsPathIndex == -1 || unixPathIndex == -1 { - return env - } - - // Ensure Unix PATH is after Windows PATH - if windowsPathIndex > unixPathIndex { - env[windowsPathIndex], env[unixPathIndex] = env[unixPathIndex], env[windowsPathIndex] - } - - for _, e := range env { - fmt.Println(e) - } - - return env -} - func projectFromRuntimeDir(cfg projectfile.ConfigGetter, runtimeDir string) string { projects := projectfile.GetProjectMapping(cfg) for _, paths := range projects { diff --git a/pkg/runtime/runtime.go b/pkg/runtime/runtime.go index 1ce0c63fad..3fce9b0db1 100644 --- a/pkg/runtime/runtime.go +++ b/pkg/runtime/runtime.go @@ -4,6 +4,7 @@ import ( "maps" "os" "path/filepath" + "runtime" "github.com/ActiveState/cli/internal/errs" "github.com/ActiveState/cli/internal/fileutils" @@ -165,9 +166,32 @@ func (r *Runtime) getEnv(inherit bool) (map[string]string, map[string]string, er execVars["PATH"] += string(os.PathListSeparator) + vars["PATH"] } + promotePath(vars) + promotePath(execVars) + return vars, execVars, nil } +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") +} + func (r *Runtime) Env(inherit bool) Environment { if inherit { return r.envInherit From 7029a4cc9f09957aedd1633d80b773104558eb56 Mon Sep 17 00:00:00 2001 From: Mike Drakos Date: Fri, 18 Oct 2024 14:52:56 -0700 Subject: [PATCH 5/6] Add comment --- pkg/runtime/runtime.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/runtime/runtime.go b/pkg/runtime/runtime.go index 3fce9b0db1..48bd8c4253 100644 --- a/pkg/runtime/runtime.go +++ b/pkg/runtime/runtime.go @@ -172,6 +172,8 @@ func (r *Runtime) getEnv(inherit bool) (map[string]string, map[string]string, er return vars, execVars, 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 From 97813a844ee9571f74b1ca9d1a845fad65dd2215 Mon Sep 17 00:00:00 2001 From: Mike Drakos Date: Fri, 18 Oct 2024 15:18:43 -0700 Subject: [PATCH 6/6] Move logic --- pkg/runtime/internal/envdef/collection.go | 28 ++++++++++++++++++++++- pkg/runtime/runtime.go | 26 --------------------- 2 files changed, 27 insertions(+), 27 deletions(-) diff --git a/pkg/runtime/internal/envdef/collection.go b/pkg/runtime/internal/envdef/collection.go index 3801ff4ba7..fbb2077b57 100644 --- a/pkg/runtime/internal/envdef/collection.go +++ b/pkg/runtime/internal/envdef/collection.go @@ -1,7 +1,9 @@ package envdef import ( + "os" "path/filepath" + "runtime" "sync" "github.com/ActiveState/cli/internal/errs" @@ -63,5 +65,29 @@ func (c *Collection) Environment(installPath string, inherit bool) (map[string]s } } constants := NewConstants(installPath) - return result.ExpandVariables(constants).GetEnv(inherit), nil + 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") } diff --git a/pkg/runtime/runtime.go b/pkg/runtime/runtime.go index 48bd8c4253..1ce0c63fad 100644 --- a/pkg/runtime/runtime.go +++ b/pkg/runtime/runtime.go @@ -4,7 +4,6 @@ import ( "maps" "os" "path/filepath" - "runtime" "github.com/ActiveState/cli/internal/errs" "github.com/ActiveState/cli/internal/fileutils" @@ -166,34 +165,9 @@ func (r *Runtime) getEnv(inherit bool) (map[string]string, map[string]string, er execVars["PATH"] += string(os.PathListSeparator) + vars["PATH"] } - promotePath(vars) - promotePath(execVars) - return vars, execVars, 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") -} - func (r *Runtime) Env(inherit bool) Environment { if inherit { return r.envInherit