This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by Quuxplusone on Apr 17 2021, 7:02 AM.

Details

Reviewers
ldionne
Mordante
Quuxplusone
Group Reviewers
Restricted Project
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

Diff Detail

Event Timeline

Quuxplusone requested review of this revision.Apr 17 2021, 7:02 AM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptApr 17 2021, 7:02 AM

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.

Poke buildkite (investigating)

libcxx/utils/ci/run-buildbot
145–146

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.

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.

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 added a comment.Apr 17 2021, 12:17 PM

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).

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

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

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

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 edited the summary of this revision. (Show Details)

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

Mordante accepted this revision as: Mordante.Apr 18 2021, 12:29 AM

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 accepted this revision.Apr 18 2021, 10:02 AM
Quuxplusone marked 2 inline comments as done.
Quuxplusone added inline comments.
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.

This revision is now accepted and ready to land.Apr 18 2021, 10:02 AM
Quuxplusone marked an inline comment as done.

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