From 4686d25cda20fd4f8514df64c9ce5af0f3e02a2f Mon Sep 17 00:00:00 2001 From: mohsenari Date: Thu, 21 Nov 2024 02:37:42 -0500 Subject: [PATCH 1/7] fix for plugin and env both editing PATH --- internal/devbox/devbox.go | 3 +++ internal/devconfig/config.go | 21 +++++++++++++++++++-- 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/internal/devbox/devbox.go b/internal/devbox/devbox.go index fdc25741b08..7df5fb25981 100644 --- a/internal/devbox/devbox.go +++ b/internal/devbox/devbox.go @@ -726,6 +726,9 @@ func (d *Devbox) computeEnv( if err != nil { return nil, err } + // fmt.Println("PATH before configEnv: ", env["PATH"]) + // fmt.Println("PATH after configEnv: ", configEnv["PATH"]) + addEnvIfNotPreviouslySetByDevbox(env, configEnv) markEnvsAsSetByDevbox(configEnv) diff --git a/internal/devconfig/config.go b/internal/devconfig/config.go index 5dbba7614c5..5947547fa75 100644 --- a/internal/devconfig/config.go +++ b/internal/devconfig/config.go @@ -9,6 +9,7 @@ import ( "net/http" "os" "path/filepath" + "strings" "syscall" "time" @@ -346,9 +347,25 @@ func (c *Config) NixPkgsCommitHash() string { func (c *Config) Env() map[string]string { env := map[string]string{} for _, i := range c.included { - maps.Copy(env, i.Env()) + // instead of overwriting PATH we need to append it + // so copying /plugin1/bin//my/bin/:$PATH into /my2/bin/:$PATH + // should result in /my/bin/:/my2/bin/:$PATH + iEnv := i.Env() + iEnvPath := iEnv["PATH"] + fmt.Println("existing env path is: ", iEnvPath) + if env["PATH"] != "" { + iEnv["PATH"] = strings.Replace(iEnvPath, "$PATH", env["PATH"], 1) + } + maps.Copy(env, iEnv) + fmt.Println("PATH now is: ", env["PATH"]) + } + fmt.Println("PATH before last copy is: ", env["PATH"]) + rootEnv := c.Root.Env + if env["PATH"] != "" { + rootEnv["PATH"] = strings.Replace(rootEnv["PATH"], "$PATH", env["PATH"], 1) } - maps.Copy(env, c.Root.Env) + maps.Copy(env, rootEnv) // this results in PATH from devbox.json overwrite PATH from plugin + fmt.Println("PATH after config copy becomes: ", env["PATH"]) return env } From 165ffec5335bab7c7f70ba8a04f522aa24a93575 Mon Sep 17 00:00:00 2001 From: mohsenari Date: Thu, 21 Nov 2024 11:24:09 -0500 Subject: [PATCH 2/7] Cleanup and moved logic to a function --- internal/devconfig/config.go | 46 +++++++++++++++++++++--------------- 1 file changed, 27 insertions(+), 19 deletions(-) diff --git a/internal/devconfig/config.go b/internal/devconfig/config.go index 5947547fa75..592e8211f13 100644 --- a/internal/devconfig/config.go +++ b/internal/devconfig/config.go @@ -347,25 +347,18 @@ func (c *Config) NixPkgsCommitHash() string { func (c *Config) Env() map[string]string { env := map[string]string{} for _, i := range c.included { - // instead of overwriting PATH we need to append it - // so copying /plugin1/bin//my/bin/:$PATH into /my2/bin/:$PATH - // should result in /my/bin/:/my2/bin/:$PATH - iEnv := i.Env() - iEnvPath := iEnv["PATH"] - fmt.Println("existing env path is: ", iEnvPath) - if env["PATH"] != "" { - iEnv["PATH"] = strings.Replace(iEnvPath, "$PATH", env["PATH"], 1) - } - maps.Copy(env, iEnv) - fmt.Println("PATH now is: ", env["PATH"]) - } - fmt.Println("PATH before last copy is: ", env["PATH"]) - rootEnv := c.Root.Env - if env["PATH"] != "" { - rootEnv["PATH"] = strings.Replace(rootEnv["PATH"], "$PATH", env["PATH"], 1) - } - maps.Copy(env, rootEnv) // this results in PATH from devbox.json overwrite PATH from plugin - fmt.Println("PATH after config copy becomes: ", env["PATH"]) + // Here, in new pluginPath we replace the keyword "$PATH" + // with what we have for PATH so far. If so far we have /plugin1/bin:$PATH + // and new plugin adds /plugin2/bin/:$PATH the result becomes + // /plugin2/bin/:/plugin1/bin/:$PATH + pluginEnv := mergePATHsFromTwoEnvs(env, i.Env()) + maps.Copy(env, pluginEnv) + } + // Here we merge the processed PATH from all plugins to PATH in config env. + // So if config env has /config/env/:$PATH merging with the plugin PATH + // it becomes /config/env/:/plugin2/bin/:/plugin1/bin/:$PATH + rootEnv := mergePATHsFromTwoEnvs(env, c.Root.Env) + maps.Copy(env, rootEnv) return env } @@ -423,3 +416,18 @@ func createIncludableFromPluginConfig(pluginConfig *plugin.Config) *Config { } return includable } + +// Instead of overwriting PATH modifications we need to merge them +// so merging of /plugin1/bin/:$PATH and /plugin2/bin/:$PATH +// and /config/env/bin/:$PATH should result in +// +// /config/env/bin/:/plugin2/bin/:/plugin1/bin/:$PATH +// +// because config env should take priority over plugins +func mergePATHsFromTwoEnvs(currentEnv map[string]string, newEnv map[string]string) map[string]string { + if currentEnv["PATH"] != "" { + slog.Debug("A Plugin or Config wants to modify PATH. Processing the merge", "AddedPATH", newEnv["PATH"]) + newEnv["PATH"] = strings.Replace(newEnv["PATH"], "$PATH", currentEnv["PATH"], 1) + } + return newEnv +} From bffbfc6cca25e18cf695f4338b9c03cf83f2de5e Mon Sep 17 00:00:00 2001 From: mohsenari Date: Thu, 21 Nov 2024 11:31:57 -0500 Subject: [PATCH 3/7] removed commented code --- internal/devbox/devbox.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/internal/devbox/devbox.go b/internal/devbox/devbox.go index 7df5fb25981..721312b6ed4 100644 --- a/internal/devbox/devbox.go +++ b/internal/devbox/devbox.go @@ -726,8 +726,6 @@ func (d *Devbox) computeEnv( if err != nil { return nil, err } - // fmt.Println("PATH before configEnv: ", env["PATH"]) - // fmt.Println("PATH after configEnv: ", configEnv["PATH"]) addEnvIfNotPreviouslySetByDevbox(env, configEnv) From 9ee0767e6dca2d3ce6d75473e0d306b2e8d2e65c Mon Sep 17 00:00:00 2001 From: mohsenari Date: Thu, 21 Nov 2024 11:32:17 -0500 Subject: [PATCH 4/7] removed empty line --- internal/devbox/devbox.go | 1 - 1 file changed, 1 deletion(-) diff --git a/internal/devbox/devbox.go b/internal/devbox/devbox.go index 721312b6ed4..fdc25741b08 100644 --- a/internal/devbox/devbox.go +++ b/internal/devbox/devbox.go @@ -726,7 +726,6 @@ func (d *Devbox) computeEnv( if err != nil { return nil, err } - addEnvIfNotPreviouslySetByDevbox(env, configEnv) markEnvsAsSetByDevbox(configEnv) From b56a5ff9484a142e19d3f5052c3071efb72994a8 Mon Sep 17 00:00:00 2001 From: mohsenari Date: Thu, 21 Nov 2024 11:34:37 -0500 Subject: [PATCH 5/7] ci lint fix --- internal/devconfig/config.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/devconfig/config.go b/internal/devconfig/config.go index 592e8211f13..7c742983943 100644 --- a/internal/devconfig/config.go +++ b/internal/devconfig/config.go @@ -424,7 +424,7 @@ func createIncludableFromPluginConfig(pluginConfig *plugin.Config) *Config { // /config/env/bin/:/plugin2/bin/:/plugin1/bin/:$PATH // // because config env should take priority over plugins -func mergePATHsFromTwoEnvs(currentEnv map[string]string, newEnv map[string]string) map[string]string { +func mergePATHsFromTwoEnvs(currentEnv, newEnv map[string]string) map[string]string { if currentEnv["PATH"] != "" { slog.Debug("A Plugin or Config wants to modify PATH. Processing the merge", "AddedPATH", newEnv["PATH"]) newEnv["PATH"] = strings.Replace(newEnv["PATH"], "$PATH", currentEnv["PATH"], 1) From 163ae999a46e6ff4b946437b22a782557e708eeb Mon Sep 17 00:00:00 2001 From: mohsenari Date: Thu, 21 Nov 2024 12:39:03 -0500 Subject: [PATCH 6/7] Fix ruby test error --- internal/devconfig/config.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/devconfig/config.go b/internal/devconfig/config.go index 7c742983943..29b2f04071f 100644 --- a/internal/devconfig/config.go +++ b/internal/devconfig/config.go @@ -425,7 +425,7 @@ func createIncludableFromPluginConfig(pluginConfig *plugin.Config) *Config { // // because config env should take priority over plugins func mergePATHsFromTwoEnvs(currentEnv, newEnv map[string]string) map[string]string { - if currentEnv["PATH"] != "" { + if currentEnv["PATH"] != "" && newEnv["PATH"] != "" { slog.Debug("A Plugin or Config wants to modify PATH. Processing the merge", "AddedPATH", newEnv["PATH"]) newEnv["PATH"] = strings.Replace(newEnv["PATH"], "$PATH", currentEnv["PATH"], 1) } From 8c844190f9be31e484aa74dc53d6a0e69a566b7a Mon Sep 17 00:00:00 2001 From: mohsenari Date: Wed, 27 Nov 2024 16:45:46 -0500 Subject: [PATCH 7/7] PR feedback added OSExpandIfPossible and unit test --- internal/devconfig/config.go | 43 ++++++------- internal/devconfig/config_test.go | 100 ++++++++++++++++++++++++++++++ 2 files changed, 118 insertions(+), 25 deletions(-) diff --git a/internal/devconfig/config.go b/internal/devconfig/config.go index 29b2f04071f..5661cab20cc 100644 --- a/internal/devconfig/config.go +++ b/internal/devconfig/config.go @@ -9,7 +9,6 @@ import ( "net/http" "os" "path/filepath" - "strings" "syscall" "time" @@ -347,18 +346,11 @@ func (c *Config) NixPkgsCommitHash() string { func (c *Config) Env() map[string]string { env := map[string]string{} for _, i := range c.included { - // Here, in new pluginPath we replace the keyword "$PATH" - // with what we have for PATH so far. If so far we have /plugin1/bin:$PATH - // and new plugin adds /plugin2/bin/:$PATH the result becomes - // /plugin2/bin/:/plugin1/bin/:$PATH - pluginEnv := mergePATHsFromTwoEnvs(env, i.Env()) - maps.Copy(env, pluginEnv) - } - // Here we merge the processed PATH from all plugins to PATH in config env. - // So if config env has /config/env/:$PATH merging with the plugin PATH - // it becomes /config/env/:/plugin2/bin/:/plugin1/bin/:$PATH - rootEnv := mergePATHsFromTwoEnvs(env, c.Root.Env) - maps.Copy(env, rootEnv) + expandedEnvFromPlugin := OSExpandIfPossible(i.Env(), env) + maps.Copy(env, expandedEnvFromPlugin) + } + rootConfigEnv := OSExpandIfPossible(c.Root.Env, env) + maps.Copy(env, rootConfigEnv) return env } @@ -417,17 +409,18 @@ func createIncludableFromPluginConfig(pluginConfig *plugin.Config) *Config { return includable } -// Instead of overwriting PATH modifications we need to merge them -// so merging of /plugin1/bin/:$PATH and /plugin2/bin/:$PATH -// and /config/env/bin/:$PATH should result in -// -// /config/env/bin/:/plugin2/bin/:/plugin1/bin/:$PATH -// -// because config env should take priority over plugins -func mergePATHsFromTwoEnvs(currentEnv, newEnv map[string]string) map[string]string { - if currentEnv["PATH"] != "" && newEnv["PATH"] != "" { - slog.Debug("A Plugin or Config wants to modify PATH. Processing the merge", "AddedPATH", newEnv["PATH"]) - newEnv["PATH"] = strings.Replace(newEnv["PATH"], "$PATH", currentEnv["PATH"], 1) +func OSExpandIfPossible(env, existingEnv map[string]string) map[string]string { + mapping := func(value string) string { + // If the value is not set in existingEnv, return the value wrapped in ${...} + if existingEnv == nil || existingEnv[value] == "" { + return fmt.Sprintf("${%s}", value) + } + return existingEnv[value] + } + + res := map[string]string{} + for k, v := range env { + res[k] = os.Expand(v, mapping) } - return newEnv + return res } diff --git a/internal/devconfig/config_test.go b/internal/devconfig/config_test.go index 521aa09f033..3a1aeb3df8c 100644 --- a/internal/devconfig/config_test.go +++ b/internal/devconfig/config_test.go @@ -327,3 +327,103 @@ func TestDefault(t *testing.T) { t.Errorf("got different JSON after load/save/load:\ninput:\n%s\noutput:\n%s", inBytes, outBytes) } } + +func TestOSExpandIfPossible(t *testing.T) { + tests := []struct { + name string + env map[string]string + existingEnv map[string]string + want map[string]string + }{ + { + name: "basic expansion", + env: map[string]string{ + "FOO": "$BAR", + "BAZ": "${QUX}", + }, + existingEnv: map[string]string{ + "BAR": "bar_value", + "QUX": "qux_value", + }, + want: map[string]string{ + "FOO": "bar_value", + "BAZ": "qux_value", + }, + }, + { + name: "missing values remain as template", + env: map[string]string{ + "FOO": "$BAR", + "BAZ": "${QUX}", + }, + existingEnv: map[string]string{ + "BAR": "bar_value", + // QUX is missing + }, + want: map[string]string{ + "FOO": "bar_value", + "BAZ": "${QUX}", + }, + }, + { + name: "nil existing env", + env: map[string]string{ + "FOO": "$BAR", + "BAZ": "${QUX}", + }, + existingEnv: nil, + want: map[string]string{ + "FOO": "${BAR}", + "BAZ": "${QUX}", + }, + }, + { + name: "empty existing env", + env: map[string]string{ + "FOO": "$BAR", + }, + existingEnv: map[string]string{}, + want: map[string]string{ + "FOO": "${BAR}", + }, + }, + { + name: "mixed literal and variable", + env: map[string]string{ + "FOO": "prefix_${BAR}_suffix", + }, + existingEnv: map[string]string{ + "BAR": "bar_value", + }, + want: map[string]string{ + "FOO": "prefix_bar_value_suffix", + }, + }, + { + name: "path special case", + env: map[string]string{ + "FOO": "/my/config:$FOO", + }, + existingEnv: map[string]string{ + "FOO": "/my/plugin:$FOO", + }, + want: map[string]string{ + "FOO": "/my/config:/my/plugin:$FOO", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := OSExpandIfPossible(tt.env, tt.existingEnv) + if len(got) != len(tt.want) { + t.Errorf("OSExpandIfPossible() got %v entries, want %v entries", len(got), len(tt.want)) + } + for k, v := range tt.want { + if got[k] != v { + t.Errorf("OSExpandIfPossible() for key %q = %q, want %q", k, got[k], v) + } + } + }) + } +}