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

Configuration improvements #1338

Closed
csholmq opened this issue Dec 12, 2017 · 21 comments
Closed

Configuration improvements #1338

csholmq opened this issue Dec 12, 2017 · 21 comments
Assignees
Labels
Feature Request fixed Check the Milestone for the release in which the fix is or will be available. Language Service
Milestone

Comments

@csholmq
Copy link
Contributor

csholmq commented Dec 12, 2017

When creating a new c_cpp_properties.json the default system includes comes first meaning that any project modules with the same name is not included. (E.g Io.h gets matched in C:/Program Files (x86)/Windows Kits/10/Include/10.0.14393.0/ucrt before the project specific)

Similarly, adding local paths gets appended to the end of the includePath array.

image

{
    "name": "Win32",
    "includePath": [
        "C:/Program Files (x86)/Windows Kits/10/Include/10.0.14393.0/um",
        "C:/Program Files (x86)/Windows Kits/10/Include/10.0.14393.0/ucrt",
        "C:/Program Files (x86)/Windows Kits/10/Include/10.0.14393.0/shared",
        "C:/Program Files (x86)/Windows Kits/10/Include/10.0.14393.0/winrt",
        "${workspaceRoot}",
        "${workspaceRoot}/A_XMC/out/obj"
    ],
    "defines": [
        "_DEBUG",
        "UNICODE"
    ],
    "intelliSenseMode": "msvc-x64",
    "browse": {
        "path": [
            "C:/Program Files (x86)/Windows Kits/10/Include/10.0.14393.0/um",
            "C:/Program Files (x86)/Windows Kits/10/Include/10.0.14393.0/ucrt",
            "C:/Program Files (x86)/Windows Kits/10/Include/10.0.14393.0/shared",
            "C:/Program Files (x86)/Windows Kits/10/Include/10.0.14393.0/winrt",
            "${workspaceRoot}"
        ],
        "limitSymbolsToIncludedHeaders": true,
        "databaseFilename": ""
    }
}
@sean-mcmanus
Copy link
Collaborator

Yeah, changing the ordering makes sense to me.

@bobbrow
Copy link
Member

bobbrow commented Dec 13, 2017

The trouble is going to be that the default ordering will be temporary if developers use the light bulb to add include paths to this property (since they are added at the end and not in the middle). We really need to split out the system includes from the project includes. I hate to add yet another 'include path' property but there may be an opportunity to do this if we move c_cpp_properties.json to settings.json in the near future. @csholmq, do you mind if we morph this issue into a design spec for how include paths could be handled in the post-c_cpp_properties.json world? I would like to hear the community's input on the subject as well.

@csholmq
Copy link
Contributor Author

csholmq commented Dec 13, 2017

I don't mind this being merged into something bigger.

Will settings.json support project specific includes? I would envision some generic ones and then project specific ones in a separate array that gets included first.

To be honest the system default ones mostly messes up my workflow since I have to remove them for every project I open as I work with embedded ones. So to be able to setup a default setting which can be overwritten would probably solve that.

@sean-mcmanus
Copy link
Collaborator

@bobbrow I assumed we should change the code action include to add the settings to the front of the list instead of the back (I haven't checked if there's a problem doing that).

@csholmq Yeah, there would be user settings that apply to all projects, and then workspace settings for the particular project, just like the normal settings.json, but we'd also have a list of configurations in the settings as well, but we haven't ironed out all the details.

@csholmq
Copy link
Contributor Author

csholmq commented Dec 18, 2017

An interim solution would be to add project specific includes to the front of the array.

@sean-mcmanus
Copy link
Collaborator

We're still discussing how to handle this (we should have more info in Jan).

@bobbrow
Copy link
Member

bobbrow commented Apr 12, 2018

Sorry for the delay. This has been a long time coming and I've gone through a couple of iterations internally before getting this into a state that is ready for comments. Most notably, we've decided that removing c_cpp_properties.json would be extremely disruptive to the community and extensions that take a dependency on us and have decided against it. We have an alternative plan that should be able to adequately address the problems that led us to consider removing it in the first place. This plan will allow us to make c_cpp_properties.json an optional configuration tool.

related issues:
#368 - global include path
#410 - include path override for generated paths
#1229 - single c_cpp_properties.json for entire workspace
#1270 - user default include path

New VS Code settings

First, we will be adding new C_Cpp.default.* settings that map to each of the properties in a configuration block of c_cpp_properties.json. Namely:

C_Cpp.default.includePath                          : string[]
C_Cpp.default.defines                              : string[]
C_Cpp.default.compileCommands                      : string
C_Cpp.default.macFrameworkPath                     : string[]
C_Cpp.default.forcedIncludes                       : string[]
C_Cpp.default.intelliSenseMode                     : string
C_Cpp.default.compilerPath                         : string
C_Cpp.default.cStandard                            : c89 | c99 | c11
C_Cpp.default.cppStandard                          : c++98 | c++03 | c++11 | c++14 | c++17
C_Cpp.default.browse.path                          : string[]
C_Cpp.default.browse.databaseFilename              : string
C_Cpp.default.browse.limitSymbolsToIncludedHeaders : boolean

These settings have all of the benefits of VS Code settings, meaning that they can have default, "User", "Workspace", and "Folder" values. So you can set a global value for C_Cpp.default.cppStandard in your "User" settings and have it apply to all of the folders you open. If any one folder needs a different value, you can override the value by adding a "Folder" or "Workspace" value.

Updated c_cpp_properties.json syntax

Second, we will add a special variable to the accepted syntax of c_cpp_properties.json that will instruct the extension to actually use the default value from the VS Code settings. If you set the value of any setting in c_cpp_properties.json to "${default}" it will instruct us to read the VS Code default setting for that property and insert it. For example:

"configurations": [
    {
        "name": "Win32",
        "includePath": [
            "additional/paths",
            "${default}"
        ],
        "defines": [
            "${default}",
        ],
        "macFrameworkPath": [
            "${default}",
            "additional/paths"
        ],
        "forceInclude": [
            "${default}",
            "additional/paths"
        ],
        "compileCommands": "${default}",
        "browse": {
            "limitSymbolsToIncludedHeaders": true,
            "databaseFilename": "${default}",
            "path": [
                "${default}",
                "additional/paths"
            ]
        },
        "intelliSenseMode": "${default}",
        "cStandard": "${default}",
        "cppStandard": "${default}",
        "compilerPath": "${default}"
    }
],

Take note that for the properties that accept string[], the syntax proposed above allows you to augment the VS Code setting with additional values, thus allowing you to have common paths listed in the VS Code settings and configuration-specific settings in c_cpp_properties.json.

If a property is missing from c_cpp_properties.json, the extension will use the value in the VS Code setting. If a developer assigns values to all of the settings that apply for a given folder, then c_cpp_properties.json could be removed from the .vscode folder as it will no longer be needed.

System includes

Third, a new setting will be added that allows you specify the system include path separate from the folder's include path. If this setting has a value, then the system include path the extension gets from the compiler specified in the compilerPath setting will not be added to the path array that the extension uses for IntelliSense. We may want to provide a VS Code command to populate this value from the compiler's default for users who are interested in using it in case they want to make some modifications to the defaults.

C_Cpp.default.systemIncludePath                    : string[]

Include Path Resolution Strategies

The extension determines the includePath to send to the IntelliSense engine in the following manner:

  1. If compileCommands has a valid value and the file open in the editor is in the database, use the compile command in the database entry to determine the include path and defines.

    • The system include path and defines are determined using the following logic (in order):
      1. If systemIncludePath has a value, use it (continue to the next step to seach for system defines).
      2. If compilerPath is valid, query it.
      3. Interpret the first argument in the command as the compiler and attempt to query it.
      4. If compilerPath is "", use an empty array for system include path and defines.
      5. If compilerPath is undefined, look for a compiler on the system and query it.
  2. If compileCommands is invalid or the current file is not listed in the database, use the includePath and defines properties in the configuration for IntelliSense.

    • The system include path and defines are determined using the following logic (in order):
      1. If systemIncludePath has a value, use it (continue to the next step to seach for system defines).
      2. If compilerPath is valid, query it.
      3. If compilerPath is "", use an empty array for system include path and defines (they are assumed to be in the includePath and defines for the current config already).
      4. If compilerPath is undefined, look for a compiler on the system and query it.

System includes should no longer be added to the "includePath" or "browse.path" variables. If the extension detects any system include paths in the "includePath" property it will silently remove them so that it can ensure system include paths are added last and in the correct order (this is especially important for GCC/Clang). In a future update we may add a notification message to the extension to remind developers to remove system include paths from their "includePath" and 'browse.path" as they will be ignored.

Advanced Scenarios

For more complex projects that would like to define base configurations and inherit from them, we can also add support for that too, but it will not come in the first wave of changes. Adding support for this will be conditional on the demand for it, so the following is merely a rough outline of how if might work and may need to be refined a bit. The basic concept would be that c_cpp_properties.json could import configs from another json file and inherit them using the following syntax:

{
    "configurations": [ ... ],
    "import": [ "/path/to/config", "/path/to/another/config" ],
    "version": 4
}

Using this syntax, you can specify either "configurations", or "import", or both. Missing both properties results in an error. The imported configuration files do not need to be named "c_cpp_properties.json". They just need to conform to the schema.

The configuration schema would add a new property: "basedOn": string which would be the name of the configuration that is being inherited. Any properties not set by the child configuration would be inherited from the parent configuration.

For example:

"configurations": [
    {
        "name": "default",
        "intelliSenseMode": "${default}",
        "cStandard": "${default}",
        "cppStandard": "${default}",
        "compilerPath": "${default}"
    },
    {
        "name": "config1",
        "basedOn": "default",
        "includePath": [ ... ],
        "defines": [ ... ],
        "cppStandard": "c++14"
    }
]

config1 would inherit the "cStandard", "intelliSenseMode", and "compilerPath" properties from the "default" configuration, and override the "cppStandard" property. For properties like "includePath" where you might want to augment an existing includePath from the parent configuration, we could add a ${base} or ${parent} variable that instructs us to look up the hierarchy for a value to insert at that location.

@csholmq
Copy link
Contributor Author

csholmq commented Apr 12, 2018

Is omitting a configuration option the same as specifying it to ${default}?

I don't fully understand if you are enabling the issue with system defaults (e.g system defaults) to not Trump project specific ones. Do I need to set the default includePath to blank to achieve this?

@bobbrow
Copy link
Member

bobbrow commented Apr 12, 2018

Is omitting a configuration option the same as specifying it to ${default}?

Yes.

I don't fully understand if you are enabling the issue with system defaults (e.g system defaults) to not Trump project specific ones. Do I need to set the default includePath to blank to achieve this?

EDITED:
We will be treating system includes as special and we will actually secretly remove them from your includePath if you put any of them there so that we can ensure they are always last. We are actually already doing this in some form on mac/linux in 0.16.x due to the number of issues we've seen with incorrectly configured include paths. The goal is for everyone to be able to remove system includes from the "includePath" but you do not need to set your includePath to blank to achieve this. I will update the spec to call this out.

@bobbrow bobbrow changed the title Project includes should trump system default includes Configuration improvements Apr 12, 2018
@sean-mcmanus
Copy link
Collaborator

sean-mcmanus commented Apr 12, 2018

@bobbrow We currently do "a. Interpret the first argument in the command as the compiler and attempt to query it." only after the compilerPath was deemed invalid. Did you intend to change that? Was there a reason? If so, then we'd need to fix the bug where "other" compile command args aren't being passed to the compiler for querying defines/includes to avoid a regression. The original intent was for the compilerPath to take precedence for when the compiler found in the compile commands was invalid, but the new way could be made to work too.

Also, is the compilerPath still queried for system defines when systemIncludePath is set?

@bobbrow
Copy link
Member

bobbrow commented Apr 12, 2018

Thanks for pointing that out @sean-mcmanus. That was an unintentional mistake. I will fix it.

@floooh
Copy link

floooh commented Apr 12, 2018

Hi, as I'm reading this, the system include paths detected by VSCode will now be separate from the 'user defined' includes paths? If this is correct then "hooray" \o/ because currently I'm doing all sorts of hacks to detect the system includes since I'm generating c_cpp_properties.json files 'out of thin air' in my cmake wrapper (with things like trying to figure out what Visual Studio versions and what Windows Kits are installed (see here for the OSX/Linux clang version: https://github.com/floooh/fips/blob/9681f85be1e220b7539f5abed0751f959d837f23/mod/tools/vscode.py#L105, and here for the Visual Studio version: https://github.com/floooh/fips/blob/9681f85be1e220b7539f5abed0751f959d837f23/mod/tools/vscode.py#L140).

It would be really great if:

  • either the system include paths would be 'auto-populated' in a location that's managed by VSCode, and where they won't be overwritten by project- or workspace-specific settings
  • or add an easy way to get the system include paths by calling some VSCode functionality from another tool (python, cmake, etc...)... best would be calling code with a command line argument, and from there dumping the output to stdout

@bobbrow
Copy link
Member

bobbrow commented Apr 12, 2018

@floooh, if you are generating c_cpp_properties.json, then you should only need to set the "compilerPath" property going forward and no longer worry about adding the system includes to the "includePath" property. They will be 'auto-populated' internally, but not explicitly listed in c_cpp_properties.json anymore. You could also not set the "compilerPath" at all and let the extension pick a default value, but if you know what the correct answer is, you might as well set it when you generate the file.

@bobbrow
Copy link
Member

bobbrow commented Apr 24, 2018

A preview of this is available in the insiders build if you would like to try it out and provide feedback.

@bobbrow
Copy link
Member

bobbrow commented May 8, 2018

This is implemented in 0.17.0.

I also added a "env" property to c_cpp_properties.json to enable creating variables shared across configs for cases where the "Advanced Scenario" syntax is overkill.

{
    "env": { ... },
    "configurations": { ... },
    "version": 4
}

@bobbrow bobbrow closed this as completed May 8, 2018
@dsanz006
Copy link

Hi,
In my current VS Code workspace I have 3 different configurations in my C_Cpp_properties.json file. I would like to get rid of this file and have all my workspace settings (both related to VS Code and to the C/C++ Extension) in the .code-workspace file. However, as far as I understand, I can only have one configuration (the "default" one) under the "settings" object of my .code-workspace, and I am not allowed to have different values for "C_Cpp.default.includePath" and "C_Cpp.default.defines" for each one of my configurations. Is this correct?

My goal is to have 1 single workspace configuration file for everything...

@bobbrow
Copy link
Member

bobbrow commented May 30, 2018

@dsanz006, you are correct. Multiple configurations are not currently served by this feature. It looks like you already opened #2060 to request this. Thanks.

@dsanz006
Copy link

dsanz006 commented Jun 6, 2018

Yes, this request is similar to #2060, but it is actually a combination of 2 requests:

  • Having a single C/C++ configuration file per workspace, even if it is multi-root (not one per folder).

  • Allowing different C/C++ configurations in the .code-workspace file, without a need for the C_Cpp_properties.json file on workspaces with multiple configurations.

@bobbrow
Copy link
Member

bobbrow commented Jun 6, 2018

If there are two requests, can you clarify in #2060 which one it covers and then open another issue for the second one?

@dsanz006
Copy link

dsanz006 commented Jun 7, 2018

Done, I just opened #2096. Thank you.

@bobbrow
Copy link
Member

bobbrow commented Jun 7, 2018

Thanks, @dsanz006

@github-actions github-actions bot locked and limited conversation to collaborators Oct 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Feature Request fixed Check the Milestone for the release in which the fix is or will be available. Language Service
Projects
None yet
Development

No branches or pull requests

5 participants