Open mhsmith opened 1 month ago
Originally posted by @freakboy3742 in https://github.com/beeware/briefcase/issues/1362#issuecomment-1650949712
Generally speaking, I agree - however, I think I'd qualify this by saying that the bar for "more useful" should be very low, and skewed in the favor of novice developers. Many of the developer tools we're wrapping leave a lot to be desired in terms of readability; if we're in a position to improve the end-user experience, we should do so.
Here's another example from Discord, where a git
command failed because the the Git version was too old:
briefcase.2024_07_09-16_47_44.new.log
Briefcase displays a misleading message:
This may be because your computer is offline, or because the repository URL is incorrect.
The actual error is buried in the middle of the log:
error: unknown option `filter=blob:none
I still argue that we should show a failing command's output to the user unless we can be certain that Briefcase has a more useful message to display instead, either by examining the command's output or its return code.
I don't think this could ever be the case with git clone
, because there are too many ways in which it can fail:
The same is true of most other failed subprocesses. We should certainly display some advice about what the most likely causes are, but we should display the original message as well. That combination would make it much easier for the user to solve the problem without asking for help.
Pip may serve as reasonable prior art to support the case to always show the details of a failed subprocess.
For instance:
> python3 -m pip install pycairo
Collecting pycairo
Downloading pycairo-1.26.1.tar.gz (346 kB)
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 346.9/346.9 kB 2.4 MB/s eta 0:00:00
Installing build dependencies ... done
Getting requirements to build wheel ... done
Preparing metadata (pyproject.toml) ... done
Building wheels for collected packages: pycairo
Building wheel for pycairo (pyproject.toml) ... error
error: subprocess-exited-with-error
× Building wheel for pycairo (pyproject.toml) did not run successfully.
│ exit code: 1
╰─> [12 lines of output]
running bdist_wheel
running build
running build_py
creating build
creating build/lib.linux-x86_64-cpython-312
creating build/lib.linux-x86_64-cpython-312/cairo
copying cairo/__init__.py -> build/lib.linux-x86_64-cpython-312/cairo
copying cairo/__init__.pyi -> build/lib.linux-x86_64-cpython-312/cairo
copying cairo/py.typed -> build/lib.linux-x86_64-cpython-312/cairo
running build_ext
'pkg-config' not found.
Command ['pkg-config', '--print-errors', '--exists', 'cairo >= 1.15.10']
[end of output]
note: This error originates from a subprocess, and is likely not a problem with pip.
ERROR: Failed building wheel for pycairo
Failed to build pycairo
ERROR: Could not build wheels for pycairo, which is required to install pyproject.toml-based projects
FWIW, the Git example is a bit of an outlier (insofar as capturing details of a subprocess call) since Briefcase doesn't, itself, shell out to run Git. Instead, this is deferred to the GitPython package. Therefore, all relevant details about the outcome of running git
must be extracted from the exception raised from GitPython. The only reason even the log file contains details is because we're including the stringified version of the exception.
I agree it is undesirable to provide inaccurate error messages. However, in this case, I'd argue the error isn't inaccurate. The provided quote is misleading - the full error message says:
Unable to clone repository 'https://github.com/beeware/briefcase-template'.
This may be because your computer is offline, or because the repository URL is incorrect.
This is exactly what has happened in the filter=blob:none
case - Briefcase was unable to clone the repository. Prior to the introduction of filtering, virtually all failures at this point were due to connectivity. The most recent Briefcase release has added a new failure mode, so the explanation for the underlying cause is isn't correct in this case - but that part of the sentence also says "may be because".
In this case, if git's error was included in the log , I'd argue the 1 piece of useful information in the output would be lost in the noise. git clone --adsf
outputs a one line message "unknown option --asdf", followed by the full usage for git - over 50 lines of output that doesn't really add anything to the debugging experience in this context. I'd need to check to be sure, but I don't think pip displays the entirety of log output - it gives a tail of the output up to a limit. So, in this case "pip-comparable" output wouldn't show anything useful; and including the full output would then require the user to read and interpret 50+ lines of output to identify a specific problem... which they then aren't really in a position to action, because they didn't opt into the usage of --filter
. Even if they do find the one useful line in the output, the connection between "error: unknown option filter=blob:none
" and "Your git version is too old" requires background knowledge that many users won't have.
In this specific case, I'd argue the problem here is that the helper message should include "or your git version is too old" as part of the helper text. Better still, we'd add an explicit check for the git version as part of git verification.
This less of a problem with the original ADB failure - mostly because Briefcase doesn't provide any real feedback. It just says "it didn't work". In that case, I agree surfacing the ADB output is a lot more helpful.
To that end, I'd suggest the following "style guide":
I agree with the "style guide", but I would argue that it still makes sense to display the Git output in this case, unless we can determine a specific cause like the Git version:
if git's error was included in the log , I'd argue the 1 piece of useful information in the output would be lost in the noise.
git clone --adsf
outputs a one line message "unknown option --asdf", followed by the full usage for git - over 50 lines of output
As I mentioned above, there are many other reasons why Git could fail, and most of them would give a shorter and clearer error message.
Even if they do find the one useful line in the output, the connection between "error: unknown option
filter=blob:none
" and "Your git version is too old" requires background knowledge that many users won't have.
Like I said above, even if the user doesn't understand what an error message means, at least they would be able to find a solution by searching for its text.
In #1362, Briefcase failed to install an Android app, but all it displayed this in the console was this:
If we'd displayed the output of the failing command, the cause would have been obvious:
INSTALL_FAILED_UPDATE_INCOMPATIBLE: Package com.example.helloworld signatures do not match previously installed version
Even if the user doesn't understand what this message means, at least they would be able to find a solution by searching for its text. As it is, there isn't much they can do except come and ask us for help, which wastes both their time and ours.
Yes, they can find the output in the log file, but apparently many users don't even think to look there. And if they do, the adb output is buried in the middle of the file between all the successful steps and the extra-verbose stack trace, so its relevance is not obvious from a user's point of view.
In general, I think that if a Briefcase command fails because of a subprocess, its output should always be shown to the user unless we can be certain that Briefcase has a more useful message to display instead.