Skip to content

Commit 51b98a7

Browse files
savilsavil
authored
Revert "[bin wrappers] add --omit-wrappers-from-path to shellenv to prevent a binary from invoking a wrapped-binary. (#1151)" (#1159)
## Summary Reverting due to regression: Within a devbox script if any environment variable has been exported, and if a devbox-plugin also sets that env-var, then the devbox-plugin env-var will be set for the wrapped-binary. Specifically, in the `lapp-stack/devbox.json`, we have: ``` "run_test": [ "mkdir -p /tmp/devbox/lapp", "export PGHOST=/tmp/devbox/lapp", "initdb", ``` but the PGHOST that `initdb` sees is a different value i.e. the one set by the devbox plugin. In the code being reverted, the wrapped-bin's bash script would always invoke `eval $(devbox shellenv --omit-wrappers-from-path=true)` in order to omit wrappers from PATH. A side-effect is that the PGHOST would also get overridden. ## How was it tested? no testing done. straight revert, tests should pass.
1 parent b70728a commit 51b98a7

File tree

7 files changed

+40
-76
lines changed

7 files changed

+40
-76
lines changed

‎devbox.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ type Devbox interface {
3333
Install(ctx context.Context) error
3434
IsEnvEnabled() bool
3535
ListScripts() []string
36-
PrintEnv(opts *devopt.PrintEnv) (string, error)
36+
PrintEnv(ctx context.Context, includeHooks bool) (string, error)
3737
PrintGlobalList() error
3838
Pull(ctx context.Context, overwrite bool, path string) error
3939
Push(url string) error

‎internal/boxcli/shell.go‎

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -59,12 +59,7 @@ func runShellCmd(cmd *cobra.Command, flags shellCmdFlags) error {
5959
if flags.printEnv {
6060
// false for includeHooks is because init hooks is not compatible with .envrc files generated
6161
// by versions older than 0.4.6
62-
opts := &devopt.PrintEnv{
63-
Ctx: cmd.Context(),
64-
IncludeHooks: false,
65-
OmitWrappersFromPath: false,
66-
}
67-
script, err := box.PrintEnv(opts)
62+
script, err := box.PrintEnv(cmd.Context(), false /*includeHooks*/)
6863
if err != nil {
6964
return err
7065
}

‎internal/boxcli/shellenv.go‎

Lines changed: 3 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -13,11 +13,10 @@ import (
1313

1414
type shellEnvCmdFlags struct {
1515
config configFlags
16-
install bool
17-
omitWrappersFromPath bool
18-
pure bool
1916
runInitHook bool
17+
install bool
2018
useCachedPrintDevEnv bool
19+
pure bool
2120
}
2221

2322
func shellEnvCmd() *cobra.Command {
@@ -46,12 +45,6 @@ func shellEnvCmd() *cobra.Command {
4645
command.Flags().BoolVar(
4746
&flags.pure, "pure", false, "If this flag is specified, devbox creates an isolated environment inheriting almost no variables from the current environment. A few variables, in particular HOME, USER and DISPLAY, are retained.")
4847

49-
// This flag is to be used by our generated bin-wrappers shell script.
50-
command.Flags().BoolVar(
51-
&flags.omitWrappersFromPath, "omit-wrappers-from-path", false, "If this flag is specified, "+
52-
"the PATH from shellenv will not include the binary wrappers")
53-
command.Flag("omit-wrappers-from-path").Hidden = true
54-
5548
// This is no longer used. Remove after 0.4.8 is released.
5649
command.Flags().BoolVar(
5750
&flags.useCachedPrintDevEnv,
@@ -81,12 +74,7 @@ func shellEnvFunc(cmd *cobra.Command, flags shellEnvCmdFlags) (string, error) {
8174
}
8275
}
8376

84-
opts := &devopt.PrintEnv{
85-
Ctx: cmd.Context(),
86-
IncludeHooks: flags.runInitHook,
87-
OmitWrappersFromPath: flags.omitWrappersFromPath,
88-
}
89-
envStr, err := box.PrintEnv(opts)
77+
envStr, err := box.PrintEnv(cmd.Context(), flags.runInitHook)
9078
if err != nil {
9179
return "", err
9280
}

‎internal/impl/devbox.go‎

Lines changed: 4 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -293,12 +293,7 @@ func (d *Devbox) RunScript(ctx context.Context, cmdName string, cmdArgs []string
293293
// creates all wrappers, but does not run init hooks. It is used to power
294294
// devbox install cli command.
295295
func (d *Devbox) Install(ctx context.Context) error {
296-
opts := &devopt.PrintEnv{
297-
Ctx: ctx,
298-
IncludeHooks: false,
299-
OmitWrappersFromPath: false,
300-
}
301-
if _, err := d.PrintEnv(opts); err != nil {
296+
if _, err := d.PrintEnv(ctx, false /* run init hooks */); err != nil {
302297
return err
303298
}
304299
return wrapnix.CreateWrappers(ctx, d)
@@ -314,8 +309,8 @@ func (d *Devbox) ListScripts() []string {
314309
return keys
315310
}
316311

317-
func (d *Devbox) PrintEnv(opts *devopt.PrintEnv) (string, error) {
318-
ctx, task := trace.NewTask(opts.Ctx, "devboxPrintEnv")
312+
func (d *Devbox) PrintEnv(ctx context.Context, includeHooks bool) (string, error) {
313+
ctx, task := trace.NewTask(ctx, "devboxPrintEnv")
319314
defer task.End()
320315

321316
if err := d.ensurePackagesAreInstalled(ctx, ensure); err != nil {
@@ -327,19 +322,9 @@ func (d *Devbox) PrintEnv(opts *devopt.PrintEnv) (string, error) {
327322
return "", err
328323
}
329324

330-
if opts.OmitWrappersFromPath {
331-
path := []string{}
332-
for _, p := range strings.Split(envs["PATH"], string(filepath.ListSeparator)) {
333-
if !strings.Contains(p, plugin.WrapperPath) {
334-
path = append(path, p)
335-
}
336-
}
337-
envs["PATH"] = strings.Join(path, string(filepath.ListSeparator))
338-
}
339-
340325
envStr := exportify(envs)
341326

342-
if opts.IncludeHooks {
327+
if includeHooks {
343328
hooksStr := ". " + d.scriptPath(hooksFilename)
344329
envStr = fmt.Sprintf("%s\n%s;\n", envStr, hooksStr)
345330
}

‎internal/impl/devopt/devboxopts.go‎

Lines changed: 1 addition & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,10 @@
11
package devopt
22

3-
import (
4-
"context"
5-
"io"
6-
)
3+
import "io"
74

85
type Opts struct {
96
Dir string
107
Pure bool
118
IgnoreWarnings bool
129
Writer io.Writer
1310
}
14-
15-
type PrintEnv struct {
16-
Ctx context.Context
17-
IncludeHooks bool
18-
OmitWrappersFromPath bool
19-
}

‎internal/wrapnix/wrapper.go‎

Lines changed: 26 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,11 @@ var wrapperTemplate = template.Must(template.New("wrapper").Parse(wrapper))
3434

3535
// CreateWrappers creates wrappers for all the executables in nix paths
3636
func CreateWrappers(ctx context.Context, devbox devboxer) error {
37+
shellEnvHash, err := devbox.ShellEnvHash(ctx)
38+
if err != nil {
39+
return err
40+
}
41+
3742
services, err := devbox.Services()
3843
if err != nil {
3944
return err
@@ -49,20 +54,22 @@ func CreateWrappers(ctx context.Context, devbox devboxer) error {
4954
bashPath := cmdutil.GetPathOrDefault("bash", "/bin/bash")
5055
for _, service := range services {
5156
if err = createWrapper(&createWrapperArgs{
52-
devboxer: devbox,
53-
BashPath: bashPath,
54-
Command: service.Start,
55-
Env: service.Env,
56-
destPath: filepath.Join(destPath, service.StartName()),
57+
devboxer: devbox,
58+
BashPath: bashPath,
59+
Command: service.Start,
60+
Env: service.Env,
61+
ShellEnvHash: shellEnvHash,
62+
destPath: filepath.Join(destPath, service.StartName()),
5763
}); err != nil {
5864
return err
5965
}
6066
if err = createWrapper(&createWrapperArgs{
61-
devboxer: devbox,
62-
BashPath: bashPath,
63-
Command: service.Stop,
64-
Env: service.Env,
65-
destPath: filepath.Join(destPath, service.StopName()),
67+
devboxer: devbox,
68+
BashPath: bashPath,
69+
Command: service.Stop,
70+
Env: service.Env,
71+
ShellEnvHash: shellEnvHash,
72+
destPath: filepath.Join(destPath, service.StopName()),
6673
}); err != nil {
6774
return err
6875
}
@@ -75,10 +82,11 @@ func CreateWrappers(ctx context.Context, devbox devboxer) error {
7582

7683
for _, bin := range bins {
7784
if err = createWrapper(&createWrapperArgs{
78-
devboxer: devbox,
79-
BashPath: bashPath,
80-
Command: bin,
81-
destPath: filepath.Join(destPath, filepath.Base(bin)),
85+
devboxer: devbox,
86+
BashPath: bashPath,
87+
Command: bin,
88+
ShellEnvHash: shellEnvHash,
89+
destPath: filepath.Join(destPath, filepath.Base(bin)),
8290
}); err != nil {
8391
return errors.WithStack(err)
8492
}
@@ -92,9 +100,10 @@ func CreateWrappers(ctx context.Context, devbox devboxer) error {
92100

93101
type createWrapperArgs struct {
94102
devboxer
95-
BashPath string
96-
Command string
97-
Env map[string]string
103+
BashPath string
104+
Command string
105+
Env map[string]string
106+
ShellEnvHash string
98107

99108
destPath string
100109
}

‎internal/wrapnix/wrapper.sh.tmpl‎

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -14,23 +14,19 @@ fi
1414
{{- end }}
1515

1616
{{/*
17+
We use ShellEnvHashKey to prevent doing shellenv if the correct environment is
18+
already set. (perf optimization)
1719

1820
We use the guard to prevent infinite loop if something in shellenv causes
1921
another wrapped binary to be called. The guard is specific to this project so shellenv
2022
could still cause another project's shellenv to be called.
21-
Note, this guard can likely be removed since we use --omit-wrappers-from-path=true below,
22-
but leaving in to minimize change.
2323

2424
DO_NOT_TRACK=1 can be removed once we optimize segment to queue events.
25-
26-
--omit-wrappers-from-path so that we do not invoke other bin-wrappers from
27-
this bin-wrapper. Instead, we directly invoke the binary from the nix store, which
28-
should be in PATH.
2925
*/ -}}
3026

31-
if [[ -z "${{ .ShellEnvHashKey }}_GUARD" ]]; then
27+
if [[ "${{ .ShellEnvHashKey }}" != "{{ .ShellEnvHash }}" ]] && [[ -z "${{ .ShellEnvHashKey }}_GUARD" ]]; then
3228
export {{ .ShellEnvHashKey }}_GUARD=true
33-
eval "$(DO_NOT_TRACK=1 devbox shellenv --omit-wrappers-from-path=true -c {{ .ProjectDir }})"
29+
eval "$(DO_NOT_TRACK=1 devbox shellenv -c {{ .ProjectDir }})"
3430
fi
3531

3632
exec {{ .Command }} "$@"

0 commit comments

Comments
 (0)