From 22b64deefc7e5cd7342634997f0fd12aa6f612f5 Mon Sep 17 00:00:00 2001 From: Trevor Brown Date: Wed, 27 Nov 2024 11:21:24 -0500 Subject: [PATCH] fix(golang-rewrite): `asdf exec` and `asdf env` command fixes * Create `CurrentEnv` and `MergeEnv` helper functions * Add another test for `paths.RemoveFromPath` function * Move executable finding functions to shims package * Correct PATH code for env and exec commands --- cmd/cmd.go | 55 +++++++++++++------------------- internal/execenv/execenv.go | 29 +++++++++++++++++ internal/execenv/execenv_test.go | 33 +++++++++++++++++++ internal/execute/execute.go | 14 -------- internal/paths/paths_test.go | 5 +++ internal/shims/shims.go | 22 +++++++++---- 6 files changed, 104 insertions(+), 54 deletions(-) diff --git a/cmd/cmd.go b/cmd/cmd.go index d108e5c5..5dd036c5 100644 --- a/cmd/cmd.go +++ b/cmd/cmd.go @@ -7,7 +7,6 @@ import ( "io" "log" "os" - osexec "os/exec" "path/filepath" "slices" "strings" @@ -20,7 +19,6 @@ import ( "asdf/internal/help" "asdf/internal/info" "asdf/internal/installs" - "asdf/internal/paths" "asdf/internal/plugins" "asdf/internal/resolve" "asdf/internal/shims" @@ -381,36 +379,23 @@ func envCommand(logger *log.Logger, shimmedCommand string, args []string) error if err != nil { return err } - callbackEnv := map[string]string{ + env := map[string]string{ "ASDF_INSTALL_TYPE": parsedVersion.Type, "ASDF_INSTALL_VERSION": parsedVersion.Value, "ASDF_INSTALL_PATH": installs.InstallPath(conf, plugin, parsedVersion), - "PATH": setPath(conf, execPaths), + "PATH": setPath(execPaths), } - var env map[string]string - var fname string - - if parsedVersion.Type == "system" { - env = execute.SliceToMap(os.Environ()) - newPath := paths.RemoveFromPath(env["PATH"], shims.Directory(conf)) - env["PATH"] = newPath - var found bool - fname, found = shims.FindSystemExecutable(conf, command) - if !found { - fmt.Println("not found") - return err - } - } else { - env, err = execenv.Generate(plugin, callbackEnv) + if parsedVersion.Type != "system" { + env, err = execenv.Generate(plugin, env) if _, ok := err.(plugins.NoCallbackError); !ok && err != nil { return err } + } - fname, err = osexec.LookPath(command) - if err != nil { - return err - } + fname, err := shims.ExecutableOnPath(env["PATH"], command) + if err != nil { + return err } err = exec.Exec(fname, realArgs, execute.MapToSlice(env)) @@ -420,9 +405,8 @@ func envCommand(logger *log.Logger, shimmedCommand string, args []string) error return err } -func setPath(conf config.Config, pathes []string) string { - currentPath := os.Getenv("PATH") - return strings.Join(pathes, ":") + ":" + paths.RemoveFromPath(currentPath, shims.Directory(conf)) +func setPath(paths []string) string { + return strings.Join(paths, ":") + ":" + os.Getenv("PATH") } func execCommand(logger *log.Logger, command string, args []string) error { @@ -438,8 +422,6 @@ func execCommand(logger *log.Logger, command string, args []string) error { } executable, plugin, version, err := getExecutable(logger, conf, command) - fmt.Printf("version %#+v\n", version) - fmt.Println("here") if err != nil { return err } @@ -451,19 +433,26 @@ func execCommand(logger *log.Logger, command string, args []string) error { } parsedVersion := toolversions.Parse(version) - fmt.Printf("parsedVersion %#+v\n", parsedVersion) - paths, err := shims.ExecutablePaths(conf, plugin, parsedVersion) + execPaths, err := shims.ExecutablePaths(conf, plugin, parsedVersion) if err != nil { return err } - callbackEnv := map[string]string{ + env := map[string]string{ "ASDF_INSTALL_TYPE": parsedVersion.Type, "ASDF_INSTALL_VERSION": parsedVersion.Value, "ASDF_INSTALL_PATH": installs.InstallPath(conf, plugin, parsedVersion), - "PATH": setPath(conf, paths), + "PATH": setPath(execPaths), + } + + if parsedVersion.Type != "system" { + env, err = execenv.Generate(plugin, env) + if _, ok := err.(plugins.NoCallbackError); !ok && err != nil { + return err + } } - env, _ := execenv.Generate(plugin, callbackEnv) + env = execenv.MergeEnv(execenv.SliceToMap(os.Environ()), env) + return exec.Exec(executable, args, execute.MapToSlice(env)) } diff --git a/internal/execenv/execenv.go b/internal/execenv/execenv.go index eeb8bcf2..28015895 100644 --- a/internal/execenv/execenv.go +++ b/internal/execenv/execenv.go @@ -4,6 +4,7 @@ package execenv import ( "fmt" + "os" "strings" "asdf/internal/execute" @@ -12,6 +13,20 @@ import ( const execEnvCallbackName = "exec-env" +// CurrentEnv returns the current environment as a map +func CurrentEnv() map[string]string { + return SliceToMap(os.Environ()) +} + +// MergeEnv takes two maps with string keys and values and merges them. +func MergeEnv(map1, map2 map[string]string) map[string]string { + for key, value := range map2 { + map1[key] = value + } + + return map1 +} + // Generate runs exec-env callback if available and captures the environment // variables it sets. It then parses them and returns them as a map. func Generate(plugin plugins.Plugin, callbackEnv map[string]string) (env map[string]string, err error) { @@ -47,3 +62,17 @@ func envMap(env string) map[string]string { return slice } + +// SliceToMap converts an env map to env slice suitable for syscall.Exec +func SliceToMap(env []string) map[string]string { + envMap := map[string]string{} + + for _, envVar := range env { + varValue := strings.Split(envVar, "=") + if len(varValue) == 2 { + envMap[varValue[0]] = varValue[1] + } + } + + return envMap +} diff --git a/internal/execenv/execenv_test.go b/internal/execenv/execenv_test.go index d705f66c..efda54aa 100644 --- a/internal/execenv/execenv_test.go +++ b/internal/execenv/execenv_test.go @@ -15,6 +15,39 @@ const ( testPluginName2 = "ruby" ) +func TestCurrentEnv(t *testing.T) { + t.Run("returns map of current environment", func(t *testing.T) { + envMap := CurrentEnv() + path, found := envMap["PATH"] + assert.True(t, found) + assert.NotEmpty(t, path) + }) +} + +func TestMergeEnv(t *testing.T) { + t.Run("merges two maps", func(t *testing.T) { + map1 := map[string]string{"Key": "value"} + map2 := map[string]string{"Key2": "value2"} + map3 := MergeEnv(map1, map2) + assert.Equal(t, map3["Key"], "value") + assert.Equal(t, map3["Key2"], "value2") + }) + + t.Run("doesn't change original map", func(t *testing.T) { + map1 := map[string]string{"Key": "value"} + map2 := map[string]string{"Key2": "value2"} + _ = MergeEnv(map1, map2) + assert.Equal(t, map1["Key2"], "value2") + }) + + t.Run("second map overwrites values in first", func(t *testing.T) { + map1 := map[string]string{"Key": "value"} + map2 := map[string]string{"Key": "value2"} + map3 := MergeEnv(map1, map2) + assert.Equal(t, map3["Key"], "value2") + }) +} + func TestGenerate(t *testing.T) { testDataDir := t.TempDir() diff --git a/internal/execute/execute.go b/internal/execute/execute.go index 561c647b..32f5c90e 100644 --- a/internal/execute/execute.go +++ b/internal/execute/execute.go @@ -66,20 +66,6 @@ func MapToSlice(env map[string]string) (slice []string) { return slice } -// SliceToMap converts an env map to env slice suitable for syscall.Exec -func SliceToMap(env []string) map[string]string { - envMap := map[string]string{} - - for _, envVar := range env { - varValue := strings.Split(envVar, "=") - if len(varValue) == 2 { - envMap[varValue[0]] = varValue[1] - } - } - - return envMap -} - func formatArgString(args []string) string { var newArgs []string for _, str := range args { diff --git a/internal/paths/paths_test.go b/internal/paths/paths_test.go index 775371f8..0d78bc5a 100644 --- a/internal/paths/paths_test.go +++ b/internal/paths/paths_test.go @@ -12,6 +12,11 @@ func TestRemoveFromPath(t *testing.T) { assert.Equal(t, got, "/foo/bar:/home/user/bin") }) + t.Run("returns PATH string with multiple matching paths removed", func(t *testing.T) { + got := RemoveFromPath("/foo/bar:/baz/bim:/baz/bim:/home/user/bin", "/baz/bim") + assert.Equal(t, got, "/foo/bar:/home/user/bin") + }) + t.Run("returns PATH string unchanged when no matching path found", func(t *testing.T) { got := RemoveFromPath("/foo/bar:/baz/bim:/home/user/bin", "/path-not-present/") assert.Equal(t, got, "/foo/bar:/baz/bim:/home/user/bin") diff --git a/internal/shims/shims.go b/internal/shims/shims.go index 9c187ed5..2ec80745 100644 --- a/internal/shims/shims.go +++ b/internal/shims/shims.go @@ -99,7 +99,7 @@ func FindExecutable(conf config.Config, shimName, currentDirectory string) (stri for plugin, toolVersions := range existingPluginToolVersions { for _, version := range toolVersions.Versions { if version == "system" { - if executablePath, found := FindSystemExecutable(conf, shimName); found { + if executablePath, found := SystemExecutableOnPath(conf, shimName); found { return executablePath, plugin, version, true, nil } @@ -122,16 +122,24 @@ func FindExecutable(conf config.Config, shimName, currentDirectory string) (stri return "", plugins.Plugin{}, "", false, NoExecutableForPluginError{shim: shimName, tools: tools, versions: versions} } -// FindSystemExecutable returns the path to the system -// executable if found -func FindSystemExecutable(conf config.Config, executableName string) (string, bool) { +// SystemExecutableOnPath returns the path to the system executable if found, +// removes asdf shim directory from search +func SystemExecutableOnPath(conf config.Config, executableName string) (string, bool) { currentPath := os.Getenv("PATH") - defer os.Setenv("PATH", currentPath) - os.Setenv("PATH", paths.RemoveFromPath(currentPath, Directory(conf))) - executablePath, err := exec.LookPath(executableName) + executablePath, err := ExecutableOnPath(paths.RemoveFromPath(currentPath, Directory(conf)), executableName) return executablePath, err == nil } +// ExecutableOnPath returns the path to an executable if one is found on the +// provided paths. `path` must be in the same format as the `PATH` environment +// variable. +func ExecutableOnPath(path, command string) (string, error) { + currentPath := os.Getenv("PATH") + defer os.Setenv("PATH", currentPath) + os.Setenv("PATH", path) + return exec.LookPath(command) +} + // GetExecutablePath returns the path of the executable func GetExecutablePath(conf config.Config, plugin plugins.Plugin, shimName, version string) (string, error) { path, err := getCustomExecutablePath(conf, plugin, shimName, version)