Skip to content

Added skip-libraries-discovery flag in Compile #1777

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

Merged
merged 2 commits into from
Jul 11, 2022

Conversation

cmaglie
Copy link
Member

@cmaglie cmaglie commented Jun 20, 2022

Please check if the PR fulfills these requirements

  • The PR has no duplicates (please search among the Pull Requests
    before creating one)
  • The PR follows our contributing guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • UPGRADING.md has been updated with a migration guide (for breaking changes)

What kind of change does this PR introduce?
Adds a hidden flag --skip-libraries-discovery, when enabled it does skip the libraries discovery phase and reuses the same libraries used in the latest "full" build. The arduino-language-server uses this feature to dramatically speed up updating and reindexing the sketch. Anyway, this feature is not suitable for direct use because it will produce a wrong build if not used correctly, that's why it's hidden.

Does this PR introduce a breaking change, and is titled accordingly?
No

@cmaglie cmaglie self-assigned this Jun 20, 2022
@per1234 per1234 added type: enhancement Proposed improvement topic: code Related to content of the project itself labels Jun 22, 2022
@kittaakos
Copy link
Contributor

kittaakos commented Jun 23, 2022

Anyway, this feature is not suitable for direct use because it will produce a wrong build if not used correctly, that's why it's hidden.

When will the LS use the new --skip-libraries-discovery flag?

The arduino-language-server uses this feature to dramatically speed up updating and reindexing the sketch

I assume, the reindexing happens on textDocument/didOpen and textDocument/didChange events. This PR enables the LS to run compile without the discovery. Therefore, it's the IDE2's responsibility to send the build_path to the LS after a verify. But the build_path hash generation is platform and FQBN independent. Isn't it a problem?

a.kitta@Akoss-MacBook-Pro arduino-cli_0.24.0_macOS_64bit % ./arduino-cli version                                   
arduino-cli  Version: 0.24.0 Commit: c1b10f56 Date: 2022-06-22T10:09:11Z
a.kitta@Akoss-MacBook-Pro arduino-cli_0.24.0_macOS_64bit % ./arduino-cli core list                                 
ID           Installed Latest Name                                        
arduino:avr  1.8.5     1.8.5  Arduino AVR Boards                          
arduino:samd 1.8.13    1.8.13 Arduino SAMD Boards (32-bits ARM Cortex-M0+)

a.kitta@Akoss-MacBook-Pro arduino-cli_0.24.0_macOS_64bit % cat ~/Documents/Arduino/sketch_jun22b/sketch_jun22b.ino 
void setup() {}
void loop() {}
a.kitta@Akoss-MacBook-Pro arduino-cli_0.24.0_macOS_64bit % ./arduino-cli compile -b arduino:avr:uno ~/Documents/Arduino/sketch_jun22b --format json | jq '.builder_result.build_path'
"/private/var/folders/z1/xkw1yh5n7rz4n8djprp1mdn80000gn/T/arduino-sketch-2A25CBEDDFEB3508AE8DDDA45D126AAA"
a.kitta@Akoss-MacBook-Pro arduino-cli_0.24.0_macOS_64bit % ./arduino-cli compile -b arduino:samd:mkr1000 ~/Documents/Arduino/sketch_jun22b --format json | jq '.builder_result.build_path'
"/private/var/folders/z1/xkw1yh5n7rz4n8djprp1mdn80000gn/T/arduino-sketch-2A25CBEDDFEB3508AE8DDDA45D126AAA"
a.kitta@Akoss-MacBook-Pro arduino-cli_0.24.0_macOS_64bit % 

The arduino:avr:uno build output will be the same as for the arduino:samd:mkr1000 board. They will override each other.

Snippet from $build_path/sketch/sketch_jun22b.ino.cpp.d with arduino:avr:uno:

/private/var/folders/z1/xkw1yh5n7rz4n8djprp1mdn80000gn/T/arduino-sketch-2A25CBEDDFEB3508AE8DDDA45D126AAA/sketch/sketch_jun22b.ino.cpp.o: \
 /private/var/folders/z1/xkw1yh5n7rz4n8djprp1mdn80000gn/T/arduino-sketch-2A25CBEDDFEB3508AE8DDDA45D126AAA/sketch/sketch_jun22b.ino.cpp \
 /Users/a.kitta/Library/Arduino15/packages/arduino/hardware/avr/1.8.5/cores/arduino/Arduino.h \

...(rest)

Snippet from $build_path/sketch/sketch_jun22b.ino.cpp.d with arduinio:same:mkr1000:

/private/var/folders/z1/xkw1yh5n7rz4n8djprp1mdn80000gn/T/arduino-sketch-2A25CBEDDFEB3508AE8DDDA45D126AAA/sketch/sketch_jun22b.ino.cpp.o: \
 /private/var/folders/z1/xkw1yh5n7rz4n8djprp1mdn80000gn/T/arduino-sketch-2A25CBEDDFEB3508AE8DDDA45D126AAA/sketch/sketch_jun22b.ino.cpp \
 /Users/a.kitta/Library/Arduino15/packages/arduino/hardware/samd/1.8.13/cores/arduino/Arduino.h \

...(rest)

References to the Arduino.h file are different, so all language features will be incorrect in IDE2. For example, Go to Definition. Installing libraries can make the situation more complex. An outdated state could lead to false-positive compiler errors. The invalid state can be "fixed" only by running a verify/upload from the IDE2.

How will the --skip-libraries-discovery flag work in action?


Perhaps the LS can do the compile with the lib discovery on textDocument/didOpen and without on textDocument/didChange event. Or maybe the CLI could include the fqbn in the build_path hash generation. Otherwise, the IDE2 could provide incorrect language features to the users without noticing it.

@cmaglie cmaglie force-pushed the skip_lib_search_result branch from 250e9e5 to 8807a74 Compare July 5, 2022 14:47
@cmaglie
Copy link
Member Author

cmaglie commented Jul 5, 2022

Perhaps the LS can do the compile with the lib discovery on textDocument/didOpen and without on textDocument/didChange event. Or maybe the CLI could include the fqbn in the build_path hash generation. Otherwise, the IDE2 could provide incorrect language features to the users without noticing it.

Actually, the LS will only take the libraries.cache file from the verify made by the IDE. This allows the LS to safely skip the libraries discovery (since it was already made by the IDE compilation), it should find the rest of the libraries via the include path.

The build path is forced by the LS to a randomly generated temp dir, so the build made by the LS will not affect the build made by the IDE in any way.

BTW this is very theoretical and very tricky to get right, we should make extensive tests to validate it.

@cmaglie cmaglie merged commit a5466d0 into arduino:master Jul 11, 2022
@cmaglie cmaglie deleted the skip_lib_search_result branch July 11, 2022 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: code Related to content of the project itself type: enhancement Proposed improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants