[breaking] Fix export binaries binding not working in gRPC interface

Author: silvanocerza

cli/compile/compile.go

@@ -55,7 +55,6 @@ var (
 	optimizeForDebug        bool     // Optimize compile output for debug, not for release
 	programmer              string   // Use the specified programmer to upload
 	clean                   bool     // Cleanup the build folder and do not use any cached build
-	exportBinaries          bool     // Copies compiled binaries to sketch folder when true
 	compilationDatabaseOnly bool     // Only create compilation database without actually compiling
 	sourceOverrides         string   // Path to a .json file that contains a set of replacements of the sketch source code.
 )
@@ -100,8 +99,7 @@ func NewCommand() *cobra.Command {
 	command.Flags().StringVarP(&programmer, "programmer", "P", "", "Optional, use the specified programmer to upload.")
 	command.Flags().BoolVar(&compilationDatabaseOnly, "only-compilation-database", false, "Just produce the compilation database, without actually compiling.")
 	command.Flags().BoolVar(&clean, "clean", false, "Optional, cleanup the build folder and do not use any cached build.")
-	// We must use the following syntax for this flag since it's also bound to settings, we could use the other one too
-	// but it wouldn't make sense since we still must explicitly set the exportBinaries variable by reading from settings.
+	// We must use the following syntax for this flag since it's also bound to settings.
 	// This must be done because the value is set when the binding is accessed from viper. Accessing from cobra would only
 	// read the value if the flag is set explicitly by the user.
 	command.Flags().BoolP("export-binaries", "e", false, "If set built binaries will be exported to the sketch folder.")
@@ -137,11 +135,6 @@ func run(cmd *cobra.Command, args []string) {
 		}
 	}
 
-	// We must read this from settings since the value is set when the binding is accessed from viper,
-	// accessing it from cobra would only read it if the flag is explicitly set by the user and ignore
-	// the config file and the env vars.
-	exportBinaries = configuration.Settings.GetBool("sketch.always_export_binaries")
-
 	var overrides map[string]string
 	if sourceOverrides != "" {
 		data, err := paths.New(sourceOverrides).ReadFile()
@@ -176,7 +169,6 @@ func run(cmd *cobra.Command, args []string) {
 		Libraries:                     libraries,
 		OptimizeForDebug:              optimizeForDebug,
 		Clean:                         clean,
-		ExportBinaries:                exportBinaries,
 		CreateCompilationDatabaseOnly: compilationDatabaseOnly,
 		SourceOverride:                overrides,
 	}

commands/compile/compile.go

@@ -45,6 +45,19 @@ import (
 // Compile FIXMEDOC
 func Compile(ctx context.Context, req *rpc.CompileReq, outStream, errStream io.Writer, debug bool) (r *rpc.CompileResp, 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
+	// package instead of the cli/compile one, otherwise we'd lose the binding.
+	exportBinaries := configuration.Settings.GetBool("sketch.always_export_binaries")
+	// If we'd just read the binding in any case, even if the request sets the export binaries setting,
+	// the settings value would always overwrite the request one and it wouldn't have any effect
+	// setting it for individual requests. To solve this we use a wrapper.BoolValue to handle
+	// the optionality of this property, otherwise we would have no way of knowing if the property
+	// was set in the request or it's just the default boolean value.
+	if reqExportBinaries := req.GetExportBinaries(); reqExportBinaries != nil {
+		exportBinaries = reqExportBinaries.Value
+	}
+
 	tags := map[string]string{
 		"fqbn":            req.Fqbn,
 		"sketchPath":      metrics.Sanitize(req.SketchPath),
@@ -59,7 +72,7 @@ func Compile(ctx context.Context, req *rpc.CompileReq, outStream, errStream io.W
 		"jobs":            strconv.FormatInt(int64(req.Jobs), 10),
 		"libraries":       strings.Join(req.Libraries, ","),
 		"clean":           strconv.FormatBool(req.GetClean()),
-		"exportBinaries":  strconv.FormatBool(req.GetExportBinaries()),
+		"exportBinaries":  strconv.FormatBool(exportBinaries),
 	}
 
 	// Use defer func() to evaluate tags map when function returns
@@ -214,7 +227,7 @@ func Compile(ctx context.Context, req *rpc.CompileReq, outStream, errStream io.W
 	}
 
 	// If the export directory is set we assume you want to export the binaries
-	if req.GetExportBinaries() || req.GetExportDir() != "" {
+	if exportBinaries || req.GetExportDir() != "" {
 		var exportPath *paths.Path
 		if exportDir := req.GetExportDir(); exportDir != "" {
 			exportPath = paths.New(exportDir)

docs/UPGRADING.md

@@ -4,6 +4,14 @@ Here you can find a list of migration guides to handle breaking changes between
 
 ## Unreleased
 
+### Change type of `CompileReq.ExportBinaries` message in gRPC interface
+
+This change affects only the gRPC consumers.
+
+In the `CompileReq` message the `export_binaries` property type has been changed from `bool` to
+`google.protobuf.BoolValue`. This has been done to handle settings bindings by gRPC consumers and the CLI in the same
+way so that they an identical behaviour.
+
 ## 0.15.0
 
 ### Rename `telemetry` settings to `metrics`

rpc/commands/compile.pb.go

@@ -19,6 +19,7 @@ package cc.arduino.cli.commands;
 
 option go_package = "github.com/arduino/arduino-cli/rpc/commands";
 
+import "google/protobuf/wrappers.proto";
 import "commands/common.proto";
 import "commands/lib.proto";
 
@@ -40,9 +41,9 @@ message CompileReq {
   bool optimizeForDebug = 16; // Optimize compile output for debug, not for release.
   string export_dir = 18; // Optional: save the build artifacts in this directory, the directory must exist.
   bool clean = 19; // Optional: cleanup the build folder and do not use any previously cached build
-  bool export_binaries = 20; // When set to `true` the compiled binary will be copied to the export directory.
   bool create_compilation_database_only = 21; // When set to `true` only the compilation database will be produced and no actual build will be performed.
   map<string, string> source_override = 22; // This map (source file -> new content) let the builder use the provided content instead of reading the corresponding file on disk. This is useful for IDE that have unsaved changes in memory. The path must be relative to the sketch directory. Only files from the sketch are allowed.
+  google.protobuf.BoolValue export_binaries = 23;  // When set to `true` the compiled binary will be copied to the export directory.
 }
 
 message CompileResp {