[breaking] legacy: refactoring of the old i18n.Logger (#1621)

* LoggerToCustomStreams must have pointer receiver

Becuase it has a mutex field that otherwise is copied.

* Removed barely used legacy i18n.Logger.UnformattedFprintln function

* Removed barely used legacy i18n.Logger.UnformattedWrite function

* Removed unused AccumulatorLogger

* Added 'percent' to TaskProgress gRPC message

* Added gRPC placeholders to report compile progress

* legacy: builder task progress is now transferred via TaskProgress callback

* Removed unused Logger.Flush interface method

* Removed Logger.Name method (use type-assertions instead)

* Added note on breaking API change

Author: cmaglie

cli/compile/compile.go

@@ -199,9 +199,9 @@ func runCompileCommand(cmd *cobra.Command, args []string) {
 	var compileRes *rpc.CompileResponse
 	var compileError error
 	if output.OutputFormat == "json" {
-		compileRes, compileError = compile.Compile(context.Background(), compileRequest, compileStdOut, compileStdErr, verboseCompile)
+		compileRes, compileError = compile.Compile(context.Background(), compileRequest, compileStdOut, compileStdErr, nil, verboseCompile)
 	} else {
-		compileRes, compileError = compile.Compile(context.Background(), compileRequest, os.Stdout, os.Stderr, verboseCompile)
+		compileRes, compileError = compile.Compile(context.Background(), compileRequest, os.Stdout, os.Stderr, nil, verboseCompile)
 	}
 
 	if compileError == nil && uploadAfterCompile {

commands/compile/compile.go

@@ -46,7 +46,7 @@ import (
 var tr = i18n.Tr
 
 // Compile FIXMEDOC
-func Compile(ctx context.Context, req *rpc.CompileRequest, outStream, errStream io.Writer, debug bool) (r *rpc.CompileResponse, e error) {
+func Compile(ctx context.Context, req *rpc.CompileRequest, outStream, errStream io.Writer, progressCB commands.TaskProgressCB, debug bool) (r *rpc.CompileResponse, e error) {
 
 	// There is a binding between the export binaries setting and the CLI flag to explicitly set it,
 	// since we want this binding to work also for the gRPC interface we must read it here in this
@@ -132,6 +132,7 @@ func Compile(ctx context.Context, req *rpc.CompileRequest, outStream, errStream
 	builderCtx.PackageManager = pm
 	builderCtx.FQBN = fqbn
 	builderCtx.SketchLocation = sk.FullPath
+	builderCtx.ProgressCB = progressCB
 
 	// FIXME: This will be redundant when arduino-builder will be part of the cli
 	builderCtx.HardwareDirs = configuration.HardwareDirectories(configuration.Settings)
@@ -206,7 +207,7 @@ func Compile(ctx context.Context, req *rpc.CompileRequest, outStream, errStream
 
 	builderCtx.ExecStdout = outStream
 	builderCtx.ExecStderr = errStream
-	builderCtx.SetLogger(legacyi18n.LoggerToCustomStreams{Stdout: outStream, Stderr: errStream})
+	builderCtx.SetLogger(&legacyi18n.LoggerToCustomStreams{Stdout: outStream, Stderr: errStream})
 	builderCtx.Clean = req.GetClean()
 	builderCtx.OnlyUpdateCompilationDatabase = req.GetCreateCompilationDatabaseOnly()
 

commands/daemon/daemon.go

@@ -262,6 +262,7 @@ func (s *ArduinoCoreServerImpl) Compile(req *rpc.CompileRequest, stream rpc.Ardu
 		stream.Context(), req,
 		utils.FeedStreamTo(func(data []byte) { stream.Send(&rpc.CompileResponse{OutStream: data}) }),
 		utils.FeedStreamTo(func(data []byte) { stream.Send(&rpc.CompileResponse{ErrStream: data}) }),
+		func(p *rpc.TaskProgress) { stream.Send(&rpc.CompileResponse{Progress: p}) },
 		false) // Set debug to false
 	if err != nil {
 		return convertErrorToRPCStatus(err)

docs/UPGRADING.md

@@ -4,6 +4,23 @@ Here you can find a list of migration guides to handle breaking changes between
 
 ## Unreleased
 
+### `commands.Compile` function change
+
+A new argument `progressCB` has been added to `commands.Compile`, the new function signature is:
+
+```go
+func Compile(
+	ctx context.Context,
+	req *rpc.CompileRequest,
+	outStream, errStream io.Writer,
+	progressCB commands.TaskProgressCB,
+	debug bool
+) (r *rpc.CompileResponse, e error) {
+```
+
+if a callback function is provided the `Compile` command will call it periodically with progress reports with the
+percentage of compilation completed, otherwise, if the parameter is `nil`, no progress reports will be performed.
+
 ### `github.com/arduino/arduino-cli/cli/arguments.ParseReferences` function change
 
 The `parseArch` parameter was removed since it was unused and was always true. This means that the architecture gets

legacy/builder/builder.go

@@ -23,7 +23,6 @@ import (
 
 	"github.com/arduino/arduino-cli/arduino/sketch"
 	"github.com/arduino/arduino-cli/i18n"
-	"github.com/arduino/arduino-cli/legacy/builder/builder_utils"
 	"github.com/arduino/arduino-cli/legacy/builder/phases"
 	"github.com/arduino/arduino-cli/legacy/builder/types"
 	"github.com/arduino/arduino-cli/legacy/builder/utils"
@@ -195,7 +194,7 @@ func runCommands(ctx *types.Context, commands []types.Command) error {
 			return errors.WithStack(err)
 		}
 		ctx.Progress.CompleteStep()
-		builder_utils.PrintProgressIfProgressEnabledAndMachineLogger(ctx)
+		ctx.PushProgress()
 	}
 	return nil
 }

legacy/builder/builder_utils/utils.go

@@ -20,7 +20,6 @@ import (
 	"os/exec"
 	"path/filepath"
 	"runtime"
-	"strconv"
 	"strings"
 	"sync"
 
@@ -35,18 +34,6 @@ import (
 
 var tr = i18n.Tr
 
-func PrintProgressIfProgressEnabledAndMachineLogger(ctx *types.Context) {
-
-	if !ctx.Progress.PrintEnabled {
-		return
-	}
-
-	log := ctx.GetLogger()
-	if log.Name() == "machine" {
-		log.Println(constants.LOG_LEVEL_INFO, tr("Progress {0}"), strconv.FormatFloat(float64(ctx.Progress.Progress), 'f', 2, 32))
-	}
-}
-
 func CompileFilesRecursive(ctx *types.Context, sourcePath *paths.Path, buildPath *paths.Path, buildProperties *properties.Map, includes []string) (paths.PathList, error) {
 	objectFiles, err := CompileFiles(ctx, sourcePath, false, buildPath, buildProperties, includes)
 	if err != nil {
@@ -215,7 +202,7 @@ func compileFilesWithRecipe(ctx *types.Context, sourcePath *paths.Path, sources
 		queue <- source
 
 		ctx.Progress.CompleteStep()
-		PrintProgressIfProgressEnabledAndMachineLogger(ctx)
+		ctx.PushProgress()
 	}
 	close(queue)
 	wg.Wait()

legacy/builder/container_setup.go

@@ -19,7 +19,6 @@ import (
 	"fmt"
 
 	sk "github.com/arduino/arduino-cli/arduino/sketch"
-	"github.com/arduino/arduino-cli/legacy/builder/builder_utils"
 	"github.com/arduino/arduino-cli/legacy/builder/types"
 	"github.com/pkg/errors"
 )
@@ -50,7 +49,7 @@ func (s *ContainerSetupHardwareToolsLibsSketchAndProps) Run(ctx *types.Context)
 			return errors.WithStack(err)
 		}
 		ctx.Progress.CompleteStep()
-		builder_utils.PrintProgressIfProgressEnabledAndMachineLogger(ctx)
+		ctx.PushProgress()
 	}
 
 	if ctx.SketchLocation != nil {
@@ -78,7 +77,7 @@ func (s *ContainerSetupHardwareToolsLibsSketchAndProps) Run(ctx *types.Context)
 		ctx.Sketch = sketch
 	}
 	ctx.Progress.CompleteStep()
-	builder_utils.PrintProgressIfProgressEnabledAndMachineLogger(ctx)
+	ctx.PushProgress()
 
 	commands = []types.Command{
 		&SetupBuildProperties{},
@@ -94,7 +93,7 @@ func (s *ContainerSetupHardwareToolsLibsSketchAndProps) Run(ctx *types.Context)
 			return errors.WithStack(err)
 		}
 		ctx.Progress.CompleteStep()
-		builder_utils.PrintProgressIfProgressEnabledAndMachineLogger(ctx)
+		ctx.PushProgress()
 	}
 
 	return nil

legacy/builder/i18n/errors.go

@@ -8,7 +8,7 @@ import (
 )
 
 func ErrorfWithLogger(logger Logger, format string, a ...interface{}) error {
-	if logger.Name() == "machine" {
+	if _, isMachineLogger := logger.(*MachineLogger); isMachineLogger {
 		logger.Fprintln(os.Stderr, constants.LOG_LEVEL_ERROR, format, a...)
 		return errors.New("")
 	}

legacy/builder/i18n/i18n.go

@@ -26,15 +26,11 @@ import (
 	"sync"
 )
 
-var PLACEHOLDER = regexp.MustCompile("{(\\d)}")
+var PLACEHOLDER = regexp.MustCompile(`{(\d)}`)
 
 type Logger interface {
 	Fprintln(w io.Writer, level string, format string, a ...interface{})
-	UnformattedFprintln(w io.Writer, s string)
-	UnformattedWrite(w io.Writer, data []byte)
 	Println(level string, format string, a ...interface{})
-	Name() string
-	Flush() string
 }
 
 type LoggerToCustomStreams struct {
@@ -43,7 +39,7 @@ type LoggerToCustomStreams struct {
 	mux    sync.Mutex
 }
 
-func (s LoggerToCustomStreams) Fprintln(w io.Writer, level string, format string, a ...interface{}) {
+func (s *LoggerToCustomStreams) Fprintln(w io.Writer, level string, format string, a ...interface{}) {
 	s.mux.Lock()
 	defer s.mux.Unlock()
 	target := s.Stdout
@@ -53,165 +49,47 @@ func (s LoggerToCustomStreams) Fprintln(w io.Writer, level string, format string
 	fmt.Fprintln(target, Format(format, a...))
 }
 
-func (s LoggerToCustomStreams) UnformattedFprintln(w io.Writer, str string) {
-	s.mux.Lock()
-	defer s.mux.Unlock()
-	target := s.Stdout
-	if w == os.Stderr {
-		target = s.Stderr
-	}
-	fmt.Fprintln(target, str)
-}
-
-func (s LoggerToCustomStreams) UnformattedWrite(w io.Writer, data []byte) {
-	s.mux.Lock()
-	defer s.mux.Unlock()
-	target := s.Stdout
-	if w == os.Stderr {
-		target = s.Stderr
-	}
-	target.Write(data)
-}
-
-func (s LoggerToCustomStreams) Println(level string, format string, a ...interface{}) {
+func (s *LoggerToCustomStreams) Println(level string, format string, a ...interface{}) {
 	s.Fprintln(nil, level, format, a...)
 }
 
-func (s LoggerToCustomStreams) Flush() string {
-	return ""
-}
-
-func (s LoggerToCustomStreams) Name() string {
-	return "LoggerToCustomStreams"
-}
-
 type NoopLogger struct{}
 
-func (s NoopLogger) Fprintln(w io.Writer, level string, format string, a ...interface{}) {}
-
-func (s NoopLogger) UnformattedFprintln(w io.Writer, str string) {}
+func (s *NoopLogger) Fprintln(w io.Writer, level string, format string, a ...interface{}) {}
 
-func (s NoopLogger) UnformattedWrite(w io.Writer, data []byte) {}
-
-func (s NoopLogger) Println(level string, format string, a ...interface{}) {}
-
-func (s NoopLogger) Flush() string {
-	return ""
-}
-
-func (s NoopLogger) Name() string {
-	return "noop"
-}
-
-type AccumulatorLogger struct {
-	Buffer *[]string
-}
-
-func (s AccumulatorLogger) Fprintln(w io.Writer, level string, format string, a ...interface{}) {
-	*s.Buffer = append(*s.Buffer, Format(format, a...))
-}
-
-func (s AccumulatorLogger) UnformattedFprintln(w io.Writer, str string) {
-	*s.Buffer = append(*s.Buffer, str)
-}
-
-func (s AccumulatorLogger) UnformattedWrite(w io.Writer, data []byte) {
-	*s.Buffer = append(*s.Buffer, string(data))
-}
-
-func (s AccumulatorLogger) Println(level string, format string, a ...interface{}) {
-	s.Fprintln(nil, level, format, a...)
-}
-
-func (s AccumulatorLogger) Flush() string {
-	str := strings.Join(*s.Buffer, "\n")
-	*s.Buffer = (*s.Buffer)[0:0]
-	return str
-}
-
-func (s AccumulatorLogger) Name() string {
-	return "accumulator"
-}
+func (s *NoopLogger) Println(level string, format string, a ...interface{}) {}
 
 type HumanTagsLogger struct{}
 
-func (s HumanTagsLogger) Fprintln(w io.Writer, level string, format string, a ...interface{}) {
+func (s *HumanTagsLogger) Fprintln(w io.Writer, level string, format string, a ...interface{}) {
 	format = "[" + level + "] " + format
 	fprintln(w, Format(format, a...))
 }
 
-func (s HumanTagsLogger) Println(level string, format string, a ...interface{}) {
+func (s *HumanTagsLogger) Println(level string, format string, a ...interface{}) {
 	s.Fprintln(os.Stdout, level, format, a...)
 }
 
-func (s HumanTagsLogger) UnformattedFprintln(w io.Writer, str string) {
-	fprintln(w, str)
-}
-
-func (s HumanTagsLogger) UnformattedWrite(w io.Writer, data []byte) {
-	write(w, data)
-}
-
-func (s HumanTagsLogger) Flush() string {
-	return ""
-}
-
-func (s HumanTagsLogger) Name() string {
-	return "humantags"
-}
-
 type HumanLogger struct{}
 
-func (s HumanLogger) Fprintln(w io.Writer, level string, format string, a ...interface{}) {
+func (s *HumanLogger) Fprintln(w io.Writer, level string, format string, a ...interface{}) {
 	fprintln(w, Format(format, a...))
 }
 
-func (s HumanLogger) Println(level string, format string, a ...interface{}) {
+func (s *HumanLogger) Println(level string, format string, a ...interface{}) {
 	s.Fprintln(os.Stdout, level, format, a...)
 }
 
-func (s HumanLogger) UnformattedFprintln(w io.Writer, str string) {
-	fprintln(w, str)
-}
-
-func (s HumanLogger) UnformattedWrite(w io.Writer, data []byte) {
-	write(w, data)
-}
-
-func (s HumanLogger) Flush() string {
-	return ""
-}
-
-func (s HumanLogger) Name() string {
-	return "human"
-}
-
 type MachineLogger struct{}
 
-func (s MachineLogger) Fprintln(w io.Writer, level string, format string, a ...interface{}) {
+func (s *MachineLogger) Fprintln(w io.Writer, level string, format string, a ...interface{}) {
 	printMachineFormattedLogLine(w, level, format, a)
 }
 
-func (s MachineLogger) Println(level string, format string, a ...interface{}) {
+func (s *MachineLogger) Println(level string, format string, a ...interface{}) {
 	printMachineFormattedLogLine(os.Stdout, level, format, a)
 }
 
-func (s MachineLogger) UnformattedFprintln(w io.Writer, str string) {
-	fprintln(w, str)
-}
-
-func (s MachineLogger) Flush() string {
-	return ""
-}
-
-func (s MachineLogger) Name() string {
-	return "machine"
-}
-
-func (s MachineLogger) UnformattedWrite(w io.Writer, data []byte) {
-	write(w, data)
-}
-
 func printMachineFormattedLogLine(w io.Writer, level string, format string, a []interface{}) {
 	a = append([]interface{}(nil), a...)
 	for idx, value := range a {
@@ -232,12 +110,6 @@ func fprintln(w io.Writer, s string) {
 	fmt.Fprintln(w, s)
 }
 
-func write(w io.Writer, data []byte) {
-	lock.Lock()
-	defer lock.Unlock()
-	w.Write(data)
-}
-
 func fprintf(w io.Writer, format string, a ...interface{}) {
 	lock.Lock()
 	defer lock.Unlock()

legacy/builder/phases/libraries_builder.go

@@ -123,7 +123,7 @@ func compileLibraries(ctx *types.Context, libraries libraries.List, buildPath *p
 		objectFiles.AddAll(libraryObjectFiles)
 
 		ctx.Progress.CompleteStep()
-		builder_utils.PrintProgressIfProgressEnabledAndMachineLogger(ctx)
+		ctx.PushProgress()
 	}
 
 	return objectFiles, nil

legacy/builder/test/i18n_test.go

@@ -17,10 +17,11 @@ package test
 
 import (
 	"fmt"
+	"testing"
+
 	"github.com/arduino/arduino-cli/legacy/builder/constants"
 	"github.com/arduino/arduino-cli/legacy/builder/i18n"
 	"github.com/stretchr/testify/require"
-	"testing"
 )
 
 func TestI18NSyntax(t *testing.T) {
@@ -50,9 +51,9 @@ func TestI18NSyntax(t *testing.T) {
 
 func TestI18NInheritance(t *testing.T) {
 	var logger i18n.Logger
-	logger = i18n.HumanLogger{}
+	logger = &i18n.HumanLogger{}
 	logger.Println(constants.LOG_LEVEL_INFO, "good {0} {1}", "morning", "vietnam!")
 
-	logger = i18n.MachineLogger{}
+	logger = &i18n.MachineLogger{}
 	logger.Println(constants.LOG_LEVEL_INFO, "good {0} {1}", "morning", "vietnam!")
 }

legacy/builder/types/context.go

@@ -26,17 +26,17 @@ import (
 	"github.com/arduino/arduino-cli/arduino/libraries/librariesmanager"
 	"github.com/arduino/arduino-cli/arduino/libraries/librariesresolver"
 	"github.com/arduino/arduino-cli/arduino/sketch"
+	"github.com/arduino/arduino-cli/commands"
 	"github.com/arduino/arduino-cli/legacy/builder/i18n"
 	rpc "github.com/arduino/arduino-cli/rpc/cc/arduino/cli/commands/v1"
 	paths "github.com/arduino/go-paths-helper"
 	properties "github.com/arduino/go-properties-orderedmap"
 )
 
 type ProgressStruct struct {
-	PrintEnabled bool
-	Progress     float32
-	StepAmount   float32
-	Parent       *ProgressStruct
+	Progress   float32
+	StepAmount float32
+	Parent     *ProgressStruct
 }
 
 func (p *ProgressStruct) AddSubSteps(steps int) {
@@ -144,6 +144,8 @@ type Context struct {
 
 	// Dry run, only create progress map
 	Progress ProgressStruct
+	// Send progress events to this callback
+	ProgressCB commands.TaskProgressCB
 
 	// Contents of a custom build properties file (line by line)
 	CustomBuildProperties []string
@@ -254,3 +256,9 @@ func (ctx *Context) GetLogger() i18n.Logger {
 func (ctx *Context) SetLogger(l i18n.Logger) {
 	ctx.logger = l
 }
+
+func (ctx *Context) PushProgress() {
+	if ctx.ProgressCB != nil {
+		ctx.ProgressCB(&rpc.TaskProgress{Percent: ctx.Progress.Progress})
+	}
+}

legacy/builder/utils/utils.go

@@ -183,7 +183,7 @@ func ExecCommand(ctx *types.Context, command *exec.Cmd, stdout int, stderr int)
 	}
 
 	if ctx.Verbose {
-		ctx.GetLogger().UnformattedFprintln(os.Stdout, PrintableCommand(command.Args))
+		ctx.GetLogger().Println("info", "{0}", PrintableCommand(command.Args))
 	}
 
 	if stdout == Capture {

rpc/cc/arduino/cli/commands/v1/common.pb.go

@@ -44,6 +44,8 @@ message TaskProgress {
   string message = 2;
   // Whether the task is complete.
   bool completed = 3;
+  // Amount in percent of the task completion (optional)
+  float percent = 4;
 }
 
 message Programmer {

rpc/cc/arduino/cli/commands/v1/common.proto

@@ -83,9 +83,9 @@ message CompileRequest {
 }
 
 message CompileResponse {
-  // The output of the compilation process.
+  // The output of the compilation process (stream)
   bytes out_stream = 1;
-  // The error output of the compilation process.
+  // The error output of the compilation process (stream)
   bytes err_stream = 2;
   // The compiler build path
   string build_path = 3;
@@ -97,6 +97,8 @@ message CompileResponse {
   PlatformReference board_platform = 6;
   // The platform used for the build (if referenced from the board platform)
   PlatformReference build_platform = 7;
+  // Completions reports of the compilation process (stream)
+  TaskProgress progress = 8;
 }
 
 message ExecutableSectionSize {