[error] Better error message for ExitErrors #675

savil · 2023-02-23T00:11:53Z

Summary

This PR makes three changes:

Handles ExitErrors specially (for non usererr.ExitError) by printing the err.Stderr when DEVBOX_DEBUG=1.
When DEVBOX_DEBUG=0, we now include the command and also suggest to the user to run with DEVBOX_DEBUG=1 for a more detailed error and suggest reporting the bug.
Renames usererr.UserExecError to usererr.ExitError which more accurately represents what that struct is doing: the error is only wrapped if it is ExitError.

How was it tested?

in testdata/unfree-packages:

BEFORE:

❯ DEVBOX_FEATURE_FLAKES=1 DEVBOX_FEATURE_UNIFIED_ENV=1 DEVBOX_DEBUG=0 devbox shell
Ensuring packages are installed.
Ensuring nixpkgs registry is downloaded.
Downloaded 'github:NixOS/nixpkgs/52e3e80afff4b16ccb7c52e9f0f5220552f03d04' to '/nix/store/j5a7i3dvxslg2ychfy53wdhg1m3xfrwm-source' (hash 'sha256-FqZ7b2RpoHQ/jlG6JPcCNmG/DoUPCIvyaropUDFhF3Q=').
Ensuring nixpkgs registry is downloaded: Success
Starting a devbox shell...
Error: exit status 1

AFTER:

and AFTER with DEBUG=1:

savil · 2023-02-23T00:12:06Z

Current dependencies on/for this PR:

main
- PR [error] Better error message for ExitErrors #675 👈
  - PR [flakes] wait some time before printing output, while prefetching nixpkgs #680

This comment was auto-generated by Graphite.

savil · 2023-02-23T00:15:08Z

Should we only display this detailed error message when DEVBOX_DEBUG=1 is true?

savil · 2023-02-23T00:16:46Z

I chose to include more information so that if a user reports it, then it is much easier to see the problem.

savil · 2023-02-23T00:19:24Z

But it could be overwhelming to a user who is new to nix, so we could show a simpler error like:

A nix command failed with error: exit status 1. Run with DEVBOX_DEBUG=1 for a detailed error message and please report to https://github.com/jetpack-io/devbox/issues/new/choose

mikeland73

This is really good (and needed!)

I do wonder if we can simplify a bit to avoid having custom error structs. Specifically, we could have

type UserExitError exec.ExitError

and not use any special error at all for generic exit errors. (this way, if we forget to wrap we still get good info!)

In the debug middleware we can have additional handling depending on the type:

var userExitError *UserExitError
var genericExitErr *exec.ExitError
if errors.As(err, userExitError) {
 //print very detailed stuff since it's their error
} else if errors.As(err, genericExitErr) {
 // print less but enough detail. If DEVBOX_DEBUG=1 is true, print everything
}

For printing the command, instead of using a custom struct you can wrap:

c := exec.Command("bad-echo", "hello")
return errors.Wrapf(c.Run(), "command: %s", c.String())

This should print something like Error: command: bad-echo hello: exec: "bad-echo": executable file not found in $PATH which is pretty helpful! I think this would be a more "golang" approach.

Anyway, don't want to block this PR so pre-approving. We can always follow up if you think the suggestions are valid.

mikeland73 · 2023-02-23T00:32:52Z

internal/boxcli/usererr/cmderr.go

+)
+
+// ExecCmdError is an error from an exec.Cmd for a devbox internal command
+type ExecCmdError struct {


Slightly simpler:

// ExecCmdError is an error from an exec.Cmd for a devbox internal command type ExecCmdError struct { error cmd *exec.Cmd } func NewExecCmdError(cmd *exec.Cmd, err error) error { if err == nil { return nil } return &ExecCmdError{ error: err, cmd: cmd, } } func (e *ExecCmdError) Error() string { var errorMsg string // ExitErrors can give us more information so handle that specially. var exitErr *exec.ExitError if errors.As(e, &exitErr) { errorMsg = fmt.Sprintf( "Error running command %s. Exit status is %d. Command stderr: %s", e.cmd, exitErr.ExitCode(), string(exitErr.Stderr), ) } else { errorMsg = fmt.Sprintf("Error running command %s. Error: %v", e.cmd, e.error.Error()) } return errorMsg }

mikeland73 · 2023-02-23T00:37:42Z

internal/boxcli/usererr/cmderr.go

+	var exitErr *exec.ExitError
+	if errors.As(e.err, &exitErr) {


Question, if error is not a *exec.ExitError do we even want to wrap it? Could NewExecCmdError simply return error as is? or maybe return errors.Wrap() and adds the command as the message? That way we only wrap stuff that really needs it.

mikeland73 · 2023-02-23T02:28:12Z

internal/boxcli/midcobra/midcobra.go

@@ -64,7 +64,7 @@ func (ex *midcobraExecutable) Execute(ctx context.Context, args []string) int {
 	err := ex.cmd.ExecuteContext(ctx)

 	var postRunErr error
-	var userExecErr *usererr.UserExecError
+	var userExecErr *usererr.UserExitError


To avoid repetition, I wonder if just usererr.ExitError

savil · 2023-02-23T17:30:17Z

@mikeland86 yes! I like your approach a lot. I was also feeling that this is a bit heavy for what I am trying to achieve. I'll update to that.

gcurtis · 2023-02-24T18:47:14Z

internal/boxcli/usererr/exiterr.go

 )

-type UserExecError struct {
+// ExitError is an ExitError for a command run on behalf of a user
+type ExitError struct {


I'm curious why we have a new type for this. Why not just use exec.ExitError directly?

@gcurtis the idea is that a user exit error and a generic exit error are treated differently in terms of what we show the user. For errors that were a result of an error caused by user code we want to always show as much info as possible. For non-user exit errors we only show all info if DEVBOX_DEBUG is true.

This type is used for errors that are caused by the user's program, as opposed to a devbox CLI error. For example, when doing devbox run -- <program> the user's program may exit with an error. So, if we can detect that circumstance, we wrap the exec.Command's error in this special type and then render the error message accordingly (i.e. making it clear that the user needs to fix their own code)

mikeland73 · 2023-02-24T19:01:04Z

internal/boxcli/usererr/exiterr.go

 		err: exitErr,
 	}
 }

-func (e *UserExecError) Error() string {
+func (e *ExitError) Error() string {


Not from this PR, but if ExitError is just an exec.ExitError you don't need to reimplement all this stuff.

mikeland73 · 2023-02-24T19:03:31Z

internal/boxcli/midcobra/debug.go

 	}

 	st := debug.EarliestStackTrace(runErr)
-	debug.Log("Error: %v\nExecutionID:%s\n%+v\n", runErr, d.executionID, st)
+	color.Red(fmt.Sprintf("Error: %v\n\n", runErr))


This should go to cmd.ErrOrStderr() instead.

mikeland73 · 2023-02-24T19:03:46Z

internal/boxcli/midcobra/debug.go

@@ -57,14 +59,17 @@ func (d *DebugMiddleware) postRun(cmd *cobra.Command, args []string, runErr erro
 		if usererr.IsWarning(runErr) {
 			ux.Fwarning(cmd.ErrOrStderr(), runErr.Error())
 		} else {
-			color.Red("\nError: " + runErr.Error() + "\n\n")
+			color.Red(runErr.Error() + "\n\n")


This has been changed to go to cmd.ErrOrStderr() (you need to rebase)

savil requested a review from mikeland73 February 23, 2023 00:13

savil marked this pull request as ready for review February 23, 2023 00:13

mikeland73 approved these changes Feb 23, 2023

View reviewed changes

savil force-pushed the savil/devbox-exec-error branch from f202816 to 4a07df0 Compare February 23, 2023 21:22

savil changed the title ~~[error] introduce usererr.ExecCmdError for better error message~~ [error] Better error message for ExitErrors Feb 23, 2023

savil requested a review from mikeland73 February 23, 2023 21:26

savil mentioned this pull request Feb 24, 2023

[flakes] wait some time before printing output, while prefetching nixpkgs #680

Closed

savil requested review from LucilleH, ipince and gcurtis February 24, 2023 01:03

gcurtis approved these changes Feb 24, 2023

View reviewed changes

mikeland73 reviewed Feb 24, 2023

View reviewed changes

savil added 2 commits February 24, 2023 11:03

[error] introduce usererr.ExecCmdError for better error message

34e3795

rename UserExitError to ExitError

e8bf8ab

mikeland73 approved these changes Feb 24, 2023

View reviewed changes

Remove usererr/cmderr.go and handle better error output in debug.PostRun

911b40d

savil force-pushed the savil/devbox-exec-error branch from 4a07df0 to 911b40d Compare February 24, 2023 19:05

write to stderr instead of print

c1fbdd8

savil merged commit e5b0710 into main Feb 24, 2023

savil deleted the savil/devbox-exec-error branch February 24, 2023 22:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[error] Better error message for ExitErrors #675

[error] Better error message for ExitErrors #675

savil commented Feb 23, 2023 •

edited

savil commented Feb 23, 2023 •

edited

savil commented Feb 23, 2023

savil commented Feb 23, 2023

savil commented Feb 23, 2023

mikeland73 left a comment

mikeland73 Feb 23, 2023

mikeland73 Feb 23, 2023

mikeland73 Feb 23, 2023 •

edited

savil commented Feb 23, 2023

gcurtis Feb 24, 2023

mikeland73 Feb 24, 2023

savil Feb 24, 2023 •

edited

mikeland73 Feb 24, 2023

mikeland73 Feb 24, 2023

mikeland73 Feb 24, 2023

[error] Better error message for ExitErrors #675

[error] Better error message for ExitErrors #675

Conversation

savil commented Feb 23, 2023 • edited

Summary

How was it tested?

savil commented Feb 23, 2023 • edited

savil commented Feb 23, 2023

savil commented Feb 23, 2023

savil commented Feb 23, 2023

mikeland73 left a comment

Choose a reason for hiding this comment

mikeland73 Feb 23, 2023

Choose a reason for hiding this comment

mikeland73 Feb 23, 2023

Choose a reason for hiding this comment

mikeland73 Feb 23, 2023 • edited

Choose a reason for hiding this comment

savil commented Feb 23, 2023

gcurtis Feb 24, 2023

Choose a reason for hiding this comment

mikeland73 Feb 24, 2023

Choose a reason for hiding this comment

savil Feb 24, 2023 • edited

Choose a reason for hiding this comment

mikeland73 Feb 24, 2023

Choose a reason for hiding this comment

mikeland73 Feb 24, 2023

Choose a reason for hiding this comment

mikeland73 Feb 24, 2023

Choose a reason for hiding this comment

savil commented Feb 23, 2023 •

edited

savil commented Feb 23, 2023 •

edited

mikeland73 Feb 23, 2023 •

edited

savil Feb 24, 2023 •

edited