diff --git a/docs/DOCS.md b/docs/DOCS.md index bc8e7753e..b57770583 100644 --- a/docs/DOCS.md +++ b/docs/DOCS.md @@ -1613,6 +1613,29 @@ The available image names depend on what the spindle operator has installed. `nixos` and `alpine` are examples. If `image` is omitted, the spindle's configured default image is used. +#### PATH + +Steps run in a login shell, and some images (Alpine, for +example) reset `PATH` in `/etc/profile`. So a `PATH` in +`environment` is **prepended** to the image's `PATH` after the +profile has run, it does not replace it. The step log prints +the final `PATH` when you set one. + +These directories are always on the front of `PATH`, so tools +installed without root (the workflow user can't write to +`/usr/local/bin`) are found: + +- `~/.local/bin` (`pip install --user`, `pipx`, `uv`) +- `~/.cargo/bin` +- `~/go/bin` +- `~/.deno/bin` +- `~/.bun/bin` +- `~/.npm-global/bin` +- `~/.local/share/pnpm` and `~/.local/share/pnpm/bin` + +`~` is `/workspace`, so downloading a binary into +`~/.local/bin` in one step makes it available to the next. + #### Dependencies On the microVM engine, `dependencies` is a flat list of diff --git a/spindle/engines/microvm/README.md b/spindle/engines/microvm/README.md index 5a79c5c7f..a0a8dba23 100644 --- a/spindle/engines/microvm/README.md +++ b/spindle/engines/microvm/README.md @@ -46,6 +46,12 @@ spindle expects, they should work. That is: as this user, and the debug shell (see below) launches its passwd shell as a login shell in its home dir. an unset or `nologin`/`false` shell breaks debug ssh, +- the profile hook, [`nix/microvm/spindle-profile.sh`](../../../nix/microvm/spindle-profile.sh), + runs at the end of `/etc/profile` (installed as `/etc/profile.d/zz-spindle.sh`, + or through `loginShellInit` on nixos). steps and the debug shell are login + shells, and this is how they get the nixos dependencies devshell, the user + install dirs and the workflow's `PATH`. images that reset `PATH` in their + profile (alpine) need it, - and the work directory is configured (`/workspace`, with `/workspace/repo` as the per-step working dir). @@ -279,14 +285,13 @@ The shell is deliberately not configurable from either end. It always: secrets, - uses the workflow user's cache directory instead of inheriting shuttle's service cache, -- and sources the activated dependency devshell before starting the interactive - login shell. +- and gets the activated dependency devshell, the user install dirs and the + workflow's `PATH` from the profile hook, because it is a login shell. The executor retains the actual `ExecStart` sent for the failed step instead of reconstructing its environment later. Shuttle uses the same process-spec builder for CI execs and debug PTYs; the debug path replaces only the argv and timeout, -then adds the SSH terminal. The dependency prelude is also generated once and -shared by both paths. +then adds the SSH terminal. The only things the client influences are the terminal type and window size (forwarded from the ssh pty request, and on resize). This relies on the image diff --git a/spindle/engines/microvm/debug.go b/spindle/engines/microvm/debug.go index 15e980216..ac8ff14c2 100644 --- a/spindle/engines/microvm/debug.go +++ b/spindle/engines/microvm/debug.go @@ -153,12 +153,11 @@ func (e *Engine) OpenDebugSession(ctx context.Context, jobID, term string, rows, filtered := &cidFilteredVsockListener{Listener: ln, cid: target.cid, logger: e.l} if err := target.agent.OpenDebugShell(&agentv1.OpenDebugShell{ - VsockPort: port, - Term: term, - Rows: clampDim(rows), - Cols: clampDim(cols), - FailedStep: target.failedStep, - ShellPrelude: depsSourcePrelude(), + VsockPort: port, + Term: term, + Rows: clampDim(rows), + Cols: clampDim(cols), + FailedStep: target.failedStep, }); err != nil { _ = ln.Close() return nil, fmt.Errorf("ask guest to open debug shell: %w", err) diff --git a/spindle/engines/microvm/debug_test.go b/spindle/engines/microvm/debug_test.go index 0fc28f2ed..5684918cd 100644 --- a/spindle/engines/microvm/debug_test.go +++ b/spindle/engines/microvm/debug_test.go @@ -69,6 +69,35 @@ func TestStepEnvironmentPreservesFailedStepContext(t *testing.T) { } } +func TestStepEnvironmentKeepsPATHOutOfTheEnv(t *testing.T) { + workflow := &models.Workflow{Environment: map[string]string{"PATH": "/workflow/bin"}} + step := Step{environment: map[string]string{"PATH": "/step/bin"}} + + got := make(map[string]string) + for _, kv := range stepEnvironment(workflow, step, nil) { + key, value, _ := strings.Cut(kv, "=") + got[key] = value + } + + if got["PATH"] != guestBasePATH { + t.Errorf("PATH = %q, want the image PATH %q", got["PATH"], guestBasePATH) + } + if got[guestPathPrependEnv] != "/step/bin" { + t.Errorf("%s = %q, want the step value", guestPathPrependEnv, got[guestPathPrependEnv]) + } +} + +func TestStepEnvironmentWithoutPATHSetsNoPrepend(t *testing.T) { + workflow := &models.Workflow{Environment: map[string]string{"CI": "true"}} + unlocked := []secrets.UnlockedSecret{{Key: "SECRET", Value: "value"}} + + for _, kv := range stepEnvironment(workflow, Step{}, unlocked) { + if strings.HasPrefix(kv, guestPathPrependEnv+"=") { + t.Fatalf("unexpected %q", kv) + } + } +} + func heldTarget(repo, actor string) debugTarget { return debugTarget{ holdRepo: repo, diff --git a/spindle/engines/microvm/engine.go b/spindle/engines/microvm/engine.go index 0e8df9653..b1af70fcb 100644 --- a/spindle/engines/microvm/engine.go +++ b/spindle/engines/microvm/engine.go @@ -36,7 +36,7 @@ import ( const ( guestWorkDir = "/workspace/repo" guestBasePATH = "/run/current-system/sw/bin:/nix/var/nix/profiles/default/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin" - guestDevShellEnvPath = "/run/spindle/devshell-env.sh" + guestPathPrependEnv = "SPINDLE_PATH_PREPEND" activationStepAction = "activate-config" agentAcceptTimeout = 2 * time.Minute agentHandshakeTimeout = 30 * time.Second @@ -471,31 +471,31 @@ func (e *Engine) SetupWorkflow(ctx context.Context, wid models.WorkflowId, wf *m return nil } -func depsSourcePrelude() string { - return fmt.Sprintf( - // check if it exists because not all images have this - `if [ -f %s ]; then . %s; export PATH="$PATH:%s"; fi; `, - guestDevShellEnvPath, guestDevShellEnvPath, guestBasePATH, - ) -} - -func applyDepsSource(command string) string { - return depsSourcePrelude() + command -} - func stepEnvironment(w *models.Workflow, step models.Step, unlocked []secrets.UnlockedSecret) []string { env := guestBaseEnv() + pathPrepend := "" + set := func(key, value string) { + // /etc/profile resets PATH on some images, so the hook prepends it after + if key == "PATH" { + pathPrepend = value + return + } + env = append(env, key+"="+value) + } for k, v := range w.Environment { - env = append(env, k+"="+v) + set(k, v) } for _, secret := range unlocked { - env = append(env, secret.Key+"="+secret.Value) + set(secret.Key, secret.Value) } if s, ok := step.(Step); ok { for k, v := range s.environment { - env = append(env, k+"="+v) + set(k, v) } } + if pathPrepend != "" { + env = append(env, guestPathPrependEnv+"="+pathPrepend) + } return env } @@ -532,7 +532,7 @@ func (e *Engine) RunStep(ctx context.Context, wid models.WorkflowId, w *models.W stdout := wfLogger.DataWriter(idx, "stdout") execStart := &agentv1.ExecStart{ - Argv: []string{state.ImageSpec.Shell, "-lc", applyDepsSource(step.Command())}, + Argv: []string{state.ImageSpec.Shell, "-lc", step.Command()}, Env: env, Cwd: guestWorkDir, User: guestWorkflowUser,