Skip to content

Conversation

mikeland73
Copy link
Contributor

@mikeland73 mikeland73 commented Oct 18, 2023

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

@mikeland73 mikeland73 requested review from gcurtis and savil October 18, 2023 18:22
@gcurtis
Copy link
Collaborator

gcurtis commented Oct 18, 2023

Does this affect the user's devbox environment or just the wrappers? I think it would be surprising for users that explicitly want the coreutils version of sed. The macOS sed and the coreutils sed have some big differences in how they handle flags.

What if we do the following:

  1. When devbox starts, check to see if DEVBOX_SYSTEM_BASH and DEVBOX_SYSTEM_SED are set.
  2. If they aren't, find them in the PATH (before we modify it at all) and then set the env vars to their absolute paths.
  3. When generating the wrappers, use those env var values. If for some reason the env vars are missing (for example, because the user unset them), assume /bin/bash and /usr/bin/sed.

@mikeland73
Copy link
Contributor Author

@gcurtis sure I think that's a reasonable solution, but to be honest there's absolutely no benefit to generating wrappers for bash and sed. Why not avoid it?

@mikeland73
Copy link
Contributor Author

FWIW, I'm pretty sure coreutils sed/bash works fine for our use case. But agree it's probably not great that the user can change which binaries we use. On the other hand if sed is not fully portable, we are not sure what binaries we're using either.

Copy link
Collaborator

@gcurtis gcurtis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be misunderstanding. I thought that if we didn't wrap bash or sed then the user's devbox environment would end up using their OS's version of those programs. If it's the user's devbox shell is still using the nix-installed bash and sed, then this looks good to me.

@mikeland73 mikeland73 changed the title [wrappers] Never wrap bash or sed [wrappers] Always use system bash and sed in bin wrappers Oct 18, 2023
@mikeland73
Copy link
Contributor Author

@gcurtis no, if we remove the wrapper, then we use binaries in the devbox nix profile.

@mikeland73 mikeland73 merged commit c167ca7 into main Oct 18, 2023
@mikeland73 mikeland73 deleted the landau/never-wrap branch October 18, 2023 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants