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

Support multiple root cpp build configurations #4603

Merged
merged 1 commit into from
Aug 7, 2019

Conversation

vince-fugnitto
Copy link
Member

@vince-fugnitto vince-fugnitto commented Mar 18, 2019

Added support for multiple root cpp build configurations.
Instead of maintaining a single active configuration, a map is
used to maintain the relationship between the workspace root and its
given CppBuildConfiguration. This feature is an experimental
PR based on new developments in clangd to support multiple
compilation databases.

  • Update the cpp manager to handle the new data structure.
  • Update the test cases (fix the task test case with incorrect imports and false positive results).

Tasks

  • Update the cpp manager.
  • Update the user interface. (need a good UI to be able to cleanly handle updating / creating / selecting the active build configuration per root. Perhaps a widget would instead be needed)
  • Update the cpp language client contribution to handle the multiple roots. @simark maybe you can help think of what would be needed in this area?

Signed-off-by: Vincent Fugnitto vincent.fugnitto@ericsson.com

@vince-fugnitto vince-fugnitto added the cpp issues related to the C/C++ language label Mar 18, 2019
@simark
Copy link
Contributor

simark commented Mar 18, 2019

In the cpp language client, we would need to send the paths to the build directories as clangd expects it. This is not an upstream feature in clangd, but rather something that @HighCommander4 prototyped. You can find the branch here:

https://github.com/HighCommander4/llvm-project/commits/multi-project

You should be able to make sense of the code to deduce the expected format. There is also this example/test that he wrote, which can guide you:

https://github.com/simark/ls-interact/blob/master/test_clangd_multi_project.py#L47

@vince-fugnitto
Copy link
Member Author

vince-fugnitto commented Mar 18, 2019

@simark also, I was thinking of removing the cpp.buildConfigurations from the preferences and actually handle it through a separate file, something like to cpp.configs.json. Having the configs directly in the preferences seemed rather odd to me, and it also does not work well at the moment with a multiple root workspace even before my changes. Thank you for the feedback regarding the language changes, I'll have to take the time to see what's needed.

@simark
Copy link
Contributor

simark commented Mar 18, 2019

@simark also, I was thinking of removing the cpp.buildConfigurations from the preferences and actually handle it through a separate file, something like to cpp.configs.json. Having the configs directly in the preferences seemed rather odd to me, and it also does not work well at the moment with a multiple root workspace even before my changes. Thank you for the feedback regarding the language changes, I'll have to take the time to see what's needed.

This is what I had done originally (use a separate file), but reviewers made me change it to use preferences instead. In the end, I think it saves quite a bit of trouble, like watching for changes, validating the format, etc.

Can you explain what doesn't work well?

What I had in mind was that each source directory (i.e. each root) would have its own .theia/settings.json, with its own build configs in it. So we would know that these build configurations apply only to that root.

@paul-marechal
Copy link
Member

What I had in mind was that each source directory (i.e. each root) would have its own .theia/settings.json, with its own build configs in it. So we would know that these build configurations apply only to that root.

For some reason, the cpp.buildConfigurations looks broken when using a multi-root workspace (no config was picked up). We did not dig very deep into it either, just didn't work.

On the other hand, it would unclutter the settings. Using preferences, there is a corner case which I am not sure to know how it would be handled by the preference system: having configurations in the workspace file and in the sub folders, would the list be merged, or would one take precedence?

If we keep the preferences in a file under each root it seems to be easier in a mutli-root scenario?

@simark
Copy link
Contributor

simark commented Mar 19, 2019

On the other hand, it would unclutter the settings. Using preferences, there is a corner case which I am not sure to know how it would be handled by the preference system: having configurations in the workspace file and in the sub folders, would the list be merged, or would one take precedence?

When you say workspace file, you mean the json file that lists all the roots? If you put regular build configurations in there, they could be available as a choice for all the roots. There could also be an option that would select that build directory for all the roots.

But most importantly, I think the workspace file should contain "Umbrella build configurations" (for the lack of a better name). These would essentially list a build configuration name for the various roots, such that when you select an umbrella build config, it's the same as selecting the build config named for each root, it's just more convenient. So let's say you have 5 roots, for each you have Debug and Release build configs, then you could have two umbrella configurations, also Debug and Release. Selecting the Debug umbrella config would select the Debug config in each root.

When you say subfolders, do you mean root, or any subfolder? I don't see a useful scenario for having a build config in a random subfolder. But I think it makes sense to have them in the root. Let's say you start with a single project, where do you put the build configs? In the .theia/settings.json of your (single-root) workspace. Then, later, you add a second project, so you add a second root. The .theia/settings.json you have created already is already at the right place for the multi-root scenario.

@vince-fugnitto vince-fugnitto force-pushed the vf/cpp-multiple-root-config branch 2 times, most recently from 8af9c27 to e779e8c Compare March 26, 2019 19:07
@vince-fugnitto vince-fugnitto force-pushed the vf/cpp-multiple-root-config branch from e779e8c to 70fee21 Compare March 29, 2019 01:05
@vince-fugnitto vince-fugnitto marked this pull request as ready for review March 29, 2019 01:07
@vince-fugnitto vince-fugnitto force-pushed the vf/cpp-multiple-root-config branch 2 times, most recently from e347464 to d6dad75 Compare March 29, 2019 11:08
@paul-marechal paul-marechal force-pushed the vf/cpp-multiple-root-config branch 3 times, most recently from 07e70a8 to 2e12159 Compare April 4, 2019 19:58
@vince-fugnitto vince-fugnitto force-pushed the vf/cpp-multiple-root-config branch 2 times, most recently from 1d85f12 to 3d1e1a1 Compare April 5, 2019 14:52
@paul-marechal paul-marechal force-pushed the vf/cpp-multiple-root-config branch from 3d1e1a1 to efaa028 Compare April 5, 2019 14:57
@paul-marechal
Copy link
Member

@thegecko @arekzaluski is this interesting for you?

@paul-marechal paul-marechal force-pushed the vf/cpp-multiple-root-config branch 2 times, most recently from 2668602 to f616a80 Compare April 5, 2019 15:05
@arekzaluski
Copy link
Contributor

@marechal-p @vince-fugnitto Yes, it looks extremely interesting. We are looking at good solution to support C/C++ instellisense in all projects in user's workspace at the same time. At the moment we ended up with creating a single compilation database for all projects in the workspace. I'll take it for a spin and check how well it works for us.

@paul-marechal
Copy link
Member

paul-marechal commented Apr 5, 2019

At the moment we ended up with creating a single compilation database for all projects in the workspace.

This is what is done here as well, but with some additional experimental support through a configuration when using a special clangd patch. (cc @HighCommander4)

I don't know how you generate your "global" database, but in this PR we simply allow users to pick a build configuration (pointing to a folder containing the compile_commands.json for that build), for each root of a multi-root workspace. From these, if more than one configurations are used, we generate a "master" database which is just the sum of everything picked by the user.

Hopefully this helps you better understand what you'll see in these changes :)

@paul-marechal paul-marechal force-pushed the vf/cpp-multiple-root-config branch from f616a80 to 7ae00d5 Compare April 5, 2019 19:53
@paul-marechal paul-marechal force-pushed the vf/cpp-multiple-root-config branch from 7ae00d5 to 9810e46 Compare May 22, 2019 21:44
@vince-fugnitto
Copy link
Member Author

@marechal-p can we rebase and uplift the PR, hopefully we can get a review soon

@paul-marechal paul-marechal force-pushed the vf/cpp-multiple-root-config branch from 9810e46 to b04964d Compare June 20, 2019 22:19
@vince-fugnitto vince-fugnitto force-pushed the vf/cpp-multiple-root-config branch from b04964d to d3f85bf Compare July 23, 2019 11:09
@vince-fugnitto vince-fugnitto force-pushed the vf/cpp-multiple-root-config branch from d3f85bf to bafb15e Compare July 23, 2019 11:22
@vince-fugnitto
Copy link
Member Author

@marechal-p I rebased the PR and addressed the conflicts :)

@simark
Copy link
Contributor

simark commented Jul 23, 2019

@HighCommander4, I guess this patch still needs you patch to support multiple compilation databases in clangd? Is this still the latest version?

https://github.com/HighCommander4/llvm-project/commits/multi-project

@HighCommander4
Copy link
Contributor

@simark Yes, let me rebase it.

@simark
Copy link
Contributor

simark commented Jul 23, 2019

@simark Yes, let me rebase it.

It's a few months old, hope it's not too bad!

@HighCommander4
Copy link
Contributor

Rebased now (same link). Let me know if it works!

@paul-marechal
Copy link
Member

@HighCommander4 it works! I just need to tweak one of the paths we send, but your change works :)

@paul-marechal paul-marechal force-pushed the vf/cpp-multiple-root-config branch from bafb15e to 55aef49 Compare July 25, 2019 20:17
@vince-fugnitto vince-fugnitto force-pushed the vf/cpp-multiple-root-config branch from 55aef49 to 41fc2c4 Compare July 30, 2019 12:10
@vince-fugnitto
Copy link
Member Author

vince-fugnitto commented Jul 30, 2019

Rebased and resolved conflicts.

@vince-fugnitto vince-fugnitto force-pushed the vf/cpp-multiple-root-config branch from 41fc2c4 to 7bff86a Compare August 7, 2019 18:13
@vince-fugnitto
Copy link
Member Author

@marechal-p I rebased the code again and fixed conflicts.
Do you think the PR can be merged?

Added support for multiple root cpp build configurations.
Instead of maintaining a single active configuration, a map is
used to maintain the relationship between the workspace root and its
given `CppBuildConfiguration`. This feature is an experimental
PR based on new developments in clangd to support multiple
compilation databases.

By default, when selecting multiple build configurations, the extension
will create a merged compilation database. Added a preference to support
an experimental clangd feature (that might either change or be removed)
which would not generate this aggregated database, but rather just
notify clangd of all the databases the user wants to use.

Until this is officially supported you shouldn't set this experimental
preference, unless for testing purposes.

- Update the `cpp` manager to handle the new data structure.
- CppBuildConfigurationManager creates a merged compilation database
  when multiple configs are used.
- Update the test cases (fix the task test case with incorrect imports
  and false positive results).

Signed-off-by: Vincent Fugnitto <vincent.fugnitto@ericsson.com>
Signed-off-by: Paul Maréchal <paul.marechal@ericsson.com>
@paul-marechal paul-marechal force-pushed the vf/cpp-multiple-root-config branch from 7bff86a to 31fe008 Compare August 7, 2019 19:40
@paul-marechal
Copy link
Member

Ok, this has been around long enough, will merge once CI is done.

@paul-marechal paul-marechal merged commit b23e3cc into master Aug 7, 2019
@paul-marechal paul-marechal deleted the vf/cpp-multiple-root-config branch August 7, 2019 20:36
@akosyakov
Copy link
Member

It seems that this PR caused a system-wide regression: #5887 @marechal-p @vince-fugnitto @simark @HighCommander4 Could you have a look into it? I will do check myself tomorrow, if it confirms and there is no a quick fix, we have to revert a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cpp issues related to the C/C++ language
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants