[libc++][CI] Fix check-generated-output.

[Mordante]

Summary

Before the script detected non-ASCII characters but let them pass. This
fixes the issue. I had a way to solve the issue, during review @Quuxplusone
suggested a better alternative. The patch has been changed to use this alternative.

Intended failed builds:

• Not updated generated files https://buildkite.com/llvm-project/libcxx-ci/builds/2822
• Not updated generated files and non-ASCII usage https://buildkite.com/llvm-project/libcxx-ci/builds/2835
• Non-ASCII usage https://buildkite.com/llvm-project/libcxx-ci/builds/2836

[Mordante]

Add a revert of 18b03b008588e829751ab4b945982ee301a58839
This should break the build with non-ASCII characters.

[• Quuxplusone]

libcxx/utils/ci/run-buildbot

10–11

@Mordante: Are you sure the script lets non-ASCII characters pass? My intent/understanding had been that set -e makes the build fail if any of these greps return nonzero; and I believe that that's what I was observing when I made the PR originally.

[Mordante]

Re-applied 18b03b008588e829751ab4b945982ee301a58839 and pretend to have completed std::format.

libcxx/utils/ci/run-buildbot

10–11

Yes I noticed the non-ASCII hyphen in some comment which was copied from the Standard. So I was surprised the build passed.
An example is this build https://buildkite.com/llvm-project/libcxx-ci/builds/2807#2ec7bc42-3bac-4a7b-98ac-c3b8d03390c7

[Mordante]

libcxx/utils/ci/run-buildbot

10–11

If you have a better suggestion how to solve the issue, I'm open to suggestions.

[• Quuxplusone]

libcxx/utils/ci/run-buildbot

10–11

Oh, gross.
https://unix.stackexchange.com/questions/65532/why-does-set-e-not-work-inside-subshells-with-parenthesis-followed-by-an-or/65564
https://stackoverflow.com/questions/57681955/set-e-does-not-respect-logical-not
The ! is flipping the exit code from 1 to 0 or vice versa (as expected) but also disabling the effects of set -e (not expected). Compare the effects of running this script:

set -e
false
echo hello  # never reached

versus this one:

set -e
! true
echo hello  # oops, yes reached

So, I'm down this rabbit hole now.
I don't want to just chain everything together with &&, though; that seems unscaleable, in that if anyone ever forgets to end a line with &&, things will silently break again. I think the right solution is to find the appropriate replacement for !.

Could you try this out?

# Check if the diffs are empty, fail otherwise.
! grep -q '^--- a' ${BUILD_DIR}/generated_output.patch || false
# Reject patches that introduce non-ASCII characters or hard tabs.
! grep -rn '[^ -~]' libcxx/include/ || false
# Check that no dependency cycles have been introduced.
python3 libcxx/utils/graph_header_deps.py >/dev/null

and obviously with a commit message that explains why the seemingly pointless || false is needed (if I'm right that it works at all, that is).

[Mordante]

Revert utils/ci/run-buildbot to its original state and try @Quuxplusone's suggestion.

[Mordante]

libcxx/utils/ci/run-buildbot

10–11

Thanks for the suggestion. I've started a build to test it. This looks more scalable and doesn't require to change the order so I like this a lot better.
If this works I'll add some comment why this is required.

[Mordante]

Test with non-ASCII and not updated generate scripts.

[Mordante]

Test with only non-ASCII to be sure.

[Mordante]

• Removes all test code.
• Restores all pipelines.
• Added documentation why the work-around is needed.

[• Quuxplusone]

libcxx/utils/ci/run-buildbot

140–151

I think I actually liked your original reordering; it puts the two || falses close together visually. Meanwhile, the comment can easily be 1 line instead of 7.

~~~
# Check if the diffs are empty, fail otherwise.
# "|| false" works around https://stackoverflow.com/questions/57681955/set-e-does-not-respect-logical-not
! grep -q '^--- a' ${BUILD_DIR}/generated_output.patch || false
# Reject patches that introduce non-ASCII characters or hard tabs.
! grep -rn '[^ -~]' libcxx/include/ || false
# Check that no dependency cycles have been introduced.
python3 libcxx/utils/graph_header_deps.py >/dev/null

WDYT?

[curdeius]

LGTM. I have no strong feelings about the order of commands (per Arthur's comment). Both are OK for me.

[Mordante]

Thanks for the reviews.

libcxx/utils/ci/run-buildbot

140–151

I reordered the code again.
I don't mind shortening the comment a bit, but I feel your version is too terse.
I changed it to two lines, including the stackoverflow link.

[Mordante]

Adresses the review comments.

libcxx/utils/ci/run-buildbot

 #!/usr/bin/env bash
 #===----------------------------------------------------------------------===##
 #
 # Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 # See https://llvm.org/LICENSE.txt for license information.
 # SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 #
 #===----------------------------------------------------------------------===##
 
 set -ex
 set -o pipefail
 
 PROGNAME="$(basename "${0}")"
 
 function usage() {
 cat <<EOF
 Usage:
 ${PROGNAME} [options] <BUILDER>
 
git-clang-format \
         -- \
             libcxx/{benchmarks,include,src,test} \
             libcxxabi/{fuzz,include,src,test} \
         | tee ${BUILD_DIR}/clang-format.patch
     # Check if the diff is empty, fail otherwise.
     ! grep -q '^--- a' ${BUILD_DIR}/clang-format.patch
 ;;
 check-generated-output)
+    # `! foo` doesn't work properly with `set -e`, use `! foo || false` instead.
+    # https://stackoverflow.com/questions/57681955/set-e-does-not-respect-logical-not
     clean
     echo "+++ Checking the output of the generator scripts"
     mkdir -p ${BUILD_DIR}
-    # Reject patches that introduce non-ASCII characters or hard tabs.
-    ! grep -rn '[^ -~]' libcxx/include/
     # Reject patches that don't update the generated output correctly.
     python3 libcxx/utils/generate_feature_test_macro_components.py
     python3 libcxx/utils/generate_header_inclusion_tests.py
     python3 libcxx/utils/generate_header_tests.py
     git diff | tee ${BUILD_DIR}/generated_output.patch
     # Check if the diffs are empty, fail otherwise.
-    ! grep -q '^--- a' ${BUILD_DIR}/generated_output.patch
+    ! grep -q '^--- a' ${BUILD_DIR}/generated_output.patch || false
+    # Reject patches that introduce non-ASCII characters or hard tabs.
+    ! grep -rn '[^ -~]' libcxx/include/ || false
     # Check that no dependency cycles have been introduced.
     python3 libcxx/utils/graph_header_deps.py >/dev/null
 ;;
 generic-cxx03)
     export CC=clang
     export CXX=clang++
     clean
     generate-cmake -C "${MONOREPO_ROOT}/libcxx/cmake/caches/Generic-cxx03.cmake"
     check-cxx-cxxabi
     check-abi-list