Add support for Visual Studio Code

RESOLVED FIXED in Firefox 55

Status

defect
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: rillian, Assigned: past)

Tracking

unspecified
mozilla55
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(3 attachments)

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.
Assignee: nobody → giles
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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

3 years ago
mozreview-review
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

3 years ago
mozreview-review-reply
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.
(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 hidden (mozreview-request)
Comment hidden (mozreview-request)
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.
Attachment #8818356 - Flags: feedback?(dglastonbury)
(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?
Flags: needinfo?(giles)
I clearly suck at using Bugzilla...
Flags: needinfo?(giles)
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
Flags: needinfo?(giles)
Attachment #8818355 - Flags: review?(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.
Attachment #8818356 - Flags: review?(gps)
Depends on: 1341585
I haven't been able to follow up on this. Someone is is welcome to take it on.
Assignee: giles → nobody
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.
Assignee: nobody → past
Status: NEW → ASSIGNED
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

2 years ago
mozreview-review
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?
Attachment #8864582 - Flags: review?(gps) → review+
Assignee

Comment 20

2 years ago
mozreview-review-reply
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 hidden (mozreview-request)

Comment 22

2 years ago
Pushed by pastithas@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4b3970c598da
Add some task definitions to run mach from VSCode . r=gps

Comment 23

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4b3970c598da
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Updated

a year ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.