Skip to content
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

[breaking] Add env variable to let tools know the cli version and the gRPC client version #1640

Merged
merged 10 commits into from
Jan 31, 2022

Conversation

cmaglie
Copy link
Member

@cmaglie cmaglie commented Jan 26, 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 new environment variable ARDUINO_MODE that is passed on all tools launched by the CLI.
The value of the variable may be:
* cli if the arduino-cli is used from terminal or scripts as a pure command-line tool.
* daemon if the arduino-cli is launched as a daemon.

Adds an ARDUINO_USER_AGENT environment variable to all the tools launched for compile and upload.
The variable is in HTTP user-agent format and contains the version of the CLI and possibly the version of the gRPC client of the user if this information is available.
Some examples:

ARDUINO_USER_AGENT=arduino-cli/0.21.0
ARDUINO_USER_AGENT=arduino-cli/0.21.0 ArduinoIDE/2.0.0-rc3

Does this PR introduce a breaking change, and is titled accordingly?
Yes, there are some breaking changes in the golang API.

@cmaglie
Copy link
Member Author

cmaglie commented Jan 26, 2022

It looks like we have a regression on MacOS, I'm setting this PR as draft until I found the reason.

@cmaglie cmaglie marked this pull request as draft January 26, 2022 16:02
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.

I know this is still WIP, but I want to add a "to do" that the final proposal should include documentation. Otherwise it will end up as one of those esoteric features nobody remembers a year from now.

It isn't clear to me which is the ideal place to document this. The available options are probably:

Any opinions on the location?

@cmaglie
Copy link
Member Author

cmaglie commented Jan 26, 2022

IMHO https://arduino.github.io/arduino-cli/dev/platform-specification/#tools is the best place, I'll write down some notes there.

@cmaglie cmaglie changed the title Add env variable to let tools know if the cli is running as a grpc daemon or not Add env variable to let tools know the cli version and the gRPC client version Jan 28, 2022
@cmaglie
Copy link
Member Author

cmaglie commented Jan 28, 2022

@PaulStoffregen
After a brief internal discussion, I've changed the env variable to ARDUINO_USER_AGENT (more detailed description in the opening post of this PR: #1640 (comment)).

The rationale is that the previous ARDUINO_MODE=cli/daemon does not contain enough information to determine if the process is running on a GUI, there are some corner cases like:

  • running the gRPC daemon as a backend for an HTTP service endpoint: in this case we don't want to display a GUI in a headless server
  • other software that uses the CLI daemon that does not necessarily have a GUI
  • in general, running the daemon does not imply running a GUI

I guess you would like to check if the user agent contains the string ArduinoIDE to actually display a GUI.

@cmaglie cmaglie marked this pull request as ready for review January 28, 2022 11:40
@cmaglie cmaglie requested a review from per1234 January 28, 2022 11:41
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.

Please also document the breaking changes to the public Go package API according to the standard policy:
https://arduino.github.io/arduino-cli/dev/CONTRIBUTING/#breaking

executils/process.go Outdated Show resolved Hide resolved
executils/process.go Outdated Show resolved Hide resolved
arduino/cores/packagemanager/package_manager.go Outdated Show resolved Hide resolved
docs/platform-specification.md Outdated Show resolved Hide resolved
@cmaglie cmaglie changed the title Add env variable to let tools know the cli version and the gRPC client version [breaking] Add env variable to let tools know the cli version and the gRPC client version Jan 28, 2022
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.

Thanks Cristian!

@cmaglie cmaglie self-assigned this Jan 28, 2022
@cmaglie cmaglie added priority: medium Resolution is a medium priority topic: CLI Related to the command line interface type: enhancement Proposed improvement labels Jan 28, 2022
@cmaglie cmaglie added this to the arduino-cli 0.21.0 milestone Jan 28, 2022
executils/process.go Outdated Show resolved Hide resolved
@cmaglie cmaglie merged commit 4256524 into arduino:master Jan 31, 2022
@cmaglie cmaglie deleted the env_for_cli_mode branch January 31, 2022 14:27
@PaulStoffregen
Copy link

Thanks!!

Today I'm still working on porting my pluggable serial monitor code to Windows. Hoping I can wrap that up tonight, and then tomorrow start using the next nightly build with this environment variable.

@cmaglie
Copy link
Member Author

cmaglie commented Jan 31, 2022

Do not rush on this one, we need to fix also the Arduino IDE to fully support it arduino/arduino-ide#790

@PaulStoffregen PaulStoffregen mentioned this pull request Mar 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: medium Resolution is a medium priority topic: CLI Related to the command line interface type: enhancement Proposed improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants