Fail on LaTeX errors when warnings are errors#43
Conversation
|
I noticed this because I introduced a mistake in AutoDoc's manual and the CI just passed... sigh |
stertooy
left a comment
stertooy
left a comment
There was a problem hiding this comment.
Seems to work as intended, just one small remark :)
action.yml
Outdated
| run: | | ||
| if grep -q -i -e "there were latex errors" $RUNNER_TEMP/output.log; then | ||
| echo "::error::LaTeX errors were found when building the documentation!" | ||
| grep -i -e "there were latex errors" $RUNNER_TEMP/output.log |
There was a problem hiding this comment.
I'm not sure what the intention of the final grep line is? It will just print "#W There were LaTeX errors:", but not the actual errors.
There was a problem hiding this comment.
Well this is modelled on the existing code a few lines below: I think the idea is to make the error "stand out" by repeating it.
But I'd be fine with removing it here -- but then also there.
There was a problem hiding this comment.
Making the errors stand out is IMO a good thing, but I'd want to see the actual errors. The existing code (mostly?) gets to deal with one-line warnings, so repeating those lines is helpful.
But for the LaTeX errors, the actual errors are on separate lines below the '#W There were LaTeX errors' line. Something like this, maybe? (untested code!)
echo "::error::LaTeX errors were found when building the documentation!"
awk '/^#[IW] / {err=0}; /There were LaTeX errors/ {err=1}; err && /^!/ {print}' "$RUNNER_TEMP/output.log"
which should then print something like
Error: LaTeX errors were found when building the documentation!
! Missing \endcsname inserted.
! Argument of \@setref has an extra }.
! Paragraph ended before \@setref was complete.
! Extra \endcsname.
! Too many }'s.
There was a problem hiding this comment.
In the case where we find any errors of any form in the log, is it worth outputting the whole log file for easier debugging?
There was a problem hiding this comment.
whenever I look at these CI logs, I need to search for "warning" or "error" either way, so I am not sure I understand why the warnings are "special" and need to be repeated for the user (to the contrary, I've been confused by this more than once "huh, why does this warning not have more context".
That said, I really don't want to be bogged down by an argument about this. I've added the removed line which greps for the warnings back.
If we really want to help the user, we'd need to find a way to highlight the specific error messages, but I don't know how to do that, even less so for the LaTeX errors; and honestly it feels like icing on the cake (I want it! but I don't need it), while the PR here is crucial (my documentation did not build correctly have a code change I made, and CI failed me in that it did not call the failure out)
There was a problem hiding this comment.
Fair enough. I don't feel that strong about the repeated warnings either way.
Fully agreed that getting this PR should be the priority. I encountered this exact problem just two days ago, and didn't notice it until another error did break the workflow.
fingolfin
force-pushed
the
codex/fail-on-latex-errors
branch
from
5d1745c to
ad6b251
Compare
Joseph-Edwards
left a comment
Joseph-Edwards
left a comment
There was a problem hiding this comment.
This all looks good to me. We may wish to output the whole log file if we find any errors for easier debugging though I don't feel too strongly about this. Otherwise I'm happy to merge
Treat GAPDoc's "There were LaTeX errors" message as a warning failure when warnings-as-errors is true. Co-authored-by: Codex <codex@openai.com>
fingolfin
force-pushed
the
codex/fail-on-latex-errors
branch
from
ad6b251 to
e1cd608
Compare
|
Merged and updated the v2 tag |
4 participants
Treat GAPDoc's "There were LaTeX errors" message as a
warning failure when warnings-as-errors is true.
Co-authored-by: Codex codex@openai.com