[hooks] Specify exit-codes and add status field to output

[dcharkes]

RE: Recognizing network failures: An alternative approach without a prefetch hook could be:

• hook/build.dart outputs a status that can be success, build_failure, or infra_failure. 🙏 @HosseinYousefi
• flutter_tools outputs a status that can be build_failure or infra_failure.
• LUCI gets support for conditionally making a step being reported as a build failure or infra failure. 🙏 @goderbauer

Originally posted by @dcharkes in #1620

Let's split this out in a new issue.

Exit codes

• 0: Success
• 1: build_failure. E.g. when CBuilder can't build the C code, or invoking gradle build fails.
• 2: infra_failure. E.g. when an asset download fails, or invoking gradle fetch fails.
• 255: uncategorized / Uncaught Exception. Current behavior with uncaught exceptions.

(Current protocol description is 0 for success and non-zero for failure. Specifying more detailed error-codes is something we need to do before removing the experimental flag.)

output.json

If the exit-code is non-zero, the hook should still output an output.json, but it might not do so. (E.g. if running the dart process fails.)

{
  "status": "failure",
  "type": "infra"
}

{
  "status": "failure",
  "type": "build"
}

{
  "status": "failure",
  "type": "uncategorized"
}

~~We could potentially extend these formats to contain structured errors which could be processed by SDKs (Dart/Flutter) in a more structured manner than only the stderr. (I don't believe there is an immediate need for this as we already have stderr right now.)~~

{
  "status": "failure",
  "type": "uncategorized",
  "error": {
    "message": "...",
    "stackTrace": "..."
  }
}

The success output.json would also get a status field:

{
  "status": "success",
  // ...
}

(Current protocol has no status field, we should add this before removing the experimental flag.)

Dart API / hook coding flow

It's convenient for hooks to be able to throw exceptions when encountering a build or an infra error. So we should add:

abstract class HookException {
  final String message;
  final Object? wrappedException;
  final StackTrace? wrappedTrace;
}

final class BuildException extends HookException { }

final class InfraException extends HookException { }

And then build( and link( would catch those, write to stderr, write out output.json, and exit with the right exit code. build( and link( would also catch any uncaught exceptions and do the same.

Then hook writers and hook helper package writers can catch an existing exception and throw an appropriate HookException.

~~Possible behavior for infra failures~~

~~We could consider adding automatic retry behavior for infra failures. (e.g. if we know something can be flaky.)~~

Fetch hooks

In addition we might add hook/fetch.dart (https://github.com/dart-lang/native/issues/1620), but that is somewhat orthogonal to this proposal which is proposing to have more structured info about failure modes.

Any feedback @goderbauer @HosseinYousefi @mosuem @liamappelbe?

[dcharkes]

Notes from discussion with @mosuem

• Explicitly mark exit codes 3-254 as reserved, so we can assign these in the future.

Notes from discussion with @mkustermann

• If there are multiple builders in a hook, who sets the exit code?
  • If we go with the exception-based approach, the builder that throws first.
    • If users don't catch exceptions and throw the hook-exceptions, their hooks output 255.
      • Either we say this is fine. And treat it the same as exit code 1, build error.
      • Or we give a warning on the stderr that they should catch their exceptions and throw a BuildException or InfraException.
• We don't have a use for structured errors right now. (Most likely the package author themselves would want to have structured errors of their package to report to Sentry or something similar.)

@goderbauer any further thoughts?

[goderbauer]

The proposal sounds good to me.