Add file watcher for compile_commands.json and other configuration files #33

rapgenic · 2020-05-21T17:10:07Z

In this pull request I added a simple file watcher that listens for events on the following files as per issue #6:

compile_commands.json
compile_flags.txt
.clang-tidy

Whenever one of these files is modified or created (but not empty to avoid false positives, such as tools that create an empty file before writing it) the clangd server is restarted. (I did not include the .clang-format file, because it seems to me that the clangd server does not need to be restarted whenever it is changed, to properly format a file)

As a side note, I understand that restarting the whole server might not be the best solution and that you may want to implement a cleaner refresh behavior in the clangd executable itself: however this pull request is very simple and may be easily got ridden of in a future when the server itself handles all the watching logic. That's why I opened it anyway, feel free to reject it if it does not meet your needs.

P.s.: This extension + clangd is an awesome work, thank you very much!

clangd/vscode-clangd#33

ArekSredzki · 2020-06-03T18:18:35Z

Looking forward to having this!

sam-mccall · 2020-06-08T11:38:34Z

Sorry about the delay here. Yes, we're going to make clangd do this natively but this isn't done yet and the existing released versions aren't going away anytime soon. So this is really useful to have in some from. Once we implement this on the server, we can have it advertise a capability to disable this on the client.

I don't think unconditionally restarting clangd is necessarily the best thing. Can we make this a setting clangd.onConfigChanged with 3 options?

restart: behavior in this patch
ignore: existing behavior
prompt: notify the user of the changed files, and ask them whether to restart (using vscode.window.showInformationMessage)

I think prompt should be the default: ignore will make this feature too hard for users to discover, while restart will get us bug reports for sure because clangd will be glitchy (preamble needs to be rebuilt, index features will stop working until it's fully reloaded etc).

What do you think?

rapgenic · 2020-06-08T20:42:48Z

You're right, more choice is better.

I'll push a commit in the next hours/day. Given that the pull request for the restart command (#35) has been merged, I thought I could use that command to restart the server, if you're good with it.

lh123 · 2020-06-10T09:02:38Z

src/extension.ts

+    if ((await vscode.workspace.fs.readFile(uri)).length) {
+      let relativePath = vscode.workspace.asRelativePath(uri, false);
+      // If it is at the top of the workspace
+      if (relativePath == path.basename(uri.fsPath)) {


Why only listen to changes in the configuration file in the top of the workspace?
Currently clangd has been able to automatically find the compilation database located in the build folder.D78629

That could be added easily. @sam-mccall are there any other default folders we should look into for file changes?

I think we should delete this check.

clangd has --compile-commands-dir option to specify any directory.

clangd already supports multiple compilation databases.

You're making fair points, however I'm not sure those possibilities are that much used, so I don't know if they're worth the false positives we could get by removing this check (for example by having multiple build directories: if i compile my code in 3 different folders I don't want to have three messages popping up asking if I want to restart clang server).

Of course my opinion is a bit biased (the example I made is exactly why I implemented that check), so before removing/keeping it I'd rather listen to the maintainer opinion

I agree with @lh123 if we want to have a watch put on the compilation database this should be set by reading the parameter that is sent to clangd. There are numerous environments where compile_commands.json resides in other directories other than the top director or the build directory. For example @mozilla we use dedicated obj directory that can be named whatever you like and the compile_commands.json can be created in that directory.

Hi and sorry for the delay. From your comments I see that i might have underestimated the utility of that option... I'll try to push some new commits in the next days (right now I do not have my pc near me)

abpostelnicu · 2020-08-05T05:18:06Z

Sorry about the delay here. Yes, we're going to make clangd do this natively but this isn't done yet and the existing released versions aren't going away anytime soon. So this is really useful to have in some from. Once we implement this on the server, we can have it advertise a capability to disable this on the client.

I don't think unconditionally restarting clangd is necessarily the best thing. Can we make this a setting clangd.onConfigChanged with 3 options?

restart: behavior in this patch

ignore: existing behavior

prompt: notify the user of the changed files, and ask them whether to restart (using vscode.window.showInformationMessage)

I think prompt should be the default: ignore will make this feature too hard for users to discover, while restart will get us bug reports for sure because clangd will be glitchy (preamble needs to be rebuilt, index features will stop working until it's fully reloaded etc).

What do you think?

Is there a phabricator issue on llvm about the implementation of this sort of feature on the server side? I would be more than happy to contribute to it.

Trass3r · 2020-08-21T08:24:42Z

No nothing yet, clangd/clangd#83

rapgenic · 2020-08-31T10:56:20Z

I agree with @lh123 if we want to have a watch put on the compilation database this should be set by reading the parameter that is sent to clangd.

I implemented this option as discussed, please tell me if it works as intended.

I wanted to note that since I do not have access to a windows/mac environment I have not been able to test those platforms, so anyone providing feedback would be welcome!

hokein

Thanks, the PR looks nice, a few nits, will try to play around it closely.

src/extension.ts

rapgenic · 2020-09-09T17:25:48Z

@hokein I implemented what you suggested! Everything is now on a new file and there should be no debug code left...

hokein

Thanks, I got a chance to play around it, it works nicely. I think we can simplify the patch a bit more.

btw, we plan to make clangd support it natively in Q4 -- it would be available in clangd 12 hopefully.

src/config-file-watcher.ts

hokein · 2020-09-25T19:38:08Z

src/config-file-watcher.ts

+  }
+
+  async handleConfigFilesChanged(uri: vscode.Uri) {
+    if ((await vscode.workspace.fs.readFile(uri)).length) {


unclear what's the purpose of this if? to check the file is not empty? can we add a comment here?

It shouldn't read the whole file for that either.

Suggested change

if ((await vscode.workspace.fs.readFile(uri)).length) {

if ((await vscode.workspace.fs.stat(uri)).size) {

Also, early exit!

Suggested change

if ((await vscode.workspace.fs.readFile(uri)).length) {

if ((await vscode.workspace.fs.stat(uri)).size <= 0)

return;

@hokein this is to check, as you understood, if the file is empty, because I have noticed that sometimes the tools that generate the compile_commands.json file, before writing to it they clear it or they delete it and create a new empty one, and this should not toggle a restart of the server.

@Trass3r much better solution thanks!

hokein · 2020-09-25T20:38:25Z

src/config-file-watcher.ts

+      let restart = false;
+
+      if (baseName == 'compile_commands.json') {
+        if (this.compileCommandsFolder &&


Things become complicated with --compile-commands-dir:

--compile-commands-dir accepts a relative or an absolute file path;

vscode's createFileSystemWatcher only watches files within the workspace folder;

if compile-commands-dir is an absolute file path:
a. the absolute file path is in the workspace folder -- this might be the case we want to support, but the current code is not correct;
b. the absolute file path is not in the workspace folder -- we can't do anything about this case, as watching files outside of the workspace folder is not possible in vscode;

if compile-commands-dir is a relative file path, clangd will use its cwd to form an absolute file path internally, I'm not sure we have a stable way to have a share view of cwd between clangd and vscode.

Given the complexity above, and the fact that compile-commands-dir is probably not a major use case, I'd suggest just not supporting it.

hokein · 2020-09-25T20:43:11Z

src/config-file-watcher.ts

+        else if (!this.compileCommandsFolder && relativePath == baseName)
+          restart = true;
+      } else {
+        if (relativePath == baseName)


if my reading is correct, we're trying to watch $workspacefolder/{compile_commands.json,compile_flags.txt,.clang-tidy}, and skip any config files in subfolders, e.g. $workspacefolder/dir1/compile_command.json would be skipped.

if so, I think we can do it in a simpler way rather than doing a post filtering vscode.workspace.createFileSystemWatcher( new RelativePattern(vscode.workspace.rootPath, '{compile_commands.json,compile_flags.txt,.clang-tidy}'));

Agreed, it doesn't make sense to install a recursive file watcher if you're only interested in the root folder.

Anyway the compile commands file is often in a build subfolder which would be found by clangd but ignored by this code it seems.

There could also be multiple subfolders for different configs like buildDebug, buildRelease, ... I'm using this together with the CMake Tools copy commands.json to base folder feature to make clangd work without explicit --compile-commands-dir.

Maybe it would be enough to watch only files up to 1 directory level deep in general. Anything else seems out of scope.

@hokein @Trass3r the main reason for using a recursive file watcher was because some users asked to support the compile-commands-dir option, which meant that the file could be in any subdirectory (even though it could still be made with the RelativePattern method suggested by @hokein).

The other reason however is that when multi folder workspaces are opened, looking only in the workspace.rootPath would not be correct. Using a generic watcher instead notifies a file change in all folders, but the small price to pay is to check if the path is the right one.

There could also be multiple subfolders for different configs like buildDebug, buildRelease

These however wouldn't be automatically recognised by clangd or would they? (Can we clarify what folders clangd looks for the compilation database except for the root of the project and the build folder?)

Of course were we to drop support for the compile-commands-dir option the code would be simpler, but I still think that a recursive watcher would be needed.

As I said I let CMake Tools copy the commands.json to the base folder after configuration finished.

Then I think we don't need to watch for other folders, because the event is triggered when the file is copied in the base folder. Unless clangd looks for compile_commands.json in more folders than I am aware of,

project's root folder

build

should be the only directories we need to watch into

this sounds good to me, it should cover most major cases.

A minor issue is that $workspacefolder/compile_commands.json is usually a symlink of $workspacefolder/build/compile_commands.json, which means if we change one of them, we will get two prompt-up dialogs, which seems a bit annoying, but I don't have better suggestion.

When the events triggers on build/compile_commands.json we could check if it exists a compile_commands.json in the root folder too and if it's the case, simply ignore the event...

I wouldn't bother. I'm affected by this too but I'd just pick "yes always" and be done with it.

src/config-file-watcher.ts

Trass3r · 2020-09-25T21:32:40Z

src/config-file-watcher.ts

+  }
+
+  async handleConfigFilesChanged(uri: vscode.Uri) {
+    if ((await vscode.workspace.fs.readFile(uri)).length) {


It shouldn't read the whole file for that either.

Suggested change

if ((await vscode.workspace.fs.readFile(uri)).length) {

if ((await vscode.workspace.fs.stat(uri)).size) {

Trass3r · 2020-09-25T21:37:21Z

src/config-file-watcher.ts

+  }
+
+  async handleConfigFilesChanged(uri: vscode.Uri) {
+    if ((await vscode.workspace.fs.readFile(uri)).length) {


Also, early exit!

Suggested change

if ((await vscode.workspace.fs.readFile(uri)).length) {

if ((await vscode.workspace.fs.stat(uri)).size <= 0)

return;

Trass3r · 2020-09-25T21:48:39Z

src/config-file-watcher.ts

+        else if (!this.compileCommandsFolder && relativePath == baseName)
+          restart = true;
+      } else {
+        if (relativePath == baseName)


Agreed, it doesn't make sense to install a recursive file watcher if you're only interested in the root folder.

Anyway the compile commands file is often in a build subfolder which would be found by clangd but ignored by this code it seems.

There could also be multiple subfolders for different configs like buildDebug, buildRelease, ... I'm using this together with the CMake Tools copy commands.json to base folder feature to make clangd work without explicit --compile-commands-dir.

Maybe it would be enough to watch only files up to 1 directory level deep in general. Anything else seems out of scope.

rapgenic · 2020-09-26T15:51:56Z

I think I fixed everything you asked.

Regarding the file watcher I made the following choices:

drop support for compile-commands-dir option
Look only on the base folder for all the configuration files and build/compile_commands.json

(The watcher is still recursive because of what I said before: when multiple folders are open there is no way that i know of to instanciate a file watcher only on the root of every folder, and to update it whenever a folder is added or removed)

As you see I have chosen the easy way: that's because as you said this will be a feature present in clangd in the near future, so in my opinion there is no need to make this feature overcomplicated considering that it is temporary.

Trass3r · 2020-09-26T15:56:18Z

The watcher is still recursive because of what I said before: when multiple folders are open there is no way that i know of to instantiate a file watcher only on the root of every folder, and to update it whenever a folder is added or removed

I see, I haven't worked with multi root workspaces yet.
https://github.com/microsoft/vscode/wiki/Adopting-Multi-Root-Workspace-APIs#eliminating-rootpath
So I guess something like

createFileSystemWatcher('{' + workspace.workspaceFolders.map(f => f.uri.fsPath).join(',') + '}/compile_commands.json')

could work. But probably not worth the hassle as you said.

rapgenic · 2020-09-27T06:50:43Z

Also that would not account for the fact that workspace folders can be added and removed, so we would need to listen for such an event and the file watcher would need to be recreated every time this happened..

Trass3r · 2020-09-27T08:42:00Z

Yep there's an event for just that.

rapgenic · 2020-09-27T09:51:50Z

Well in the end I gave it a try... :) Might be useful on larger projects

Trass3r · 2020-09-27T09:53:16Z

Interesting so my quickly hacked together code actually worked 😁

hokein

Thanks! It looks nice.

src/config-file-watcher.ts

hokein · 2020-09-28T13:04:01Z

@rapgenic, thanks for the contribution!

ghost · 2020-11-06T10:44:20Z

@hokein Is there any ETA for the next release of the extension? The file watcher is highly anticipated in our team :)

hokein · 2020-11-09T11:54:23Z

@JanHildenbrandBosch I'll make a new release this week.

rapgenic force-pushed the temp/filewatcher branch from 98df164 to 12d3f42 Compare May 21, 2020 17:12

fannheyward added a commit to clangd/coc-clangd that referenced this pull request May 28, 2020

chore(ctx): reload clangd when compile_commands.json changed

372af93

clangd/vscode-clangd#33

rapgenic added 2 commits June 8, 2020 23:44

Add server configuration files watcher

f822a7b

Add message and setting for action on config files change

cc0113b

rapgenic force-pushed the temp/filewatcher branch from 12d3f42 to cc0113b Compare June 8, 2020 21:52

lh123 reviewed Jun 10, 2020

View reviewed changes

Trass3r mentioned this pull request Jun 25, 2020

[feature request] support for vscode-clangd microsoft/vscode-cmake-tools#654

Closed

rapgenic added 2 commits August 31, 2020 12:43

Change compile_commands.json watched folder according to clangd argument

9ed9fcd

Merge remote-tracking branch 'upstream/master' into temp/filewatcher

edbab83

hokein reviewed Sep 1, 2020

View reviewed changes

src/extension.ts Outdated Show resolved Hide resolved

src/extension.ts Outdated Show resolved Hide resolved

src/extension.ts Outdated Show resolved Hide resolved

rapgenic added 3 commits September 9, 2020 13:04

Fixes as discussed in pull request: separate file and improve message

3c43101

Merge remote-tracking branch 'upstream/master' into temp/filewatcher

89f98fd

Remove unused import

96c9039

hokein reviewed Sep 25, 2020

View reviewed changes

Trass3r suggested changes Sep 25, 2020

View reviewed changes

Fixes according to discussion on pull request

3677675

rapgenic force-pushed the temp/filewatcher branch from 387736d to ffcabeb Compare September 27, 2020 09:43

Remove recursive file watcher

c1ac952

rapgenic force-pushed the temp/filewatcher branch from ffcabeb to c1ac952 Compare September 27, 2020 09:44

Remove useless modifications in extension.ts

0318803

hokein approved these changes Sep 28, 2020

View reviewed changes

src/config-file-watcher.ts Outdated Show resolved Hide resolved

Fix nit

09fe24c

rapgenic force-pushed the temp/filewatcher branch from 548dffd to 09fe24c Compare September 28, 2020 08:46

hokein merged commit aed58f9 into clangd:master Sep 28, 2020

hokein mentioned this pull request Sep 28, 2020

Watch for compile_commands.json #6

Closed

HighCommander4 mentioned this pull request Nov 6, 2020

[Feature Request] Add a command to force clangd to reparse the compilation database clangd/clangd#192

Closed

makestuff mentioned this pull request Nov 8, 2020

Command 'clangd.restart' tries to re-register 'clangd.memoryUsage'? #95

Closed

HighCommander4 mentioned this pull request Nov 13, 2020

Error restarting server when using onConfigChanged #98

Closed

HighCommander4 mentioned this pull request Dec 20, 2020

"On Config Changed" doesn't support compile-commands-dir #123

Closed

shinjitumala mentioned this pull request Jun 28, 2021

Crashes on C++20 when starting to type a word. #202

Closed

Da-Viper mentioned this pull request Oct 23, 2022

The Inlay hints are repeated when changing the function name #400

Closed

	if ((await vscode.workspace.fs.readFile(uri)).length) {
	if ((await vscode.workspace.fs.stat(uri)).size) {

	if ((await vscode.workspace.fs.readFile(uri)).length) {
	if ((await vscode.workspace.fs.stat(uri)).size <= 0)
	return;

Add file watcher for compile_commands.json and other configuration files #33

Add file watcher for compile_commands.json and other configuration files #33

Conversation

rapgenic commented May 21, 2020

ArekSredzki commented Jun 3, 2020

sam-mccall commented Jun 8, 2020

rapgenic commented Jun 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abpostelnicu commented Aug 5, 2020

Trass3r commented Aug 21, 2020

rapgenic commented Aug 31, 2020

hokein left a comment

Choose a reason for hiding this comment

rapgenic commented Sep 9, 2020

hokein left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rapgenic commented Sep 26, 2020

Trass3r commented Sep 26, 2020 • edited Loading

rapgenic commented Sep 27, 2020

Trass3r commented Sep 27, 2020

rapgenic commented Sep 27, 2020

Trass3r commented Sep 27, 2020

hokein left a comment

Choose a reason for hiding this comment

hokein commented Sep 28, 2020

ghost commented Nov 6, 2020

hokein commented Nov 9, 2020

Trass3r commented Sep 26, 2020 •

edited

Loading