[libc++] [CI] Fail if the headers contain cyclic dependencies

[• Quuxplusone]

Summary

Since we have a tool to detect cycles now; and since we're entering
a phase where people can easily introduce cycles by accident (D100682)
or by request (D90999), I think it's increasingly important to shift
the burden of detecting these cycles onto the buildbot instead of
the poor human reviewer.

The buildkite output from a "cyclic dependency" failure looks like this: https://buildkite.com/llvm-project/libcxx-ci/builds/2610#485cdaa4-03e9-47af-9878-9b14d2647a1d
The buildkite output from a "non-ASCII in the headers" failure looks like this: https://buildkite.com/llvm-project/libcxx-ci/builds/2608#8ae5209d-9f5d-459c-ac7a-271e93ae70c7

[Mordante]

htps://buildkite.com/llvm-project/libcxx-ci/builds/2595#c3483aa2-87b6-44fd-870e-4ce28b115ccc shows build errors. I assume these aren't the intended errors.
I would like to see how a build failure for this script is intended to look at the CI.

[• Quuxplusone]

Poke buildkite (investigating)

[• Quuxplusone]

libcxx/utils/ci/run-buildbot

148–149

https://buildkite.com/llvm-project/libcxx-ci/builds/2595#c3483aa2-87b6-44fd-870e-4ce28b115ccc shows build errors. I assume these aren't the intended errors. I would like to see how a build failure for this script is intended to look at the CI.

Yikes, that's interesting! 'ascii' codec can't decode byte 0xe2 in position 392 means we're getting some sort of UTF-8 (e2 is a common onset in UTF-8) somewhere in one of the header files. But I don't see any such thing on my local laptop. I'll investigate by spamming buildkite. Please ignore the next few pokes. :) Once I diagnose that issue, I'll cause the intended build failure and link you to it.

[• Quuxplusone]

Update: <type_traits> and maybe some other headers contain U+200B ZERO-WIDTH SPACE (attn: @cjdb). I'll just remove those characters. However, it does indicate that this will be a good check (at least once I improve the error message it gives ;)) because it'll incidentally help keep libc++ ASCII-friendly.

[tschuett]

The C++ draft has some of these lints:
https://github.com/cplusplus/draft/blob/master/tools/check-source.sh#L25-L37

# Find non-ASCII (Unicode) characters in the source
LC_ALL=C grep -ne '[^ -~	]' *.tex |
    fail 'non-ASCII character' || failed=1

# Trailing whitespace in a line.
grep -ne '\s$' *.tex |
    fail 'trailing whitespace' || failed=1

# Trailing empty lines
for f in *.tex; do
    [ $(tail -c 2 $f | wc -l) -eq 1 ] ||
	echo "$f" | fail 'trailing empty lines' || failed=1
done

[cjdb]

In D100703#2696809, @Quuxplusone wrote:

Update: <type_traits> and maybe some other headers contain U+200B ZERO-WIDTH SPACE (attn: @cjdb). I'll just remove those characters. However, it does indicate that this will be a good check (at least once I improve the error message it gives ;)) because it'll incidentally help keep libc++ ASCII-friendly.

I welcome this since U+200B ZERO-WIDTH SPACE isn't consistently copied in (and I rely on Phab + remembering to delete them).

[• Quuxplusone]

Poke buildkite, having removed invisible Unicode characters from the headers.

[tschuett]

I thought the Beyonce rule was: if you like a world without unicode characters, you need to put a CI test on it.

[• Quuxplusone]

Introduce three intentional errors for testing purposes: hard tab, Unicode, and cyclic dependency. Poke buildkite.

[• Quuxplusone]

The hard tab and UTF-8 character é are reported here: https://buildkite.com/llvm-project/libcxx-ci/builds/2608#8ae5209d-9f5d-459c-ac7a-271e93ae70c7

Poking buildkite to see what an intentional cyclic-dependency build failure looks like.

[• Quuxplusone]

Put everything back the way it was, and poke buildkite one last time. (This time it should succeed.)

[Mordante]

LGTM modulo some minor requests.

libcxx/utils/ci/run-buildbot

140

Can you change this to grep -rn to show the line number of the offending line.

149

The output now looks like

Cycle detected between /home/libcxx-builder/.buildkite-agent/builds/356c9f0d4df7-1/llvm-project/libcxx-ci/libcxx/include/__memory/shared_ptr.h and /home/libcxx-builder/.buildkite-agent/builds/356c9f0d4df7-1/llvm-project/libcxx-ci/libcxx/include/__memory/pointer_traits.h

Would it be easy to remove a part of the prefix to make the output easier to read? It would be great if we can omit the part identical to our current working directory.

[• Quuxplusone]

libcxx/utils/ci/run-buildbot

149

Turns out it's easy! I just reused the same function that strips prefixes for generating the graph's node labels.

[• Quuxplusone]

Landed — but my commit message said "Differential Review:" instead of "Differential Revision:" so it didn't get picked up by Phab. Oops.

libcxx/utils/ci/run-buildbot

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)
     clean
-    echo "+++ Checking the output of the generated scripts"
+    echo "+++ Checking the output of the generator scripts"
     mkdir -p ${BUILD_DIR}
+    # Reject patches that introduce non-ASCII characters or hard tabs.
+    ! grep -r '[^ -~]' 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
+    # 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

libcxx/utils/graph_header_deps.py

 #!/usr/bin/env python
 #===----------------------------------------------------------------------===##
 #
 # 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
 #
 #===----------------------------------------------------------------------===##
 
 import argparse
 import os
 import re
+import sys
 
 
 def is_config_header(h):
     return os.path.basename(h) in ['__config', '__libcpp_version']
 
 
 def is_experimental_header(h):
     return ('experimental/' in h) or ('ext/' in h)
def locate_header_file(h, paths):
                 return fullname
         if options.error_on_file_not_found:
             raise RuntimeError('Header not found: %s, included by %s' % (h, fname))
         return None
 
     local_includes = []
     system_includes = []
     linecount = 0
-    with open(fname, 'r') as f:
+    with open(fname, 'r', encoding='ascii') as f:
+        try:
             for line in f.readlines():
                 linecount += 1
                 m = re.match(r'\s*#\s*include\s+"([^"]*)"', line)
                 if m is not None:
                     local_includes.append(m.group(1))
                 m = re.match(r'\s*#\s*include\s+<([^>]*)>', line)
                 if m is not None:
                     system_includes.append(m.group(1))
+        except UnicodeDecodeError as e:
+            sys.stderr.write('Non-ASCII detected in %s\n' % fname)
+            raise
 
     fully_qualified_includes = [
         locate_header_file(h, options.search_dirs)
         for h in system_includes
     ] + [
         locate_header_file(h, os.path.dirname(fname))
         for h in local_includes
     ]
for fname in list(graph.keys()):
         else:
             del graph[fname]
 
     # Look for cycles.
     no_cycles_detected = True
     for fname, entry in graph.items():
         for h in entry.includes:
             if transitively_includes(graph, h, fname):
-                print('Cycle detected between %s and %s' % (fname, h))
+                sys.stderr.write('Cycle detected between %s and %s\n' % (fname, h))
                 no_cycles_detected = False
     assert no_cycles_detected
 
     print(get_graphviz(graph, options))