[bin wrappers] add --omit-wrappers-from-path to shellenv to prevent a binary from invoking a wrapped-binary. #1151
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Current dependencies on/for this PR: This comment was auto-generated by Graphite. |
|
gah, I have a lot of local changes that didn't get added to the commit here. Updating! |
savil
force-pushed
the
savil/wrapper-path-fix-v1
branch
from
96d423b to
70589b0
Compare
… binary from invoking a wrapped-binary.
savil
force-pushed
the
savil/wrapper-path-fix-v1
branch
from
70589b0 to
b3b8636
Compare
savil
commented
Jun 14, 2023
mikeland73
approved these changes
Jun 14, 2023
| */ -}} | ||
|
|
||
| if [[ "${{ .ShellEnvHashKey }}" != "{{ .ShellEnvHash }}" ]] && [[ -z "${{ .ShellEnvHashKey }}_GUARD" ]]; then | ||
| if [[ -z "${{ .ShellEnvHashKey }}_GUARD" ]]; then |
There was a problem hiding this comment.
I think the guard can be removed (the guard is in place to avoid infinite wrapper loop but since we're removing it from path it can't happen.
This is hard to test so we may be better off leaving as is for now. Maybe add a comment
There was a problem hiding this comment.
yeah, I'll leave a comment. Would rather be super conservative with these changes.
internal/boxcli/shellenv.go
Outdated
| } | ||
|
|
||
| envStr, err := box.PrintEnv(cmd.Context(), flags.runInitHook) | ||
| envStr, err := box.PrintEnv(cmd.Context(), flags.runInitHook, flags.omitWrappersFromPath) |
There was a problem hiding this comment.
When adding a second boolean is a good time to switch to a struct :P
There was a problem hiding this comment.
ah, I knew you'd say this :p
My personal threshold was three args, but sure 👍🏾
…d in wrapper template
1 task
savil
added a commit
that referenced
this pull request
Jun 15, 2023
…revent 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.
savil
added a commit
that referenced
this pull request
Jun 15, 2023
…ers (#1160) ## 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Motivation
In elixir projects,
iex -S mix <script>has theiexinterpreter invoking themixbuild tool. However, themixbuild tool has been wrapped by devbox in a bash script.And this leads to a failure as
iexis unable to interpret a bash script.Fix
Binaries should be able to directly invoke other installed binaries.
We implemented the binary-wrappers so that the shell-environment is correctly
configured prior to the installed binary being invoked. In this case, the
iexbinary wrapper will run thereby correctly configuring the environment. Any subsequent
binary invoked in the same sub-shell will inherit this up-to-date environment. So,
the other binaries can be directly invoked (instead of the binary-wrapper scripts).
Implementation
In this PR, we change the
PATHthat is set within a binary-wrapper.Specifically, the
PATHhas the form:Within the binary-wrapper, we now omit the
.wrappers/bindirectory from PATHby using a new hidden flag
--omit-wrappers-from-pathondevbox shellenv.perf analysis
I expect the perf impact to be minimal.
Speed up: Binaries invoking other binaries will be a bit faster since they no longer first execute the binary-wrapper.
Slowdown: we remove one layer of caching. Previously, we'd check the shellenv-hash in the binary wrapper, and skip invoking
devbox shellenvif the hash was up-to-date. Now, on every invocation of a binary-wrapper we'll calldevbox shellenv. This should still be fast due to the internal caching we do withindevbox shellenv.Fixes #1142
How was it tested?
BEFORE: doing
devbox run -- iex -S mixwould result in the error in the linked issue.AFTER:
Also - did test plan of #1093 to verify
devbox updatecontinues working. A sanity check since we dropped ShellEnvHash from the wrappers and I wanted to ensure this works as expected.