Skip to content

Surface raw errors when check_output fails#2125

Merged
mhsmith merged 8 commits intobeeware:mainfrom
freakboy3742:check-output-errors
Jan 23, 2025
Merged

Surface raw errors when check_output fails#2125
mhsmith merged 8 commits intobeeware:mainfrom
freakboy3742:check-output-errors

Conversation

@freakboy3742
Copy link
Member

Briefcase generally attempts to provide actionable feedback to the user when things go wrong, and stay silent otherwise. However, there are many edge cases of command failure that result in Briefcase providing generic advice like "Unable to sign app" when the underlying command output provides useful context.

If the call was made with subprocess.run or subprocess.Popen, then this context will be visible. However, if subprocess.check_output is used, the context will only be visible in the logs.

This PR modifies subprocess.check_output so that in case of error, output is surfaced (with muted markup) so that any useful context can be seen by the user. This can be disabled by passing quiet=1 (which will still log the output) or quiet=2 (which won't log the output either).

quiet=1 is often needed when the failure is expected - such as when probing for whether Xcode has been installed. In this case, we look for specific error messages in the output, and if they're found, we return the specific actionable advice. If the expected text isn't present, we can reproduce the default output behavior. Essentially if we are doing any post-processing of the CalledProcessError, we should probably be using quiet=1.

Fixes #1907

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

@freakboy3742 freakboy3742 requested a review from mhsmith January 21, 2025 03:59
@freakboy3742 freakboy3742 requested a review from mhsmith January 22, 2025 04:45
@mhsmith
Copy link
Member

mhsmith commented Jan 22, 2025

There's at least one more call in Android which should be quiet, as I get this message every time I run briefcase run android:

Running Command:
    /Users/msmith/Library/Android/sdk/platform-tools/adb -s 28261FDH2009MJ emu avd name
Return code: 1

Since ADB.run wraps subprocess.check_output, its other calls should also be reviewed to check if any of them have expected failures.

@freakboy3742
Copy link
Member Author

Since ADB.run wraps subprocess.check_output, its other calls should also be reviewed to check if any of them have expected failures.

Good catch - I audited the calls to adb in the android platform backend, but forgot that there was a bunch of them wrapped up in the Android integration itself - and this particular one only manifests when you have a physical Android device plugged in, so it didn't appear in my testing.

I also spotted an extra one in the java backend - clearly I missed this in the volume of output that is generated at the start of a briefcase run android call, combined with the fact that the error handling doesn't match the usual "special case of one error code/output" that most quiet=1 usages have.

@mhsmith mhsmith merged commit e3ec21c into beeware:main Jan 23, 2025
57 checks passed
@freakboy3742 freakboy3742 deleted the check-output-errors branch January 23, 2025 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

If a subprocess fails for an unknown reason, display its output in the console

2 participants