Handle multiple compile commands on client side (needs native server side support) #12960

yiftahw · 2024-11-13T17:12:28Z

DONE:

updates to the c_cpp_properties.json schema and compileCommands setting to accept either a string or array of strings.
updates to the settings.json schema and default.compileCommands setting to accept either a string or array of strings.
a single path with non-zero length in c_cpp_properties.json or settings.json is placed inside an array of length 1.
empty strings are filtered from the array of paths, a single empty string or an empty array are handled as undefined.
file watcher support for the list of files provided by the user.
single compile_commands.json is converted to a list of size 1 to consistently send an array (or undefined).
handle squiggles for errors in c_cpp_properties.json.
updated C/C++: Edit Configurations (UI) panel to handle multiple compile_commands.json paths.

…-empty strings

Extension/c_cpp_properties.schema.json

bobbrow · 2024-11-13T18:17:21Z

Extension/c_cpp_properties.schema.json

+                                "uniqueItems": true,
+                                "default": []
+                            }
+                        ],
                        "markdownDescription": "Full path to `compile_commands.json` file for the workspace.",


This description should be updated

updated the description, feel free to suggest something different, English is not my native language.

bobbrow · 2024-11-14T17:07:24Z

Extension/src/LanguageServer/configurations.ts

@@ -684,7 +685,7 @@ export class CppProperties {
            this.parsePropertiesFile(); // Clear out any modifications we may have made internally.
            const config: Configuration | undefined = this.CurrentConfiguration;
            if (config) {
-                config.compileCommands = path;
+                config.compileCommands = [path];


Question for my team. Do we want to prefer storing the compileCommands property as a string in the actual c_cpp_properties.json file, or as an array? Either way is acceptable here because the json will be reparsed in handleConfigurationChange and converted back to an array.

One other thing to note is that we should change the Edit Configurations UI to make the input for compileCommands be a textarea instead, along with whatever associated changes should come with that. The answer to the question above would also guide the implementation for the storage of the data in the json file. (e.g. if we prefer string for the single entry case, we'd need to test for that when it comes via the UI and react accordingly)

Quick suggestion for your first question, might be an overkill. Instead of forcing the value to be a string array both in parsePropertiesFile() and in handleSquiggles(), we create a "version 5" of the properties and add a function "updateToVersion5()" which actually saves a single entry as an array in the json file.
Again, this might be overkill, just a suggestion.

We have considered making changes to the version number, but this gets disruptive because many people commit their configurations to source control. We received a lot of negative feedback in the past when version changes were more frequent so we avoid it as best we can.

We are technically breaking versioning and assuming some risk by not incrementing the version number with this change, but the addition is very subtle. We will need to discuss this as a team and decide if this is acceptable or not.

@bobbrow Can't it be a string or an array in the c_cpp_properties.json?

Should this thread be resolved now?

bobbrow · 2024-11-14T17:10:14Z

Extension/src/LanguageServer/configurations.ts

-                    // Start tracking the fallback time for a new path.
-                    this.compileCommandsFileWatcherFallbackTime.set(configuration.compileCommands, new Date());
-                }
+                configuration.compileCommands = configuration.compileCommands?.map((path: string) => this.resolvePath(path));


minor: I don't believe you need to use ?. here or on the line below because the existence of configuration.compileCommands was tested before entering this code block.

yiftahw · 2024-11-14T19:32:06Z

@bobbrow squiggles are now handled as well. I believe most of the work on the client side is done.
let me know if you have more feedback.

please note there is a new telemetry entry and a new localized string:

message = localize("multiple.paths.should.be.separate.entries", "Multiple paths should be separate entries in an array.");
newSquiggleMetrics.MultiplePathsShouldBeSeparated++;

bobbrow · 2024-11-14T19:45:49Z

Extension/src/LanguageServer/configurations.ts

-        }
+
+        currentConfiguration.compileCommands?.forEach((file: string) => {
+            paths.push(`${file}`)


a semicolon is missing on this line.

bobbrow · 2024-11-14T19:47:57Z

@bobbrow squiggles are now handled as well. I believe most of the work on the client side is done.
let me know if you have more feedback.

Thank you very much. Would you also be able to do the changes to the Edit Configurations UI in Extension/ui/settings.* so that multiple paths can be displayed/edited in that view?

yiftahw · 2024-11-14T20:05:10Z

looking into it... putting the PR back to draft for now

yiftahw · 2024-11-14T21:14:11Z

@bobbrow updated the UI panel.
note: adding a single path in the UI will save it as an array of size 1 (similar to how includePath is handled)

sean-mcmanus · 2024-12-06T03:32:05Z

@yiftahw Your changes/fixes and cpptools updates to allow an array for compileCommands is available in https://github.com/microsoft/vscode-cpptools/releases/tag/v1.23.2

yiftahw · 2024-12-06T07:13:40Z

@sean-mcmanus I downloaded the latest pre-release version and copied the binaries to my project folder, and I don't see @Colengms 's added string used
should I copy something else besides cpptools and cpptools-srv ?

[12/6/2024, 8:23:41 AM] "module.cxx" not found in "PLAYGROUND". 'includePath' from c_cpp_properties.json in folder 'undefined' will be used for this file instead.

looks like this string matches file_not_found_in_path instead of file_not_found_in_path2 from #12964
I expected it to look like:

[12/6/2024, 8:23:41 AM] "module.cxx" not found in compile_commands.json files. 'includePath' from c_cpp_properties.json in folder 'PLAYGROUND' will be used for this file instead.

Colengms · 2024-12-06T19:28:00Z

I don't see @Colengms 's added string used

Hi @yiftahw . It looks like these strings' contents got reversed/mismatched. The next insiders should correct that. Otherwise, I believe support for use of an array of strings instead of a single string in the compileCommands field of the configuration object sent to the native side, should now be supported.

Extension/src/LanguageServer/configurations.ts

sean-mcmanus · 2024-12-13T02:07:19Z

@bobbrow Were you going to resolve the comments you made?

Waisman, Yiftah and others added 7 commits November 10, 2024 21:12

accept an array in compile commands schema

f346723

handle settings.json of multiple compile commands

2dc0fa3

update schemas

02a2812

an array is returned from default settings only if non-empty with non…

280aee1

…-empty strings

basic handling of compile commands array in configurations

b934a92

Merge branch 'main' into handle-multiple-compile-commands

e273731

fixes after merge

0c8ccb1

yiftahw changed the title ~~Handle multiple compile commands on client side~~ Handle multiple compile commands on client side (needs native server side support) Nov 13, 2024

bobbrow reviewed Nov 13, 2024

View reviewed changes

Extension/c_cpp_properties.schema.json Show resolved Hide resolved

bobbrow reviewed Nov 13, 2024

View reviewed changes

yiftahw added 5 commits November 14, 2024 06:52

yarn lint

8e84183

unify check behavior

22de1e0

update desription in schema

62a8915

handle parsed values from c_cpp_properties.json

d7e2a4a

reverse logic to default as undefined

b1614de

bobbrow reviewed Nov 14, 2024

View reviewed changes

yiftahw added 4 commits November 14, 2024 20:12

squiggles for multi paths

7fe9904

remove not needed optional guard

82c4e53

squiggles finalized

69c4013

paths should be separated telemetry enabled for a single string

d2bf51d

yiftahw marked this pull request as ready for review November 14, 2024 19:32

yiftahw mentioned this pull request Nov 14, 2024

Merge multiple compile_commands.json files #12911

Closed

5 tasks

bobbrow reviewed Nov 14, 2024

View reviewed changes

yarn lint

6aa66f6

yiftahw marked this pull request as draft November 14, 2024 20:05

update UI to handle multiple compile commands files

45bb041

yiftahw marked this pull request as ready for review November 14, 2024 21:14

Merge branch 'main' into handle-multiple-compile-commands

fbe7a2b

yiftahw requested a review from a team as a code owner December 6, 2024 06:09

merge main

2438391

sean-mcmanus reviewed Dec 13, 2024

View reviewed changes

Extension/src/LanguageServer/configurations.ts Outdated Show resolved Hide resolved

sean-mcmanus reviewed Dec 13, 2024

View reviewed changes

Extension/src/LanguageServer/configurations.ts Outdated Show resolved Hide resolved

sean-mcmanus requested a review from a team December 13, 2024 02:08

yiftahw added 3 commits December 13, 2024 06:38

typos and linter

dc3e543

Merge branch 'main' into handle-multiple-compile-commands

82d5c0b

Merge branch 'main' into handle-multiple-compile-commands

c7a9c05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle multiple compile commands on client side (needs native server side support) #12960

Handle multiple compile commands on client side (needs native server side support) #12960

yiftahw commented Nov 13, 2024 •

edited

Loading

bobbrow Nov 13, 2024

yiftahw Nov 14, 2024

bobbrow Nov 14, 2024 •

edited

Loading

yiftahw Nov 15, 2024

bobbrow Nov 15, 2024

sean-mcmanus Nov 15, 2024

sean-mcmanus Dec 13, 2024

bobbrow Nov 14, 2024

yiftahw Nov 14, 2024

yiftahw commented Nov 14, 2024 •

edited

Loading

bobbrow Nov 14, 2024

bobbrow commented Nov 14, 2024

yiftahw commented Nov 14, 2024

yiftahw commented Nov 14, 2024

sean-mcmanus commented Dec 6, 2024 •

edited

Loading

yiftahw commented Dec 6, 2024 •

edited

Loading

Colengms commented Dec 6, 2024 •

edited

Loading

sean-mcmanus commented Dec 13, 2024

Handle multiple compile commands on client side (needs native server side support) #12960

Are you sure you want to change the base?

Handle multiple compile commands on client side (needs native server side support) #12960

Conversation

yiftahw commented Nov 13, 2024 • edited Loading

DONE:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobbrow Nov 14, 2024 • edited Loading

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

yiftahw commented Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

bobbrow commented Nov 14, 2024

yiftahw commented Nov 14, 2024

yiftahw commented Nov 14, 2024

sean-mcmanus commented Dec 6, 2024 • edited Loading

yiftahw commented Dec 6, 2024 • edited Loading

Colengms commented Dec 6, 2024 • edited Loading

sean-mcmanus commented Dec 13, 2024

yiftahw commented Nov 13, 2024 •

edited

Loading

bobbrow Nov 14, 2024 •

edited

Loading

yiftahw commented Nov 14, 2024 •

edited

Loading

sean-mcmanus commented Dec 6, 2024 •

edited

Loading

yiftahw commented Dec 6, 2024 •

edited

Loading

Colengms commented Dec 6, 2024 •

edited

Loading