Differential D116958
[libc++] Alphabetize CMakeLists.txt and module.modulemap; enforce in CI. Authored by • Quuxplusone on Jan 10 2022, 10:27 AM.
Details Following up on @philnik's suggestion in D116809. Is this too much extra bureaucratic code for too little gain? I'm not sure, but it doesn't seem like a crazy amount of bureaucratic code. Here, as in D116809, I'm assuming that Python3 string comparisons (for ASCII characters) are sane and unaffected by locale in any way that would harm us. It'd be awesome if some Python guru could confirm or deny that.
Diff Detail
Event TimelineComment Actions I would like to suggest a different approach, which would make sure that these tests are executed all the time, not only in the CI. You could create a file like libcxx/test/libcxx/lint_cmakelist.sh.py with something like this inside it: # RUN: %{python} %s '%{libcxx}'
libcxx = sys.argv[1]
cmakelist = os.path.join(libcxx, 'include', 'CMakeLists.txt')
assert os.path.exists(cmakelist)
with open(cmakelist, 'r') as f:
lines = f.readlines()
assert lines[0] == 'set(files\n'
okay = True
prevline = lines[1]
for line in lines[2:]:
if (line == ' )\n'):
break
if (line < prevline):
okay = False
print('LINES OUT OF ORDER in libcxx/include/CMakeLists.txt!')
print(prevline)
print(line)
prevline = line
assert okayThen, this would be executed whenever we run the tests. That would require moving the %{python}-related logic in libcxx/test/libcxx/selftest/dsl/lit.local.cfg to libcxx/test/libcxx.
Comment Actions I'm gonna land the NFC changes to CMakeLists.txt and module.modulemap, just to reduce merge-conflict potential; and then keep this open to explore Louis's test approach.
Comment Actions Will that work as expected with remote executors? I don't believe the sources are guaranteed to be accessible? Maybe it would also make sense to add a .arclint file to run these scripts when uploading patches with arc (in addition to the CI check). Comment Actions Experimenting with @ldionne's suggested approach. This works for me locally; let's see if buildkite likes it. I have temporarily added some misalphabetizations in all the places that should be caught by these tests; therefore I expect 3 tests to fail in buildkite, with nice helpful messages identifying all the misalphabetizations. FWIW, I don't know if @arichardson's concern is justified or not. The easiest way to find out might be to ship this and see if it breaks any buildbots? Comment Actions The buildkite failure output looks like I expected — good! https://buildkite.com/llvm-project/libcxx-ci/builds/7742 Remove the intentional errors; CI should become green now. @ldionne, if this is green shall I land it? Or are we worried about places where CI is run without access to the original sources? (In those cases, if they exist, maybe we can figure out how to XFAIL this test.)
Comment Actions In the past I have regularly been running libc++ tests with remote executors where the source code is not available on the system that runs the tests, so I'm almost certain this would fail. I haven't done this for a while so I can't remember if there is a "remote-executor" or similar lit feature that can be used for UNSUPPORTED. Comment Actions This test is run only on the build host, not on the remote target. So this would work as long as the build host has Python (which it has to because it must run Lit). This wouldn't work if we used %{exec} %{python} ..., which would try to run on the remote target.
Comment Actions For each of these tests, can you please add a short comment saying something like # This test checks that include files in the CMakeLists.txt are sorted alphabetically, and similarly for the other ones.
| |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Note that the fact that we'd be running tests as part of a target that generates files is what got me to suggest the alternative approach with a shell test.