Add --recompute flag for commands: shell and run #2391

savil · 2024-10-28T22:55:19Z

Summary

This PR continues the work from #2013 to also add the --recompute flag option for devbox run and devbox shell.
For some users on bad networks, this can save them annoyance and time for when they know their devbox environment is up-to-date.

Fixes #2315

How was it tested?

This PR affects 3 commands: run, shell and shellenv.

For run:
Added "hello": "latest", to devbox.json of this project.

devbox run --recompute=false -- echo "hello world"
Warning: Your devbox environment may be out of date. Run with --recompute=true to update it.
hello world

then

devbox run -- echo "hello world"
Info: Ensuring packages are installed.
✓ Computed the Devbox environment.
hello world

For shell. Ran similar commands as above.
For shellenv. Followed test plan of feat: Add recompute flag to shellenv command #1963.

Changed the .envrc to be:

.envrc
@@ -1,7 +1,13 @@
 # Automatically sets up your devbox environment whenever you cd into this
 # directory via our direnv integration:

-eval "$(devbox generate direnv --print-envrc)"
+#eval "$(devbox generate direnv --print-envrc)"

+ # output of `devbox generate direnv --print-envrc` to modify it
+use_devbox() {
+    watch_file devbox.json devbox.lock
+    # eval "$(devbox shellenv --init-hook --install --no-refresh-alias)"
+    eval "$(devbox shellenv --init-hook --no-refresh-alias --recompute=false)"
+}
+use devbox
 # check out https://www.jetify.com/devbox/docs/ide_configuration/direnv/
 # for more details

Then modified devbox.json and saw the warning get printed.

mikeland73 · 2024-10-29T19:20:07Z

internal/devbox/devbox.go

+	if envOpts.RecomputeEnv.Disabled {
+		upToDate, _ := d.lockfile.IsUpToDateAndInstalled(isFishShell())
+		if !upToDate {
+			ux.FHidableWarning(


This won't work. Hidable warnings need the format string to match. You are using a non format string as a format string (which is why govet is complaining). That means this string won't match the HideMessage call earlier on.

One way to fix:

Have StateOutOfDateMessage be only the part that differs. So something like:

ux.FHidableWarning( ctx, d.stderr, StateOutOfDateMessage, envOpts.RecomputeEnv.StateOutOfDateMessage, )

Not a blocker, but I this feels a bit fragile and connects the cobra command to the devbox code in a funky way. Maybe one way to improve this in the future would be to use lifecycle hooks:

envOpts.Lifecycle.OnEnvNotUpToDate = func() { ux.Warning }

Yeah, this feels very icky. I like the lifecycles hooks approach. 👍🏾

mikeland73 · 2024-10-29T19:23:34Z

internal/boxcli/run.go

@@ -122,8 +125,12 @@ func runScriptCmd(cmd *cobra.Command, args []string, flags runCmdFlags) error {
 	envOpts := devopt.EnvOptions{
 		OmitNixEnv: flags.omitNixEnv,
 		Pure:       flags.pure,
+		RecomputeEnv: &devopt.RecomputeEnvOpts{
+			Disabled:              !flags.recomputeEnv,
+			StateOutOfDateMessage: fmt.Sprintf(devbox.StateOutOfDateMessage, "with --recompute=true"),


A few questions/comments:

Is this message needed at all? It feels like if someone is voluntarily using --recompute=false, they know what they want.

This is copy pasted a bunch of times, maybe create a var for this?

If this message is not needed, maybe inside devbox package it can simply skip the warning if StateOutOfDateMessage is empty. That way you only have to set this field in one place (in shellenv)

Answers to the questions:

Yeah, it will be emitted as a warning only when the environment is out of date. So, this serves as a head's up to the user that they're executing with stale state. The user may be using the --recompute=false as an optimization but still want to know when the state may be stale.

its copied in two locations... I think its okay. This should correspond to the flag used above, and it feels overkill to abstract this out.

N/A since i think it is needed

mikeland73 · 2024-10-29T19:24:52Z

internal/devbox/devbox.go

-	if err := d.ensureStateIsUpToDate(ctx, ensure); isConnectionError(err) {
-		if !fileutil.Exists(d.nixPrintDevEnvCachePath()) {
-			ux.Ferrorf(
+	if envOpts.RecomputeEnv.Disabled {


Changes in ensureStateIsUpToDateAndComputeEnv look OK, but please double triple check. This is very core functioning stuff.

mikeland73 · 2024-10-29T19:26:15Z

internal/devbox/devopt/devboxopts.go

@@ -76,4 +75,10 @@ type EnvOptions struct {
 	OmitNixEnv        bool
 	PreservePathStack bool
 	Pure              bool
+	RecomputeEnv      *RecomputeEnvOpts


Why pointer? Default to structs unless you a) need mutating methods b) struct is massive and you want to avoid copying data.

In general, I go with pointers because its easier for the zero value when writing instantiations of the struct. This is especially relevant for test-cases. The downside is that the caller has to be more defensive for the nil-case.

This file uses all non-pointers, so in the interest of consistency we should keep that.

I don't follow why zero value is easier with non-pointer. Structs default to their zero values, no need to initialize anything.

The downside is quite large. A nil pointer use is a panic.

internal/boxcli/shell.go

internal/devbox/devopt/devboxopts.go

mikeland73 · 2024-11-04T19:06:38Z

internal/devbox/devbox.go

+		upToDate, _ := d.lockfile.IsUpToDateAndInstalled(isFishShell())
+		if !upToDate {
+			if envOpts.Hooks.OnStaleStateWithSkipRecompute != nil {
+				envOpts.Hooks.OnStaleStateWithSkipRecompute()
+			}
+		}


I was playing around with putting this outside the envOpts.SkipRecompute if statement.

It allows the hook to be more generic (i.e. OnStaleState). The downside is it can be a bit wasteful (calling IsUpToDateAndInstalled if we don't need to). IsUpToDateAndInstalled is fast (~1ms) but can be slower for large projects with remote plugins etc. So maybe we're all good as is. Just food for thought.

Discussed in slack that this should be okay (for perf) to make generic into OnStaleState

## Summary Missed updating this comment in last round of revisions for #2391 ## How was it tested?

sentry-io · 2024-11-05T14:34:47Z

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

‼️ **exec.ExitError: error running script in Devbox: <redacted errors.withStack>: <redacted errors... go.jetpack.io/devbox/internal/nix in (*Nix).Pri... View Issue
‼️ **Generic Error: <redacted errors.withStack>: <redacted usererr.combined> go.jetpack.io/devbox/internal/boxcli/usererr in... View Issue

_{Did you find this useful? React with a 👍 or 👎}

savil force-pushed the savil/recompute-env branch from 9542b07 to 2ecb5ed Compare October 29, 2024 00:21

savil changed the title ~~recompute env in shell and run~~ Add --recompute flag for commands: shell and run Oct 29, 2024

savil requested a review from mikeland73 October 29, 2024 00:27

savil force-pushed the savil/recompute-env branch from 2ecb5ed to 58b52f5 Compare October 29, 2024 00:34

savil marked this pull request as ready for review October 29, 2024 16:05

mikeland73 requested changes Oct 29, 2024

View reviewed changes

savil force-pushed the savil/recompute-env branch 4 times, most recently from 8d2d8e0 to 63fcbec Compare November 1, 2024 00:28

savil requested a review from mikeland73 November 1, 2024 00:29

savil force-pushed the savil/recompute-env branch from 63fcbec to cd09583 Compare November 1, 2024 00:30

mikeland73 approved these changes Nov 4, 2024

View reviewed changes

savil and others added 3 commits November 4, 2024 13:56

recompute env in shell and run

8833565

use lifecycle hook

b9616c9

addressed feedback requests

fdc03f5

savil force-pushed the savil/recompute-env branch from cd09583 to fdc03f5 Compare November 5, 2024 00:25

savil merged commit f7b01fc into main Nov 5, 2024
29 checks passed

savil deleted the savil/recompute-env branch November 5, 2024 00:48

savil mentioned this pull request Nov 5, 2024

[easy] comment fix OnStaleState #2405

Merged

savil added a commit that referenced this pull request Nov 5, 2024

[easy] comment fix OnStaleState (#2405)

c0331c7

## Summary Missed updating this comment in last round of revisions for #2391 ## How was it tested?

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --recompute flag for commands: shell and run #2391

Add --recompute flag for commands: shell and run #2391

savil commented Oct 28, 2024 •

edited

Loading

mikeland73 Oct 29, 2024

savil Nov 1, 2024

mikeland73 Oct 29, 2024

savil Oct 31, 2024

mikeland73 Oct 29, 2024

mikeland73 Oct 29, 2024

savil Nov 1, 2024

mikeland73 Nov 4, 2024

mikeland73 Nov 4, 2024

savil Nov 5, 2024

sentry-io bot commented Nov 5, 2024 •

edited

Loading

Add --recompute flag for commands: shell and run #2391

Add --recompute flag for commands: shell and run #2391

Conversation

savil commented Oct 28, 2024 • edited Loading

Summary

How was it tested?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sentry-io bot commented Nov 5, 2024 • edited Loading

Suspect Issues

savil commented Oct 28, 2024 •

edited

Loading

sentry-io bot commented Nov 5, 2024 •

edited

Loading