wrappers] ensure bin-wrappers invoke other binaries and not bin-wrappers #1160

savil · 2023-06-15T00:25:11Z

Summary

This is an alternate to #1151 (see explanation in #1159)

In this approach, the wrapped-binary always calls:
eval $(devbox shellenv only-path-without-wrappers)
to ensure that the PATH is set without the wrappers/bin directory.

In contrast to #1151, we do not always set any other environment variables

Fixes #1142

How was it tested?

issue from #1142 is fixed:

> devbox run -- iex -S mix
* creating .nix-mix/archives/hex-2.0.6
* creating .nix-mix/elixir/1-14/rebar3
Resolving Hex dependencies...
Resolution completed in 0.016s
Unchanged:
  cowboy 2.9.0
  cowlib 2.11.0
  ranch 1.8.0
All dependencies are up to date
Erlang/OTP 25 [erts-13.2.1] [source] [64-bit] [smp:8:8] [ds:8:8:10] [async-threads:1]

Hello World!
Interactive Elixir (1.14.4) - press Ctrl+C to exit (type h() ENTER for help)
iex(1)> Goodbye World

17:18:09.349 [notice] Application elixir_hello exited: shutdown

BREAK: (a)bort (A)bort with dump (c)ontinue (p)roc info (i)nfo
       (l)oaded (v)ersion (k)ill (D)b-tables (d)istribution

testscripts should pass

savil · 2023-06-15T00:25:21Z

Current dependencies on/for this PR:

main
- PR Revert "[bin wrappers] add --omit-wrappers-from-path to shellenv to prevent a binary from invoking a wrapped-binary. (#1151)" #1159
  - PR wrappers] ensure bin-wrappers invoke other binaries and not bin-wrappers #1160 👈

This comment was auto-generated by Graphite.

LucilleH

LGTM

In this case shellenv will be invoked on every binary call, which will impact performance.

As a follow up, perhaps we can generate this PATH env variable and store it in a file. In every wrapped binary, we simply source that file.

The file only needs to be re-generated if __DEVBOX_SHELLENV_HASH_ does not match.

I don't know if you want to re-think this PR that way 🤷‍♀️

Edit: btw the test is failing because you need to export PGHOST

mohsenari

Nice. Thank you. Just one nit comment

mohsenari · 2023-06-15T00:47:45Z

internal/boxcli/shellenv.go

+func shellEnvOnlyPathWithoutWrappersCmd() *cobra.Command {
+	flags := shellEnvOnlyPathWithoutWrappersCmdFlags{}
+	command := &cobra.Command{
+		Use:     "only-path-without-wrappers",


Nit, drop the only and let the command be "path-without-wrappers"

🤔 I'd prefer leaving it in to make it clear that this is not also returning other env-vars of shellenv.

…pers

savil · 2023-06-15T03:55:57Z

In this case shellenv will be invoked on every binary call, which will impact performance.

That was the case with the fix in #1151 as well.

I'm not very concerned because a cached devbox shellenv is somewhat fast (30-50 milliseconds if I do time eval $(devbox shellenv)).

For now, would prefer focussing on correctness, and once we've settled on the final design - optimize it from there.

## Summary follow up to #1160 correctness: - consolidate some code into calling `wrapnix.GetWrapperBinPath` - the earlier code had a bug where I forgot to call `-c <project dir>`, but that is moot now (see below change) perf: - call DO_NOT_TRACK=1 to minimize segment perf impact - simplify OnlyPathWithoutWrappers to directly call `os.Getenv("PATH")`, remove WrapperBins, and exportify it. This function is used in a very specific context, so we can _know_ we can skip the nix.ComputeEnv calculation. - Doing `time devbox shellenv only-path-without-wrappers` is now consistently ~ 18-25 milliseconds. I have a slight preference for this over writing PATH to a file and sourcing it, since we may run into correctness issues with that file getting outdated, and it'll introduce perf regression on every shellenv call to write the file. ## How was it tested? - [x] testscripts pass - [x] `iex -S mix` works visual inspection: ``` 1 #!/nix/store/w849dr5qcbrrnxv69sy7kdnifa9jpjyf-bash-5.2-p15/bin/bash 2 3 4 5 if [[ "$__DEVBOX_SHELLENV_HASH_ac6a60d54a9d1ba6618cce8d40bfdfb50059d814edfe5f49bca3168c5c34e970" != "b1a4903de70aa4d610ecb00ffb3dd755913b4505651e6bebbf41571a406d415c" ]] & & [[ -z "$__DEVBOX_SHELLENV_HASH_ac6a60d54a9d1ba6618cce8d40bfdfb50059d814edfe5f49bca3168c5c34e970_GUARD" ]]; then 6 export __DEVBOX_SHELLENV_HASH_ac6a60d54a9d1ba6618cce8d40bfdfb50059d814edfe5f49bca3168c5c34e970_GUARD=true 7 eval "$(DO_NOT_TRACK=1 devbox shellenv -c /Users/savil/code/jetpack/devbox/examples/development/elixir/elixir_hello)" 8 fi 9 10 eval "$(DO_NOT_TRACK=1 devbox shellenv only-path-without-wrappers -c /Users/savil/code/jetpack/devbox/examples/development/elixir/elixir_hello)" 11 12 exec /nix/store/47whvyx9x48ishndcjw71xs7l0a6v6sd-elixir-1.14.4/bin/iex "$@" ```

mikeland73 · 2023-06-15T23:36:17Z

internal/boxcli/shell.go

@@ -59,7 +59,7 @@ func runShellCmd(cmd *cobra.Command, flags shellCmdFlags) error {
 	if flags.printEnv {
 		// false for includeHooks is because init hooks is not compatible with .envrc files generated
 		// by versions older than 0.4.6
-		script, err := box.PrintEnv(cmd.Context(), false /*includeHooks*/)
+		script, err := box.PrintEnv(&devopt.PrintEnv{Ctx: cmd.Context()})


super golang nit, generally context and struct are passed as 2 args. The reason is that storing context in a struct is risky because we may accidentally keep it in state somewhere.

good point, this has been undone in a future PR already!

mikeland73 · 2023-06-15T23:42:04Z

internal/impl/devbox.go

@@ -322,9 +322,23 @@ func (d *Devbox) PrintEnv(ctx context.Context, includeHooks bool) (string, error
 		return "", err
 	}

+	if opts.OnlyPathWithoutWrappers {
+		path := []string{}
+		for _, p := range strings.Split(envs["PATH"], string(filepath.ListSeparator)) {


filepath.SplitList ?

mikeland73 · 2023-06-15T23:58:35Z

internal/wrapnix/wrapper.sh.tmpl

@@ -29,4 +29,9 @@ export {{ .ShellEnvHashKey }}_GUARD=true
 eval "$(DO_NOT_TRACK=1 devbox shellenv -c {{ .ProjectDir }})"
 fi

+# So that we do not invoke other bin-wrappers from
+# this bin-wrapper. Instead, we directly invoke the binary from the nix store, which


nit, it's the binary of the profile (which I guess points to the nix store path)

the emphasis here is on binary as opposed to wrapper

@Lagoja

## Summary This replaces our existing `only-path-without-wrappers` wrapper implementation to use `sed`. Context: We use `only-path-without-wrappers` to remove bin wrappers in the environment of our binaries. This is particularly useful when a binary calls another binary. See #1160 for more context. Unfortunately this adds a perf hit to every single binary call. In my informal experiments this cost was ~40ms. For example: Previously, `time go version` was taking ~60ms even if environment is fully up to date. Now, `time go version` takes ~20ms. @Lagoja I'm getting an error in the elixer example (since that was the motivation for the original fix) but it looks like nix elixer on Sonoma is broken: https://elixirforum.com/t/bus-error-after-upgrading-to-sonoma-beta/56354/33 (Our CICD runners will test on Ubuntu and older macOS, Monterrey I believe) ## How was it tested? * Tested time to call `go version` before and after * Inspected PATH of shell to ensure `.wrappers` was still there. * Copy pasted `sed` code from one of the bin wrappers

savil mentioned this pull request Jun 15, 2023

Revert "[bin wrappers] add --omit-wrappers-from-path to shellenv to prevent a binary from invoking a wrapped-binary. (#1151)" #1159

Merged

savil force-pushed the savil/omit-wrappers-alt branch from 160387f to 9242e8a Compare June 15, 2023 00:27

savil requested review from mikeland73, LucilleH and mohsenari June 15, 2023 00:29

savil force-pushed the savil/omit-wrappers-alt branch from 9242e8a to 8583ba3 Compare June 15, 2023 00:30

LucilleH approved these changes Jun 15, 2023

View reviewed changes

mohsenari approved these changes Jun 15, 2023

View reviewed changes

Base automatically changed from savil/revert-1151 to main June 15, 2023 03:46

savil added 2 commits June 14, 2023 20:49

wrappers] ensure bin-wrappers invoke other binaries and not bin-wrap…

6a1a42a

…pers

revert lapp-stack change

f075729

savil force-pushed the savil/omit-wrappers-alt branch from 8583ba3 to f075729 Compare June 15, 2023 03:50

savil merged commit f19dbfc into main Jun 15, 2023

savil deleted the savil/omit-wrappers-alt branch June 15, 2023 04:10

savil mentioned this pull request Jun 15, 2023

[bin wrappers] fixes for only-path-without-wrappers call #1163

Merged

2 tasks

mikeland73 reviewed Jun 15, 2023

View reviewed changes

mikeland73 mentioned this pull request Oct 6, 2023

[perf] Replace only-path-without-wrappers with sed #1535

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

wrappers] ensure bin-wrappers invoke other binaries and not bin-wrappers #1160

wrappers] ensure bin-wrappers invoke other binaries and not bin-wrappers #1160

Uh oh!

savil commented Jun 15, 2023 •

edited

Loading

Uh oh!

savil commented Jun 15, 2023

Uh oh!

LucilleH left a comment •

edited

Loading

Uh oh!

mohsenari left a comment

Uh oh!

mohsenari Jun 15, 2023

Uh oh!

savil Jun 15, 2023

Uh oh!

savil commented Jun 15, 2023 •

edited

Loading

Uh oh!

mikeland73 Jun 15, 2023

Uh oh!

savil Jun 16, 2023

Uh oh!

mikeland73 Jun 15, 2023

Uh oh!

mikeland73 Jun 15, 2023

Uh oh!

savil Jun 16, 2023

Uh oh!

Uh oh!

wrappers] ensure bin-wrappers invoke other binaries and not bin-wrappers #1160

wrappers] ensure bin-wrappers invoke other binaries and not bin-wrappers #1160

Uh oh!

Conversation

savil commented Jun 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

How was it tested?

Uh oh!

savil commented Jun 15, 2023

Uh oh!

LucilleH left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mohsenari left a comment

Choose a reason for hiding this comment

Uh oh!

mohsenari Jun 15, 2023

Choose a reason for hiding this comment

Uh oh!

savil Jun 15, 2023

Choose a reason for hiding this comment

Uh oh!

savil commented Jun 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikeland73 Jun 15, 2023

Choose a reason for hiding this comment

Uh oh!

savil Jun 16, 2023

Choose a reason for hiding this comment

Uh oh!

mikeland73 Jun 15, 2023

Choose a reason for hiding this comment

Uh oh!

mikeland73 Jun 15, 2023

Choose a reason for hiding this comment

Uh oh!

savil Jun 16, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

savil commented Jun 15, 2023 •

edited

Loading

LucilleH left a comment •

edited

Loading

savil commented Jun 15, 2023 •

edited

Loading