[libcxx-commits] [PATCH] D116958: [libc++] Alphabetize CMakeLists.txt and module.modulemap; enforce in CI.

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone marked 3 inline comments as done.
Quuxplusone added inline comments.


================
Comment at: libcxx/test/libcxx/lint/lint_cmakelists.sh.py:3
+
+import os
+
----------------
arichardson wrote:
> 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.
FWLIW, I have no objection (or particular opinion) on that. :)


================
Comment at: libcxx/test/libcxx/lint/lint_cmakelists.sh.py:6
+
+if __name__ == '__main__':
+    libcxx_test_libcxx_lint = os.path.dirname(os.path.abspath(__file__))
----------------
ldionne wrote:
> Is this really useful? IMO it just has the effect of indenting everything artificially.
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.


================
Comment at: libcxx/test/libcxx/lint/lint_cmakelists.sh.py:7-10
+    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)
----------------
ldionne wrote:
> arichardson wrote:
> > 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.
> Any reason why you're not inferring this from `%{libcxx}` instead? It would future proof this test in case it gets moved around.
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.


================
Comment at: libcxx/test/libcxx/lint/lint_cmakelists.sh.py:7-13
+    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()
----------------
Quuxplusone wrote:
> ldionne wrote:
> > arichardson wrote:
> > > 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.
> > Any reason why you're not inferring this from `%{libcxx}` instead? It would future proof this test in case it gets moved around.
> 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.
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).



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116958/new/

https://reviews.llvm.org/D116958