Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Oct 6, 2023
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5ccb59a
[perf] Replace only-path-without-wrappers with sed
mikeland73 Oct 6, 2023
0ec1902
typo
mikeland73 Oct 6, 2023
87eb31f
Remove deprecated field
mikeland73 Oct 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions internal/boxcli/shellenv.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,11 +95,12 @@ func shellEnvFunc(cmd *cobra.Command, flags shellEnvCmdFlags) (string, error) {

func shellEnvOnlyPathWithoutWrappersCmd() *cobra.Command {
command := &cobra.Command{
Use: "only-path-without-wrappers",
Hidden: true,
Short: "[internal] Print shell command that exports the system $PATH without the bin-wrappers paths.",
Args: cobra.ExactArgs(0),
PreRunE: ensureNixInstalled,
Use: "only-path-without-wrappers",
Deprecated: "This command is deprecated and will be removed after devbox 0.7.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

lets confirm this prints to Stderr and not Stdout, otherwise existing bin-wrappers may not work.

That said, I suppose we will re-generate the bin-wrappers due to the Devbox version change when this is released, so may be not a concern either way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. It's actually OutOrStdErr which is currently Stderr, but we can't guarantee that. Will remove.

Hidden: true,
Short: "[internal] Print shell command that exports the system $PATH without the bin-wrappers paths.",
Args: cobra.ExactArgs(0),
PreRunE: ensureNixInstalled,
RunE: func(cmd *cobra.Command, args []string) error {
s := shellEnvOnlyPathWithoutWrappersFunc()
fmt.Fprintln(cmd.OutOrStdout(), s)
Expand Down
2 changes: 2 additions & 0 deletions internal/wrapnix/wrapper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ func CreateWrappers(ctx context.Context, args CreateWrappersArgs) error {

for _, bin := range args.NixBins {
if err := createWrapper(&createWrapperArgs{
WrapperBinPath: destPath,
CreateWrappersArgs: args,
BashPath: bashPath,
Command: bin,
Expand Down Expand Up @@ -130,6 +131,7 @@ type createWrapperArgs struct {
Command string
destPath string
DevboxSymlinkDir string
WrapperBinPath string // This is the directory where all bin wrappers live
}

func createWrapper(args *createWrapperArgs) error {
Expand Down
7 changes: 4 additions & 3 deletions internal/wrapnix/wrapper.sh.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,13 @@ eval "$(DO_NOT_TRACK=1 devbox shellenv --preserve-path-stack -c {{ .ProjectDir }
fi

{{/*
We call only-path-without-wrappers so that we do not invoke other bin-wrappers from
Remove wrapper bin path from PATH so that we don't call more bin-wrappers from
this bin-wrapper. Instead, we directly invoke the binary from the nix store, which
should be in PATH.

DO_NOT_TRACK=1 can be removed once we optimize segment to queue events.
This is implemented in sed for efficiency. sed is POSIX so we assume it's available.
Copy link
Collaborator

Choose a reason for hiding this comment

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

oh interesting. Availability was one of the concerns earlier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@savil if it's a concern we can implement in pure bash. Here's an implementation that only uses bash builtins:

IFS=: read -ra dirs <<< "$PATH"  # Split PATH into an array using colon as a delimiter

new_PATH=""
for dir in "${dirs[@]}"; do
    if [[ "$dir" != "/path/to/remove" ]]; then
        if [[ -z "$new_PATH" ]]; then
            new_PATH="$dir"
        else
            new_PATH="$new_PATH:$dir"
        fi
    fi
done

export PATH="$new_PATH"

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh not too bad. That's fairly understandable except for:

IFS=: read -ra dirs <<< "$PATH"

:D

Copy link
Collaborator

Choose a reason for hiding this comment

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

but now that makes sense too...


*/ -}}
eval "$(DO_NOT_TRACK=1 devbox shellenv only-path-without-wrappers)"
export PATH=$(echo $PATH | sed -e 's#:{{ .WrapperBinPath }}##' -e 's#{{ .WrapperBinPath }}:##' -e 's#{{ .WrapperBinPath }}##')

exec {{ .Command }} "$@"