[libc++] Improve the output of the generated-output CI job

[ldionne]

The step that checked for ignore_format.txt being consistent with the tree wouldn't print any explicit diagnostic when failing, which led to confusion. After this patch, an explicit diagnostic will be printed by the job along with the required diff to ignore_format.txt.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Louis Dionne (ldionne)

Changes

The step that checked for ignore_format.txt being consistent with the tree wouldn't print any explicit diagnostic when failing, which led to confusion. After this patch, an explicit diagnostic will be printed by the job along with the required diff to ignore_format.txt.

---

Full diff: https://github.com/llvm/llvm-project/pull/68903.diff

1 Files Affected:

• (modified) libcxx/utils/ci/run-buildbot (+10-5)

diff --git a/libcxx/utils/ci/run-buildbot b/libcxx/utils/ci/run-buildbot
index a71318123db3b12..b5c48568c995e3c 100755
--- a/libcxx/utils/ci/run-buildbot
+++ b/libcxx/utils/ci/run-buildbot
@@ -209,6 +209,8 @@ check-generated-output)
     clean
     generate-cmake
 
+    set +x # Printing all the commands below just creates extremely confusing output
+
     # Reject patches that forgot to re-run the generator scripts.
     echo "+++ Making sure the generator scripts were run"
     ${NINJA} -vC "${BUILD_DIR}" libcxx-generate-files
@@ -222,20 +224,23 @@ check-generated-output)
         false
     fi
 
+    echo "+++ Making sure libcxx/utils/data/ignore_format.txt was updated appropriately"
+    cp ${MONOREPO_ROOT}/libcxx/utils/data/ignore_format.txt ${BUILD_DIR}/before.txt
     ${MONOREPO_ROOT}/libcxx/utils/generate_ignore_format.sh
-    git diff | tee ${BUILD_DIR}/generated_output.patch
-    git ls-files -o --exclude-standard | tee ${BUILD_DIR}/generated_output.status
-    ! grep -q '^--- a' ${BUILD_DIR}/generated_output.patch || false
-    if [ -s ${BUILD_DIR}/generated_output.status ]; then
+    diff ${BUILD_DIR}/before.txt ${MONOREPO_ROOT}/libcxx/utils/data/ignore_format.txt | tee ${BUILD_DIR}/ignore_format.diff || true
+    if [ -s ${BUILD_DIR}/ignore_format.diff ]; then
         echo "It looks like the list of not formatted files has changed."
         echo "If a file is now properly formatted with clang-format, remove the file name from "
         echo "libcxx/utils/data/ignore_format.txt. Otherwise you have to fix the"
-        echo "formatting of some of the changed files."
+        echo "formatting of some of the changed files. The diff above represents the "
+        echo "changes that would be needed to ignore_format.txt to keep it representative "
+        echo "of which files are mis-formatted in the project."
         false
     fi
 
     # Reject patches that introduce non-ASCII characters or hard tabs.
     # Depends on LC_COLLATE set at the top of this script.
+    set -x
     ! grep -rn '[^ -~]' libcxx/include libcxx/src libcxx/test libcxx/benchmarks \
            --exclude '*.dat' \
            --exclude '*unicode*.cpp' \

[EricWF]

Thanks for this. That said, we should be checking the diff, not the entire tree.

I think that until we can get this to stop spuriously failing, it needs to stop breaking the CI. Can we also turn this to not block the remainder of the checks?

It's not clear to me that this should even be a blocking check (it certainly didn't block the broken change from getting checked in.

[EricWF]

This is better, so that's good.

But I don't think we should be blocking changes based off formatting. Or perhaps said a better way, until this check passes on any change that has has clang-format applied, it shouldn't be on.

[EricWF]

As mentioned in libc++ chat, there's already a code format checker running on our changes. And it completes in 1 minute as opposed to 2+ hours of queuing.

(See this change for an example of that format check).

[ldionne]

The GH Actions checks that the files are formatted properly. The generated-files check modified by this PR checks that we maintain an up-to-date ignore_format.txt file -- those have different purposes.

I agree it's terrible to wait 2hrs only for this check to fail. I am working on moving that to a GH action here: ldionne#2