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

CMake Tools and cpptools integration #198

Closed
dcourtois opened this issue Jun 30, 2017 · 10 comments
Closed

CMake Tools and cpptools integration #198

dcourtois opened this issue Jun 30, 2017 · 10 comments
Milestone

Comments

@dcourtois
Copy link
Contributor

Hi there, there's been quite a lot of stuff going on in the cpptools extension recently (the official C/C++ extension, that everyone using CMake is probably using :p) and I was wondering if we could make CMake Tools and cpptools work a bit better together.

For the moment, to use intellisense, you need to create a c_cpp_properties.json file, and in this file you need to specify the includePath and the defines you want to use. Both things that CMake knows perfectly well. So, that would be reeeeeaaally nice if CMake Tools could have the possibility to take over this file and update it depending on the currently selected build type ! Especially on big projects like the ones I'm working on where I have something like 50+ include paths to maintain and tens of defines, changing for each build type !

What do you think ? Does it seems feasible ? (as usual, I'll try to implement that as a POC, but in the meantime, wanted to share the idea)

@ytimenkov
Copy link
Contributor

There is another extension which does what you're asking for, CMake Tools Helper by @maddouri.
🤔 Was thinking about it this morning: should they be merged or kept separately?

On one hand they may have different responsibilities: CMake Tools is a language-agnostic, brings functionality of running CMake to VS Code, while Helper introduces support for particular language (C++) and integrates with particular extension (from cpptools from Microsoft).

One the other hand, there are already some assumptions on cpptools and having single complete solution provides much better user experiencer than need to install 4 different extensions.
Besides, CMake is used for C++ project in 90% cases (my wild guess. other languages just complementary).
Detecting presence of cpptools dynamically is fairly easy.

@ytimenkov
Copy link
Contributor

Related discussion is going in #174.

@dcourtois
Copy link
Contributor Author

Ah, didn't know about CMake Tools Helper. Currently testing this. If it works, I think I'll close this issue.
I think it's still reasonable to let another extension handle this, and keep CMake Tools as simple as possible.

@dcourtois
Copy link
Contributor Author

dcourtois commented Jun 30, 2017

Hmm, I've been trying to get CMake Tools Helper to work correctly, and I didn't have much chance.
Also the fact that it doesn't support the "all" target (which is the target I'm usually using) is a big show stopper for me.

Adding basic c_cpp_properties.json generation is pretty easy (I've done it already) and works fine. The problem is for the "all" target. On my project, I simply join the include paths and defines of all my targets in this case, but I can see how this would be really bad on other setup :P

I think a reasonable approach would be to add a "cmake.cpptools.defaultTarget" (or something equivalent) that would be used any time the current target is "all". And if this option is not set, just notify the user via a popup, log or whatever. What do you think ?

Edit: ok, that was simpler than I thought. It's mostly done, everything in the c_cpp_properties.json can be configured through options, and the defines + include paths are correctly updated. The all target can be forwarded to an existing target, and if no target is found, it falls back to the first target available and notifies the user. I'm going to test it on my bigger projects, see how it behaves.

Edit 2: works fine on my bigger (and much more complex) professional project, at the exception of: the standard include paths are not explicitly specified in CMakeLists.txt, so they are not added to c_cpp_properties.json ... I need to figure out how to detect and add those.

@maddouri
Copy link
Contributor

maddouri commented Jun 30, 2017

Hello,

Thank you @ytimenkov for mentioning the Helper extension :)

Was thinking about it this morning: should they be merged or kept separately?

I would like to merge the cpptools-related work of the Helper extension into CMake Tools but there are some points that have to be addressed first. You can find the current state of this effort at maddouri/vscode-cmake-tools-helper#2 (btw, I worked with cpptools because, IMHO, currently it is the most useful C++ LSP extension available... even though it's not perfect. When the clang language server extension gets more feature-rich, I will definitely try to interface it with CMake Tools)

Other features, such as downloading/installing (and perhaps, in the future, compiling) CMake, might be interesting to merge as well. It could be useful to discuss these in separate issues, what do you think? (perhaps CMake Tools Helper could serve as a testing ground for experimental features that, if deemed useful by the users, might be proposed for merging into CMake Tools?)

@dcourtois I will try to answer your questions here, but feel free to take the discussion to the Helper extension's repo https://github.com/maddouri/vscode-cmake-tools-helper if you have suggestions/bug reports ;)

  • Handling the "all" target: I agree that it's a shame I don't handle it in the extension. l am open to using all the includes/defines... perhaps I'll add an option like "cmake_tools_helper.all_target_behavior: "null|all_includes_defines"
  • c_cpp_properties.json: The whole point of the extension is to handle this file for you, you shouldn't be editing it. (the extension will overwrite it if you change the CMake target... more details on the extension's readme/marketplace page)
  • Standard includes/defines: again, it's a shame, but I couldn't find a way to get them from CMake Tools. If you have a solution to get them from anywhere, please consider sharing it. Here's the relevant issue in the extension's repo retain c_cpp_properties.json information maddouri/vscode-cmake-tools-helper#4

I hope it helps!

@dcourtois
Copy link
Contributor Author

Hi. The only real question that I had was : does specifying a default target in the settings to use when a special target (e.g. all, install, etc) is select seems appealing to anyone reading this PR ?

To specifically answer your 3 points :

  • I don't want those options: null is out of the question, most of the time I'm on "all" target, and I want my intellisense to work. And all_include_defines is also not realistic for big projects: we have something like 10 targets producing various helper tools, each linking with various libraries, etc. If I were to add all the include paths of all the targets, I would include half of the world ... As I said: setting a "defaultTarget" is working perfectly even for big projects: this setting can go to the workspace settings and be embedded with the project, and it works as soon as the user specify an invalid target.

  • I'm not manually editing it, the purpose of the issue, and what I did on my repo was to do that automatically, like your extension, so I don't really understand this point ?

  • for this one, I need to check how the cpptools extension works, but if I can get it to tell me what the current compiler is, it should be pretty straightforward: all compilers have command line options to dump their standard include paths.

And to clarify : I like when things are simple. If CMake Tool Helpers would have worked on my project, I would have been happy to use it. But as it stands, it doesn't. And doesn't tell me why (no popup, no log) And it doesn't handle "all" anyway. So if I need to spend time investigating why something doesn't work, and eventually fixing things myself, I will tend to do it on CMake Tools since that would allow me to only have 1 extension to install.

@dcourtois
Copy link
Contributor Author

WIP of cpptools integration here : https://github.com/dcourtois/vscode-cmake-tools/tree/feature/cpptools_integration (I will add a pull request when the last glitches are fixed. See the end of the comment)

Features:

  • Allow full control of the configuration of what's in c_cpp_properties.json (e.g. you can configure the intellisense mode and all the options of the file in the settings)
  • Automatically update the file when the built type changes and when the target changes. If something's wrong (invalid target or whatever) the user is notified.
  • For all targets that do not have include paths, defines, etc. (basically all target of type META : "all", "install", etc.) you can specify another target name to fallback to in the settings.
  • You can specify additional include paths in the settings to handle standard include paths (didn't find a way to get the standard include paths from CMake)
  • All of the above is optional. You can disable the integration with cpptools with a simple boolean (it's actually disabled by default)

I still need to fix some corner cases (in some cases the c_cpp_settings.json file is not created / updated correctly, need to fix that) but basically it's working like a charm.

@dcourtois
Copy link
Contributor Author

Pull request ready, all last little things that were bugging me are now fixed.
You can check it here: #201

@Flamefire
Copy link

VSC now supports compilation databases. Could CMake Tools just set this according to the current config (build-directory)?

@vector-of-bool
Copy link
Contributor

CMake Tools 1.1 will support enhanced configuration for cpptools. I expect to release before the end of July.

@vector-of-bool vector-of-bool added this to the 1.1.0 milestone Jul 12, 2018
@github-actions github-actions bot locked and limited conversation to collaborators Jan 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants