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

auto-populate the browse.path when compileCommands is used #1715

Closed
tksuoran opened this issue Mar 21, 2018 · 17 comments
Closed

auto-populate the browse.path when compileCommands is used #1715

tksuoran opened this issue Mar 21, 2018 · 17 comments

Comments

@tksuoran
Copy link

I try to get use compile commands json, but I do not get it to work. I have included a minimal repro project files inline below.

Repro steps - case 1 - define:

  • Open foo/main.c (and have no other files open)
  • Hover mouse over RETURN_VALUE
  • Right click and "Go to Definition"

Result I observe:

  • Message "No definition found for 'RETURN_VALUE'"

Expected result:

  • Editor opens bar/header.h

I also cannot use Go to Definition to navigate to the header file. If header.h is already open, then Go to Definition works.

  • Extension Name: cpptools
  • Extension Version: 0.15.0
  • OS Version: Linux x64 4.13.0-37-generic
  • VSCode version: 1.21.1

We have written the needed data into your clipboard. Please paste:

{
	"activationTimes": {
		"startup": false,
		"codeLoadingTime": 123,
		"activateCallTime": 7,
		"activateResolvedTime": 526,
		"activationEvent": "onLanguage:c"
	}
}

.vscode/c_cpp_properties.json:

{
    "configurations": [
        {
            "name": "Linux",
            "compileCommands": "${workspaceRoot}/compile_commands.json",
            "includePath": [],
            "defines": [],
            "intelliSenseMode": "clang-x64",
            "browse": {
                "path": [],
                "limitSymbolsToIncludedHeaders": false,
                "databaseFilename": ""
            }
        }
    ],
    "version": 3
}

.vscode/settings.json:

{
    "C_Cpp.intelliSenseEngine": "Default"
}

compile_commands.json:

[
    {
        "directory": "foo",
        "file": "main.c",
        "arguments":
        [
            "/usb/bin/clang",
            "-Ibar",
            "foo/main.c"
        ],
        "command": "/usr/bin/clang -Ibar foo/main.c"
    }
]

foo/main.c:

#include "header.h"

int main(int argc, char** argv)
{
    return RETURN_VALUE;
}

bar/header.h:

#define RETURN_VALUE (0)

build.sh:

#!/bin/bash
clang -Ibar foo/main.c
@sean-mcmanus
Copy link
Contributor

Go to Definition doesn't use compile_commands.json data yet, which is only used to configure includePath and defines for IntelliSense -- only the browse.path affects Go to Definition, and it's recursive. I think we're using #255 to track that. Although it would be relatively simple to enable Go to Definition for definitions in the current translation unit (or declarations), enabling this for definitions in other translations is a major task.

Let us know if you're experiencing other bugs with compile_commands.json. Our pending 0.16.0 should fix some compile_commands.json issues, such as enabling forced includes and compiler defines to be picked up correctly.

@sean-mcmanus sean-mcmanus added duplicate more info needed The issue report is not actionable in its current state labels Mar 21, 2018
@sean-mcmanus
Copy link
Contributor

Uh...but maybe we could use the include paths parsed from the compile_commands to populate the browse.path? The problem is that it wasn't designed to be used that way.

@tksuoran
Copy link
Author

My understanding is that both "includePath" and "browse" are inferior, because they are global. IMHO this is a design flaw. Global settings for includes and defines could perhaps work for a single "visual studio project", for example a library, but not a whole "visual studio solution" that contains several libraries, each with their own include paths and defines. And even within single project, sometimes there are file specific settings (includes, defines etc.).

compileCommands allows to set includes and defines individually for each file, and it allows to use VSCode similar to a Visual Studio solution which contains multiple projects.

Populating browse from compileCommands obviously would fail when different files have different settings. The settings should be file specific, and compileCommands does decent job with that.

@sean-mcmanus
Copy link
Contributor

sean-mcmanus commented Mar 22, 2018

The VS Code workspace and workspaceFolder correspond to the VS solution and project, so in that respect the includePath is settable per-folder/project, but not file-specific ones that compile commands enables. The other alternative method using one workspaceFolder is to switch configs when you change to subfolders with different includePaths, but that would be sort of annoying to do if you switching a lot.

The browse.path is for global symbols for the entire workspace folder. Only one database is created. Typically, you just set it to ${workspaceFolder} and then any external libraries that are not part of the workspace. browse.path is not used in compilation of a file like the includePath is.

We discussed having the includePath be a more complicated structure that enabled it to be set on a per directory/file basis, but decided it was an unnecessary complication and we haven't any user requests for that yet.

My idea for populating the browse.path based on the compileCommands would basically be to just add all the paths (assuming our code that removes duplicates works correctly). However, this may not be a good idea, and it might be better to force users to manually set it to the root paths they want symbols for.

And our goal is to replace most of the browse.path-based features with the "IntelliSense" implementation (which would use compile commands). We only have single translation unit IntelliSense implemented right now though, so external definitions require the browse.path symbol info.

@bobbrow
Copy link
Member

bobbrow commented Mar 22, 2018

If browse.path is removed as part of the settings refactor (and recursive includes feature), then the extensionwould need to compute the equivalent of browse.path for the compile_commands.json case. I would want to do this task as part of that work to reduce the potential for confusion.

@bobbrow bobbrow changed the title c_cpp_properties.json / compileCommands / Go to Definition not working auto-populate the browse.path when compileCommands is used Jun 26, 2018
@sean-mcmanus sean-mcmanus added this to the Triage milestone Dec 22, 2018
@sean-mcmanus sean-mcmanus removed the more info needed The issue report is not actionable in its current state label Jan 4, 2019
@johschmitz
Copy link

I am facing the issue Include file not found in browse.path. on a header file when using cpptools in a CMake project with a compile_commands.json.
I have cross-checked that the include path containing the header file is available in compile_commands.json.
The compile_commands.json itself is detected and added to c_cpp_properties.json so I would expect Intellisense to work without any further manual configuration.
What is the current status of this?

I guess this is related to issues #1163 and #2940? Which one is the relevant one for me?

@bobbrow bobbrow modified the milestones: Triage, Backlog May 2, 2019
@sean-mcmanus
Copy link
Contributor

sean-mcmanus commented May 3, 2019

@johschmitz The status is that we don't currently plan to fix it. Both the linked issues seem like the same issue as this.

@ed-alertedh
Copy link

What I gather from other issues is that the reason for holding off is due to an eventual plan to consolidate all the paths into one setting, is that a fair summary?

I would really like this feature as I am trying to use conan to manage my dependencies and this means the locations of the headers are dynamic, e.g. /home/devuser/.conan/data/zlib/1.2.11/conan/stable/package/880e5642afc26dae7f4019501d6632b3a6dba009/include

This seems to be the only remaining thing stopping me from being able to configure everything with Conan and CMake without needing to manually keep c_cpp_properties.json in sync. As it is, having the include paths automatically set up is a pretty nice start!

@sean-mcmanus
Copy link
Contributor

sean-mcmanus commented Jul 3, 2019

The reason is that the work hasn't been high enough priority, but since we're fixing compile_command.json related issues for July/August, maybe we could fix this?

In most cases, we expect users to be able to set the browse.path to the root of all folders that will need to have global symbols found, as a workaround for us not automatically doing that based on the compile_commands.json.

@sean-mcmanus
Copy link
Contributor

sean-mcmanus commented Jul 15, 2019

I think the command is "-I", which we support ("bar" is the argument folder). The issue is we don't use the -I/etc. include paths to populate the browse.path...might be as simple as copying them over?

@Jasdriel
Copy link
Contributor

I am actually trying to replicate the problem but for me go to definition works well using only compile commands

@sean-mcmanus
Copy link
Contributor

The -I path has to be outside of the ${workspaceFolder} (I think that's used as the default browse.path).

@Jasdriel
Copy link
Contributor

Jasdriel commented Jul 15, 2019

I create an external folder with a header, and still working.

@johschmitz
Copy link

johschmitz commented Jul 16, 2019

When you have additional libraries that you need to link against installed in /opt/libname and you have set LD_LIBRARY_PATH and PKG_CONFIG_PATH to let cmake find them, does that work, does vscode find those header files now just by looking at the compile_commands.json?

@sean-mcmanus
Copy link
Contributor

@johschmitz Our extension never deals with "linking". If the libraries have headers that are referenced in the compile_commands.json, then it will correctly be used for IntelliSense, but the files are only parsed after being opened (or from another opened file that includes it), so the symbols may not be found globally, i.e. Go to Def may fail (if they're not under the workspace folder). This fix for this issue would be to make sure all the paths referenced in the compile_commands.json get added to the browse.path.

@johschmitz
Copy link

I am aware that the extension doesn't deal with linking, I just wanted to provide a complete scenario or maybe you call it "user story".
What I would expect is what you describe in your last sentence. All referenced paths should be added indeed.

@bobbrow
Copy link
Member

bobbrow commented Mar 12, 2020

Duplicate of #1163

@bobbrow bobbrow marked this as a duplicate of #1163 Mar 12, 2020
@bobbrow bobbrow closed this as completed Mar 12, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Oct 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants