[script-exit-on-error] disable for init-hooks (jetify-com#1493)

## Summary @Lagoja identified an issue with scripts-exit-on-error. We source init-hooks into the host shell. So, `set -e` will get set in the host shell. Any subsequent error will cause the shell to exit (error may be from the init-hook, or later in the shell). For now, this PR disables this feature entirely for init hooks. We'll revisit this later: jetify-com#1494 Also, this PR undoes the previous change to restrict this feature to fish-shells, since that only applied to init-hooks. Regular Devbox scripts always run in `sh`. ## How was it tested? testscript unit-tests did a sanity check that regular init_hooks work.
KarolosLykos · Sep 20, 2023 · 2b8c495 · 2b8c495
1 parent fbdbf01
commit 2b8c495
Show file tree

Hide file tree

Showing 3 changed files with 36 additions and 40 deletions.
diff --git a/internal/impl/devbox.go b/internal/impl/devbox.go
@@ -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()

diff --git a/internal/impl/shell.go b/internal/impl/shell.go
@@ -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.
@@ -325,7 +319,7 @@ func (s *DevboxShell) writeDevboxShellrc() (path string, err error) {
 	}()
 
 	tmpl := shellrcTmpl
-	if s.IsFish() {
+	if s.name == shFish {
 		tmpl = fishrcTmpl
 	}
 

diff --git a/internal/shellgen/scripts.go b/internal/shellgen/scripts.go
@@ -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
 }
@@ -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)
 	}
@@ -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 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 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`.
+		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")
 }