[breaking] Clean up and refactor `--format json` and, in general, how Arduino CLI output is handled #2003

cmaglie · 2022-12-07T13:00:35Z

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?

The feedback golang API has changed, this should fix all the issues where the JSON output is mixed with text.
A static source code analyzer has been added in the unit-tests, it should detect when the feedback package is bypassed:

=== RUN   TestNoDirectOutputToStdOut
burnbootloader/burnbootloader.go:85:5: object `os.Stdout` should not be used in this package (use `feedback.*` instead)
burnbootloader/burnbootloader.go:85:16: object `os.Stderr` should not be used in this package (use `feedback.*` instead)
compile/compile.go:235:84: object `os.Stdout` should not be used in this package (use `feedback.*` instead)
compile/compile.go:235:95: object `os.Stderr` should not be used in this package (use `feedback.*` instead)
compile/compile.go:274:72: object `os.Stdout` should not be used in this package (use `feedback.*` instead)
compile/compile.go:274:83: object `os.Stderr` should not be used in this package (use `feedback.*` instead)
compile/compile.go:296:4: function `fmt.Println` should not be used in this package (use `feedback.*` instead)
compile/compile.go:297:4: function `fmt.Println` should not be used in this package (use `feedback.*` instead)
compile/compile.go:298:4: function `fmt.Println` should not be used in this package (use `feedback.*` instead)
compile/compile.go:305:3: function `fmt.Println` should not be used in this package (use `feedback.*` instead)
compile/compile.go:306:3: function `fmt.Println` should not be used in this package (use `feedback.*` instead)
compile/compile.go:307:3: function `fmt.Println` should not be used in this package (use `feedback.*` instead)
compile/compile.go:308:3: function `fmt.Println` should not be used in this package (use `feedback.*` instead)
compile/compile.go:309:3: function `fmt.Println` should not be used in this package (use `feedback.*` instead)
compile/compile.go:311:3: function `fmt.Println` should not be used in this package (use `feedback.*` instead)
compile/compile.go:313:4: function `fmt.Println` should not be used in this package (use `feedback.*` instead)
compile/compile.go:319:4: function `fmt.Println` should not be used in this package (use `feedback.*` instead)
compile/compile.go:321:5: function `fmt.Println` should not be used in this package (use `feedback.*` instead)
compile/compile.go:325:4: function `fmt.Println` should not be used in this package (use `feedback.*` instead)
compile/compile.go:326:4: function `fmt.Print` should not be used in this package (use `feedback.*` instead)
completion/completion.go:58:34: object `os.Stdout` should not be used in this package (use `feedback.*` instead)
completion/completion.go:61:38: object `os.Stdout` should not be used in this package (use `feedback.*` instead)
completion/completion.go:63:32: object `os.Stdout` should not be used in this package (use `feedback.*` instead)
completion/completion.go:66:32: object `os.Stdout` should not be used in this package (use `feedback.*` instead)
completion/completion.go:68:38: object `os.Stdout` should not be used in this package (use `feedback.*` instead)
daemon/interceptors.go:29:19: object `os.Stdout` should not be used in this package (use `feedback.*` instead)
debug/debug.go:106:82: object `os.Stdout` should not be used in this package (use `feedback.*` instead)
monitor/term.go:38:9: object `os.Stdout` should not be used in this package (use `feedback.*` instead)
output/rpc_progress.go:115:4: function `fmt.Print` should not be used in this package (use `feedback.*` instead)
output/rpc_progress.go:117:5: function `fmt.Println` should not be used in this package (use `feedback.*` instead)
output/rpc_progress.go:119:5: function `fmt.Println` should not be used in this package (use `feedback.*` instead)
upload/upload.go:163:5: object `os.Stdout` should not be used in this package (use `feedback.*` instead)
upload/upload.go:163:16: object `os.Stderr` should not be used in this package (use `feedback.*` instead)
cli.go:215:4: function `fmt.Println` should not be used in this package (use `feedback.*` instead)
--- FAIL: TestNoDirectOutputToStdOut (0.01s)
FAIL
exit status 1
FAIL	github.com/arduino/arduino-cli/cli	0.019s

As a nice side effect, the new feedback API helped to clean up some commands behavior when --format json is specified.

What is the current / new behavior?

If the interactive terminal is required, JSON format will produce an error

CURRENT:

$ arduino-cli monitor -p /dev/ttyACM0 --format json
"Connected to /dev/ttyACM0! Press CTRL-C to exit."
^C
$

NEW:

$ arduino-cli monitor -p /dev/ttyACM0 --format json
{
  "error": "interactive terminal not supported for the 'json' output format"
}

JSON output produces an error if not compatible with the given command/args combination. Fatal errors are encoded in JSON

CURRENT:

$ arduino-cli daemon --format json --debug
{
  "IP": "127.0.0.1",
  "Port": "50051"
}
^C
$

NEW:

$ arduino-cli daemon --format json --debug
{
  "error": "Debug log is only available in text format"
}
$ arduino-cli daemon --format json --debug --debug-file log.txt
{
  "IP": "127.0.0.1",
  "Port": "50051"
}
^C

Warnings are printed in text mode, or accumulated in the json result in JSON mode

CURRENT:

$ arduino-cli compile -b arduino:samd:arduino_zero_native --format json 
Sketches with .pde extension are deprecated, please rename the following files to .ino:
/home/megabug/Arduino/Blink/A.pde
{
  "compiler_out": "Sketch uses 40192 bytes (15%) of program storage space. Maximum is 262144 bytes.\nGlobal variables use 4012 bytes (12%) of dynamic memory, leaving 28756 bytes for local variables. Maximum is 32768 bytes.\n",
  "compiler_err": "",
  "builder_result": { .... },
  "success": true
}

NEW:

$ arduino-cli compile -b arduino:samd:arduino_zero_native --format json 
{
  "compiler_err": "",
  "compiler_out": "Sketch uses 40192 bytes (15%) of program storage space. Maximum is 262144 bytes.\nGlobal variables use 4012 bytes (12%) of dynamic memory, leaving 28756 bytes for local variables. Maximum is 32768 bytes.\n",
  "builder_result": { ... },
  "success": true,
  "warnings": [
    "Sketches with .pde extension are deprecated, please rename the following files to .ino:\n - /home/megabug/Arduino/Blink/A.pde"
  ]
}

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

Yes.

internal/cli/feedback/feedback_cmd.go

internal/cli/feedback/stdio.go

internal/cli/arguments/arguments.go

internal/cli/compile/compile.go

internal/cli/feedback/feedback.go

internal/cli/feedback/feedback_cmd.go

internal/cli/feedback/stdio.go

The first test run gives: === RUN TestNoDirectOutputToStdOut burnbootloader/burnbootloader.go:85:5: object `os.Stdout` should not be used in this package (use `feedback.*` instead) burnbootloader/burnbootloader.go:85:16: object `os.Stderr` should not be used in this package (use `feedback.*` instead) compile/compile.go:235:84: object `os.Stdout` should not be used in this package (use `feedback.*` instead) compile/compile.go:235:95: object `os.Stderr` should not be used in this package (use `feedback.*` instead) compile/compile.go:274:72: object `os.Stdout` should not be used in this package (use `feedback.*` instead) compile/compile.go:274:83: object `os.Stderr` should not be used in this package (use `feedback.*` instead) compile/compile.go:296:4: function `fmt.Println` should not be used in this package (use `feedback.*` instead) compile/compile.go:297:4: function `fmt.Println` should not be used in this package (use `feedback.*` instead) compile/compile.go:298:4: function `fmt.Println` should not be used in this package (use `feedback.*` instead) compile/compile.go:305:3: function `fmt.Println` should not be used in this package (use `feedback.*` instead) compile/compile.go:306:3: function `fmt.Println` should not be used in this package (use `feedback.*` instead) compile/compile.go:307:3: function `fmt.Println` should not be used in this package (use `feedback.*` instead) compile/compile.go:308:3: function `fmt.Println` should not be used in this package (use `feedback.*` instead) compile/compile.go:309:3: function `fmt.Println` should not be used in this package (use `feedback.*` instead) compile/compile.go:311:3: function `fmt.Println` should not be used in this package (use `feedback.*` instead) compile/compile.go:313:4: function `fmt.Println` should not be used in this package (use `feedback.*` instead) compile/compile.go:319:4: function `fmt.Println` should not be used in this package (use `feedback.*` instead) compile/compile.go:321:5: function `fmt.Println` should not be used in this package (use `feedback.*` instead) compile/compile.go:325:4: function `fmt.Println` should not be used in this package (use `feedback.*` instead) compile/compile.go:326:4: function `fmt.Print` should not be used in this package (use `feedback.*` instead) completion/completion.go:58:34: object `os.Stdout` should not be used in this package (use `feedback.*` instead) completion/completion.go:61:38: object `os.Stdout` should not be used in this package (use `feedback.*` instead) completion/completion.go:63:32: object `os.Stdout` should not be used in this package (use `feedback.*` instead) completion/completion.go:66:32: object `os.Stdout` should not be used in this package (use `feedback.*` instead) completion/completion.go:68:38: object `os.Stdout` should not be used in this package (use `feedback.*` instead) daemon/interceptors.go:29:19: object `os.Stdout` should not be used in this package (use `feedback.*` instead) debug/debug.go:106:82: object `os.Stdout` should not be used in this package (use `feedback.*` instead) monitor/term.go:38:9: object `os.Stdout` should not be used in this package (use `feedback.*` instead) output/rpc_progress.go:115:4: function `fmt.Print` should not be used in this package (use `feedback.*` instead) output/rpc_progress.go:117:5: function `fmt.Println` should not be used in this package (use `feedback.*` instead) output/rpc_progress.go:119:5: function `fmt.Println` should not be used in this package (use `feedback.*` instead) upload/upload.go:163:5: object `os.Stdout` should not be used in this package (use `feedback.*` instead) upload/upload.go:163:16: object `os.Stderr` should not be used in this package (use `feedback.*` instead) cli.go:215:4: function `fmt.Println` should not be used in this package (use `feedback.*` instead) --- FAIL: TestNoDirectOutputToStdOut (0.01s) FAIL exit status 1 FAIL github.com/arduino/arduino-cli/cli 0.019s

Also changed a condition in a more meaningful way.

We may think of bringing it back again in the future if the global instance will be removed in a major refactoring of the CLI.

This is the better place where it belongs, and slighlty simplifies the golang API.

- Final "\n" is automatically printed when entering non-passwords (no need to do a Println() - The final "\n" is included in the result of ReadBytes() so it will be removed.

per1234

UPDATE: resolved

0c2dc76 caused the error to be printed twice (once to stdout, and once to stderr):
$ arduino-cli version arduino-cli.exe Version: git-snapshot Commit: 0c2dc76b Date: 2023-01-13T15:38:07Z $ mkdir /tmp/FooSketch $ printf "#error\nvoid setup() {}\nvoid loop() {}" > /tmp/FooSketch/FooSketch.ino $ arduino-cli compile --fqbn arduino:avr:uno /tmp/FooSketch C:\Users\per\AppData\Local\Temp\FooSketch\FooSketch.ino:1:2: error: #error #error ^~~~~ Used platform Version Path arduino:avr 1.8.6 C:\Users\per\AppData\Local\Arduino15\packages\arduino\hardware\avr\1.8.6 Error during build: exit status 1 Error during build: exit status 1

~~(note there are two "Error during build: exit status 1" in the output)~~

internal/cli/feedback/stdio.go

`Can't write debug log: available only in text format` error is thrown by the CLI if the `--debug` flag is present. Ref: arduino/arduino-cli#2003 Closes #1942 Signed-off-by: Akos Kitta <a.kitta@arduino.cc>

cmaglie changed the title ~~Clean up and refactor how --format json works and general Arduino CLI output is handled~~ Clean up and refactor --format json and, in general, how Arduino CLI output is handled Dec 7, 2022

cmaglie force-pushed the feedback_refactor branch 5 times, most recently from 7075255 to 6a6885b Compare December 9, 2022 14:01

per1234 added type: enhancement Proposed improvement topic: infrastructure Related to project infrastructure topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project labels Dec 12, 2022

cmaglie self-assigned this Dec 12, 2022

cmaglie requested review from umbynos, MatteoPologruto and per1234 December 12, 2022 16:08

cmaglie added this to the Arduino CLI 1.0 milestone Dec 12, 2022

cmaglie marked this pull request as ready for review December 12, 2022 16:08

This was linked to issues Dec 16, 2022

Machine-readable --format yaml produces text output while --format json doesn't #1872

Closed

Missing handling of new output formats #1635

Closed

CLI upload command seems to ignore "--format json" option #1961

Closed

per1234 requested changes Dec 16, 2022

View reviewed changes

cmaglie added 10 commits January 3, 2023 16:43

Slightly improved naming/docs of OutputFormat enumeration

3604efa

Also changed a condition in a more meaningful way.

Removed feedback.Feedback since only the global instance is used

922a3eb

We may think of bringing it back again in the future if the global instance will be removed in a major refactoring of the CLI.

Moved cli/output package into cli/feedback

4d593ad

This is the better place where it belongs, and slighlty simplifies the golang API.

Print progress bar and task progess only on interactive terminals

93c2d62

Use feedback functions to output task progress

766da7f

User-input functions are now moved into feedback package

344669e

Fix user-input function

c850c16

- Final "\n" is automatically printed when entering non-passwords (no need to do a Println() - The final "\n" is included in the result of ReadBytes() so it will be removed.

Added cmd to test feedback functions

53c80b0

Better error message

94cb4b5

cmaglie changed the title ~~Clean up and refactor --format json and, in general, how Arduino CLI output is handled~~ [breaking] Clean up and refactor --format json and, in general, how Arduino CLI output is handled Jan 5, 2023

cmaglie force-pushed the feedback_refactor branch 2 times, most recently from ee2d2c3 to dc8c25d Compare January 13, 2023 13:25

cmaglie added 4 commits January 13, 2023 15:23

feedback: remove stray '\r' on Windows on interactive input

6a4a1dc

Ban use of os.Exit from cli package

722138d

Removed unused parameter in compile.Compile

ca2451b

Non-interactive stream are always buffered

d2db935

cmaglie force-pushed the feedback_refactor branch from dc8c25d to 0c2dc76 Compare January 13, 2023 14:24

per1234 requested changes Jan 13, 2023

View reviewed changes

internal/cli/feedback/stdio.go Outdated Show resolved Hide resolved

cmaglie force-pushed the feedback_refactor branch from 0c2dc76 to 8cf4d6a Compare January 13, 2023 16:06

cmaglie added 3 commits January 13, 2023 17:12

Use direct streams where appropiate

3ef55a6

Compile outputs profile dump as part of the result

3b20b8d

Report saved warnings also when erroring out

ab8a756

cmaglie force-pushed the feedback_refactor branch from 8cf4d6a to 2988ecd Compare January 13, 2023 16:12

cmaglie added 3 commits January 13, 2023 17:40

Print compile error and suggestions as part of the result

4e05142

Add trailing newline only if compiler has produced output

5735f8a

FatalResult now outputs the error on stderr

3d5ea52

cmaglie force-pushed the feedback_refactor branch from 2988ecd to 3d5ea52 Compare January 13, 2023 16:41

per1234 approved these changes Jan 14, 2023

View reviewed changes

umbynos approved these changes Jan 16, 2023

View reviewed changes

cmaglie merged commit b3e8f8a into arduino:master Jan 16, 2023

cmaglie deleted the feedback_refactor branch January 16, 2023 11:03

MatteoPologruto mentioned this pull request Feb 8, 2023

[skip-changelog] Fix upload test according to CLI output refactoring #2067

Merged

5 tasks

kittaakos mentioned this pull request Mar 7, 2023

2.0.4 does not start with "arduino.cli.daemon.debug": true settings arduino/arduino-ide#1942

Closed

3 tasks

kittaakos mentioned this pull request Mar 10, 2023

fix: use text --format for the CLI arduino/arduino-ide#1947

Merged

6 tasks

umbynos mentioned this pull request Jun 13, 2023

Update Arduino CLI and general enhancements arduino/arduino-fwuploader#171

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[breaking] Clean up and refactor `--format json` and, in general, how Arduino CLI output is handled #2003

[breaking] Clean up and refactor `--format json` and, in general, how Arduino CLI output is handled #2003

Uh oh!

cmaglie commented Dec 7, 2022 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

per1234 left a comment •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

[breaking] Clean up and refactor --format json and, in general, how Arduino CLI output is handled #2003

[breaking] Clean up and refactor --format json and, in general, how Arduino CLI output is handled #2003

Uh oh!

Conversation

cmaglie commented Dec 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Please check if the PR fulfills these requirements

What kind of change does this PR introduce?

What is the current / new behavior?

If the interactive terminal is required, JSON format will produce an error

JSON output produces an error if not compatible with the given command/args combination. Fatal errors are encoded in JSON

Warnings are printed in text mode, or accumulated in the json result in JSON mode

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

per1234 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

[breaking] Clean up and refactor `--format json` and, in general, how Arduino CLI output is handled #2003

[breaking] Clean up and refactor `--format json` and, in general, how Arduino CLI output is handled #2003

cmaglie commented Dec 7, 2022 •

edited

Loading

per1234 left a comment •

edited

Loading