Page MenuHomePhabricator

[clangd] Move clangd tests to clangd directory. check-clangd is no longer part of check-clang-tools.
ClosedPublic

Authored by sammccall on Apr 26 2019, 7:25 AM.

Details

Summary

Motivation:

  • this layout is a pain to work with
  • without a common root, it's painful to express things like "disable clangd" (D61122)
  • CMake/lit configs are a maintenance hazard, and the more the one-off hacks for various tools are entangled, the more we see apathy and non-ownership.

This attempts to use the bare-minimum configuration needed (while still
supporting the difficult cases: windows, standalone clang build, dynamic libs).
In particular the lit.cfg.py and lit.site.cfg.py.in are merged into lit.cfg.in.
The logic in these files is now minimal.

(Much of clang-tools-extra's lit configs can probably be cleaned up by reusing
lit.llvm.llvm_config.use_clang(), and every llvm project does its own version of
LDPATH mangling. I haven't attempted to fix any of those).

Docs are still in clang-tools-extra/docs, I don't have any plans to touch those.

Diff Detail

Repository
rL LLVM

Event Timeline

sammccall created this revision.Apr 26 2019, 7:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 26 2019, 7:25 AM
sammccall added a comment.EditedApr 26 2019, 7:27 AM

@ilya-biryukov Can you check this with a shared library build?

Probably clangd, check-clang-tools, and check-clangd is enough.

ormris removed a subscriber: ormris.Apr 26 2019, 9:35 AM

Zap test/Unit directory, put lit.cfg in unittests directly.

For the record: the tests pass in the shared build configuration.

Also for the record, none of the buildbots in zorg mention check-clang-tools.

Bots run tests by enabling clang-tools-extra and then check-all, so this patch shouldn't change their behavior.

gribozavr accepted this revision.Apr 29 2019, 1:06 AM
This revision is now accepted and ready to land.Apr 29 2019, 1:06 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 29 2019, 1:42 AM
MaskRay added a comment.EditedApr 29 2019, 2:40 AM

@arphaman @jkorous @thakis I committed https://reviews.llvm.org/rL359428 to fix the following build error. Can one of you check if the test is still built? Thanks! (I don't have a Mac...)

    if config.clangd_xpc_support:                    
AttributeError: TestingConfig instance has no attribute 'clangd_xpc_support'                                    
                                                                       
FAILED: tools/clang/tools/extra/test/CMakeFiles/check-clang-tools
// clangd/test/lit.cfg.in     there is now:
if @CLANGD_BUILD_XPC@:
  config.available_features.add('clangd-xpc-support')

This is also making our Mac builders fail on configuration, and looks like a directory wasn't included properly when a test moved to a subdirectory -- would you mind taking a look? The CMake command we're running is at the top of the attached log.

CMake Error at /b/s/w/ir/k/llvm-project/clang-tools-extra/clangd/unittests/xpc/CMakeLists.txt:11 (add_extra_unittest):
  Unknown CMake command "add_extra_unittest".

Full Log: https://logs.chromium.org/logs/fuchsia/buildbucket/cr-buildbucket.appspot.com/8914851701929934272/+/steps/clang/0/steps/configure/0/stdout

I'm confused by some of the decisions made in this change, see below.

clang-tools-extra/trunk/clangd/test/lit.cfg.in
29

trailing newline

clang-tools-extra/trunk/clangd/unittests/lit.cfg.in
1

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.

11

Did you mean to commit this XXX?

23

Lots of trailing newlines.

thakis added inline comments.Apr 30 2019, 8:20 AM
clang-tools-extra/trunk/clangd/unittests/lit.cfg.in
1

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.

thakis added inline comments.Apr 30 2019, 8:33 AM
clang-tools-extra/trunk/clangd/test/CMakeLists.txt
5

Since you're touching this anyway, there's now one test that calls clang-indexer iirc.

MaskRay added inline comments.Apr 30 2019, 9:11 AM
clang-tools-extra/trunk/clangd/unittests/lit.cfg.in
1

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

sammccall marked 3 inline comments as done.Apr 30 2019, 9:44 AM
sammccall added inline comments.
clang-tools-extra/trunk/clangd/unittests/lit.cfg.in
1

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.

11

Yes. Probably should have been spelled FIXME.

23

Are these actually confusing, or significant in some way?

thakis added inline comments.Apr 30 2019, 9:50 AM
clang-tools-extra/trunk/clangd/unittests/lit.cfg.in
1

motivated by reducing the number of moving parts

That's a great motivation, and I agree the current state is hard to understand. But if every LLVM subsubproject does its own lit setup that they feel is simpler, the resulting whole will be even harder to understand than what we have now. Are you planning on changing the rest of c-t-e, clang, lld, llvm to use this new lit setup as well? If not, clangd should use the standard setup imho.

Can you elaborate, or point to some docs?

It's just that Windows uses file extensions for file type associations heavily. If the file is called foo.py, you can double-click it in explorer and an editor will appear. See revision history of all the lit.(site.)cfg.py files in c-t-e, clang, llvm, lld for Zach's patches to move things to this world.

AFAICS it never did for unit tests?

Yes, this only works for lit tests. LLVM in general tries to do most of its testing via lit tests.

In case anyone else runs into this: When doing incremental builds, the old ClangdTests binary stays around in ./tools/clang/tools/unittests/clangd and is still run by check-clang-tools. In case you don't want that (it makes the old test target slower; or maybe your local ClangdTests binary was broken due to local changes and now your failing tests don't go away if you undo your local changes), you have to manually delete the stale executable at the old location.

FYI, we had to disable clangd entirely after this patch, getting a weird problem with lit that we haven't figured out yet.
In case anybody else has seen something like this, and knows what's going on, please let us know.

  Running all regression tests
  llvm-lit.py: C:/j/w/opensource/opensource_to_sce_merge__2/o/llvm\utils\lit\lit\llvm\config.py:336: fatal: couldn't find 'clang' program, try setting CLANG in your environment
C:\Program Files (x86)\MSBuild\Microsoft.Cpp\v4.0\V140\Microsoft.CppCommon.targets(171,5): error MSB6006: "cmd.exe" exited with code 2. [C:\j\w\opensource\opensource_to_sce_merge__2\build\check-all.vcxproj]
dyung added a subscriber: dyung.May 1 2019, 4:46 PM

FYI, we had to disable clangd entirely after this patch, getting a weird problem with lit that we haven't figured out yet.
In case anybody else has seen something like this, and knows what's going on, please let us know.

  Running all regression tests
  llvm-lit.py: C:/j/w/opensource/opensource_to_sce_merge__2/o/llvm\utils\lit\lit\llvm\config.py:336: fatal: couldn't find 'clang' program, try setting CLANG in your environment
C:\Program Files (x86)\MSBuild\Microsoft.Cpp\v4.0\V140\Microsoft.CppCommon.targets(171,5): error MSB6006: "cmd.exe" exited with code 2. [C:\j\w\opensource\opensource_to_sce_merge__2\build\check-all.vcxproj]

To expand on this, I attempted to do a clean upstream build/test with a recent revision (r359727 if it matters), and I am seeing this error when attempting to use Visual Studio to build/test on Windows. Due to the nature of the problem, I suspect anyone using XCode to build/test would also run into a similar problem.

In my build, if I put a breakpoint in the script right before it emits the error and dump the path, the problem is pretty obvious in the first entry:

'c:\\src\\upstream\\359727-win32-release\\%(build_mode)s\\bin

This appears to be a cmake related problem because the "%(build_mode)s" should have been replaced by "Release" in this case. These kinds of problems often only manifest when building using Visual Studio or XCode since the build configuration isn't set until build time.

Both the clangd and clangd unit tests seem to have this problem. The problem is not eliminated until I remove both sets of tests from the command line used to run the tests. Unfortunately I don't really have the expertise to fix these problems, but maybe someone else does. But until it is fixed, testing of clangd with either Visual Studio or XCode is likely broken.

So, apologies everyone for the trouble caused by this patch, and thanks for all the information.
It was a mistake to try to change so many things at once. I'll switch the layout back as far as possible while preserving the directory structure.

This is a frustrating as the complexity here has been causing real problems. For clangd, the costs seem to outweigh the benefits (such as re-running lit tests and supporting visual studio project generation). But I accept that this is something that each project doesn't get to decide, and I'll need to accept these decisions or push to move clangd out of LLVM.