-
Notifications
You must be signed in to change notification settings - Fork 249
wrappers] ensure bin-wrappers invoke other binaries and not bin-wrappers #1160
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,4 +31,4 @@ | |
] | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -55,6 +55,9 @@ func shellEnvCmd() *cobra.Command { | |
// This is used by bin wrappers and not meant for end users. | ||
command.Flag("use-cached-print-dev-env").Hidden = true | ||
flags.config.register(command) | ||
|
||
command.AddCommand(shellEnvOnlyPathWithoutWrappersCmd()) | ||
|
||
return command | ||
} | ||
|
||
|
@@ -74,7 +77,60 @@ func shellEnvFunc(cmd *cobra.Command, flags shellEnvCmdFlags) (string, error) { | |
} | ||
} | ||
|
||
envStr, err := box.PrintEnv(cmd.Context(), flags.runInitHook) | ||
opts := &devopt.PrintEnv{ | ||
Ctx: cmd.Context(), | ||
IncludeHooks: flags.runInitHook, | ||
} | ||
envStr, err := box.PrintEnv(opts) | ||
if err != nil { | ||
return "", err | ||
} | ||
|
||
return envStr, nil | ||
} | ||
|
||
type shellEnvOnlyPathWithoutWrappersCmdFlags struct { | ||
config configFlags | ||
} | ||
|
||
func shellEnvOnlyPathWithoutWrappersCmd() *cobra.Command { | ||
flags := shellEnvOnlyPathWithoutWrappersCmdFlags{} | ||
command := &cobra.Command{ | ||
Use: "only-path-without-wrappers", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit, drop the only and let the command be "path-without-wrappers" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤔 I'd prefer leaving it in to make it clear that this is not also returning other env-vars of shellenv. |
||
Hidden: true, | ||
Short: "Print shell commands that export PATH without the bin-wrappers", | ||
Args: cobra.ExactArgs(0), | ||
PreRunE: ensureNixInstalled, | ||
RunE: func(cmd *cobra.Command, args []string) error { | ||
s, err := shellEnvOnlyPathWithoutWrappersFunc(cmd, &flags) | ||
if err != nil { | ||
return err | ||
} | ||
fmt.Fprintln(cmd.OutOrStdout(), s) | ||
fmt.Fprintln(cmd.OutOrStdout(), "hash -r") | ||
return nil | ||
}, | ||
} | ||
flags.config.register(command) | ||
return command | ||
} | ||
|
||
func shellEnvOnlyPathWithoutWrappersFunc(cmd *cobra.Command, flags *shellEnvOnlyPathWithoutWrappersCmdFlags) (string, error) { | ||
|
||
box, err := devbox.Open(&devopt.Opts{ | ||
Dir: flags.config.path, | ||
Writer: cmd.ErrOrStderr(), | ||
Pure: false, | ||
}) | ||
if err != nil { | ||
return "", err | ||
} | ||
|
||
opts := &devopt.PrintEnv{ | ||
Ctx: cmd.Context(), | ||
OnlyPathWithoutWrappers: true, | ||
} | ||
envStr, err := box.PrintEnv(opts) | ||
if err != nil { | ||
return "", err | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -293,7 +293,7 @@ func (d *Devbox) RunScript(ctx context.Context, cmdName string, cmdArgs []string | |
// creates all wrappers, but does not run init hooks. It is used to power | ||
// devbox install cli command. | ||
func (d *Devbox) Install(ctx context.Context) error { | ||
if _, err := d.PrintEnv(ctx, false /* run init hooks */); err != nil { | ||
if _, err := d.PrintEnv(&devopt.PrintEnv{Ctx: ctx}); err != nil { | ||
return err | ||
} | ||
return wrapnix.CreateWrappers(ctx, d) | ||
|
@@ -309,8 +309,8 @@ func (d *Devbox) ListScripts() []string { | |
return keys | ||
} | ||
|
||
func (d *Devbox) PrintEnv(ctx context.Context, includeHooks bool) (string, error) { | ||
ctx, task := trace.NewTask(ctx, "devboxPrintEnv") | ||
func (d *Devbox) PrintEnv(opts *devopt.PrintEnv) (string, error) { | ||
ctx, task := trace.NewTask(opts.Ctx, "devboxPrintEnv") | ||
defer task.End() | ||
|
||
if err := d.ensurePackagesAreInstalled(ctx, ensure); err != nil { | ||
|
@@ -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)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
if !strings.Contains(p, plugin.WrapperPath) { | ||
path = append(path, p) | ||
} | ||
} | ||
|
||
// reset envs to be PATH only! | ||
envs = map[string]string{ | ||
"PATH": strings.Join(path, string(filepath.ListSeparator)), | ||
} | ||
} | ||
|
||
envStr := exportify(envs) | ||
|
||
if includeHooks { | ||
if opts.IncludeHooks { | ||
hooksStr := ". " + d.scriptPath(hooksFilename) | ||
envStr = fmt.Sprintf("%s\n%s;\n", envStr, hooksStr) | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,19 @@ | ||
package devopt | ||
|
||
import "io" | ||
import ( | ||
"context" | ||
"io" | ||
) | ||
|
||
type Opts struct { | ||
Dir string | ||
Pure bool | ||
IgnoreWarnings bool | ||
Writer io.Writer | ||
} | ||
|
||
type PrintEnv struct { | ||
Ctx context.Context | ||
IncludeHooks bool | ||
OnlyPathWithoutWrappers bool | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit, it's the binary of the profile (which I guess points to the nix store path) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the emphasis here is on binary as opposed to wrapper |
||
# should be in PATH. | ||
eval "$(devbox shellenv only-path-without-wrappers)" | ||
|
||
exec {{ .Command }} "$@" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point, this has been undone in a future PR already!