Page MenuHomePhabricator

[clangd] Fixes in lit tests
ClosedPublic

Authored by ArcsinX on Tue, Jul 14, 4:10 AM.

Details

Summary

Changes:

  • background-index.test Add Windows support, don't create redundant *-e files on macOS
  • did-change-configuration-params.test Replace cat | FileCheck with FileCheck --input-file
  • test-uri-windows.test This test did not run on Windows displite REQUIRES: windows-gnu || windows-msvc (replacement: UNSUPPORTED: !(windows-gnu || windows-msvc)).

Diff Detail

Event Timeline

ArcsinX created this revision.Tue, Jul 14, 4:10 AM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Jul 14, 4:10 AM

What do you think of this patch? I'm not sure if Windows is important OS for developers, and that some of these changes might be awkward to maintain, but maybe we can enable background-index.test at least?

What do you think of this patch? I'm not sure if Windows is important OS for developers.

Windows is most certainly an important OS for developers and something the whole llvm project has been trying to catch up on where possible.

hi, sorry for the late replay :/

We discussed this offline with Sam and are both concerned with the duplications it introduces, haven't really thought about a nice way to address this yet.

As you mentioned the background-index test in itself looks a lot better do, so we can land that one separately and think about the rest in the meantime.

clang-tools-extra/clangd/test/background-index.test
17–18

are you sure these don't need changes on windows bots ? AFAICT this contains both backslashes(coming from %t) and forwards slashes, and command is ls which might not be available on a default windows prompt.

ArcsinX marked an inline comment as done.Tue, Jul 21, 2:11 AM
ArcsinX added inline comments.
clang-tools-extra/clangd/test/background-index.test
17–18

I am not sure. How I can check it? The only thing I can say, that I tested this on mingw and visual studio builds on my local machine.

this contains both backslashes(coming from %t) and forwards slashes,

For me that was OK, but I agree that ls %t/.cache/clangd/index/foo.cpp.*.idx should be replaced with ls %/t/.cache/clangd/index/foo.cpp.*.idx.

As for ls, it's a part of mingw, so I think ls absence probability is the same as for sed

kadircet added inline comments.Tue, Jul 21, 2:46 AM
clang-tools-extra/clangd/test/background-index.test
17–18

I am not sure. How I can check it? The only thing I can say, that I tested this on mingw and visual studio builds on my local machine.

unfortunately there is no good way (that I know of) for checking these, apart from landing the patch and monitoring the buildbots(http://lab.llvm.org:8011/) for breakages and reverting/fixing forward accordingly.

ArcsinX updated this revision to Diff 279482.Tue, Jul 21, 4:37 AM

Revert tests split; Replace '\' with '/' in background-index.test.

ArcsinX retitled this revision from [clangd] Port lit tests to Windows to [clangd] Fixes in lit tests.Tue, Jul 21, 4:40 AM
ArcsinX edited the summary of this revision. (Show Details)
kadircet accepted this revision.Tue, Jul 21, 7:52 AM

thanks, LGTM!

as i mentioned please watch out for buildbot breakages after landing this. (and again let me know if I should land this for you)

This revision is now accepted and ready to land.Tue, Jul 21, 7:52 AM
ArcsinX added a comment.EditedTue, Jul 21, 8:49 AM

as i mentioned please watch out for buildbot breakages after landing this. (and again let me know if I should land this for you)

Thanks for review.

I got commit access recently and will try to commit by myself.

This revision was automatically updated to reflect the committed changes.

Looks like this breaks tests on macOS (and probably with non-gnu greps): http://45.33.8.238/mac/17611/step_9.txt

Please take a look, and revert while you investigate if it takes a while to fix.

Looks like this breaks tests on macOS (and probably with non-gnu greps): http://45.33.8.238/mac/17611/step_9.txt

Please take a look, and revert while you investigate if it takes a while to fix.

Reverted.

Seems the problem is in sed without gnu extensions, but I wonder how other tests with similar sed command could pass (e.g. compile-commands-path-in-initialize.test)

Seem the problem is in -i option.
According with OSX man:
sed [-Ealn] command [file ... ]
sed [-Ealn] [-e command] [-f command_file] [-i extension] [file ...].

Seems on macOS -E is treated as an argument for -i in command like sed -i -E -e ... .

Also, seems commands like sed -i -e ... are unsafe on macOS. Such commands will create backup with name <filename>-e, but I do not have macOS to check it.
@thakis could you please verify that after tests you have extra <filename>-e files in /Users/thakis/src/llvm-project/out/gn/obj/clang-tools-extra/clangd/test/Output/background-index.test.tmp/ ?

I think we could avoid -i usage to fix this problem.

Seem the problem is in -i option.
According with OSX man:
sed [-Ealn] command [file ... ]
sed [-Ealn] [-e command] [-f command_file] [-i extension] [file ...].

Seems on macOS -E is treated as an argument for -i in command like sed -i -E -e ... .

Also, seems commands like sed -i -e ... are unsafe on macOS. Such commands will create backup with name <filename>-e, but I do not have macOS to check it.
@thakis could you please verify that after tests you have extra <filename>-e files in /Users/thakis/src/llvm-project/out/gn/obj/clang-tools-extra/clangd/test/Output/background-index.test.tmp/ ?

I think we could avoid -i usage to fix this problem.

your diagnosis seems correct. in addition to avoiding -i altogether, you can try passing a value to it. e.g. sed -i.bak then both linux and macos should hopefully pick it up and the following -e should be interpreted correctly.

ArcsinX reopened this revision.Wed, Jul 22, 3:25 AM
This revision is now accepted and ready to land.Wed, Jul 22, 3:25 AM
ArcsinX updated this revision to Diff 279754.Wed, Jul 22, 3:35 AM

[macOS] Fix background-index.test failure, don't create *-e files during background-index.test

I believe it's okay now on macOS.

ArcsinX edited the summary of this revision. (Show Details)Wed, Jul 22, 3:42 AM
kadircet added inline comments.Wed, Jul 22, 4:20 AM
clang-tools-extra/clangd/test/background-index.test
6

i don't think there's much point in dropping -i from only some commands.

I would suggest either:

  • dropping it from all, by changing the original files to have a .tmpl suffix, and dropping the suffix in sed'd versions
  • keeping it for all comments by adding a -i.bak to all
ArcsinX marked an inline comment as done.Wed, Jul 22, 4:37 AM
ArcsinX added inline comments.
clang-tools-extra/clangd/test/background-index.test
6

For definition.jsonrpc we have two sed commands and we can keep the file name unchanged without -i option.
But for compile_commands.json we have only one sed command and we need -i option to keep the file name unchanged.

changing the original files to have a .tmpl suffix

In other tests .1 suffix is used, that's why I use this suffix here.
E.g. compile-commands-path-in-initialize.test

# RUN: sed -e "s|INPUT_DIR|%/t.dir|g" %s > %t.test.1
...
# RUN: sed -E -e 's|"file://([A-Z]):/|"file:///\1:/|g' %t.test.1 > %t.test
kadircet added inline comments.Wed, Jul 22, 4:50 AM
clang-tools-extra/clangd/test/background-index.test
6

For definition.jsonrpc we have two sed commands and we can keep the file name unchanged without -i option.

yes for that one we could still move into a .1 (or .tmp) suffixed version.

In other tests .1 suffix is used, that's why I use this suffix here.

right, I was talking about renaming clang-tools-extra/clangd/test/Inputs/background-index/definition.jsonrpc to clang-tools-extra/clangd/test/Inputs/background-index/definition.jsonrpc.tmpl (likewise the compile_commands.json). That way you can always do a sed, and write to a different file (i.e. the version without the suffix).

ArcsinX updated this revision to Diff 279781.Wed, Jul 22, 5:26 AM

Get rid of -i sed option

ArcsinX marked an inline comment as done.Wed, Jul 22, 5:27 AM
ArcsinX added inline comments.
clang-tools-extra/clangd/test/background-index.test
6

Got it, fixed, thanks.

kadircet accepted this revision.Wed, Jul 22, 5:55 AM

thanks, LGTM.

This revision was automatically updated to reflect the committed changes.