[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`