Skip to content

Commit

Permalink
Improve handling when no SHELL env-var is defined (#700)
Browse files Browse the repository at this point in the history
## Summary

Previously, if `SHELL` env-var was not set, we'd return an error from
`nix.DetectShell`
and initialize an empty struct `nix.Shell{}`. Then, in `Shell.Run` we'd
see that `Shell.binPath` is empty
and call `nix-shell`.

This has the following problems:
1. Initializing an empty `nix.Shell{}` means that we drop all the other
`ShellOptions`
in the constructor.
2. In `Shell.Run`, we'd use `nix-shell` but fail if the flakes feature
was not enabled. This `nix-shell` also loses all the print-dev-env work
that we have done for Unified-Env.

To solve and/or mitigate these issues, I propose the following changes:
- Rename `nix.Shell` to `nix.DevboxShell` to make it clear that its not
a raw nix shell. TODO for future to move this out of `nix` package.
- Rename `nix.DetectShell` to `nix.NewDevboxShell` to make clear that it
is a constructor function. Decompose its functionality into helper
functions `shellPath` and `initShellBinaryFields`.
- If `SHELL` is undefined, ~we loop over a list of common shells to see
if they are accessible in the `PATH`.~ we use the bash from nix.
- Failing to find ~any recognizable shell in `PATH`,~ bash from nix, we
fail the program with an error.
- Remove the fallback to `nix-shell` in `Shell.Run`. This means we
_always_ use the Unified-Env codepath.
- Update `shell_test` for Flakes feature, since it now finds a shell
from PATH and uses that (previously, would error).


## How was it tested?

in `examples/testdata/go/go-1.19`:
```
❯ DEVBOX_DEBUG=0 SHELL= DEVBOX_FEATURE_FLAKES=0 devbox shell
Ensuring packages are installed.
Starting a devbox shell...
(devbox) Savil-Srivastavas-MacBook-Pro:go-1.19 savil$ echo $SHELL
/nix/store/1bsjl5incfnszv7scdh4d02sh45vw2w1-bash-5.1-p16/bin/bash
(devbox) Savil-Srivastavas-MacBook-Pro:go-1.19 savil$ exit
exit

❯ DEVBOX_DEBUG=0 SHELL= DEVBOX_FEATURE_FLAKES=1 devbox shell
Ensuring packages are installed.

Installing package: go_1_19.

[1/1] go_1_19
[1/1] go_1_19: Success
Starting a devbox shell...
(devbox) Savil-Srivastavas-MacBook-Pro:go-1.19 savil$ echo $SHELL
/nix/store/1bsjl5incfnszv7scdh4d02sh45vw2w1-bash-5.1-p16/bin/bash
(devbox) Savil-Srivastavas-MacBook-Pro:go-1.19 savil$
```

Also: `go test ./...` with flakes feature enabled and disabled.
  • Loading branch information
savil committed Mar 2, 2023
1 parent e12d3b6 commit a1d3832
Show file tree
Hide file tree
Showing 4 changed files with 132 additions and 86 deletions.
13 changes: 2 additions & 11 deletions internal/boxcli/shell_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,6 @@ import (
"time"

"github.com/stretchr/testify/assert"
"go.jetpack.io/devbox/internal/boxcli/featureflag"
"go.jetpack.io/devbox/internal/nix"
"go.jetpack.io/devbox/internal/testframework"
)

Expand Down Expand Up @@ -314,13 +312,6 @@ func TestShell(t *testing.T) {
err := td.SetDevboxJSON(devboxJSON)
assert.NoError(t, err)
output, err := td.RunCommand(ShellCmd())
if featureflag.Flakes.Enabled() {
assert.Error(t, err)
if !errors.Is(err, nix.ErrNoDefaultShellUnsupportedInFlakesMode) {
assert.Fail(t, "Expected error %s but received %s", nix.ErrNoDefaultShellUnsupportedInFlakesMode, err)
}
} else {
assert.NoError(t, err)
assert.Contains(t, output, "Starting a devbox shell...")
}
assert.NoError(t, err)
assert.Contains(t, output, "Starting a devbox shell...")
}
14 changes: 7 additions & 7 deletions internal/impl/devbox.go
Original file line number Diff line number Diff line change
Expand Up @@ -271,10 +271,9 @@ func (d *Devbox) Shell() error {
nix.WithShellStartTime(shellStartTime),
}

shell, err := nix.DetectShell(opts...)
shell, err := nix.NewDevboxShell(d.cfg.Nixpkgs.Commit, opts...)
if err != nil {
// Fall back to using a plain Nix shell.
shell = &nix.Shell{}
return err
}

shell.UserInitHook = d.cfg.Shell.InitHook.String()
Expand Down Expand Up @@ -358,11 +357,11 @@ func (d *Devbox) RunScriptInNewNixShell(scriptName string) error {
nix.WithPKGConfigDir(d.pluginVirtenvPath()),
}

shell, err := nix.DetectShell(opts...)
shell, err := nix.NewDevboxShell(d.cfg.Nixpkgs.Commit, opts...)

if err != nil {
fmt.Fprint(d.writer, err)
shell = &nix.Shell{}
return err
}

shell.UserInitHook = d.cfg.Shell.InitHook.String()
Expand All @@ -381,7 +380,8 @@ func (d *Devbox) RunScriptInShell(scriptName string) error {
return usererr.New("unable to find a script with name %s", scriptName)
}

shell, err := nix.DetectShell(
shell, err := nix.NewDevboxShell(
d.cfg.Nixpkgs.Commit,
nix.WithProfile(profileDir),
nix.WithHistoryFile(filepath.Join(d.projectDir, shellHistoryFile)),
nix.WithUserScript(scriptName, script.String()),
Expand All @@ -390,7 +390,7 @@ func (d *Devbox) RunScriptInShell(scriptName string) error {

if err != nil {
fmt.Fprint(d.writer, err)
shell = &nix.Shell{}
return err
}

return shell.RunInShell()
Expand Down

0 comments on commit a1d3832

Please sign in to comment.