Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 29 commits into from
Dec 30, 2024

Conversation

yiftahw
Copy link
Contributor

@yiftahw yiftahw commented Nov 13, 2024

closes #7029

  • 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.

@yiftahw 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
@yiftahw
Copy link
Contributor Author

yiftahw commented Nov 14, 2024

@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++;

@yiftahw yiftahw marked this pull request as ready for review November 14, 2024 19:32
@bobbrow
Copy link
Member

bobbrow commented Nov 14, 2024

@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
Copy link
Contributor Author

yiftahw commented Nov 14, 2024

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

@yiftahw yiftahw marked this pull request as draft November 14, 2024 20:05
@sean-mcmanus
Copy link
Contributor

sean-mcmanus commented Dec 6, 2024

@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 yiftahw requested a review from a team as a code owner December 6, 2024 06:09
@yiftahw
Copy link
Contributor Author

yiftahw commented Dec 6, 2024

@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
Copy link
Contributor

Colengms commented Dec 6, 2024

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.

@sean-mcmanus
Copy link
Contributor

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

@sean-mcmanus sean-mcmanus requested a review from a team December 13, 2024 02:08
sean-mcmanus
sean-mcmanus previously approved these changes Dec 20, 2024
sean-mcmanus
sean-mcmanus previously approved these changes Dec 20, 2024
Extension/c_cpp_properties.schema.json Outdated Show resolved Hide resolved
@sean-mcmanus sean-mcmanus merged commit 2bff033 into microsoft:main Dec 30, 2024
6 checks passed
@sean-mcmanus sean-mcmanus added this to the 1.23.3 milestone Dec 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Support multiple compile commands files per project
4 participants