-
Notifications
You must be signed in to change notification settings - Fork 50
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
Add a Meson cpptools configProvider #218
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to me like a few of the commits could be squashed together.
src/cpptoolsconfigprovider.ts
Outdated
this.buildDir = buildDir; | ||
} | ||
|
||
name = "Meson build"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make this "Meson Build"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make these private
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renaming to "Meson Build". However, name and extensionId have to remain public because this class implements cpptools.CustomConfigurationProvider
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah Thanks for the explanation!
src/cpptoolsconfigprovider.ts
Outdated
|
||
export class CpptoolsProvider implements cpptools.CustomConfigurationProvider { | ||
cppToolsAPI?: cpptools.CppToolsApi; | ||
private buildDir: string = ""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the default value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/cpptoolsconfigprovider.ts
Outdated
import * as vscode from "vscode"; | ||
import * as cpptools from "vscode-cpptools"; | ||
import { getMesonBuildOptions, getMesonCompilers, getMesonDependencies } from "./introspection"; | ||
var fs = require("fs"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't use require. Use import.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/cpptoolsconfigprovider.ts
Outdated
let machine = ""; | ||
const buildOptions = await getMesonBuildOptions(this.buildDir); | ||
for (let option of buildOptions) { | ||
if (option.name === "cpp_std") { | ||
browseConfig = Object.assign({}, browseConfig, { standard: option.value }); | ||
machine = option.machine; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible for us to not have a machine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested the simple meson test-case gtk-doc with a native build. A machine was also defined ("host"). So it looks like meson always defines a machine, even with empty/default options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add an assert and make machine
string | undefined
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright! Added a commit to handle an undefined machine. I used an if rather than assert because we are still able to provide a browsePath without this option.
src/cpptoolsconfigprovider.ts
Outdated
}; | ||
|
||
const dependencies = await getMesonDependencies(this.buildDir); | ||
for (let dep of dependencies) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use const instead of let
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
canProvideBrowseConfigurationsPerFolder(token?: vscode.CancellationToken | undefined): Thenable<boolean> { | ||
return Promise.resolve(false); | ||
} | ||
async provideFolderBrowseConfiguration( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whitespace around functions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
var fs = require("fs"); | ||
|
||
export class CpptoolsProvider implements cpptools.CustomConfigurationProvider { | ||
cppToolsAPI?: cpptools.CppToolsApi; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make this private?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It needs to be accessed by registerCppToolsProvider
cpptools = new CpptoolsProvider(buildDir); | ||
registerCppToolsProvider(ctx, cpptools); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we only do this if it is a C or C++ project?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it matters. If the user does not add the "C_Cpp.default.configurationProvider": "mesonbuild.mesonbuild"
setting, then non of the configuration methods will be called. And they do, this only affects C/C++ files detected by the cpptools extension.
src/cpptoolsconfigprovider.ts
Outdated
@@ -0,0 +1,120 @@ | |||
import path = require("path"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should document this in the README.md somewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
The compiler fields will be used to provide the configuration for the cpptools extension.
This contains the API required to register as a configProvider with the C/C++ extension.
Untill now the meson extension provided C++ linting through compile_commands.json. This would not configure: - New file that haven't been compiled yet - External libraries that are not part of the project (when browsing/debugging) This was especially problematic when using a cross-compilation toolchain since none of the default host headers should be used. This adds a custom configuration provider for the cpptools extension. It introspects the compiler settings of the project to configure linting and browsing. Currently, file-specific configurations are not supported. Some other advanced meson options might also not be supported. However it provides a strong experience for cross-compilation and external library support. Here is an example settings.json configuration to activate this provider: ```json { "C_Cpp.default.configurationProvider": "mesonbuild.mesonbuild" } ```
The meson docs specifies that meson-info.json is the last file to be updated when reconfiguring a project. We use this as an event to notify that the configuration must be reloaded.
Meson projects which do not use C++ have different configurations which we must handle separately. If both are defined, we can only provide one, hence C++ since it is "compatible" with C.
Similarly to us configuring the compile_commands.json, we can configure the default configuration provider.
For simple projects which do not define an explicit compiler, the introspection unit will throw an error. This commit adds a try-catch to handle this case with default arguments.
Thanks a lot for your review. I made the recommended changes. Also added 2 commits for default configurations that improved the experience when I tested a native simple meson test-case. |
The machine configuration is usually defined in meson introspection. Just to be sure, add a check for undefined machine. For instance, if someone or something has tampered with the build files.
Description from the commit message
Testing the configuration
One can quickly test the generated configuration by removing the
compile_commands.json
and browsing C/C++ files. Most include paths and compiler flags should still appear effective. You can for instance add a dependency to a nonstandard library and the#include
statement should not produce error squiggles.Either remove the option in the .vscode/settings.json file at runtime, or use a patch