Skip to content

[script-exit-on-error] disable for init-hooks #1493

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.

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 2 commits into from
Sep 20, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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: 0 additions & 11 deletions internal/impl/devbox.go
Original file line number Diff line number Diff line change
Expand Up @@ -210,17 +210,6 @@ 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: 1 addition & 7 deletions internal/impl/shell.go
Original file line number Diff line number Diff line change
Expand Up @@ -269,12 +269,6 @@ 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 @@ -325,7 +319,7 @@ func (s *DevboxShell) writeDevboxShellrc() (path string, err error) {
}()

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

Expand Down
57 changes: 35 additions & 22 deletions internal/shellgen/scripts.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ type devboxer interface {
Lockfile() *lock.File
AllInstallablePackages() ([]*devpkg.Package, error)
InstallablePackages() []*devpkg.Package
IsUserShellFish() (bool, error)
PluginManager() *plugin.Manager
ProjectDir() string
}
Expand Down Expand Up @@ -53,7 +52,7 @@ func WriteScriptsToFiles(devbox devboxer) error {
}
hooks := strings.Join(append(pluginHooks, devbox.Config().InitHook().String()), "\n\n")
// always write it, even if there are no hooks, because scripts will source it.
err = WriteScriptFile(devbox, HooksFilename, hooks)
err = writeHookFile(devbox, hooks)
if err != nil {
return errors.WithStack(err)
}
Expand Down Expand Up @@ -82,39 +81,53 @@ func WriteScriptsToFiles(devbox devboxer) error {
return nil
}

func WriteScriptFile(devbox devboxer, name string, body string) (err error) {
script, err := os.Create(ScriptPath(devbox.ProjectDir(), name))
func writeHookFile(devbox devboxer, body string) (err error) {
script, err := createScriptFile(devbox, HooksFilename)
if err != nil {
return errors.WithStack(err)
}
defer func() {
cerr := script.Close()
if err == nil {
err = cerr
}
}()
err = script.Chmod(0755)
defer func() { _ = script.Close() }() // best effort: close file

_, err = script.WriteString(body)
return errors.WithStack(err)
}

func WriteScriptFile(devbox devboxer, name string, body string) (err error) {
script, err := createScriptFile(devbox, name)
if err != nil {
return errors.WithStack(err)
}
defer func() { _ = script.Close() }() // best effort: close file

if featureflag.ScriptExitOnError.Enabled() {
// Fish cannot run scripts with `set -e`.
// NOTE: Devbox scripts will run using `sh` for consistency. However,
// init_hooks in a fish shell will run using `fish` shell, and need this
// check.
isFish, err := devbox.IsUserShellFish()
if err != nil {
return errors.WithStack(err)
}
if !isFish {
body = fmt.Sprintf("set -e\n\n%s", body)
}
// NOTE: Devbox scripts will run using `sh` for consistency.
// However, we need to disable this for `fish` shell if/when we allow this for init_hooks,
// since init_hooks run in the host shell, and not `sh`.
Comment on lines +103 to +105
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we'll never be able to add this to init hooks unless we decide not to source.

body = fmt.Sprintf("set -e\n\n%s", body)
}
_, err = script.WriteString(body)
return errors.WithStack(err)
}

func createScriptFile(devbox devboxer, name string) (script *os.File, err error) {
script, err = os.Create(ScriptPath(devbox.ProjectDir(), name))
if err != nil {
return nil, errors.WithStack(err)
}
defer func() {
// best effort: close file if there was some subsequent error
if err != nil {
_ = script.Close()
}
}()

err = script.Chmod(0755)
if err != nil {
return nil, errors.WithStack(err)
}
return script, nil
}

func ScriptPath(projectDir, scriptName string) string {
return filepath.Join(projectDir, scriptsDir, scriptName+".sh")
}
Expand Down