jetify-com / devbox

Instant, easy, and predictable development environments
https://www.jetify.com/devbox/
Apache License 2.0
7.83k stars 187 forks source link

[devbox] run: skip re-computing Devbox State if in devbox shell #2144

Closed savil closed 2 weeks ago

savil commented 2 weeks ago

Summary

This PR implements a change to the semantics of devbox run. BEFORE: devbox run would always ensure that the Devbox State and Environment is up-to-date. AFTER: devbox run will only do so when outside a Devbox Environment (i.e. devbox shell or equivalent like direnv-enabled shell).

The motivation is two fold:

  1. Speed. Users often want to just run a quick script, this slows them down.
  2. Users often have init-hooks which are more appropriate for one-time setup, rather than when running a utility script inside a devbox shell that has already been set up appropriately.

This was shared in the Discord channel for feedback: https://discord.com/channels/903306922852245526/1009933892385517619/1248724103750483978

Notably, the focus there was on skipping the init-hooks, but in this PR, we are going one-step beyond that to ensure the Devbox State itself is not re-computed. The reason is that rather than partially updating the Devbox State and Environment (partially, because init-hooks are part of setting up the environment) we'd rather eschew the step entirely. This feels more straightforward for users to reason about.

How was it tested?

Did a basic sanity test in two scenarios:

  1. Running the script in a devbox shell or direnv-enabled environment does NOT recompute the Devbox state.
  2. Repeated the sanity test when outside of the Devbox environment, and confirmed it does recompute the Devbox state.
savil commented 2 weeks ago

init hooks are baked into the scripts that are written to (.devbox/gen/scripts).

oh yeah, good point!

savil commented 2 weeks ago

@mikeland73 PTAL. Made the update as discussed IRL.

Notably, I chose to keep the init-hook-wrapper.tmpl unchanged. Reason: we need it to run for devbox shell scenario that calls . /path/to/projectDir/.devbox/gen/scripts/.hooks.sh (recall, init-hook-wrapper.tmpl generates .hooks.sh).

gcurtis commented 2 weeks ago

For some reason I still see the init hooks running twice. Maybe there's something weird with my setup, but this is what I see:

$ devbox shell
Info: Ensuring packages are installed.
✓ Computed the Devbox environment.
Starting a devbox shell...
this is my init hook
(devbox) $ devbox run test
this is my init hook
this is my script

Config:

{
  "$schema": "https://raw.githubusercontent.com/jetify-com/devbox/0.0.0-dev/.schema/devbox.schema.json",
  "packages": [],
  "shell": {
    "init_hook": "echo this is my init hook",
    "scripts": {
      "test": "echo this is my script"
    }
  }
}

Build Info:

$ command -v devbox
/Users/gcurtis/bin/devbox
$ go version -m ~/bin/devbox | rg -o vcs.+
vcs=git
vcs.revision=6f165e38b605247f62f8e68d147d6f8a9b7e7d42
vcs.time=2024-06-13T23:02:04Z
vcs.modified=false
gcurtis commented 2 weeks ago

If we don't recompute the state, does that mean changes to devbox.json won't take effect from inside the shell? For example, what would be printed if I did:

$ echo '{"shell":{"scripts":{"test":"echo aaa"}}}'
$ devbox shell
(devbox) $ devbox run test
aaa
(devbox) $ echo '{"shell":{"scripts":{"test":"echo bbb"}}}'
(devbox) $ devbox run test
??? # would I get "aaa" or "bbb"?
savil commented 2 weeks ago

For some reason I still see the init hooks running twice. Maybe there's something weird with my setup, but this is what I see:

Can you double check which devbox binary you are running?

Here's the output I see with your devbox.json:

% devbox shell
Info: Ensuring packages are installed.
✓ Computed the Devbox environment.
Starting a devbox shell...
this is my init hook
(devbox) savil@Savil-Srivastavas-MacBook-Pro no-recompute-if-run-in-shell % devbox run test
this is my script
(devbox) savil@Savil-Srivastavas-MacBook-Pro no-recompute-if-run-in-shell %
savil commented 2 weeks ago

If we don't recompute the state, does that mean changes to devbox.json won't take effect from inside the shell? For example, what would be printed if I did:

$ echo '{"shell":{"scripts":{"test":"echo aaa"}}}'
$ devbox shell
(devbox) $ devbox run test
aaa
(devbox) $ echo '{"shell":{"scripts":{"test":"echo bbb"}}}'
(devbox) $ devbox run test
??? # would I get "aaa" or "bbb"?

in this case, the output will be bbb because we still re-generate the script files via shellgen.WriteScriptsToFiles(d) on line 241. This lets one iterate on scripts.

However, changes to the shell environment will not show up until the user executes the refresh alias (when inside the devbox shell)

savil commented 2 weeks ago

Notably, I chose to keep the init-hook-wrapper.tmpl unchanged. Reason: we need it to run for devbox shell scenario that calls . /path/to/projectDir/.devbox/gen/scripts/.hooks.sh (recall, init-hook-wrapper.tmpl generates .hooks.sh).

Can you elaborate on this?

Why does init-hook-wrapper.tmpl need to set export {{ .InitHookHash }}=true ?

My understanding would be:

If an init hook has a devbox run in it, by the time it runs d.IsEnvEnabled() returns true (since init hooks run after env is already set). This means that any devbox run inside init hooks would not run the hook again (thanks to the code in this PR).

A benefit is that removing the wrapper would simplify code. Basically:

No need to generate the wrapper, so hooks can be sourced directly.

We could also rename the env var to something more accurate, e.g. SkipInitHooks

🤔 I think I mis-diagnosed this...or mis-tested something yesterday. You are correct that I can comment out the env-var lines in init-hook-wrapper and it works as expected. Updating...

savil commented 2 weeks ago

@mikeland73 PTAL