ci: more visibility of errors output by tools/check.sh

[JohelEGP]

@burblebee commented at #4520 (comment) about wanting something like that. Does the following meet your expectations? https://github.com/johelegp/draft/runs/1985020607?check_suite_focus=true#step:4:2160. Maybe what you actually want is a bot that comments the errors on the PR. Or have the errors output in a separate step, so that you don't have to scroll all the LaTeX build output.

[jwakely]

I was about to open an issue about exactly this.

Why doesn't the script just print "Checking foo..." before each check, so if there's output from a check you know where it comes from? Seems easy. Or if you want to keep the output empty when nothing fails, make those headers only printed with a --verbose switch (so we can at least run it locally to find the problem). Or filter the output of failed checks through sed 's/^/SOME IDENTIFIYING TOKEN/'

There are lots of ways to make this less abstruse.

[jwakely]

Adding "Error: " is better than nothing, but for many of the checks in that script there is no context at all. The "non-ASCII character" output is clear enough for me, but there is no context for checks for things like \returns not being followed by a newline.

[jwakely]

For example, the failure at https://github.com/cplusplus/draft/runs/1978623293?check_suite_focus=true just ends like this:

texlive/texmf-dist/fonts/type1/public/rsfs/rsfs10.pfb>
Output written on std.pdf (1849 pages, 7045850 bytes).
Transcript written on std.log.
Latexmk: Log file says output to 'std.pdf'
Latexmk: All targets (std.pdf) are up-to-date
utilities.tex:584:\returns \tcode{static_cast<underlying_type_t<T>>(value)}.
Error: Process completed with exit code 1.

I'm pretty familiar with the draft sources and the conventions, but that had me stumped until I read tools/check.sh in detail.

This change:

diff --git a/tools/check.sh b/tools/check.sh
index 15ee9f80..8710d991 100755
--- a/tools/check.sh
+++ b/tools/check.sh
@@ -1,5 +1,9 @@
 #!/bin/bash
 
+fail() {
+  sed "s/^/Error: $1: /;q 1" || exit 1
+}
+
 # Ignore files where rules may be violated within macro definitions.
 texfiles=$(ls *.tex | grep -v macros.tex | grep -v layout.tex | grep -v tables.tex)
 texlibdesc="support.tex concepts.tex diagnostics.tex utilities.tex strings.tex containers.tex iterators.tex ranges.tex algorithms.tex numerics.tex time.tex locales.tex iostreams.tex regex.tex atomics.tex threads.tex"
@@ -43,7 +47,7 @@ grep -n 'opt{}' *.tex && exit 1
 grep -n "// not defined" $texfiles | sed 's/$/ <--- use \\notdef instead/' | grep . && exit 1
 
 # Library element introducer followed by stuff.
-grep -ne '^\\\(constraints\|mandates\|expects\|effects\|sync\|ensures\|returns\|throws\|complexity\|remarks\|errors\).\+$' $texlibdesc && exit 1
+grep -ne '^\\\(constraints\|mandates\|expects\|effects\|sync\|ensures\|returns\|throws\|complexity\|remarks\|errors\).\+$' $texlibdesc | fail "stuff after library element"
 # Fixup: sed 's/^\\\(constraints\|mandates\|expects\|effects\|sync\|ensures\|returns\|throws\|complexity\|remarks\|errors\)\s*\(.\)/\\\1\n\2/'
 # Fixup: sed 's/^\\ //'
 

would make that error look like this:

Error: stuff after library element: utilities.tex:584:\returns \tcode{static_cast<underlying_type_t<T>>(value)}.

So instead of && exit 1 at the end of a grep command, just pipe through fail "some descriptive string" and it will exit if there is any output, adding the descriptive string.

[jwakely]

(Sorry for closing it, I tabbed my way to the close button and hit enter 🤦)

[jwakely]

Would it make sense to run the check script first (because it's really fast) so if the document builds but fails a lint check you don't have to scroll past thousands of lines of output from latex to find the error?

If a lint check is going to fail the CI run anyway, why bother building the document? It's probably safe to assume the person making the change has built it, even if they didn't run the linter checks. So building it first adds no value.

Edit: looks like we can't run it earlier, because the script depends on some of the build artefacts created during the make step. Maybe we want two scripts then, or a "quick check" mode for the script, so we can run some of it first to fail fast, then do the build and re-check.

[jwakely]

And maybe the CI build could also say "Running tools/check.sh" before running it, so it's more obvious where that error comes from. More context is good.

[tkoeppe]

There's definitely a lot we could improve about the checking. Remember, this grew out of 1) nothing, then 2) a travis yaml command to run latexmk (plus some other stuff), then 3) we started adding some ad-hoc grep checks to the end of that command to look at the logs, which later extended to more grepping of the sources themselves, and finally we moved that to a separate script.

We should definitely divide the work into 1) source checking, 2) building, 3) output checking. And yes, we should also state the checked condition in a human-readable form.

Thanks a lot for doing this! Let me know if you'd like any help, either with the scripting or with the GitHub runner setup!

[jwakely]

I'm not sure why the install step is failing in the CI checks for my pull request. Does my fork need something enabled?

[JohelEGP]

Edit: looks like we can't run it earlier, because the script depends on some of the build artefacts created during the make step. Maybe we want two scripts then, or a "quick check" mode for the script, so we can run some of it first to fail fast, then do the build and re-check.

Seems worth it. It could even run before the "install" step, which takes ~40 seconds, compared to the ~30 seconds from the check script (which I got by comparing a failed run vs a successful run. We should get the exact time once it's in its own step).

I'm not sure why the install step is failing in the CI checks for my pull request. Does my fork need something enabled?

Seems like all your runs were done on Ubuntu 20.04, which doesn't have the texlive-generic-recommended package.

[burblebee]

Issues I have with the checker script:

• it doesn't work on iMacs due to Apple's broken sed (I had to modify the script locally to remove the checks that don't work on the mac, but it means I can't run those extra checks before committing)
• it doesn't check for embedded tabs (the travis checks did)
• not all errors are flagged red (it's the only sensible way to find errors in the reams of build output)