#714: Use the build cache to speed up the LS #1107

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

Jump to bottom

Merged

kittaakos merged 4 commits into main from #714-signed

Jul 18, 2022

Merged

#714: Use the build cache to speed up the LS #1107

kittaakos merged 4 commits into main from #714-signed

Jul 18, 2022

Conversation

kittaakos

Copy link

Contributor

@kittaakos kittaakos commented Jun 23, 2022 •

edited

Loading

Motivation

Avoid reindexing libraries on every single text document change event.

Change description

After a verify (both success and failure), the IDE2 calls the arduino.languageserver.notifyBuildDidComplete command (exposed by the VS Code extension) and pushes the most recent build_path to the LS via the custom ino/buildDidComplete JSON-RPC.

Use 0.25.0-rc1 CLI,
Use 0.0.2-beta.4 VS Code extension, and
Use 0.7.1 Arduino LS

Dev:

Updated the build scripts; can build the LS from sources besides downloading it from S3 or GH.

Other information

#714

Reviewer checklist

PR addresses a single concern.
The PR has no duplicates (please search among the Pull Requests before creating one)
PR title and description are properly filled.
Docs have been added / updated (for bug fixes / features)

@kittaakos kittaakos force-pushed the #714-signed branch from 2ddd244 to 6e62f09 Compare

June 23, 2022 11:30

@kittaakos kittaakos changed the title ~~(削除) WIP: Use the build cache to speed up the LS (削除ここまで)~~ (追記) [WIP] #714: Use the build cache to speed up the LS (追記ここまで)

Jun 23, 2022

@kittaakos kittaakos added topic: code type: imperfection topic: language server labels

Jun 23, 2022

@kittaakos kittaakos force-pushed the #714-signed branch 5 times, most recently from a7c85b7 to 8d462f2 Compare

July 12, 2022 08:19

@kittaakos kittaakos marked this pull request as ready for review

July 12, 2022 08:22

@kittaakos kittaakos changed the title ~~(削除) [WIP] #714: Use the build cache to speed up the LS (削除ここまで)~~ (追記) #714: Use the build cache to speed up the LS (追記ここまで)

Jul 12, 2022


 Notify the LS about the new build_path after verify.

16aa5ed

Closes #714
Signed-off-by: Akos Kitta <a.kitta@arduino.cc>

@kittaakos kittaakos force-pushed the #714-signed branch from 8d462f2 to 16aa5ed Compare

July 14, 2022 08:47

@kittaakos kittaakos requested review from per1234, ubidefeo and fstasi

July 14, 2022 08:48


 Support for language.realTimeDiagnostics.

30649b1

Signed-off-by: Akos Kitta <a.kitta@arduino.cc>

per1234

per1234 approved these changes

Jul 15, 2022

View reviewed changes

Copy link

Contributor

@per1234 per1234 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is working exactly as intended in all my tests.

I hope this will resolve the issues some people have experienced resulting from the resource impact of the language server, and also avoid the distress that can be caused by spurious problem detection results.

Thanks Akos and Cristian for your work on this!

fstasi

fstasi suggested changes

Jul 15, 2022

View reviewed changes

Copy link

Contributor

@fstasi fstasi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, only minor concerns on using kittaakos fork for the vscode-arduino-tools

arduino-ide-extension/src/browser/contributions/core-error-handler.ts

Comment on lines +14 to +15

this.errors.length = 0;

this.errors.push(...error.data);

Copy link

Contributor

@fstasi fstasi Jul 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this equivalent to

Suggested change

this.errors.length = 0;

this.errors.push(...error.data);

this.errors = error.data;

performance wise is also faster

Copy link

Contributor Author

@kittaakos kittaakos Jul 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fields are readonly. Your proposal is a compiler error. Did I overlook something?

electron/build/template-package.json Outdated

"theiaPlugins": {

"vscode-builtin-cpp": "https://open-vsx.org/api/vscode/cpp/1.52.1/file/vscode.cpp-1.52.1.vsix",

"vscode-arduino-tools": "https://downloads.arduino.cc/vscode-arduino-tools/vscode-arduino-tools-0.0.2-beta.2.vsix",

"vscode-arduino-tools": "https://github.com/kittaakos/vscode-arduino-tools/raw/realTimeDiagnostics/build-artifacts/vscode-arduino-tools-0.0.2-beta.3.vsix",

Copy link

Contributor

@fstasi fstasi Jul 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we rely on https://github.com/arduino/vscode-arduino-tools ?

Copy link

Contributor Author

@kittaakos kittaakos Jul 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure sure. I did not want to create an RC before testing it out.

package.json

"theiaPluginsDir": "plugins",

"theiaPlugins": {

"vscode-builtin-cpp": "https://open-vsx.org/api/vscode/cpp/1.52.1/file/vscode.cpp-1.52.1.vsix",

"vscode-arduino-tools": "https://downloads.arduino.cc/vscode-arduino-tools/vscode-arduino-tools-0.0.2-beta.2.vsix",

Copy link

Contributor

@fstasi fstasi Jul 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as before, I'd rather go with https://github.com/arduino/vscode-arduino-tools if possible

This was referenced Jul 15, 2022

Can start the LS without real-time diagnostics. arduino/vscode-arduino-tools#36

Merged

Added flag to disable real-time PublishDiagnostics push arduino/arduino-language-server#124

Merged

Akos Kitta added 2 commits

July 15, 2022 14:48


 Use published VSX.

7ac2179

Signed-off-by: Akos Kitta <a.kitta@arduino.cc>


 Use latest LS.

b4570ff

Signed-off-by: Akos Kitta <a.kitta@arduino.cc>

@kittaakos

Copy link

Contributor Author

kittaakos commented Jul 15, 2022

The PR pinned the version of the VS Code extension and the LS. @fstasi, please review and merge the PR if you are happy with it. Thank you!

@AlbyIanna AlbyIanna requested a review from fstasi

July 15, 2022 16:41

fstasi

fstasi approved these changes

Jul 18, 2022

View reviewed changes

@kittaakos kittaakos merged commit 57841b3 into main

Jul 18, 2022

@kittaakos kittaakos deleted the #714-signed branch

July 18, 2022 08:19

@per1234 per1234 mentioned this pull request

Jul 18, 2022

arduino.language.realTimeDiagnostics setting has no effect #1207

Closed

3 tasks

@per1234 per1234 mentioned this pull request

Jul 19, 2022

Pulled remote sketches don't show error underlines and markers #669

Closed

@per1234 per1234 mentioned this pull request

Aug 6, 2022

Error window with navigation #1300

Closed

3 tasks

@per1234 per1234 mentioned this pull request

Aug 24, 2022

Language server "Building sketch..." spinner seems to hang #709

Closed

@per1234 per1234 mentioned this pull request

Nov 27, 2022

Display code errors in red in real time #1712

Closed

3 tasks

Labels

topic: code topic: language server type: imperfection

3 participants

@kittaakos @fstasi @per1234

Uh oh!

#714: Use the build cache to speed up the LS #1107

#714: Use the build cache to speed up the LS #1107

Uh oh!

Conversation

@kittaakos kittaakos commented Jun 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Change description

Other information

Reviewer checklist

Uh oh!

@per1234 per1234 left a comment

Choose a reason for hiding this comment

Uh oh!

@fstasi fstasi left a comment

Choose a reason for hiding this comment

Uh oh!

@fstasi fstasi Jul 15, 2022

Choose a reason for hiding this comment

Uh oh!

@kittaakos kittaakos Jul 15, 2022

Choose a reason for hiding this comment

Uh oh!

@fstasi fstasi Jul 15, 2022

Choose a reason for hiding this comment

Uh oh!

@kittaakos kittaakos Jul 15, 2022

Choose a reason for hiding this comment

Uh oh!

@fstasi fstasi Jul 15, 2022

Choose a reason for hiding this comment

Uh oh!

kittaakos commented Jul 15, 2022

Uh oh!

Uh oh!

@kittaakos kittaakos commented Jun 23, 2022 •

edited

Loading