First pass at devbox scripts implementation #254

ipince · 2022-11-03T22:28:39Z

Summary

Devbox scripts allows you to define scripts to run inside a devbox shell. You can add them to your devbox.json like so:

{
  "packages": [],
  "shell": {
    "init_hook": "echo hook",
    "scripts": {
      "a": "echo hello",
      "b": [
        "echo good",
        "echo bye"
      ]
    }
  }
}

And then you can run with devbox run <script>. For example:

> devbox run b
Installing nix packages. This may take a while... done.
Starting a devbox shell...
hook
good
bye

Note that the shell's init hook will always run.

How was it tested?

With a devbox.json as written above and running devbox run <script> with no script, invalid script, valid script (single command and multi-command).

savil

Nice! Approving to unblock but interested in discussion on the comments below

savil · 2022-11-04T22:43:04Z

boxcli/run.go

+		Use:               "run <script>",
+		Hidden:            true,
+		Short:             "Starts a new devbox shell and runs the target script",
+		Long:              "Starts a new interactive shell and runs your target script in it. The shell will exit once your target script is completed or when it is terminated via CTRL-C. Scripts can be defined in your `devbox.json`",


if we're running a script command, does the shell need to be interactive?

I guess the point is that you can start a long-running process, like a server, and it's stays in the foreground so you can Ctrl+C out of it when you want.

savil · 2022-11-04T22:54:11Z

boxcli/run.go

+	script := ""
+	if len(args) == 1 {
+		script = args[0]
+	} else if len(args) > 1 {
+		return "", "", errors.Errorf("too many arguments (expected 1, got %d)", len(args))
+	}


instead, would it make sense to set Args: cobra.MaximumNArgs(1), above when defining the cobra command?

Yeah I guess.. the error messages from the cobra validators are pretty bad though, but I'll see if this is reasonable.

savil · 2022-11-04T22:58:20Z

boxcli/run.go

+	}
+
+	if devbox.IsDevboxShellEnabled() {
+		return errors.New("You are already in an active devbox shell.\nRun 'exit' before calling devbox run again. Shell inception is not supported yet.")


I like how you added yet compared to the same error in shell.go :)

savil · 2022-11-04T23:01:25Z

config.go

@@ -37,7 +37,8 @@ type Config struct {
 	// Shell configures the devbox shell environment.
 	Shell struct {
 		// InitHook contains commands that will run at shell startup.
-		InitHook ConfigShellCmds `json:"init_hook,omitempty"`
+		InitHook ConfigShellCmds             `json:"init_hook,omitempty"`
+		Scripts  map[string]*ConfigShellCmds `json:"scripts,omitempty"`


in launchpad.yaml, we had a similar undeterministic-ordering issue with the Services. We handled it by:

defining it as Scripts []ConfigShellCmds and adding the "name" as a field in ConfigShellCmds

adding custom marshal and unmarshal functions to have it print as a map with the key

Would you wanna try that? Could do as a followup. I feel it would be really nice to ensure determinism. With determinism, we won't get random diffs of devbox.json when we write to it for (other unrelated) reasons.

oh interesting, i'll see about it!

I'll do as a follow-up

savil · 2022-11-04T23:02:50Z

config.go

+func (c *Config) validate() error {
+	for k := range c.Shell.Scripts {
+		if k == "" {
+			return errors.New("cannot have script with empty name in devbox.json")
+		}
+	}
+	return nil


oh boy, I ended up adding another validate function today, so will need to merge it. It should be fairly straightforward.

@loreto had requested me to see if we can use the cue validation instead, which I am yet to do.

Yeah, I actually attempted to use cue for validation, but I couldn't find what I needed quickly enough so I gave up and added the validation function. I can merge it with yours

savil · 2022-11-04T23:07:23Z

devbox.go

+	if err := d.ensurePackagesAreInstalled(install); err != nil {
+		return err
+	}
+
+	plan := d.ShellPlan()
+
+	profileDir, err := d.profileDir()
+	if err != nil {
+		return err
+	}
+
+	nixShellFilePath := filepath.Join(d.srcDir, ".devbox/gen/shell.nix")
+	script := d.cfg.Shell.Scripts[scriptName]
+	if script == nil {
+		return errors.Errorf("unable to find a script with name %s", scriptName)
+	}
+
+	shell, err := nix.DetectShell(
+		nix.WithPlanInitHook(strings.Join(plan.ShellInitHook, "\n")),
+		nix.WithProfile(profileDir),
+		nix.WithHistoryFile(filepath.Join(d.srcDir, shellHistoryFile)),
+		nix.WithUserScript(scriptName, script.String()))
+
+	if err != nil {
+		fmt.Print(err)
+		shell = &nix.Shell{}
+	}
+
+	shell.UserInitHook = d.cfg.Shell.InitHook.String()
+	return shell.Run(nixShellFilePath)


oh boyo boy. I thought this would reuse the implementation of box.Exec....
but I agree with your implementation more (as in, we should strive to emulate the devbox shell environment which box.Exec doesn't seem to).

So, there are three different implementations now:

this RunScript

box.Exec

the box.Shell (except this doesn't run a specific command, but starts an interactive shell)

I feel:

box.Exec and RunScript should use the exact same implementation

box.Shell should also be refactored to use the same core implementation as box.Exec and RunScript (which is your TODO above)

Hmmm ok I'm not too familiar with box.Exec, I can look into it. I'm new to this code, so this type of feedback is really useful.

ipince requested a review from savil November 3, 2022 22:28

ipince force-pushed the rodrigo/scripts branch from 6b28661 to 6b642c0 Compare November 3, 2022 22:35

ipince marked this pull request as ready for review November 3, 2022 22:36

savil approved these changes Nov 4, 2022

View reviewed changes

Lagoja and others added 6 commits November 10, 2022 11:39

Implement devbox scripts

6fead57

undo vscode stuff

1565140

undo pointer

1e735a4

fix tests

43128b2

consistent start/ends

8d8d6b3

fix typo

4a5bea0

ipince force-pushed the rodrigo/scripts branch from 0498ec1 to 4a5bea0 Compare November 10, 2022 16:39

ipince added 4 commits November 10, 2022 12:40

fix assignment

04073ae

use cobra validation for >1 arg

3916b9d

better error; sort scripts

d92fdbe

merge validation with existing validation mechanism

ebb472d

ipince merged commit 0d8fb9d into main Nov 10, 2022

ipince deleted the rodrigo/scripts branch November 10, 2022 18:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

First pass at devbox scripts implementation #254

First pass at devbox scripts implementation #254

Uh oh!

ipince commented Nov 3, 2022

Uh oh!

savil left a comment •

edited

Loading

Uh oh!

savil Nov 4, 2022

Uh oh!

ipince Nov 7, 2022

Uh oh!

savil Nov 4, 2022

Uh oh!

ipince Nov 7, 2022

Uh oh!

savil Nov 4, 2022

Uh oh!

ipince Nov 7, 2022

Uh oh!

savil Nov 4, 2022

Uh oh!

ipince Nov 10, 2022

Uh oh!

ipince Nov 10, 2022

Uh oh!

savil Nov 4, 2022

Uh oh!

ipince Nov 7, 2022

Uh oh!

ipince Nov 10, 2022

Uh oh!

savil Nov 4, 2022

Uh oh!

ipince Nov 7, 2022

Uh oh!

Uh oh!

First pass at devbox scripts implementation #254

First pass at devbox scripts implementation #254

Uh oh!

Conversation

ipince commented Nov 3, 2022

Summary

How was it tested?

Uh oh!

savil left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

savil left a comment •

edited

Loading