[sed] Fix bin wrappers when sed is installed with devbox #1566
Conversation
|
Interesting, so this issue specifically happens if the user installs |
yep, it creates infinite recursion. coreutils also installs |
| @@ -48,6 +48,7 @@ func CreateWrappers(ctx context.Context, args CreateWrappersArgs) error { | |||
| _ = os.MkdirAll(destPath, 0o755) | |||
|
|
|||
| bashPath := cmdutil.GetPathOrDefault("bash", "/bin/bash") | |||
| sedPath := cmdutil.GetPathOrDefault("sed", "sed") | |||
There was a problem hiding this comment.
Won't the default value trigger the bug again? Should the default be /usr/bin/sed to mirror /bin/bash above?
There was a problem hiding this comment.
Yeah if sed is not in path this will trigger same issue. Though if sed is not in path the wrapper would have failed anyway. sed is POSIX so I kinda assume it exists. I can fix it, but
There's an additional problem that affects sed and bash. If you are using global and a project devbox and both install sed and/or bash, you could end up cycling between both wrappers recursivelly. For example:
- Add coreutils to project (bin wrappers use
/usr/bin/sed) - Add coreutils to global while shelled in to project (bin wrappers use
sedin project) - Make some change to project, now sed in bin wrapper points to global sed
- Cycle
Another edge case:
- if 2 devbox processes are creating wrappers at the same time, theres a chance the second one will point to the bin wrapper incorrectly.
These are pretty edge cases, but would suck. I think a better solution is to just special case sed and bash and never bin wrap them. Thoughts?
There was a problem hiding this comment.
Ah yeah, nested environments would throw everything off. I commented in the other PR with some ideas.
## Summary This feels like the safest solution to #1566 (comment) It also avoids creating bin wrappers for `bash` and `sed` which reduces risk and improves performance. ## How was it tested? Used setup from #1566 ``` devbox shell devbox add bash hash -r which bash # ensure I get profile bash instead of wrapped bash ```
Summary
TSIA
How was it tested?
Using this config: https://discord.com/channels/903306922852245526/1009933892385517619/1163763578399105064
Pre-change, could not shell in, got bash nesting error.
Post-change, could shell in.