New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ci: more visibility of errors output by tools/check.sh #4529
Comments
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 There are lots of ways to make this less abstruse. |
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 |
For example, the failure at https://github.com/cplusplus/draft/runs/1978623293?check_suite_focus=true just ends like this:
I'm pretty familiar with the draft sources and the conventions, but that had me stumped until I read 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:
So instead of |
(Sorry for closing it, I tabbed my way to the close button and hit enter 🤦) |
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 |
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. |
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 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! |
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 worth it. It could even run before the "install" step, which takes ~40 seconds, compared to the ~
Seems like all your runs were done on Ubuntu 20.04, which doesn't have the texlive-generic-recommended package. |
Issues I have with the checker script:
|
We can probably make the use of GNU sed features optional, or replace them with POSIX alternatives. The CI actions use |
Yes. It is intentionally only quiet only for the first pass: e9563c2. |
Making them optional doesn't help me - I want to be able to run all checks to catch errors before committing. |
@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.
The text was updated successfully, but these errors were encountered: