This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Alphabetize CMakeLists.txt and module.modulemap; enforce in CI.
ClosedPublic

Authored by Quuxplusone on Jan 10 2022, 10:27 AM.

Details

Summary

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 Timeline

Quuxplusone requested review of this revision.Jan 10 2022, 10:27 AM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald Transcript

LGTM, assuming locale doesn't mess with your code.

philnik accepted this revision.Jan 10 2022, 11:23 AM
ldionne requested changes to this revision.Jan 10 2022, 12:21 PM

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 okay

Then, 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.

This revision now requires changes to proceed.Jan 10 2022, 12:21 PM
ldionne added inline comments.Jan 10 2022, 12:22 PM
libcxx/utils/CMakeLists.txt
31–32 ↗(On Diff #398690)

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.

Quuxplusone planned changes to this revision.Jan 10 2022, 12:36 PM

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.

libcxx/utils/CMakeLists.txt
31–32 ↗(On Diff #398690)

The logic of what I did is that I think of this target not as "generating files" per se, but rather "restoring repo invariants (such as 'every detail header has a submodule test') which will plausibly be broken on a regular basis if we're not careful." Anyone who adds a new file will have to run ninja libcxx-generate-files (or run the equivalent Python scripts manually) to restore the repo invariants. They won't necessarily think or care to run all of the llvm-lit tests in addition.

However, making this a .sh.py test might produce more understandable output on failure. "This test failed" is easier to understand than "some random build step seems to have returned non-zero." So that's what I like about your alternative approach. (It looks like test/libcxx/selftest/dsl/dsl.sh.py is the only .sh.py test in the repo right now, but that's at least a non-zero amount of precedent for adding more.)

The other good thing about your approach is that it moves these scripts into our existing place for "dump random code to test random things" — the test suite — as opposed to bloating libcxx/utils/ and potentially making it harder to navigate.

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 okay

Then, 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.

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

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?

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

arichardson added inline comments.Jan 12 2022, 7:21 AM
libcxx/test/libcxx/lint/lint_cmakelists.sh.py
3

I just discovered https://github.com/cheshirekow/cmake_format which does things like sorting source lists. I wonder if using that in the future would make sense? I just tried running cmake-format on some of the files and it doesn't seem too bad although the defaults probably need tweaking to avoid unnecessary churn.

7–13

I find python path manipulation a bit easier to read with from pathlib import Path and we depend on 3.5 (or 3.6?) now so it can be used.

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

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.

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.

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.

libcxx/test/libcxx/lint/lint_cmakelists.sh.py
6

Is this really useful? IMO it just has the effect of indenting everything artificially.

7–10

Any reason why you're not inferring this from %{libcxx} instead? It would future proof this test in case it gets moved around.

Quuxplusone marked 3 inline comments as done.Jan 12 2022, 12:20 PM
Quuxplusone added inline comments.
libcxx/test/libcxx/lint/lint_cmakelists.sh.py
3

FWLIW, I have no objection (or particular opinion) on that. :)

6

No more than int main() { in C++. :) In normal Python programs, this idiom prevents the script from auto-running and having crazy side effects when you import lint_cmakelists (not that anyone should ever do that). Anyway, it's consistent with libcxx/utils/*.py, and with my prejudices, so I propose to keep it.

7–10

I tried your suggested %{python} %s '%{libcxx}' originally, but it failed for me (locally) in a way that clearly implied that %{libcxx} was getting passed as a literal instead of substituted with anything. So I changed it to "%{libcxx}" with double quotes, but it still failed; and then I think I also tried with no quotes at all; but basically I couldn't figure out how to make %{libcxx} work no matter what I did. This way seems easy enough IMHO.

7–13

Maybe I'm a Luddite: I figure the os.path stuff isn't any worse — except for the '..', '..', '..' which I'd be happy to replace with '../../..'. Python and C++ both suffer from the silliness of libraries pretending / doesn't work on Windows (when of course it does).

ldionne accepted this revision.Jan 12 2022, 1:24 PM

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.

libcxx/test/libcxx/lint/lint_cmakelists.sh.py
6

Yeah, I'm aware of the convention for normal Python scripts, but I don't see the point here.

I'll make my vote neutral, so keep it if you want.

libcxx/test/libcxx/lint/lit.local.cfg
3

Can you try adding print(config.substitutions) here and checking whether %{libcxx} is in there somewhere? It should.

This revision is now accepted and ready to land.Jan 12 2022, 1:24 PM
Quuxplusone marked 3 inline comments as done.Jan 12 2022, 3:41 PM
Quuxplusone added inline comments.
libcxx/test/libcxx/lint/lit.local.cfg
3
[('%{cxx}', '/Users/aodwyer/llvm-project/build/bin/clang++'),
 ('%{flags}', '-isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX10.15.sdk -v --target=x86_64-apple-darwin19.6.0'),
 ('%{compile_flags}', '-include /Users/aodwyer/llvm-project/libcxx/test/support/nasty_macros.h -nostdinc++ -I/Users/aodwyer/llvm-project/build2/include/c++/v1 -I/Users/aodwyer/llvm-project/build2/projects/libcxx/include/c++build -I/Users/aodwyer/llvm-project/libcxx/test/support -std=c++2b -fmodules -Xclang -fmodules-local-submodule-visibility -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-atomic-alignment -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER'),
 ('%{link_flags}', '-lc++experimental -L/Users/aodwyer/llvm-project/build2/./lib -Wl,-rpath,/Users/aodwyer/llvm-project/build2/./lib -L/Users/aodwyer/llvm-project/build2/./lib -Wl,-rpath,/Users/aodwyer/llvm-project/build2/./lib -nodefaultlibs -lc++ -lSystem'),
 ('%{install}', '/Users/aodwyer/llvm-project/build2'),
 ('%{exec}', '"/Users/aodwyer/env3/bin/python3.9" /Users/aodwyer/llvm-project/libcxx/test/../utils/run.py --execdir %T --codesign_identity "" --env  -- '),
 ('%{triple}', 'x86_64-apple-darwin19.6.0'),
 ('%{python}', '/Users/aodwyer/env3/bin/python3.9')]

Nope, nothing. Could this be due to my running llvm-lit manually (./bin/llvm-lit -sv --param std=c++2b --param enable_modules=True ../libcxx/test/libcxx/lint)? Could this be due to my build directory still having been cmake'd with LLVM_ENABLE_PROJECTS instead of LLVM_ENABLE_RUNTIMES?

ldionne added inline comments.Jan 18 2022, 7:50 AM
libcxx/test/libcxx/lint/lit.local.cfg
3

I just realized the legacy testing config was still being used by default. I'm 99% sure that's why, I don't think the legacy testing config is used cmake-bridge.cfg.in.