Always print tracebacks in logs when Sphinx encounters an internal error

[kdeldycke]

Allow the full traceback to be printed when Sphinx encounters an internal fatal error.

Purpose

• Improve in-situ debuguability of Sphinx.
• Improve bug reporting of third-party extensions.

Detail

For instance, when Sphinx is run in CI/CD (like GitHub actions workflows), we do not have access to the file-system. So when Sphinx encounter a fatal internal error, it just print the last frace of the traceback and dump the full traceback in a local temporary file.

This can produce error logs as uninformative as:

(...)
Exception occurred:
  File "<docs>", line 3, in <module>
AssertionError
The full traceback has been saved in /tmp/sphinx-err-wjogkybw.log, if you want to report the issue to the developers.
Please also report this if it was a user error, so that a better error message can be provided next time.
A bug report can be filed in the tracker at <https://github.com/sphinx-doc/sphinx/issues>. Thanks!

So by always printing the full traceback, this PR will improve identification of root causes, especially if they're coming from third-party plugins and extensions. I expect this change to have the nice side effect of improving bug reports to the community.

[AA-Turner]

Thank you for the very nice PR @kdeldycke, and sorry it has taken a month to review.

The only real question I have is on design -- is a config option the right choice here? Should we use an environment variable or command-line flag instead?

In other words, what are the scenarios for disabling this feature, and when are people most likely to do so? I am half-minded to unconditionally enable it, but there might be complaints of overly-long console output.

A

[kdeldycke]

is a config option the right choice here? Should we use an environment variable or command-line flag instead?

I made that feature available through a configuration option because it made it more visible to the end user. And also because I didn't wanted to be too bold and force to change the current behavior of Sphinx.

But if it would be only on me, I think it might be a good idea to always enable it: when you encounter a fatal error, we are past the point where a clean, uninformative log output is helpful.

[AA-Turner]

I agree. Could you update the PR to make it unconditional for now? If there is push-back we could add an environment variable or config option later down the line.

A

[AA-Turner]

Should we move this earlier? Currently it only executes on the general case of Exception after the specific exceptions have been handled before it.

[kdeldycke]

As you wish. My thinking was to print in the log as soon as we have the traceback, then write on the disk, as there is a slightly greater chance that writing on the disk might failed compared to the failing to print in the log. But I might over think things. So I'm going to just applied your suggestion.

[AA-Turner]

Sorry, I didn't mean changing the order of print vs saving to disk, but moving this entire chunk upwards -- see the rest of the function for where various other error types are handled. What do you think?

[kdeldycke]

Oh OK. So the idea would be to move the unconditional traceback printing before the handling of each specific kind of exception, each of which can interrupt the execution of the function with a return.

At that stage, I just wonder if there is any point in handling each exception type, since the traceback_output already does the job of providing all the contextual information.

So I update the PR to show you what my proposal would look like, simplifying the code and displaying the detailed traceback if any exception occur. If you don't like it I'll revert that change.

[AA-Turner]

There's also no real need to force push, we squash merge all commits.

[kdeldycke]

There's also no real need to force push, we squash merge all commits.

Oh OK. Sorry. I didn't knew that.

I know all projects have their own preferences, so my default policy is to keep my PRs as simple as possible, and as readable as possible. It also make the work easier for less experience maintainers to resolve conflicting merges. Hence my habits to force push things.

[kdeldycke]

Apart from the failing readthedocs.org build, this PR should be good to go.