1323308 - Add support for Visual Studio Code

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Ralph Giles (:rillian)]

The Visual Studio Code (vscode) editor needs some in-tree config files to know how to build and debug a project. Hook this up to our standard mach commands so there's a minimal working config out of the box.

Comment 1

[Ralph Giles (:rillian)]

Attachment: Bug 1323308 - Add task definitions for vscode.

Add basic build task descriptions for the Microsoft
Visual Studio Code editor environment. Just calls
`mach build`, `mach build binaries`, `mach clobber`,
and `mach configure`.

Based on a file by Dan Glastonbury.

Review commit: https://reviewboard.mozilla.org/r/98436/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/98436/

Comment 2

[Ralph Giles (:rillian)]

Attachment: Bug 1323308 - Add include paths for vscode.

Add a C/C++ properties config file for Microsoft
Visual Studio Code.

This adds a clang-format config and some common include
paths so symbol lookup works a little bit. For generated
headers, it gives the default object directory paths,
but this will likely be incorrect for many people.
Especially as vscode is messy about in-tree generated
code already, so an external object directory is a common
config.

Unfortunately without some way to set this programmatically
like we do with .ycm_extra_conf.py I don't see a way to make
this work in general without local modifications.

Review commit: https://reviewboard.mozilla.org/r/98438/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/98438/

Comment 3

[Ralph Giles (:rillian)]

The task definitions should be fine, but I worry the c_cpp_properties will collide with local configs. I didn't find a way to set the object directory programmatically. Is there an override mechanism which would at least allow local modification?

Comment 4

[Gregory Szorc [:gps]]

Comment on attachment 8818356 [details]
Bug 1323308 - Add include paths for vscode.

https://reviewboard.mozilla.org/r/98438/#review98764

This is not exactly robust since each directory can have its own include paths.

Did you see the code in `visualstudio.py` for generating Visual Studio project files? I haven't looked at the Visual Studio Code config files in detail yet, but my guess is the complexity of the Firefox build system will warrant dynamic generation of these files.

Comment 5

[Jean-Yves Avenard [:jya]]

Comment on attachment 8818356 [details]
Bug 1323308 - Add include paths for vscode.

https://reviewboard.mozilla.org/r/98438/#review98764

Unlike proper VisualStudio, all files are parsed and indexed. You only need to provide the root directory to Visual Studio Code.
So the configuration provided will works regardless of the file structure.

Comment 6

[Ralph Giles (:rillian)]

(In reply to Gregory Szorc [:gps] from comment #4)

> Did you see the code in `visualstudio.py` for generating Visual Studio
> project files? I haven't looked at the Visual Studio Code config files in
> detail yet, but my guess is the complexity of the Firefox build system will
> warrant dynamic generation of these files.

I knew we had such code, and thought we might have to do something dynamic in the end. However, I had trouble in testing with vscode not reloading updated config files, so that might take changes on their side too. Until I'd investigated further I wanted to land something static to get things started. I also wasn't sure how to handle generating files in topsrcdir.

Anyway, found a typo. Updating the patch and switching reviewers since mystor is away.

Comment 7

[Ralph Giles (:rillian)]

Comment on attachment 8818355 [details]
Bug 1323308 - Add task definitions for vscode.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/98436/diff/1-2/

Comment 8

[Ralph Giles (:rillian)]

Comment on attachment 8818356 [details]
Bug 1323308 - Add include paths for vscode.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/98438/diff/1-2/

Comment 9

[Ralph Giles (:rillian)]

Comment on attachment 8818356 [details]
Bug 1323308 - Add include paths for vscode.

Dan, what do you think about having `c_cpp_properties.json` in tree? It would be nice to get something minimal available for everyone, but I worry about diff noise with the object directory, and of course the include paths could be wrong for some targets.

Comment 10

[u480271]

(In reply to Ralph Giles (:rillian) needinfo me from comment #9)
> Comment on attachment 8818356 [details]
> Bug 1323308 - Add include paths for vscode.
> 
> Dan, what do you think about having `c_cpp_properties.json` in tree? It
> would be nice to get something minimal available for everyone, but I worry
> about diff noise with the object directory, and of course the include paths
> could be wrong for some targets.

Ralph, I don't think it's good to have checked into .vscode/

In my opinion, we need something similar to mach clang-complete command, which will take the mozconfig into account generate c_cpp_properties.json for the user. (either into .vscode/ folder, or to stdout)

What do you think of this idea?

Comment 11

[u480271]

Clearing ni?

Comment 12

[u480271]

I clearly suck at using Bugzilla...

Comment 13

[Ralph Giles (:rillian)]

Ok, thanks. I think there's value to having a `launch.json` and `tasks.json` checked in, so people don't have to know to run the magic mach command, but it sounds like the second patch adding search paths will be problematic.

I filed an issue upstream to try and get an overlay system to support, but they've not been keen so far. https://github.com/Microsoft/vscode-cpptools/issues/410

Comment 14

[Gregory Szorc [:gps]]

Comment on attachment 8818356 [details]
Bug 1323308 - Add include paths for vscode.

Sorry, but this disappeared in the bowels of my review queue and I'll be gone for ~1 month.

Please pursue this enhancement with another build peer if you want action on this in the next ~1 month. I recommend Ted for this topic.

Comment 15

[Ralph Giles (:rillian)]

I haven't been able to follow up on this. Someone is is welcome to take it on.

Comment 16

[Panos Astithas (he/him) [:past] (please ni?)]

I have an updated tasks definition file to provide a few more options that I'm currently finishing testing. I'll post it for review soon.

Comment 17

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Add some task definitions to run mach from VSCode (bug 1323308).

Review commit: https://reviewboard.mozilla.org/r/136264/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/136264/

Comment 18

[Panos Astithas (he/him) [:past] (please ni?)]

I fixed the ignore rules in this patch. I'm investigating adding a launch.json file, but I haven't managed to make it work yet.

Comment 19

[Gregory Szorc [:gps]]

Comment on attachment 8864582 [details]
Add some task definitions to run mach from VSCode (bug 1323308).

https://reviewboard.mozilla.org/r/136264/#review140170

Even though I have questions, I'm giving this r+ because this isn't part of the build system proper and I don't think the bar needs to be super high just yet. I'll leave it to your discretion on whether to land it without another review.

::: .vscode/tasks.json:5
(Diff revision 1)
> +{
> +    // See https://go.microsoft.com/fwlink/?LinkId=733558
> +    // for the documentation about the tasks.json format
> +    "version": "2.0.0",
> +    "command": "${workspaceRoot}/mach",

Does this actually work? `mach` isn't a win32 executable: it has a shebang, which Windows doesn't recognize. You need to invoke `mach` via MozillaBuild's python27.exe or sh.exe.

::: .vscode/tasks.json:25
(Diff revision 1)
> +        "isBuildCommand": true,
> +        "problemMatcher": {
> +          "owner": "cpp",
> +          "fileLocation": "absolute",
> +          "pattern": {
> +            "regexp": "^.*?tools([^\\s]*):(\\d+):(\\d+):\\s+(warning|error):\\s+(.*)$",

There are some regular expressions in python/mozbuild/mozbuild/compilation/warnings.py to catch the actual compiler warnings. Alternatively, `mach build` will output its own summary of these warnings, which `^warning:` should catch.

I'm not sure what this regexp is trying to catch. What warnings/errors begin with "tools"?

::: .vscode/tasks.json:74
(Diff revision 1)
> +          "pattern": [
> +              {
> +                  "regexp": "^([^\\s].*)$",
> +                  "file": 1
> +              },
> +              {
> +                  "regexp": "^\\s+(\\d+):(\\d+)\\s+(error|warning|info)\\s+(.*)\\s\\s+(.*)$",
> +                  "line": 1,
> +                  "column": 2,
> +                  "severity": 3,
> +                  "message": 4,
> +                  "code": 5,
> +                  "loop": true
> +              }
> +          ]

https://code.visualstudio.com/docs/editor/tasks#_processing-task-output-with-problem-matchers says VScode ships with built-in detection for ESLint. Can that be used instead?

Comment 20

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8864582 [details]
Add some task definitions to run mach from VSCode (bug 1323308).

https://reviewboard.mozilla.org/r/136264/#review140170

> Does this actually work? `mach` isn't a win32 executable: it has a shebang, which Windows doesn't recognize. You need to invoke `mach` via MozillaBuild's python27.exe or sh.exe.

I have been focusing on macOS and Linux so far, Windows will be my next target. It's a pain to coordinate changes to the same file in multiple systems when said file is not under version control.

> There are some regular expressions in python/mozbuild/mozbuild/compilation/warnings.py to catch the actual compiler warnings. Alternatively, `mach build` will output its own summary of these warnings, which `^warning:` should catch.
> 
> I'm not sure what this regexp is trying to catch. What warnings/errors begin with "tools"?

This regexp catches compiler warnings and errors that at the time of capture appear at the same line as the progress footer (the line that starts with 'TIER' and ends with 'tools'). I took a look at the mozbuild regexps (thanks!) and apart from using the python format, they are identical. The only difference is that mozbuild splits 'message' to 'message'+'flag', and VSC has no equivalent for the latter.

> https://code.visualstudio.com/docs/editor/tasks#_processing-task-output-with-problem-matchers says VScode ships with built-in detection for ESLint. Can that be used instead?

Good catch, $eslint-stylish works with the output of 'mach eslint'.

Comment 21

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8864582 [details]
Add some task definitions to run mach from VSCode (bug 1323308).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136264/diff/1-2/

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4b3970c598da
Add some task definitions to run mach from VSCode . r=gps

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4b3970c598da