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

[• Quuxplusone]

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.

[philnik]

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

[ldionne]

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.

[ldionne]

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]

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.

[arichardson]

In D116958#3232507, @ldionne wrote:

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

[• Quuxplusone]

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?

[ChuanqiXu]

LGTM

[• Quuxplusone]

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]

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

4

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.

8–14

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.

[arichardson]

In D116958#3234252, @Quuxplusone wrote:

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.

[ldionne]

In D116958#3237512, @arichardson wrote:

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

7

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

8–11

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

[• Quuxplusone]

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

4

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

7

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.

8–11

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.

8–14

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]

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

7

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

4

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

[• Quuxplusone]

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

4

[('%{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]

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

4

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.

libcxx/include/__debug

 #pragma GCC system_header
 #endif
 
 #if defined(_LIBCPP_HAS_NO_NULLPTR)
 # include <cstddef>
 #endif
 
 #if _LIBCPP_DEBUG_LEVEL >= 1 || defined(_LIBCPP_BUILDING_LIBRARY)
-#   include <cstdlib>
-#   include <cstdio>
 #   include <cstddef>
+#   include <cstdio>
+#   include <cstdlib>
 #endif
 
 #if _LIBCPP_DEBUG_LEVEL == 0
 #   define _LIBCPP_DEBUG_ASSERT(x, m) ((void)0)
 #   define _LIBCPP_ASSERT_IMPL(x, m) ((void)0)
 #elif _LIBCPP_DEBUG_LEVEL == 1
 #   define _LIBCPP_DEBUG_ASSERT(x, m) ((void)0)
 #   define _LIBCPP_ASSERT_IMPL(x, m) ((x) ? (void)0 : _VSTD::__libcpp_debug_function(_VSTD::__libcpp_debug_info(__FILE__, __LINE__, #x, m)))

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

+# RUN: %{python} %s
+
+# Verify that libcxx/include/CMakeLists.txt's list of header files
+# is maintained in alphabetical order.
+
+import os
+
+
+if __name__ == '__main__':
+    libcxx_test_libcxx_lint = os.path.dirname(os.path.abspath(__file__))
+    libcxx = os.path.abspath(os.path.join(libcxx_test_libcxx_lint, '../../..'))
+    cmakelists_name = os.path.join(libcxx, 'include/CMakeLists.txt')
+    assert os.path.isfile(cmakelists_name)
+
+    with open(cmakelists_name, '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

libcxx/test/libcxx/lint/lint_headers.sh.py

+# RUN: %{python} %s
+
+# Verify that each run of consecutive #include directives
+# in each libcxx/include/ header is maintained in alphabetical order.
+
+import glob
+import os
+import re
+
+
+def exclude_from_consideration(path):
+    return (
+        path.endswith('.txt') or
+        path.endswith('.modulemap') or
+        os.path.basename(path) == '__config' or
+        os.path.basename(path) == '__locale' or
+        not os.path.isfile(path)
+    )
+
+
+if __name__ == '__main__':
+    libcxx_test_libcxx_lint = os.path.dirname(os.path.abspath(__file__))
+    libcxx_include = os.path.abspath(os.path.join(libcxx_test_libcxx_lint, '../../../include'))
+    assert os.path.isdir(libcxx_include)
+
+    def pretty(path):
+        return path[len(libcxx_include) + 1:]
+
+    all_headers = [
+        p for p in (
+            glob.glob(os.path.join(libcxx_include, '*')) +
+            glob.glob(os.path.join(libcxx_include, '__*/*.h'))
+        ) if not exclude_from_consideration(p)
+    ]
+
+    okay = True
+    for fname in all_headers:
+        with open(fname, 'r') as f:
+            lines = f.readlines()
+        # Examine each consecutive run of #include directives.
+        prevline = None
+        for line in lines:
+            if re.match(r'^\s*#\s*include ', line):
+                if (prevline is not None) and (line < prevline):
+                    okay = False
+                    print('LINES OUT OF ORDER in libcxx/include/%s!' % pretty(fname))
+                    print(prevline)
+                    print(line)
+                prevline = line
+            else:
+                prevline = None
+    assert okay

libcxx/test/libcxx/lint/lint_modulemap.sh.py

+# RUN: %{python} %s
+
+# Verify that each list of private submodules in libcxx/include/module.modulemap
+# is maintained in alphabetical order.
+
+import os
+import re
+
+
+if __name__ == '__main__':
+    libcxx_test_libcxx_lint = os.path.dirname(os.path.abspath(__file__))
+    libcxx = os.path.abspath(os.path.join(libcxx_test_libcxx_lint, '../../..'))
+    modulemap_name = os.path.join(libcxx, 'include/module.modulemap')
+    assert os.path.isfile(modulemap_name)
+
+    okay = True
+    prevline = None
+    with open(modulemap_name, 'r') as f:
+        for line in f.readlines():
+            if re.match(r'^\s*module.*[{]\s*private', line):
+                if (prevline is not None) and (line < prevline):
+                    okay = False
+                    print('LINES OUT OF ORDER in libcxx/include/module.modulemap!')
+                    print(prevline)
+                    print(line)
+                prevline = line
+            else:
+                prevline = None
+    assert okay

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

+# The tests in this directory need to run Python
+import pipes, sys
+config.substitutions.append(('%{python}', pipes.quote(sys.executable)))