This is an archive of the discontinued LLVM Phabricator instance.

Differential D61187

[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

Reviewers
gribozavr
Commits
rCTE359424: [clangd] Move clangd tests to clangd directory. check-clangd is no longer part…
rGb804eef09052: [clangd] Move clangd tests to clangd directory. check-clangd is no longer part…
rL359424: [clangd] Move clangd tests to clangd directory. check-clangd is no longer part…
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
Herald added subscribers: cfe-commits, jfb, kadircet and 5 others. · View Herald Transcript
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.

Herald added a subscriber: ormris. · View Herald TranscriptApr 26 2019, 7:27 AM
Harbormaster completed remote builds in B31032: Diff 196848.Apr 26 2019, 7:27 AM
ormris removed a subscriber: ormris.Apr 26 2019, 9:35 AM
sammccall updated this revision to Diff 197058.Apr 29 2019, 12:20 AM

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

Harbormaster completed remote builds in B31081: Diff 197058.Apr 29 2019, 12:20 AM
ilya-biryukov added a comment.Apr 29 2019, 12:22 AM

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

sammccall added a comment.Apr 29 2019, 12:29 AM

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
Closed by commit rL359424: [clangd] Move clangd tests to clangd directory. check-clangd is no longer part… (authored by sammccall). · Explain WhyApr 29 2019, 1:42 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
Herald added a subscriber: llvm-commits. · View Herald Transcript
MaskRay mentioned this in rCTE359428: [clangd] Delete config.clangd_xpc_support from test/ to unbreak check-llvm-tools.Apr 29 2019, 2:35 AM
MaskRay mentioned this in rL359428: [clangd] Delete config.clangd_xpc_support from test/ to unbreak check-llvm-tools.
MaskRay mentioned this in rG16a2131c6229: [clangd] Delete config.clangd_xpc_support from test/ to unbreak check-llvm-tools.
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')
juliehockett added a subscriber: juliehockett.Apr 29 2019, 10:01 AM

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

jkorous added a comment.Apr 29 2019, 10:20 AM

@MaskRay @juliehockett I'll take a look.

jkorous added a comment.Apr 29 2019, 11:15 AM

Patch with fix for XPC tests https://reviews.llvm.org/D61271

thakis added a comment.Apr 30 2019, 7:45 AM

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.

thakis added a comment.May 1 2019, 8:35 AM

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.

probinson added a subscriber: probinson.May 1 2019, 9:07 AM

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
In D61187#1486249, @probinson wrote:

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.

sammccall added a comment.May 1 2019, 11:49 PM

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.

Large Diff

This large diff affects 216 files. Files without inline comments have been collapsed. Expand All Files

Revision Contents

PathSize
clang-tools-extra/
trunk/
12 lines
clangd/
20 lines
test/
28 lines
Inputs/
19 lines
1 line
background-index/
5 lines
51 lines
4 lines
2 lines
7 lines
17 lines
20 lines
29 lines
106 lines
56 lines
72 lines
34 lines
11 lines
45 lines
48 lines
55 lines
56 lines
68 lines
6 lines
4 lines
13 lines
132 lines
212 lines
69 lines
187 lines
24 lines
6 lines
12 lines
53 lines
21 lines
17 lines
29 lines
6 lines
110 lines
40 lines
39 lines
27 lines
13 lines
14 lines
84 lines
11 lines
11 lines
39 lines
7 lines
28 lines
50 lines
92 lines
16 lines
32 lines
xpc/
10 lines
92 lines
unittests/
39 lines
53 lines
465 lines
92 lines
65 lines
1162 lines
86 lines
2523 lines
160 lines
56 lines
753 lines
652 lines
347 lines
153 lines
50 lines
123 lines
371 lines
688 lines
51 lines
312 lines
151 lines
279 lines
253 lines
408 lines
205 lines
199 lines
102 lines
483 lines
37 lines
259 lines
219 lines
395 lines
1272 lines
339 lines
59 lines
151 lines
710 lines
73 lines
129 lines
57 lines
118 lines
76 lines
153 lines
64 lines
127 lines
190 lines
455 lines
187 lines
1504 lines
23 lines
xpc/
21 lines
35 lines
test/
34 lines
clangd/
Inputs/
19 lines
1 line
background-index/
5 lines
51 lines
4 lines
2 lines
7 lines
17 lines
Unit/
9 lines
20 lines
29 lines
106 lines
56 lines
72 lines
34 lines
11 lines
45 lines
48 lines
55 lines
56 lines
68 lines
6 lines
4 lines
13 lines
132 lines
212 lines
69 lines
187 lines
24 lines
6 lines
12 lines
53 lines
21 lines
17 lines
6 lines
110 lines
40 lines
39 lines
27 lines
13 lines
14 lines
84 lines
11 lines
11 lines
39 lines
7 lines
28 lines
50 lines
92 lines
16 lines
32 lines
xpc/
10 lines
92 lines
unittests/
10 lines
clangd/
39 lines
53 lines
465 lines
82 lines
65 lines
1162 lines
86 lines
2523 lines
160 lines
56 lines
753 lines
652 lines
347 lines
153 lines
50 lines
123 lines
371 lines
688 lines
51 lines
312 lines
151 lines
279 lines
253 lines
408 lines
205 lines
199 lines
102 lines
483 lines
37 lines
259 lines
219 lines
395 lines
1272 lines
339 lines
59 lines
151 lines
710 lines
73 lines
129 lines
57 lines
118 lines
76 lines
153 lines
64 lines
127 lines
190 lines
455 lines
187 lines
1504 lines
xpc/
21 lines
35 lines

Diff 197065

clang-tools-extra/trunk/CMakeLists.txt

clang-tools-extra/trunk/clangd/CMakeLists.txt

clang-tools-extra/trunk/clangd/test/CMakeLists.txt

set(CLANGD_TEST_DEPS
clangd
ClangdTests
# No tests for these, but we should still make sure they build.
clangd-indexer
thakisUnsubmitted
Not Done
ReplyInline Actions

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

thakis: Since you're touching this anyway, there's now one test that calls clang-indexer iirc.
dexp
)
if(CLANGD_BUILD_XPC)
list(APPEND CLANGD_TEST_DEPS clangd-xpc-test-client)
endif()
foreach(dep FileCheck count not)
if(TARGET ${dep})
list(APPEND CLANGD_TEST_DEPS ${dep})
endif()
endforeach()
configure_lit_site_cfg(
${CMAKE_CURRENT_SOURCE_DIR}/lit.cfg.in
${CMAKE_CURRENT_BINARY_DIR}/lit.cfg)
configure_lit_site_cfg(
${CMAKE_CURRENT_SOURCE_DIR}/../unittests/lit.cfg.in
${CMAKE_CURRENT_BINARY_DIR}/../unittests/lit.cfg)
add_lit_testsuite(check-clangd "Running the Clangd regression tests"
${CMAKE_CURRENT_BINARY_DIR}/Unit;${CMAKE_CURRENT_BINARY_DIR}
DEPENDS ${CLANGD_TEST_DEPS})

clang-tools-extra/trunk/clangd/test/Inputs/BenchmarkHeader.h

clang-tools-extra/trunk/clangd/test/Inputs/BenchmarkSource.cpp

clang-tools-extra/trunk/clangd/test/Inputs/background-index/compile_commands.json

clang-tools-extra/trunk/clangd/test/Inputs/background-index/definition.jsonrpc

clang-tools-extra/trunk/clangd/test/Inputs/background-index/foo.h

clang-tools-extra/trunk/clangd/test/Inputs/background-index/foo.cpp

clang-tools-extra/trunk/clangd/test/Inputs/requests.json

clang-tools-extra/trunk/clangd/test/Inputs/symbols.test.yaml

clang-tools-extra/trunk/clangd/test/background-index.test

clang-tools-extra/trunk/clangd/test/compile-commands-path-in-initialize.test

clang-tools-extra/trunk/clangd/test/completion-auto-trigger.test

clang-tools-extra/trunk/clangd/test/completion-snippets.test

clang-tools-extra/trunk/clangd/test/completion.test

clang-tools-extra/trunk/clangd/test/crash-non-added-files.test

clang-tools-extra/trunk/clangd/test/delimited-input-comment-at-the-end.test

clang-tools-extra/trunk/clangd/test/diagnostic-category.test

clang-tools-extra/trunk/clangd/test/diagnostics-notes.test

clang-tools-extra/trunk/clangd/test/diagnostics.test

clang-tools-extra/trunk/clangd/test/did-change-configuration-params.test

clang-tools-extra/trunk/clangd/test/execute-command.test

clang-tools-extra/trunk/clangd/test/exit-with-shutdown.test

clang-tools-extra/trunk/clangd/test/exit-without-shutdown.test

clang-tools-extra/trunk/clangd/test/filestatus.test

clang-tools-extra/trunk/clangd/test/fixits-codeaction.test

clang-tools-extra/trunk/clangd/test/fixits-command.test

clang-tools-extra/trunk/clangd/test/fixits-embed-in-diagnostic.test

clang-tools-extra/trunk/clangd/test/formatting.test

clang-tools-extra/trunk/clangd/test/hover.test

clang-tools-extra/trunk/clangd/test/index-tools.test

clang-tools-extra/trunk/clangd/test/initialize-params-invalid.test

clang-tools-extra/trunk/clangd/test/initialize-params.test

clang-tools-extra/trunk/clangd/test/initialize-sequence.test

clang-tools-extra/trunk/clangd/test/input-mirror.test

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

@LIT_SITE_CFG_IN_HEADER@
import lit.llvm
import lit.formats
# Reuse clang configuration (PATH setup, etc).
config.clang_tools_dir = "@CLANG_TOOLS_DIR@"
config.llvm_tools_dir = "@LLVM_TOOLS_DIR@"
config.clang_libs_dir = "@CLANG_LIBS_DIR@"
config.llvm_libs_dir = "@LLVM_LIBS_DIR@"
config.target_triple = "@TARGET_TRIPLE@"
config.host_triple = "@LLVM_HOST_TRIPLE@"
lit.llvm.initialize(lit_config, config)
lit.llvm.llvm_config.use_clang()
config.name = 'Clangd'
config.suffixes = ['.test']
config.excludes = ['Inputs']
config.test_format = lit.formats.ShTest(not lit.llvm.llvm_config.use_lit_shell)
config.test_source_root = "@CMAKE_CURRENT_SOURCE_DIR@"
config.test_exec_root = "@CMAKE_CURRENT_BINARY_DIR@"
# Clangd-specific lit environment.
config.substitutions.append(('%clangd-benchmark-dir',
"@CMAKE_CURRENT_BINARY_DIR@/../benchmarks"))
if @CLANGD_BUILD_XPC@:
config.available_features.add('clangd-xpc-support')
thakisUnsubmitted
Not Done
ReplyInline Actions

trailing newline

thakis: trailing newline

clang-tools-extra/trunk/clangd/test/lit.local.cfg

clang-tools-extra/trunk/clangd/test/protocol.test

clang-tools-extra/trunk/clangd/test/references.test

clang-tools-extra/trunk/clangd/test/rename.test

clang-tools-extra/trunk/clangd/test/signature-help.test

clang-tools-extra/trunk/clangd/test/spaces-in-delimited-input.test

clang-tools-extra/trunk/clangd/test/symbol-info.test

clang-tools-extra/trunk/clangd/test/symbols.test

clang-tools-extra/trunk/clangd/test/test-uri-posix.test

clang-tools-extra/trunk/clangd/test/test-uri-windows.test

clang-tools-extra/trunk/clangd/test/textdocument-didchange-fail.test

clang-tools-extra/trunk/clangd/test/too_large.test

clang-tools-extra/trunk/clangd/test/trace.test

clang-tools-extra/trunk/clangd/test/tweaks-format.test

clang-tools-extra/trunk/clangd/test/type-hierarchy.test

clang-tools-extra/trunk/clangd/test/unsupported-method.test

clang-tools-extra/trunk/clangd/test/utf8.test

clang-tools-extra/trunk/clangd/test/xpc/initialize.test

clang-tools-extra/trunk/clangd/test/xrefs.test

clang-tools-extra/trunk/clangd/unittests/Annotations.h

clang-tools-extra/trunk/clangd/unittests/Annotations.cpp

clang-tools-extra/trunk/clangd/unittests/BackgroundIndexTests.cpp

clang-tools-extra/trunk/clangd/unittests/CMakeLists.txt

clang-tools-extra/trunk/clangd/unittests/CancellationTests.cpp

clang-tools-extra/trunk/clangd/unittests/ClangdTests.cpp

clang-tools-extra/trunk/clangd/unittests/ClangdUnitTests.cpp

clang-tools-extra/trunk/clangd/unittests/CodeCompleteTests.cpp

clang-tools-extra/trunk/clangd/unittests/CodeCompletionStringsTests.cpp

clang-tools-extra/trunk/clangd/unittests/ContextTests.cpp

clang-tools-extra/trunk/clangd/unittests/DexTests.cpp

clang-tools-extra/trunk/clangd/unittests/DiagnosticsTests.cpp

clang-tools-extra/trunk/clangd/unittests/DraftStoreTests.cpp

clang-tools-extra/trunk/clangd/unittests/ExpectedTypeTest.cpp

clang-tools-extra/trunk/clangd/unittests/FSTests.cpp

clang-tools-extra/trunk/clangd/unittests/FileDistanceTests.cpp

clang-tools-extra/trunk/clangd/unittests/FileIndexTests.cpp

clang-tools-extra/trunk/clangd/unittests/FindSymbolsTests.cpp

clang-tools-extra/trunk/clangd/unittests/FunctionTests.cpp

clang-tools-extra/trunk/clangd/unittests/FuzzyMatchTests.cpp

clang-tools-extra/trunk/clangd/unittests/GlobalCompilationDatabaseTests.cpp

clang-tools-extra/trunk/clangd/unittests/HeadersTests.cpp

clang-tools-extra/trunk/clangd/unittests/IndexActionTests.cpp

clang-tools-extra/trunk/clangd/unittests/IndexTests.cpp

clang-tools-extra/trunk/clangd/unittests/JSONTransportTests.cpp

clang-tools-extra/trunk/clangd/unittests/Matchers.h

clang-tools-extra/trunk/clangd/unittests/PrintASTTests.cpp

clang-tools-extra/trunk/clangd/unittests/QualityTests.cpp

clang-tools-extra/trunk/clangd/unittests/RIFFTests.cpp

clang-tools-extra/trunk/clangd/unittests/SelectionTests.cpp

clang-tools-extra/trunk/clangd/unittests/SerializationTests.cpp

clang-tools-extra/trunk/clangd/unittests/SourceCodeTests.cpp

clang-tools-extra/trunk/clangd/unittests/SymbolCollectorTests.cpp

clang-tools-extra/trunk/clangd/unittests/SymbolInfoTests.cpp

clang-tools-extra/trunk/clangd/unittests/SyncAPI.h

clang-tools-extra/trunk/clangd/unittests/SyncAPI.cpp

clang-tools-extra/trunk/clangd/unittests/TUSchedulerTests.cpp

clang-tools-extra/trunk/clangd/unittests/TestFS.h

clang-tools-extra/trunk/clangd/unittests/TestFS.cpp

clang-tools-extra/trunk/clangd/unittests/TestIndex.h

clang-tools-extra/trunk/clangd/unittests/TestIndex.cpp

clang-tools-extra/trunk/clangd/unittests/TestScheme.h

clang-tools-extra/trunk/clangd/unittests/TestTU.h

clang-tools-extra/trunk/clangd/unittests/TestTU.cpp

clang-tools-extra/trunk/clangd/unittests/ThreadingTests.cpp

clang-tools-extra/trunk/clangd/unittests/TraceTests.cpp

clang-tools-extra/trunk/clangd/unittests/TweakTests.cpp

clang-tools-extra/trunk/clangd/unittests/TypeHierarchyTests.cpp

clang-tools-extra/trunk/clangd/unittests/URITests.cpp

clang-tools-extra/trunk/clangd/unittests/XRefsTests.cpp

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

@LIT_SITE_CFG_IN_HEADER@
thakisUnsubmitted
Not Done
ReplyInline Actions

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.

thakis: Every other LLVM project puts the lit.cfg.in for the unit tests in foo/test/Unit/lit.cfg.in.
thakisUnsubmitted
Not Done
ReplyInline Actions

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: One consequence of these differences (except things being harder to understand) is that running…
MaskRayUnsubmitted
Not Done
ReplyInline Actions

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

MaskRay: I know little about lit configurations but not being able to run a single test…
sammccallAuthorUnsubmitted
Done
ReplyInline Actions

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.

sammccall: > Every other LLVM project puts the lit.cfg.in for the unit tests in foo/test/Unit/lit.cfg.in.
thakisUnsubmitted
Not Done
ReplyInline Actions

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.

thakis: > motivated by reducing the number of moving parts That's a great motivation, and I agree the…
# This is a shim to run the gtest unittests in ../unittests using lit.
import lit.formats
config.name = "Clangd Unit Tests"
config.test_format = lit.formats.GoogleTest('.', 'Tests')
config.test_source_root = "@CMAKE_CURRENT_BINARY_DIR@"
config.test_exec_root = "@CMAKE_CURRENT_BINARY_DIR@"
# Point the dynamic loader at dynamic libraries in 'lib'.
# XXX: it seems every project has a copy of this logic. Move it somewhere.
thakisUnsubmitted
Not Done
ReplyInline Actions

Did you mean to commit this XXX?

thakis: Did you mean to commit this XXX?
sammccallAuthorUnsubmitted
Done
ReplyInline Actions

Yes. Probably should have been spelled FIXME.

sammccall: Yes. Probably should have been spelled FIXME.
import platform
if platform.system() == 'Darwin':
shlibpath_var = 'DYLD_LIBRARY_PATH'
elif platform.system() == 'Windows':
shlibpath_var = 'PATH'
else:
shlibpath_var = 'LD_LIBRARY_PATH'
config.environment[shlibpath_var] = os.path.pathsep.join((
"@SHLIBDIR@", "@LLVM_LIBS_DIR@",
config.environment.get(shlibpath_var,'')))
thakisUnsubmitted
Not Done
ReplyInline Actions

Lots of trailing newlines.

thakis: Lots of trailing newlines.
sammccallAuthorUnsubmitted
Done
ReplyInline Actions

Are these actually confusing, or significant in some way?

sammccall: Are these actually confusing, or significant in some way?

clang-tools-extra/trunk/clangd/unittests/xpc/CMakeLists.txt

clang-tools-extra/trunk/clangd/unittests/xpc/ConversionTests.cpp

clang-tools-extra/trunk/test/CMakeLists.txt

clang-tools-extra/trunk/test/clangd/Inputs/BenchmarkHeader.h

clang-tools-extra/trunk/test/clangd/Inputs/BenchmarkSource.cpp

clang-tools-extra/trunk/test/clangd/Inputs/background-index/compile_commands.json

clang-tools-extra/trunk/test/clangd/Inputs/background-index/definition.jsonrpc

clang-tools-extra/trunk/test/clangd/Inputs/background-index/foo.h

clang-tools-extra/trunk/test/clangd/Inputs/background-index/foo.cpp

clang-tools-extra/trunk/test/clangd/Inputs/requests.json

clang-tools-extra/trunk/test/clangd/Inputs/symbols.test.yaml

clang-tools-extra/trunk/test/clangd/Unit/lit.site.cfg.py.in

clang-tools-extra/trunk/test/clangd/background-index.test

clang-tools-extra/trunk/test/clangd/compile-commands-path-in-initialize.test

clang-tools-extra/trunk/test/clangd/completion-auto-trigger.test

clang-tools-extra/trunk/test/clangd/completion-snippets.test

clang-tools-extra/trunk/test/clangd/completion.test

clang-tools-extra/trunk/test/clangd/crash-non-added-files.test

clang-tools-extra/trunk/test/clangd/delimited-input-comment-at-the-end.test

clang-tools-extra/trunk/test/clangd/diagnostic-category.test

clang-tools-extra/trunk/test/clangd/diagnostics-notes.test

clang-tools-extra/trunk/test/clangd/diagnostics.test

clang-tools-extra/trunk/test/clangd/did-change-configuration-params.test

clang-tools-extra/trunk/test/clangd/execute-command.test

clang-tools-extra/trunk/test/clangd/exit-with-shutdown.test

clang-tools-extra/trunk/test/clangd/exit-without-shutdown.test

clang-tools-extra/trunk/test/clangd/filestatus.test

clang-tools-extra/trunk/test/clangd/fixits-codeaction.test

clang-tools-extra/trunk/test/clangd/fixits-command.test

clang-tools-extra/trunk/test/clangd/fixits-embed-in-diagnostic.test

clang-tools-extra/trunk/test/clangd/formatting.test

clang-tools-extra/trunk/test/clangd/hover.test

clang-tools-extra/trunk/test/clangd/index-tools.test

clang-tools-extra/trunk/test/clangd/initialize-params-invalid.test

clang-tools-extra/trunk/test/clangd/initialize-params.test

clang-tools-extra/trunk/test/clangd/initialize-sequence.test

clang-tools-extra/trunk/test/clangd/input-mirror.test

clang-tools-extra/trunk/test/clangd/lit.local.cfg

clang-tools-extra/trunk/test/clangd/protocol.test

clang-tools-extra/trunk/test/clangd/references.test

clang-tools-extra/trunk/test/clangd/rename.test

clang-tools-extra/trunk/test/clangd/signature-help.test

clang-tools-extra/trunk/test/clangd/spaces-in-delimited-input.test

clang-tools-extra/trunk/test/clangd/symbol-info.test

clang-tools-extra/trunk/test/clangd/symbols.test

clang-tools-extra/trunk/test/clangd/test-uri-posix.test

clang-tools-extra/trunk/test/clangd/test-uri-windows.test

clang-tools-extra/trunk/test/clangd/textdocument-didchange-fail.test

clang-tools-extra/trunk/test/clangd/too_large.test

clang-tools-extra/trunk/test/clangd/trace.test

clang-tools-extra/trunk/test/clangd/tweaks-format.test

clang-tools-extra/trunk/test/clangd/type-hierarchy.test

clang-tools-extra/trunk/test/clangd/unsupported-method.test

clang-tools-extra/trunk/test/clangd/utf8.test

clang-tools-extra/trunk/test/clangd/xpc/initialize.test

clang-tools-extra/trunk/test/clangd/xrefs.test

clang-tools-extra/trunk/unittests/CMakeLists.txt

clang-tools-extra/trunk/unittests/clangd/Annotations.h

clang-tools-extra/trunk/unittests/clangd/Annotations.cpp

clang-tools-extra/trunk/unittests/clangd/BackgroundIndexTests.cpp

clang-tools-extra/trunk/unittests/clangd/CMakeLists.txt

clang-tools-extra/trunk/unittests/clangd/CancellationTests.cpp

clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp

clang-tools-extra/trunk/unittests/clangd/ClangdUnitTests.cpp

clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp

clang-tools-extra/trunk/unittests/clangd/CodeCompletionStringsTests.cpp

clang-tools-extra/trunk/unittests/clangd/ContextTests.cpp

clang-tools-extra/trunk/unittests/clangd/DexTests.cpp

clang-tools-extra/trunk/unittests/clangd/DiagnosticsTests.cpp

clang-tools-extra/trunk/unittests/clangd/DraftStoreTests.cpp

clang-tools-extra/trunk/unittests/clangd/ExpectedTypeTest.cpp

clang-tools-extra/trunk/unittests/clangd/FSTests.cpp

clang-tools-extra/trunk/unittests/clangd/FileDistanceTests.cpp

clang-tools-extra/trunk/unittests/clangd/FileIndexTests.cpp

clang-tools-extra/trunk/unittests/clangd/FindSymbolsTests.cpp

clang-tools-extra/trunk/unittests/clangd/FunctionTests.cpp

clang-tools-extra/trunk/unittests/clangd/FuzzyMatchTests.cpp

clang-tools-extra/trunk/unittests/clangd/GlobalCompilationDatabaseTests.cpp

clang-tools-extra/trunk/unittests/clangd/HeadersTests.cpp

clang-tools-extra/trunk/unittests/clangd/IndexActionTests.cpp

clang-tools-extra/trunk/unittests/clangd/IndexTests.cpp

clang-tools-extra/trunk/unittests/clangd/JSONTransportTests.cpp

clang-tools-extra/trunk/unittests/clangd/Matchers.h

clang-tools-extra/trunk/unittests/clangd/PrintASTTests.cpp

clang-tools-extra/trunk/unittests/clangd/QualityTests.cpp

clang-tools-extra/trunk/unittests/clangd/RIFFTests.cpp

clang-tools-extra/trunk/unittests/clangd/SelectionTests.cpp

clang-tools-extra/trunk/unittests/clangd/SerializationTests.cpp

clang-tools-extra/trunk/unittests/clangd/SourceCodeTests.cpp

clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp

clang-tools-extra/trunk/unittests/clangd/SymbolInfoTests.cpp

clang-tools-extra/trunk/unittests/clangd/SyncAPI.h

clang-tools-extra/trunk/unittests/clangd/SyncAPI.cpp

clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp

clang-tools-extra/trunk/unittests/clangd/TestFS.h

clang-tools-extra/trunk/unittests/clangd/TestFS.cpp

clang-tools-extra/trunk/unittests/clangd/TestIndex.h

clang-tools-extra/trunk/unittests/clangd/TestIndex.cpp

clang-tools-extra/trunk/unittests/clangd/TestScheme.h

clang-tools-extra/trunk/unittests/clangd/TestTU.h

clang-tools-extra/trunk/unittests/clangd/TestTU.cpp

clang-tools-extra/trunk/unittests/clangd/ThreadingTests.cpp

clang-tools-extra/trunk/unittests/clangd/TraceTests.cpp

clang-tools-extra/trunk/unittests/clangd/TweakTests.cpp

clang-tools-extra/trunk/unittests/clangd/TypeHierarchyTests.cpp

clang-tools-extra/trunk/unittests/clangd/URITests.cpp

clang-tools-extra/trunk/unittests/clangd/XRefsTests.cpp

clang-tools-extra/trunk/unittests/clangd/xpc/CMakeLists.txt

clang-tools-extra/trunk/unittests/clangd/xpc/ConversionTests.cpp