sammccall marked 3 inline comments as done. sammccall added inline comments.
================ Comment at: clang-tools-extra/trunk/clangd/unittests/lit.cfg.in:1 +@LIT_SITE_CFG_IN_HEADER@ +# This is a shim to run the gtest unittests in ../unittests using lit. ---------------- MaskRay wrote: > thakis wrote: > > thakis wrote: > > > Every other LLVM project puts the lit.cfg.in for the unit tests in > > > foo/test/Unit/lit.cfg.in. Any reason why clangd is different? > > > > > > Also, almost all LLVM projects call their lit.cfg lit.cfg.py and this > > > file lit.cfg.py.in since it helps Windows. Can you rename the > > > lit.cfg(.in) files you added to have .py in them please? > > > > > > Also, why did you merge the site.cfg.py.in file with the non-generated > > > non-site file? This too is different from all other LLVM projects. > > One consequence of these differences (except things being harder to > > understand) is that running `bin/llvm-lit > > ../llvm-project/clang-tools-extra/clangd/test` doesn't do anything, and > > running a single test that way doesn't work either. This works fine for > > individual llvm, clang, lld, clang-tools-extra tests and it used to work > > for clangd tests. > I know little about lit configurations but not being able to run a single > test (`$build/bin/llvm-lit clangd/test/path/to/some/test`) seems a big > downside.. > > (offtopic: compiler-rt tests have two modes: i386 and x86_64. I wonder if > llvm-lit can run a test in the i386 mode (if it defaults to x86_64)) > Every other LLVM project puts the lit.cfg.in for the unit tests in > foo/test/Unit/lit.cfg.in. Any reason why clangd is different? ... Also, why > did you merge the site.cfg.py.in Most of these changes were motivated by reducing the number of moving parts, which inhibit understanding. It's likely to be harder to understand if you're knowledgeable about the way things are done in LLVM. My reasoning in weighing simplicity over matching LLVM precisely: - the overlap in active contributors appears fairly small (maybe I'm missing other ways people interact with the code) - average understanding of the fairly elaborate LLVM setup seems fairly low, I would describe the median response to e.g. the `Unit` directory as "baffled". > Also, almost all LLVM projects call their lit.cfg lit.cfg.py and this file > lit.cfg.py.in since it helps Windows Can you elaborate, or point to some docs? > One consequence of these differences (except things being harder to > understand) is that running `bin/llvm-lit > ../llvm-project/clang-tools-extra/clangd/test` doesn't do anything Indeed. This works from the build directory though, which is where we typically run tests. It also works for `.../unittests`, which is (IMO) much more obvious than `.../test/Unit`. > and running a single test that way doesn't work either AFAICS it never did for unit tests? lit tests are not a significant fraction of our tests. ================ Comment at: clang-tools-extra/trunk/clangd/unittests/lit.cfg.in:11 +# Point the dynamic loader at dynamic libraries in 'lib'. +# XXX: it seems every project has a copy of this logic. Move it somewhere. +import platform ---------------- thakis wrote: > Did you mean to commit this XXX? Yes. Probably should have been spelled FIXME. ================ Comment at: clang-tools-extra/trunk/clangd/unittests/lit.cfg.in:23 + + ---------------- thakis wrote: > Lots of trailing newlines. Are these actually confusing, or significant in some way? Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61187/new/ https://reviews.llvm.org/D61187 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
