[global] global shellenv should not recompute env #1534
Conversation
internal/boxcli/root.go
Outdated
| command.AddCommand(setupCmd()) | ||
| command.AddCommand(shellCmd()) | ||
| command.AddCommand(shellEnvCmd()) | ||
| command.AddCommand(shellEnvCmd(lo.ToPtr(false) /*dontRecomputeEnv*/)) |
There was a problem hiding this comment.
this reads as dontRecomputeEnv param is set to false. I think this is the opposite of what is happening?
lo.ToPtr(false) /*recomputeEnvIfNeeded*/)
OR
// don't recompute the env in shellenv:
command.AddCommand(shellEnvCmd(lo.ToPtr(false)))
|
|
||
| envStr, err := box.NixEnv(cmd.Context(), flags.runInitHook) | ||
| envStr, err := box.NixEnv(cmd.Context(), devopt.NixEnvOpts{ | ||
| DontRecomputeEnvironment: !recomputeEnvIfNeeded, |
There was a problem hiding this comment.
nbd, but I prefer the recomputeEnvIfNeeded as a param name. Can we reuse that within NixEnvOpts as well?
Alternatively, could it be skipRecomputeEnvironment?
There was a problem hiding this comment.
Yeah I struggled with this as well. Here's why I went with this:
The issue with recomputeEnvIfNeeded is that we would need to default it to true. I prefer if the default is true and only pass in a param to make it false.
The reason that flag is the other way around is because devbox global shellenv --no-recompute=false would be super awkward because of the double negative.
| func (d *Devbox) EnvVars(ctx context.Context) ([]string, error) { | ||
| ctx, task := trace.NewTask(ctx, "devboxEnvVars") | ||
| defer task.End() | ||
| // this only returns env variables for the shell environment excluding hooks |
There was a problem hiding this comment.
hmm I think this comment was actually for nixEnv, right? If so, could we leave it in place?
There was a problem hiding this comment.
Its explaining why Mohsen did nixEnv and not NixEnv.
internal/impl/devbox.go
Outdated
| if !usePrintDevEnvCache { | ||
| if err := d.ensurePackagesAreInstalled(ctx, ensure); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
🤔 I think this code will result in running nix print-dev-env two times in this scenario.
I believe we can set:
usePrintDevEnvCache = true
after ensurePackagesAreInstalled, because inside ensurePackagesAreInstalled we'll call computeNixEnv(ctx, true).
Or, even better directly hardcode below:
return d.computeNixEnv(ctx, true /*usePrintDevEnvCache*/)
|
|
||
| if err := d.ensurePackagesAreInstalled(ctx, ensure); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Should we move Starting a devbox shell... to after nixEnv so the printed output is as before:
first, nixEnv output about recomputing devbox environment, etc.
then, "Starting a devbox shell....`
then, any init-hook output or shellrc output
internal/impl/devbox.go
Outdated
| ux.Finfo( | ||
| d.stderr, | ||
| "Your devbox environment may be out of date. Please run \n\n"+ | ||
| "eval \"$(devbox global shellenv -r)\"\n\n", |
There was a problem hiding this comment.
oh two comments on this:
-
could we say:
devbox global shellenv --recompute? I assume users will copy-paste it so not a problem to be a bit more verbose, and does explain better what it'll do. -
For fish shells, we need to print a different output:
devbox global shellenv -r | source
Summary
This causes the default behavior for
devbox global shellenvto no longer recompute environment. It adds a new flag:devbox global shellenv -rwhich causes the recompute if needed (i.e. not cached)Recomputing the environment entails two steps:
It also removed a bunch of
ensurePackagesAreInstalledcalls that are now handled bynixEnv(). Note that in all cases, we were callingensurePackagesAreInstalled(ensure)followed bynixEnv()How was it tested?
eval "$(devbox global shellenv -r)"