Skip to content

[script-on-error] disable for fish shell, and add testscript unit-test #1488

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Sep 18, 2023
Merged

[script-on-error] disable for fish shell, and add testscript unit-test #1488

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c896da9
[script-on-error] disable for fish shell, and add testscript unit-test
savil Sep 18, 2023
b2d3eea
lint fix
savil Sep 18, 2023
b96cdb6
add clarifying comment
savil Sep 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions internal/impl/devbox.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,17 @@ func (d *Devbox) Shell(ctx context.Context) error {
return shell.Run()
}

// IsUserShellFish returns true if the user's shell is fish.
// This wrapper function over DevboxShell enables querying from other packages that
// make a devboxer interface.
func (d *Devbox) IsUserShellFish() (bool, error) {
sh, err := NewDevboxShell(d)
if err != nil {
return false, err
}
return sh.IsFish(), nil
}

func (d *Devbox) RunScript(ctx context.Context, cmdName string, cmdArgs []string) error {
ctx, task := trace.NewTask(ctx, "devboxRun")
defer task.End()
Expand Down
8 changes: 7 additions & 1 deletion internal/impl/shell.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,12 @@ func (s *DevboxShell) Run() error {
return errors.WithStack(err)
}

// IsFish returns whether this DevboxShell wraps a fish shell. Fish shells are non-posix compatible,
// and so sometimes we may need to switch logic based on this function's result.
func (s *DevboxShell) IsFish() bool {
return s.name == shFish
}

func (s *DevboxShell) shellRCOverrides(shellrc string) (extraEnv map[string]string, extraArgs []string) {
// Shells have different ways of overriding the shellrc, so we need to
// look at the name to know which env vars or args to set when launching the shell.
Expand Down Expand Up @@ -319,7 +325,7 @@ func (s *DevboxShell) writeDevboxShellrc() (path string, err error) {
}()

tmpl := shellrcTmpl
if s.name == shFish {
if s.IsFish() {
tmpl = fishrcTmpl
}

Expand Down
13 changes: 10 additions & 3 deletions internal/shellgen/scripts.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ type devboxer interface {
Lockfile() *lock.File
AllInstallablePackages() ([]*devpkg.Package, error)
InstallablePackages() []*devpkg.Package
IsUserShellFish() (bool, error)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really introduced in this PR, but I think we should cut down on this interface pattern for pure data like this. (I admin to be much to blame for this)

There are some cases where the circular dependency is hard to avoid (for example if you need to call a function with side-effects that would create a cycle). If what you are passing in is pure data, cheap to compute (like in this case), then using a struct argument keeps code simpler and more linear.

Copy link
Collaborator Author

@savil savil Sep 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I agree 💯

In this particular package, there are more public APIs than I think are good. For example, WriteScriptToFile should likely not be a public method. Having a tighter public interface, would enable a constructor which can take some of these arguments (as you suggest), minimizing the need for the interface.

PluginManager() *plugin.Manager
ProjectDir() string
}
Expand Down Expand Up @@ -80,8 +81,8 @@ func WriteScriptsToFiles(devbox devboxer) error {
return nil
}

func WriteScriptFile(d devboxer, name string, body string) (err error) {
script, err := os.Create(ScriptPath(d.ProjectDir(), name))
func WriteScriptFile(devbox devboxer, name string, body string) (err error) {
script, err := os.Create(ScriptPath(devbox.ProjectDir(), name))
if err != nil {
return errors.WithStack(err)
}
Expand All @@ -97,7 +98,13 @@ func WriteScriptFile(d devboxer, name string, body string) (err error) {
}

if featureflag.ScriptExitOnError.Enabled() {
body = fmt.Sprintf("set -e\n\n%s", body)
isFish, err := devbox.IsUserShellFish()
if err != nil {
return errors.WithStack(err)
}
if !isFish {
body = fmt.Sprintf("set -e\n\n%s", body)
}
}
_, err = script.WriteString(body)
return errors.WithStack(err)
Expand Down
20 changes: 20 additions & 0 deletions testscripts/run/script_exit_on_error.test.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
# Testscript to ensure that the script exits on error.

! exec devbox run multi_line
stdout 'first line'
! stdout 'second line'

-- devbox.json --
{
"packages": [
],
"shell": {
"scripts": {
"multi_line": [
"echo \"first line\"",
"exit 1",
"echo \"second line\""
]
}
}
}