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

Added system to store and query properties from the active C/C++ configuration #5453

Merged
merged 8 commits into from
Jun 15, 2020

Conversation

bugengine
Copy link
Contributor

I ran into complexity issues when setting up configurations in cpptools. The main issue was that there was no way to affect the launch configuration depending on the currently active C++ configuration. In particular, my C++ configurations place the output binary in different folders. From the launch.json file, I did not have a way to query the output path or any property, that would allow a single launch configuration to "do the right thing". The workaround was to create one launch configuration per C++ configuration, which is a lot of additional work to maintain.
I described the issue on StackOverflow there: https://stackoverflow.com/questions/61685124/how-can-i-tie-together-the-c-c-configurations-in-vscode-with-the-tasks-and-lau

I went ahead and added a system that lets users store arbitrary properties in the C/C++ configuration files and to query them using a new command.

Example c_cpp_properties:

{
    "configurations": [
        {
            "name": "linux_gnu_amd64-clang_amd64-10.0.0:debug",
            "includePath": [],
            "defines": [],
            "compileCommands": "${workspaceFolder}/.vscode/linux_gnu_amd64-clang_amd64-10.0.0/debug/compile_commands.json",
            "properties": {
                "toolchain": "linux_gnu_amd64-clang_amd64-10.0.0",
                "variant": "debug"
            }
        },
        {
            "name": "linux_gnu_amd64-clang_amd64-10.0.0:final",
            "includePath": [],
            "defines": [],
            "compileCommands": "${workspaceFolder}/.vscode/linux_gnu_amd64-clang_amd64-10.0.0/final/compile_commands.json",
            "properties": {
                "toolchain": "linux_gnu_amd64-clang_amd64-10.0.0",
                "variant": "final"
            }
        }
    ],
    "version": 4
}

How to use it in launch.json:

{
    "version": "0.2.0",
    "configurations": [
        {
            "name": "(gdb) Launch",
            "type": "cppdbg",
            "request": "launch",
            "program": "${workspaceFolder}/bld/${input:toolchain}/${input:variant}/bin/a.out",
            "args": [],
            "stopAtEntry": false,
            "cwd": "${workspaceFolder}",
            "environment": [],
            "externalConsole": false,
            "MIMode": "gdb"
        }
    ],
    "inputs": [
        {
            "id": "toolchain",
            "type": "command",
            "command": "cpptools.activeConfigProperty",
            "args": "toolchain"
        },
        {
            "id": "variant",
            "type": "command",
            "command": "cpptools.activeConfigProperty",
            "args": "variant"
        }
    ]
}

@msftclas
Copy link

msftclas commented May 10, 2020

CLA assistant check
All CLA requirements met.

@sean-mcmanus sean-mcmanus added this to the 0.29.0 milestone May 11, 2020
@sean-mcmanus sean-mcmanus self-requested a review May 11, 2020 19:15
@sean-mcmanus sean-mcmanus requested review from a team, michelleangela and Colengms June 10, 2020 17:30
@sean-mcmanus
Copy link
Collaborator

@bugengine All the other properties in c_cpp_properties.json have a corresponding C_Cpp.default.* setting to enable a default to be set when no c_cpp_properties.json exist. Do you think adding that would be good? If not, it seems like we could wait for other user requests for it.

@bugengine
Copy link
Contributor Author

Hi Sean, I added the possibility to set default properties in the settings like other entries. While doing this I used the convention that if in a configuration, the "${default}" property is added, then the default properties from the settings will be pulled from the configuration. Some notes:

  • I'm not very happy with the syntax, but I have not managed to find better; the property block can contains a dummy entry with a key "${default}" and the value is whatever, it will be discarded
  • In case a key appears in both the default property set and the configuration, the configuration always overrides the default
  • The values containing ${env:}, ${workspaceFolder} and so on will be expanded (even if they come from the default properties)

While implementing this I noticed that the setting cannot be edited with the UI and I did not manage to implement this as I did not find a control for editing dictionaries of strings (other settings are string or list of strings), so the properties can only be set by editing the JSON file.

@michelleangela
Copy link
Contributor

@bugengine All the other properties in c_cpp_properties.json have a corresponding C_Cpp.default.* setting to enable a default to be set when no c_cpp_properties.json exist. Do you think adding that would be good? If not, it seems like we could wait for other user requests for it.

Does it make sense to have a default setting for the usage purpose of this new "property"?

@sean-mcmanus
Copy link
Collaborator

Hi Sean, I added the possibility to set default properties in the settings like other entries. While doing this I used the convention that if in a configuration, the "${default}" property is added, then the default properties from the settings will be pulled from the configuration. Some notes:

  • I'm not very happy with the syntax, but I have not managed to find better; the property block can contains a dummy entry with a key "${default}" and the value is whatever, it will be discarded
  • In case a key appears in both the default property set and the configuration, the configuration always overrides the default
  • The values containing ${env:}, ${workspaceFolder} and so on will be expanded (even if they come from the default properties)

While implementing this I noticed that the setting cannot be edited with the UI and I did not manage to implement this as I did not find a control for editing dictionaries of strings (other settings are string or list of strings), so the properties can only be set by editing the JSON file.

Yeah, the settings UI is limited in it's ability to work with objects/dictionaries. I think it would require changes from VS Code.

@sean-mcmanus
Copy link
Collaborator

@bugengine All the other properties in c_cpp_properties.json have a corresponding C_Cpp.default.* setting to enable a default to be set when no c_cpp_properties.json exist. Do you think adding that would be good? If not, it seems like we could wait for other user requests for it.

Does it make sense to have a default setting for the usage purpose of this new "property"?

I'm not sure, but it seems possible. Do you disagree?

@michelleangela
Copy link
Contributor

Regarding the purpose of this new "property":

added a system that lets users store arbitrary properties in the C/C++ configuration files and to query them using a new command.

Does it make sense to add it to the C/C++ configuration settings? The C/C++ configuration settings primary purpose is to set information for IntelliSense engine and not info for launching.

@sean-mcmanus
Copy link
Collaborator

Regarding the purpose of this new "property":

added a system that lets users store arbitrary properties in the C/C++ configuration files and to query them using a new command.

Does it make sense to add it to the C/C++ configuration settings? The C/C++ configuration settings primary purpose is to set information for IntelliSense engine and not info for launching.

It makes sense to me -- I've always wanted an easy way to switch between Debug and Release configurations similar to VS. We have the config in the UI to switch, so overloading that to switch build values to use seems convenient.

@michelleangela
Copy link
Contributor

Regarding the purpose of this new "property":

added a system that lets users store arbitrary properties in the C/C++ configuration files and to query them using a new command.

Does it make sense to add it to the C/C++ configuration settings? The C/C++ configuration settings primary purpose is to set information for IntelliSense engine and not info for launching.

It makes sense to me -- I've always wanted an easy way to switch between Debug and Release configurations similar to VS. We have the config in the UI to switch, so overloading that to switch build values to use seems convenient.

The concern is also on user experience. Configuring IntelliSense settings is not straightforward as it is, so adding this new setting may cause more confusion.

@sean-mcmanus
Copy link
Collaborator

@michelleangela @Colengms Is this okay to be based off and targetting the release branch? Usually we work out of master, although sometimes we recommend external users use the release branch if master has breaking changes that aren't compatible with the published cpptools binary. If we check this into the release branch, we'll want to cherry pick it into master afterwards.

@sean-mcmanus
Copy link
Collaborator

Regarding the purpose of this new "property":

added a system that lets users store arbitrary properties in the C/C++ configuration files and to query them using a new command.

Does it make sense to add it to the C/C++ configuration settings? The C/C++ configuration settings primary purpose is to set information for IntelliSense engine and not info for launching.

It makes sense to me -- I've always wanted an easy way to switch between Debug and Release configurations similar to VS. We have the config in the UI to switch, so overloading that to switch build values to use seems convenient.

The concern is also on user experience. Configuring IntelliSense settings is not straightforward as it is, so adding this new setting may cause more confusion.

We definitely get lots of users who confused by thinking the c_cpp_properties.json properties should affect their tasks.json builds (i.e. the includePath isn't used for compiling). I figure that customProperties would be considered an advanced property that most users would ignore when setting up IntelliSense (i.e. it wouldn't appear in the UI), so the amount of additional confusion seems like it would be minimal and its use is unlikely to cause breakage, since it's opt-in.

@bugengine
Copy link
Contributor Author

@michelleangela @Colengms Is this okay to be based off and targetting the release branch?

Using master was my first choice, but I read the contributing guide and there was no specific mention of a branch, or I missed it. The documentation for building and debugging the extension does mention to clone the release branch, so I went with that. I have learnt for the future :)
I can base it off master if you want, it should be easy enough for me to cherrypick my changes into my master fork. I think it would close this pull request though.

The concern is also on user experience. Configuring IntelliSense settings is not straightforward as it is, so adding this new setting may cause more confusion.

I agree it's actually not the right place, I think the issue is that there is no right place in VSCode as VSCode does not offer the concept of build variants directly. For instance the CMake tools for VSCode also added their variants as an extension and provide commands to fetch some values. If I could influence VSCode, I would say that the tasks.json file should direct that information, and cpptools/intellisense should be a client of the variant name and te properties (including user properties).
As it is though, cpptools currently offers the only UI that drives a configuration/variant option, so I don't see any other possibility, unless I missed something?

@michelleangela
Copy link
Contributor

Confirmed that this can go to into release branch, and we'll cherry pick into master after.

@sean-mcmanus sean-mcmanus merged commit eedadd9 into microsoft:release Jun 15, 2020
sean-mcmanus pushed a commit that referenced this pull request Jun 15, 2020
…iguration (#5453)

* Added system to store and query properties from the active C/C++ configuration
Co-authored-by: yngwe@farnsworth <yngwe@farnsworth>
Co-authored-by: yngwe@bender <yngwe@bender>
@sean-mcmanus
Copy link
Collaborator

@bugengine This is available with https://github.com/microsoft/vscode-cpptools/releases/tag/0.29.0-insiders now.

@SafwenBenJha
Copy link

Is "activeConfigCustomVariable" usable in settings.json so I can use it in cmake configuration? The variables is defined in c_cpp_properties.json this way:
{ "configuration { "name" : "myconfigName", "customConfigurationVariables": { "myvar": "value" } } }
In settings.json, I am using "variant": "${command:cpptools.activeConfigCustomVariable:myvar}"

This is not working and the content of "variant" is actually the string ${command:cpptools.activeConfigCustomVariable:myvar}

Please note that "${command:cpptools.activeConfigName}" works as expted. So Iam suspecting that the way I am calling :myvar is incorrect. Can you plz help me resolve this?

Thanks

@sean-mcmanus
Copy link
Collaborator

@SafwenBenJha I think it only works in launch.json via something like:

    "inputs": [
        {
            "id": "toolchain",
            "type": "command",
            "command": "cpptools.activeConfigProperty",
            "args": "toolchain"
        },
        {
            "id": "variant",
            "type": "command",
            "command": "cpptools.activeConfigProperty",
            "args": "variant"
        }
    ]

@PetaSoft
Copy link

PetaSoft commented Sep 19, 2020

@bugengine @sean-mcmanus The solution presented here is useful, but I have some problems with it:

  1. Input commands seem to return only a string value (following the documentation for command variables and for input variables) though in c_cpp_properties.json it is possible to define variables which represent arrays of strings (example here). Variables defined as part of the customConfigurationVariables property of the c_cpp_properties.json file allow also only a string value. I get a problem with that restriction in the case when I want to define a variable for the arguments of a compiler command. In that case, I want to have a variable value which is an array of strings, and I want to use that variable in tasks.json for the args property of a "type": "shell" command to specify the arguments of the cl.exe compiler command. That is currently not possible using the presented solution. A workaround is to use a variable for each compiler argument but then the list of input variables gets really large and unclear.
  2. Input variables do not seem to be the right solution in principle. It is combersome to define an input variable (which always calls the cpptools.activeConfigCustomVariable command) to link variables from c_cpp_properties.json to tasks.json and launch.json. It would be better to define a new kind of variables, such that e.g. a variable INCLUDE defined as part of customConfigurationVariables can be used in tasks.json with the variable substitution ${cpptools:INCLUDE} (which does not need any input variables). In combination with 1) variable substitution should also work for arrays of strings similar to the example for c_cpp_properties.json.
  3. In c_cpp_properties.json the property is called customConfigurationVariables but the input command is called cpptools.activeConfigCustomVariable. That is a bit confusing as the order of the words are turned for the command. A better command name would habe been cpptools.activeCustomConfigVariable.

@sean-mcmanus
Copy link
Collaborator

@PetaSoft Did you have a suggestion on how to make it better in regards to 1 and 2?

@PetaSoft
Copy link

@sean-mcmanus I think my comment is already a suggestion to make it better:

  1. Allow customConfigurationVariables to have property values which are arrays of strings;
  2. Add a new class of VS Code variables which can be used in tasks.json and launch.json. These variables can be used in substitutions with a syntax like ${cpptools:<Variablename>}. This is similar to the syntax for environment variables, configuration variables, and command variables. These substitutions should also be allowed as part of an array of strings similar to the example for env variablles in c_cpp_properties.json files. So in arrays of strings the variable can not only represent a string but also an array of strings similar to the following lines in the mentioned example:
    "env": { "myDefaultIncludePath": ["${workspaceFolder}", "${workspaceFolder}/include"] },
    "includePath": ["${myDefaultIncludePath}", "/another/path"]
    which is substituted to
    "includePath": ["${workspaceFolder}", "${workspaceFolder}/include", "/another/path"]

@github-actions github-actions bot locked and limited conversation to collaborators Oct 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants