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

Allow other extensions to query properties of the current configuration #1524

Closed
thejcannon opened this issue Feb 6, 2018 · 28 comments
Closed
Labels
Feature Request fixed Check the Milestone for the release in which the fix is or will be available. help wanted Can be fixed in the public (open source) repo. Language Service

Comments

@thejcannon
Copy link
Contributor

I'm writing an extension that plugs in our companies build processes on top of vscode-cpptools. Stuff like generating c_cpp_properties.json, launch.json, etc...

Ideally, the user would have their workspace set up so that they've selected a configuration for intellisense, and when they hit F5, the appropriate launch.json configuration is selected for their selected c_cpp_properties.json configuration.

The easiest way I can think of would be to allow other extensions to query vscode-cpptools for the current configuration, and then clients can use that to get the name or any other property.

I'm still getting my hands dirty with extensions, so let me know if this already exists, or there's a better way to accomplish this.

Thanks!

@bobbrow bobbrow added Language Service Feature Request help wanted Can be fixed in the public (open source) repo. labels Feb 6, 2018
@sean-mcmanus
Copy link
Contributor

VS code allows extensions to export functions that other extensions can call. So we could export some GetCurrentConfiguration method. Another potential option would be to store the current configuration name in a setting, and then the setting could be referenced using ${config.} in launch.json. We currently just save the current configuration as a number/index, which we send to the processes when the number changes. You should be able to implement this yourself with our open source typescript and we could accept a pull request.

@thejcannon
Copy link
Contributor Author

I think exposing the current configuration (as the full JSON) would probably be more beneficial to clients.

As far as exposing a function goes, I found a way of executing another extension's commands, is that what you're referring to? If so I think it would be easy to add a new command (that isn't attached to anything in the UI) that returns the current configuration.

@sean-mcmanus
Copy link
Contributor

sean-mcmanus commented Feb 7, 2018

I don't know what you mean by "exposing the current configuration (as the full JSON)". We currently use a c_cpp_properties.json that other extensions can read/write (the same way our extension does using JSON apis), but the currently selected configuration is not available anywhere. We plan to migrate away from that json file and just use the normal settings.json later though, although c_cpp_properties.json might still be supported for backwards compatibility unless do a conversion, in which case the currently selected configuration might just be a setting as well.

I don't think we want to add a "command", which are listed in the command palette for users to execute. I'm referring to returning a public API from our activate method: https://code.visualstudio.com/docs/extensionAPI/vscode-api#_extensions:
`Extension writers can provide APIs to other extensions by returning their API public surface from the activate-call.
export function activate(context: vscode.ExtensionContext) {
let api = {
sum(a, b) {
return a + b;
},
mul(a, b) {
return a * b;
}
};
// 'export' public api-surface
return api;
}
When depending on the API of another extension add an extensionDependency-entry to package.json, and use the getExtension-function and the exports-property, like below:
let mathExt = extensions.getExtension('genius.math');
let importedApi = mathExt.exports;

console.log(importedApi.mul(42, 1));`

@thejcannon
Copy link
Contributor Author

thejcannon commented Feb 7, 2018

I don't know what you mean by "exposing the current configuration (as the full JSON)"
but the currently selected configuration is not available anywhere.

What I was hoping to expose was the contents of the configuration in c_cpp_properties.json for the selected configuration. (I.e. if I've selected configuration "A", and I ask vscode-cpptools for the current configuration, I get back the JSON object that contains "name": "A").

We plan to migrate away from that json file and just use the normal settings.json
in which case the currently selected configuration might just be a setting as well

That sounds pretty promising, and would be pretty useful. Depending on the timeline, I could live without the feature I'm requesting if the feature you describe was implemented. Any idea on when that change might take place?

@thejcannon
Copy link
Contributor Author

Actually. If we just exposed the current configuration's name through settings.json, it would solve our problems (as far as needing the configuration exposed) and be a stepping stone to having vscode-cpptools exclusively use settings.json.

Would you be opposed to that solution?

@sean-mcmanus
Copy link
Contributor

Yeah, that sounds good to me. I can't think of any blocking problems that would cause (or a need to modify the language server process). @bobbrow @grdowns @ronglums Do you guys see any issues? If the setting name doesn't match any names in the c_cpp_properties.json it should switch to the default and update the workspace setting to be valid. Did you want to add this yourself? I'm not sure when we'd be able to add it.

@thejcannon
Copy link
Contributor Author

I'd be happy to try 😄

@sean-mcmanus
Copy link
Contributor

Sure, let us know if you have any questions or want help with something.

@bobbrow
Copy link
Member

bobbrow commented Feb 9, 2018

I think it would be better to export an API (it's something I eventually want to do for CMake integration anyway) than add a new setting for this. A setting opens up some weird cases since it can be set in any of three scopes (global, workspace, folder) and this really only ever applies to a folder.

Another potential option would be to write the active configuration index into the c_cpp_properties.json file (it sounds like you're reading the configs from there already?). I think I considered doing this at one point, but ended up using the persistent storage stuff instead to avoid incrementing the version number of c_cpp_properties.json.

We still haven't sat down and thought through the migration of c_cpp_properties.json to settings.json so I have concerns about adding a setting that has the potential to be obsoleted in a month or two. Exporting an API that can provide this information sidesteps any potential issues here and is the safest option, IMO.

@sean-mcmanus
Copy link
Contributor

sean-mcmanus commented Feb 9, 2018

The global/workspace scopes are still valid as defaults if the folder setting isn't set yet. The problem with the export API is that I don't think you can add that to a task/launch.json, which I assumed was a goal -- see #1444 for another recent user request.

Way back in December 2016, I was thinking about something like...
C_Cpp.includePaths: { “Linux: <> “Win32”: <> }

Whatever the exact setting layout is, I don't think having the configuration name in a setting would become obsolete. Putting the configuration in the c_cpp_properties.json seems more likely to become obsolete.

@bobbrow
Copy link
Member

bobbrow commented Feb 9, 2018

Fair point. I forgot about tasks.

@thejcannon
Copy link
Contributor Author

thejcannon commented Feb 12, 2018

From a client's perspective, settings.json is much easier to consume (since you can use ${config:C_Cpp.foo} in tasks.json or launch.json.
From the perspective of an extension writer, it's easier as well since there's vscode.workspace.getConfiguration()[C_Cpp].
Which means there's no API to maintain for you guys.

@bobbrow
Copy link
Member

bobbrow commented Feb 12, 2018

Either way, there's always something to maintain. 😉

@thejcannon
Copy link
Contributor Author

Alright, now time for the tough parts. When the index was internal, it wasn't really a possibility that it got out of sync with the config file (if the file changes, we just reset the index). Now that the "index" is a name in the settings file, it makes it getting out of sync a lot easier.

What would you guys prefer the extension do in situations where the config file changes (in a way that "invalidates" the current configuration name)? Or if the user adds/changes the current configuration name to one that isn't valid?

@thejcannon
Copy link
Contributor Author

Fair note: Blasting the user's setting is a valid possibility in either scenario 😄

@bobbrow
Copy link
Member

bobbrow commented Feb 13, 2018

What we really want is a read-only setting, but is it possible to write to the environment and have the task work with that (e.g. ${env:c_cpp_activeConfig})?

But stepping back for a second... you're building an extension, right? Do you even need to use tasks/launch.json or can you encapsulate your "tasks" in a command? This is what the CMake extension does. You drive the inputs to your commands via your extension's settings and you don't require your users to meddle with tasks/launch.json. If you expose your tasks as commands, then our extension can supply the necessary information via an exported API and then we don't need to deal with bad user input.

@thejcannon
Copy link
Contributor Author

That's definitely an option for us. But my ideal solution is that my extension generates tasks/launch.json the client. That way they don't have to learn a new workflow.
An API or reading settings doesn't really effect me much, so whatever you guys would rather support I'm happy to consume (and to make a PR, since I'm asking for the feature 😄 )

@sean-mcmanus
Copy link
Contributor

sean-mcmanus commented Feb 13, 2018

I was thinking that that the setting would be read/write but would be updated to match a valid value if invalid input was used and if the current config name changed, it would update the setting. I'm not aware of any technical problem with keeping the 2 settings in-sync, but there could be.

Oh, I remember wanting to be able add a setting to our package.json as an extension provided built-in variable, but it looked like VS Code didn't support that.

@bobbrow
Copy link
Member

bobbrow commented Feb 13, 2018

What I'm hung up on is that I really want the setting to be read-only. Sorry for being a stickler on this.

If our extension exported a read-only API (or if writing to the environment works), your extension could consume that and save the config name to your own setting (e.g. fooExtension.activeConfig) that could be consumed by your tasks if you want to continue using tasks.

@thejcannon
Copy link
Contributor Author

Environment variables would get messy, since the user could have several instances of vscode running.

For insight, I don't explicitly need a setting. My extension will be generating c_cpp_properties.json, tasks.json and launch.json (and keeping them up to date if the user changes their dependencies, etc...). So all I need is the current configuration when they want to do something (like launch the debugger).

@sean-mcmanus
Copy link
Contributor

Another previous request was #742 .

@thejcannon
Copy link
Contributor Author

I can prototype a read-only setting and see how it turns out. Might feel clunky to the user, depending on some of the design decisions.

What I'm imagining is that if the user changes it to be invalid, it "snaps" back to the valid value. Similarly if the user re-configures the c_cpp_properties, it "snaps" to a valid value.

It's too bad I can't dynamically set enum values. That would be pretty useful 😃

@sean-mcmanus
Copy link
Contributor

sean-mcmanus commented Feb 15, 2018

What do you mean by "re-configures the c_cpp_properties"? What is being snapped to a valid value?

The package.json can be dynamically modified with different enum values, but the problem is that the change won't be picked up until the extension reloads.

@thejcannon
Copy link
Contributor Author

thejcannon commented Feb 15, 2018

"re-configures the c_cpp_properties" means if the user removes some configurations (maybe the current one?)
"snapped to a valid value" means if they try to go from "C_Cpp.configName": "Foo" to "C_Cpp.configName": "Food" (where "Food" isn't a valid name). The extension would (somehow) listen to the change, and change it back to "C_Cpp.configName": "Foo".

Although I'll admit, I haven't thought about this in the context of the hierarchy of settings (global, workspace, folder).

The package.json can be dynamically modified with different enum values, but the problem is that the changed won't be picked up in a the extension reloads.

I didn't know that was a feature, mind pointing me to the documentation?

@sean-mcmanus
Copy link
Contributor

sean-mcmanus commented Feb 15, 2018

Ah, the "it" you're referring to is the setting snapping to valid value and not the c_cpp_properties.json changing.

I don't think I've seen documentation on the package.json re-writing behavior. We do it in main.ts in the rewriteManifest() method and we also potentially do it in processCpptoolsJson() in abTesting.ts, but that code isn't currently hittable. We used to do more package.json re-writing for the debugger, but we got rid of that because users found the extra reload requirement annoying.

I think only the workspace setting (or folder setting for multi-root) would need to be snapped to a valid value because it takes precedence over the higher settings, e.g. a user may have an default config set that is only valid for 50% of the folders they open, so we wouldn't want to change the user setting.

@bobbrow
Copy link
Member

bobbrow commented Feb 26, 2018

FYI, I've been discussing this with the VSCode team and asking them for a way to do this that doesn't require us to expose a setting or an API.
microsoft/vscode#43782

@thejcannon
Copy link
Contributor Author

I like the sound of that. It definitely allows for more interesting things, while keeping the active config private in the cpptools extension.

bobbrow added a commit that referenced this issue Feb 27, 2019
* Enable ${command:cpptools.activeConfigName} in tasks #1524

* PR feedback
@sean-mcmanus sean-mcmanus added the fixed Check the Milestone for the release in which the fix is or will be available. label Mar 1, 2019
@sean-mcmanus
Copy link
Contributor

We've added ${command:cpptools.activeConfigName} in our 0.22.0-insiders2 build: https://github.com/Microsoft/vscode-cpptools/releases . Let us know if you experience any issues with that.

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. help wanted Can be fixed in the public (open source) repo. Language Service
Projects
None yet
Development

No branches or pull requests

3 participants