[Workflow] Expand code-format-helper.py error reporting

[rprichard]

• Consider the darker/clang-format command to have failed if the exit code is non-zero, regardless of the stdout/stderr output.
• Allow stderr from the formatter command to propagate to the script's caller (and into the GitHub log).
• On success, dump stdout to the caller, so it ends up in GitHub's log. I'm not sure what this would ever be, but if it exists, it should be preserved.
• Just before the script exits, if any formatter failed, print a line showing which formatters failed.

Replace [str] with list[str] to satisfy mypy/pytype.

[rprichard]

I'm trying to fix a couple of issue I recently ran into:

• darker failed, but @EricWF and I also thought that git-clang-format had failed. References:

  • [libc++][Android] Mark tests XFAIL/UNSUPPORTED #69271 (comment)
  • https://github.com/llvm/llvm-project/actions/runs/6581095590/job/17880364421#step:8:63

  The last lines in the log are:

  Running: git-clang-format --diff 3694697003bbf443fd644c6746a1c7c937657fce 2eeb907aa716fb45ebac1d3e57abfa6587afe1cd -- libcxx/test/libcxx/input.output/iostream.format/print.fun/vprint_unicode_posix.pass.cpp libcxx/test/std/input.output/iostream.format/print.fun/no_file_description.pass.cpp libcxx/test/std/input.output/iostream.format/print.fun/print.file.pass.cpp libcxx/test/std/input.output/iostream.format/print.fun/println.file.pass.cpp libcxx/test/std/input.output/iostream.format/print.fun/vprint_nonunicode.file.pass.cpp libcxx/test/std/input.output/iostream.format/print.fun/vprint_unicode.file.pass.cpp libcxx/test/std/input.output/string.streams/stringstream/stringstream.members/gcount.pass.cpp libcxxabi/test/test_demangle.pass.cpp
  Error: Process completed with exit code 1.
  

• I recently merged a change that added new Python files that don't pass black/darker. When I run the darker command myself locally, it fails and prints a diff. However, the change passed the check on GitHub without printing any error. References:

  • [libc++][Android] Support libc++ testing on Android #69274 (comment)
  • https://github.com/llvm/llvm-project/actions/runs/6541446281/job/17762983123?pr=69274#step:8:1

I'm not sure how to test this change, but it does pass mypy and black.

I was a bit confused by the param: [str] type annotations... AFAICT mypy and pytype don't accept it and require param: list[str] instead, so I change the annotations.

[tru]

Hi, thanks for the updates, they seem sane to me. It's a little tough to review since it has a lot of stylistic changes mixed in with the functional ones. I would like to know if we could split the PR up so it's easier to review.

[rprichard]

I'll try splitting out an earlier CL that just does black/mypy. That should help.

[rprichard]

The "[Workflow] make code-format-helper.py mypy-safe (NFC)" commit in this PR is actually part of #69691 instead.

[tru]

LGTM, I assume this has been tested somehow right?

[rprichard]

LGTM, I assume this has been tested somehow right?

Unfortunately, it still needs to be tested somehow. I did notice that you had some way to demo it on tru#8.

[tru]

LGTM, I assume this has been tested somehow right?

Unfortunately, it still needs to be tested somehow. I did notice that you had some way to demo it on tru#8.

Yes, the way I tested it was that I pushed my changes to a branch in my fork (i.e. formatter), then I created a new branch based on formatter called something like test_formatter and do a small change where the formatted would fail. Then I would open a PR from test_formatter towards formatter and it will run the code.

[rprichard]

I noticed that darker also accepts a -- argument before its file list, like git-clang-format does. Maybe we should add it for darker? It can be a separate PR.

---

Edit: I got darker and git-clang-format backwards.

[rprichard]

One of my test PRs was supposed to fail C++ and Python formatting, but instead it only failed on C++, and it reproduced the failure mode where darker simply ignores code formatting errors:

https://github.com/rprichard/llvm-project/actions/runs/6592874295/job/17914345865?pr=4

Apparently darker is succeeding with no stdout/stderr output?

The darker command fails for me when I run it locally:

darker --check --diff -r e9a6609b7d4aedffa9109bb26ce1bee31d92dbb6..88be1bb44e96c38f550da9385f89aa427d4360b5 llvm/utils/git/test1.py

I still think this CL is an improvement on the error reporting, though. (I assume the latest fixup commit is OK.)

I used these PRs to test this PR so far: rprichard#1 rprichard#2 rprichard#3 rprichard#4 rprichard#5

[tru]

One of my test PRs was supposed to fail C++ and Python formatting, but instead it only failed on C++, and it reproduced the failure mode where darker simply ignores code formatting errors:

https://github.com/rprichard/llvm-project/actions/runs/6592874295/job/17914345865?pr=4

Apparently darker is succeeding with no stdout/stderr output?

Hmm that doesn't seem right. I am pretty sure I had it failing with the right code before. Are you sure this isn't related to your changes?

[rprichard]

One of my test PRs was supposed to fail C++ and Python formatting, but instead it only failed on C++, and it reproduced the failure mode where darker simply ignores code formatting errors:
https://github.com/rprichard/llvm-project/actions/runs/6592874295/job/17914345865?pr=4
Apparently darker is succeeding with no stdout/stderr output?

Hmm that doesn't seem right. I am pretty sure I had it failing with the right code before. Are you sure this isn't related to your changes?

I don't think so? darker did the same thing to me on an actual PR, which motivated me to try and expand the error reporting. #69274. Maybe it's not checking newly added files?... I should see if I can get this to reproduce.

[rprichard]

It appears that darker/black process their file list using the currently-checked out files, not by querying git for the contents of the two git revisions. A newly-added file isn't checked out, because the current GitHub workflow checks something else out instead (the target branch of the PR maybe?)

https://github.com/rprichard/llvm-project/actions/runs/6622055992/job/17986970016?pr=10

I think the fix is to specify a different commit to checkout in pr-code-format.yml.

[rprichard]

These changes are pretty well-tested via https://github.com/rprichard/llvm-project/pulls.