[skip-changelog] Some small refactoring on legacy package (#1064)

cmaglie · matthijskooijman · web-flow · commit df9f204f33a3 · 2020-11-12T12:34:47.000+01:00
* legacy: output --preprocess result on ExecStdout

* legacy: removed redundant argument filters

* legacy: moving path-relativization code out of PrepareCommand

* legacy: removed i18n on unneded message

* legacy: replacing ParseCommandLine with the equivalent library call

* legacy: removed parameter that happens to be always false

* legacy: removed constants.MSG_PATTERN_MISSING from i18n

* Let ExecRecipe return the command executed

This prepares for building a compilation database later. The returned
command is not currently used anywhere yet, so this commit should not
change behaviour.

* Fixed typo

Co-authored-by: Matthijs Kooijman &lt;matthijs@stdin.nl&gt;
diff --git a/legacy/builder/builder.go b/legacy/builder/builder.go
@@ -16,7 +16,6 @@
 package builder
 
 import (
-	"fmt"
 	"os"
 	"reflect"
 	"strconv"
@@ -159,7 +158,7 @@ func (s *Preprocess) Run(ctx *types.Context) error {
 	}
 
 	// Output arduino-preprocessed source
-	fmt.Println(ctx.Source)
+	ctx.ExecStdout.Write([]byte(ctx.Source))
 	return nil
 }
 
diff --git a/legacy/builder/builder_utils/utils.go b/legacy/builder/builder_utils/utils.go
@@ -25,7 +25,6 @@ import (
 	"sync"
 
 	"github.com/arduino/arduino-cli/legacy/builder/constants"
-	"github.com/arduino/arduino-cli/legacy/builder/i18n"
 	"github.com/arduino/arduino-cli/legacy/builder/types"
 	"github.com/arduino/arduino-cli/legacy/builder/utils"
 	"github.com/arduino/go-paths-helper"
@@ -249,7 +248,7 @@ func compileFileWithRecipe(ctx *types.Context, sourcePath *paths.Path, source *p
 		return nil, errors.WithStack(err)
 	}
 	if !objIsUpToDate {
-		_, _, err = ExecRecipe(ctx, properties, recipe, false /* stdout */, utils.ShowIfVerbose /* stderr */, utils.Show)
+		_, _, _, err := ExecRecipe(ctx, properties, recipe, utils.ShowIfVerbose /* stdout */, utils.Show /* stderr */)
 		if err != nil {
 			return nil, errors.WithStack(err)
 		}
@@ -480,46 +479,57 @@ func ArchiveCompiledFiles(ctx *types.Context, buildPath *paths.Path, archiveFile
 		properties.SetPath(constants.BUILD_PROPERTIES_ARCHIVE_FILE_PATH, archiveFilePath)
 		properties.SetPath(constants.BUILD_PROPERTIES_OBJECT_FILE, objectFile)
 
-		if _, _, err := ExecRecipe(ctx, properties, constants.RECIPE_AR_PATTERN, false /* stdout */, utils.ShowIfVerbose /* stderr */, utils.Show); err != nil {
+		if _, _, _, err := ExecRecipe(ctx, properties, constants.RECIPE_AR_PATTERN, utils.ShowIfVerbose /* stdout */, utils.Show /* stderr */); err != nil {
 			return nil, errors.WithStack(err)
 		}
 	}
 
 	return archiveFilePath, nil
 }
 
-func ExecRecipe(ctx *types.Context, buildProperties *properties.Map, recipe string, removeUnsetProperties bool, stdout int, stderr int) ([]byte, []byte, error) {
+func ExecRecipe(ctx *types.Context, buildProperties *properties.Map, recipe string, stdout int, stderr int) (*exec.Cmd, []byte, []byte, error) {
 	// See util.ExecCommand for stdout/stderr arguments
-	command, err := PrepareCommandForRecipe(ctx, buildProperties, recipe, removeUnsetProperties)
+	command, err := PrepareCommandForRecipe(buildProperties, recipe, false)
 	if err != nil {
-		return nil, nil, errors.WithStack(err)
+		return nil, nil, nil, errors.WithStack(err)
 	}
 
-	return utils.ExecCommand(ctx, command, stdout, stderr)
+	outbytes, errbytes, err := utils.ExecCommand(ctx, command, stdout, stderr)
+	return command, outbytes, errbytes, err
 }
 
 const COMMANDLINE_LIMIT = 30000
 
-func PrepareCommandForRecipe(ctx *types.Context, buildProperties *properties.Map, recipe string, removeUnsetProperties bool) (*exec.Cmd, error) {
-	logger := ctx.GetLogger()
+func PrepareCommandForRecipe(buildProperties *properties.Map, recipe string, removeUnsetProperties bool) (*exec.Cmd, error) {
 	pattern := buildProperties.Get(recipe)
 	if pattern == "" {
-		return nil, i18n.ErrorfWithLogger(logger, constants.MSG_PATTERN_MISSING, recipe)
+		return nil, errors.Errorf("%s pattern is missing", recipe)
 	}
 
-	var err error
 	commandLine := buildProperties.ExpandPropsInString(pattern)
 	if removeUnsetProperties {
 		commandLine = properties.DeleteUnexpandedPropsFromString(commandLine)
 	}
 
-	relativePath := ""
+	command, err := utils.PrepareCommand(commandLine)
 
+	// if the overall commandline is too long for the platform
+	// try reducing the length by making the filenames relative
+	// and changing working directory to build.path
 	if len(commandLine) > COMMANDLINE_LIMIT {
-		relativePath = buildProperties.Get("build.path")
+		relativePath := buildProperties.Get("build.path")
+		for i, arg := range command.Args {
+			if _, err := os.Stat(arg); os.IsNotExist(err) {
+				continue
+			}
+			rel, err := filepath.Rel(relativePath, arg)
+			if err == nil && !strings.Contains(rel, "..") && len(rel) < len(arg) {
+				command.Args[i] = rel
+			}
+		}
+		command.Dir = relativePath
 	}
 
-	command, err := utils.PrepareCommand(commandLine, logger, relativePath)
 	if err != nil {
 		return nil, errors.WithStack(err)
 	}
diff --git a/legacy/builder/constants/constants.go b/legacy/builder/constants/constants.go
@@ -90,7 +90,6 @@ const MSG_BUILD_OPTIONS_CHANGED = "Build options changed, rebuilding all"
 const MSG_CANT_FIND_SKETCH_IN_PATH = "Unable to find {0} in {1}"
 const MSG_FQBN_INVALID = "{0} is not a valid fully qualified board name. Required format is targetPackageName:targetPlatformName:targetBoardName."
 const MSG_FIND_INCLUDES_FAILED = "Error while detecting libraries included by {0}"
-const MSG_INVALID_QUOTING = "Invalid quoting: no closing [{0}] char found."
 const MSG_LIB_LEGACY = "(legacy)"
 const MSG_LIBRARIES_MULTIPLE_LIBS_FOUND_FOR = "Multiple libraries were found for \"{0}\""
 const MSG_LIBRARIES_NOT_USED = " Not used: {0}"
@@ -101,7 +100,6 @@ const MSG_LOOKING_FOR_RECIPES = "Looking for recipes like {0}*{1}"
 const MSG_MISSING_BUILD_BOARD = "Warning: Board {0}:{1}:{2} doesn''t define a ''build.board'' preference. Auto-set to: {3}"
 const MSG_MISSING_CORE_FOR_BOARD = "Selected board depends on '{0}' core (not installed)."
 const MSG_PACKAGE_UNKNOWN = "{0}: Unknown package"
-const MSG_PATTERN_MISSING = "{0} pattern is missing"
 const MSG_PLATFORM_UNKNOWN = "Platform {0} (package {1}) is unknown"
 const MSG_PROGRESS = "Progress {0}"
 const MSG_PROP_IN_LIBRARY = "Missing '{0}' from library in {1}"
diff --git a/legacy/builder/create_cmake_rule.go b/legacy/builder/create_cmake_rule.go
@@ -240,8 +240,8 @@ func canExportCmakeProject(ctx *types.Context) bool {
 	return ctx.BuildProperties.Get("compiler.export_cmake") != ""
 }
 
-func extractCompileFlags(ctx *types.Context, receipe string, defines, dynamicLibs, linkerflags, linkDirectories *[]string, logger i18n.Logger) {
-	command, _ := builder_utils.PrepareCommandForRecipe(ctx, ctx.BuildProperties, receipe, true)
+func extractCompileFlags(ctx *types.Context, recipe string, defines, dynamicLibs, linkerflags, linkDirectories *[]string, logger i18n.Logger) {
+	command, _ := builder_utils.PrepareCommandForRecipe(ctx.BuildProperties, recipe, true)
 
 	for _, arg := range command.Args {
 		if strings.HasPrefix(arg, "-D") {
diff --git a/legacy/builder/ctags_runner.go b/legacy/builder/ctags_runner.go
@@ -18,7 +18,6 @@ package builder
 import (
 	"github.com/arduino/arduino-cli/legacy/builder/constants"
 	"github.com/arduino/arduino-cli/legacy/builder/ctags"
-	"github.com/arduino/arduino-cli/legacy/builder/i18n"
 	"github.com/arduino/arduino-cli/legacy/builder/types"
 	"github.com/arduino/arduino-cli/legacy/builder/utils"
 	"github.com/pkg/errors"
@@ -29,19 +28,18 @@ type CTagsRunner struct{}
 func (s *CTagsRunner) Run(ctx *types.Context) error {
 	buildProperties := ctx.BuildProperties
 	ctagsTargetFilePath := ctx.CTagsTargetFile
-	logger := ctx.GetLogger()
 
 	properties := buildProperties.Clone()
 	properties.Merge(buildProperties.SubTree(constants.BUILD_PROPERTIES_TOOLS_KEY).SubTree(constants.CTAGS))
 	properties.SetPath(constants.BUILD_PROPERTIES_SOURCE_FILE, ctagsTargetFilePath)
 
 	pattern := properties.Get(constants.BUILD_PROPERTIES_PATTERN)
 	if pattern == constants.EMPTY_STRING {
-		return i18n.ErrorfWithLogger(logger, constants.MSG_PATTERN_MISSING, constants.CTAGS)
+		return errors.Errorf("%s pattern is missing", constants.CTAGS)
 	}
 
 	commandLine := properties.ExpandPropsInString(pattern)
-	command, err := utils.PrepareCommand(commandLine, logger, "")
+	command, err := utils.PrepareCommand(commandLine)
 	if err != nil {
 		return errors.WithStack(err)
 	}
diff --git a/legacy/builder/gcc_preproc_runner.go b/legacy/builder/gcc_preproc_runner.go
@@ -69,7 +69,7 @@ func prepareGCCPreprocRecipeProperties(ctx *types.Context, sourceFilePath *paths
 		properties.Set(constants.RECIPE_PREPROC_MACROS, GeneratePreprocPatternFromCompile(properties.Get(constants.RECIPE_CPP_PATTERN)))
 	}
 
-	cmd, err := builder_utils.PrepareCommandForRecipe(ctx, properties, constants.RECIPE_PREPROC_MACROS, true)
+	cmd, err := builder_utils.PrepareCommandForRecipe(properties, constants.RECIPE_PREPROC_MACROS, true)
 	if err != nil {
 		return nil, errors.WithStack(err)
 	}
diff --git a/legacy/builder/phases/libraries_builder.go b/legacy/builder/phases/libraries_builder.go
@@ -58,7 +58,7 @@ func findExpectedPrecompiledLibFolder(ctx *types.Context, library *libraries.Lib
 	// Add fpu specifications if they exist
 	// To do so, resolve recipe.cpp.o.pattern,
 	// search for -mfpu=xxx -mfloat-abi=yyy and add to a subfolder
-	command, _ := builder_utils.PrepareCommandForRecipe(ctx, ctx.BuildProperties, constants.RECIPE_CPP_PATTERN, true)
+	command, _ := builder_utils.PrepareCommandForRecipe(ctx.BuildProperties, constants.RECIPE_CPP_PATTERN, true)
 	fpuSpecs := ""
 	for _, el := range strings.Split(command.String(), " ") {
 		if strings.Contains(el, FPU_CFLAG) {
diff --git a/legacy/builder/phases/linker.go b/legacy/builder/phases/linker.go
@@ -80,7 +80,7 @@ func link(ctx *types.Context, objectFiles paths.PathList, coreDotARelPath *paths
 			properties.Set("archive_file", archive.Base())
 			properties.SetPath("archive_file_path", archive)
 			properties.SetPath("object_file", object)
-			_, _, err := builder_utils.ExecRecipe(ctx, properties, constants.RECIPE_AR_PATTERN, false, utils.ShowIfVerbose /* stdout */, utils.Show /* stderr */)
+			_, _, _, err := builder_utils.ExecRecipe(ctx, properties, constants.RECIPE_AR_PATTERN, utils.ShowIfVerbose /* stdout */, utils.Show /* stderr */)
 			if err != nil {
 				return err
 			}
@@ -97,7 +97,7 @@ func link(ctx *types.Context, objectFiles paths.PathList, coreDotARelPath *paths
 	properties.Set(constants.BUILD_PROPERTIES_ARCHIVE_FILE_PATH, coreArchiveFilePath.String())
 	properties.Set("object_files", objectFileList)
 
-	_, _, err := builder_utils.ExecRecipe(ctx, properties, constants.RECIPE_C_COMBINE_PATTERN, false, utils.ShowIfVerbose /* stdout */, utils.Show /* stderr */)
+	_, _, _, err := builder_utils.ExecRecipe(ctx, properties, constants.RECIPE_C_COMBINE_PATTERN, utils.ShowIfVerbose /* stdout */, utils.Show /* stderr */)
 	return err
 }
 
diff --git a/legacy/builder/phases/sizer.go b/legacy/builder/phases/sizer.go
@@ -112,7 +112,7 @@ func checkSize(ctx *types.Context, buildProperties *properties.Map) error {
 }
 
 func execSizeRecipe(ctx *types.Context, properties *properties.Map) (textSize int, dataSize int, eepromSize int, resErr error) {
-	out, _, err := builder_utils.ExecRecipe(ctx, properties, constants.RECIPE_SIZE_PATTERN, false /* stdout */, utils.Capture /* stderr */, utils.Show)
+	_, out, _, err := builder_utils.ExecRecipe(ctx, properties, constants.RECIPE_SIZE_PATTERN, utils.Capture /* stdout */, utils.Show /* stderr */)
 	if err != nil {
 		resErr = errors.New("Error while determining sketch size: " + err.Error())
 		return
diff --git a/legacy/builder/preprocess_sketch.go b/legacy/builder/preprocess_sketch.go
@@ -24,7 +24,6 @@ import (
 
 	bldr "github.com/arduino/arduino-cli/arduino/builder"
 	"github.com/arduino/arduino-cli/legacy/builder/constants"
-	"github.com/arduino/arduino-cli/legacy/builder/i18n"
 	"github.com/arduino/arduino-cli/legacy/builder/types"
 	"github.com/arduino/arduino-cli/legacy/builder/utils"
 	properties "github.com/arduino/go-properties-orderedmap"
@@ -78,7 +77,6 @@ type ArduinoPreprocessorRunner struct{}
 func (s *ArduinoPreprocessorRunner) Run(ctx *types.Context) error {
 	buildProperties := ctx.BuildProperties
 	targetFilePath := ctx.PreprocPath.Join(constants.FILE_CTAGS_TARGET_FOR_GCC_MINUS_E)
-	logger := ctx.GetLogger()
 
 	properties := buildProperties.Clone()
 	toolProps := buildProperties.SubTree("tools").SubTree("arduino-preprocessor")
@@ -102,11 +100,11 @@ func (s *ArduinoPreprocessorRunner) Run(ctx *types.Context) error {
 
 	pattern := properties.Get(constants.BUILD_PROPERTIES_PATTERN)
 	if pattern == constants.EMPTY_STRING {
-		return i18n.ErrorfWithLogger(logger, constants.MSG_PATTERN_MISSING, "arduino-preprocessor")
+		return errors.New("arduino-preprocessor pattern is missing")
 	}
 
 	commandLine := properties.ExpandPropsInString(pattern)
-	command, err := utils.PrepareCommand(commandLine, logger, "")
+	command, err := utils.PrepareCommand(commandLine)
 	if err != nil {
 		return errors.WithStack(err)
 	}
diff --git a/legacy/builder/recipe_runner.go b/legacy/builder/recipe_runner.go
@@ -47,7 +47,7 @@ func (s *RecipeByPrefixSuffixRunner) Run(ctx *types.Context) error {
 		if ctx.DebugLevel >= 10 {
 			logger.Fprintln(os.Stdout, constants.LOG_LEVEL_DEBUG, constants.MSG_RUNNING_RECIPE, recipe)
 		}
-		_, _, err := builder_utils.ExecRecipe(ctx, properties, recipe, false /* stdout */, utils.ShowIfVerbose /* stderr */, utils.Show)
+		_, _, _, err := builder_utils.ExecRecipe(ctx, properties, recipe, utils.ShowIfVerbose /* stdout */, utils.Show /* stderr */)
 		if err != nil {
 			return errors.WithStack(err)
 		}
diff --git a/legacy/builder/test/utils_test.go b/legacy/builder/test/utils_test.go
@@ -16,45 +16,12 @@
 package test
 
 import (
-	"github.com/arduino/arduino-cli/legacy/builder/i18n"
-	"github.com/arduino/arduino-cli/legacy/builder/utils"
-	"github.com/stretchr/testify/require"
 	"strings"
 	"testing"
-)
-
-func TestCommandLineParser(t *testing.T) {
-	command := "\"/home/federico/materiale/works_Arduino/Arduino/build/hardware/tools/coan\" source -m -E -P -kb -c -g -Os -w -ffunction-sections -fdata-sections -MMD -mmcu=atmega32u4 -DF_CPU=16000000L -DARDUINO=010600 -DARDUINO_AVR_LEONARDO -DARDUINO_ARCH_AVR  -DUSB_VID=0x2341 -DUSB_PID=0x8036 '-DUSB_MANUFACTURER=' '-DUSB_PRODUCT=\"Arduino Leonardo\"' \"/tmp/sketch321469072.cpp\""
 
-	parts, err := utils.ParseCommandLine(command, i18n.HumanLogger{})
-	NoError(t, err)
-
-	require.Equal(t, 23, len(parts))
-
-	require.Equal(t, "/home/federico/materiale/works_Arduino/Arduino/build/hardware/tools/coan", parts[0])
-	require.Equal(t, "source", parts[1])
-	require.Equal(t, "-m", parts[2])
-	require.Equal(t, "-E", parts[3])
-	require.Equal(t, "-P", parts[4])
-	require.Equal(t, "-kb", parts[5])
-	require.Equal(t, "-c", parts[6])
-	require.Equal(t, "-g", parts[7])
-	require.Equal(t, "-Os", parts[8])
-	require.Equal(t, "-w", parts[9])
-	require.Equal(t, "-ffunction-sections", parts[10])
-	require.Equal(t, "-fdata-sections", parts[11])
-	require.Equal(t, "-MMD", parts[12])
-	require.Equal(t, "-mmcu=atmega32u4", parts[13])
-	require.Equal(t, "-DF_CPU=16000000L", parts[14])
-	require.Equal(t, "-DARDUINO=010600", parts[15])
-	require.Equal(t, "-DARDUINO_AVR_LEONARDO", parts[16])
-	require.Equal(t, "-DARDUINO_ARCH_AVR", parts[17])
-	require.Equal(t, "-DUSB_VID=0x2341", parts[18])
-	require.Equal(t, "-DUSB_PID=0x8036", parts[19])
-	require.Equal(t, "-DUSB_MANUFACTURER=", parts[20])
-	require.Equal(t, "-DUSB_PRODUCT=\"Arduino Leonardo\"", parts[21])
-	require.Equal(t, "/tmp/sketch321469072.cpp", parts[22])
-}
+	"github.com/arduino/arduino-cli/legacy/builder/utils"
+	"github.com/stretchr/testify/require"
+)
 
 func TestPrintableCommand(t *testing.T) {
 	parts := []string{
@@ -75,13 +42,6 @@ func TestPrintableCommand(t *testing.T) {
 	require.Equal(t, correct, result)
 }
 
-func TestCommandLineParserError(t *testing.T) {
-	command := "\"command missing quote"
-
-	_, err := utils.ParseCommandLine(command, i18n.HumanLogger{})
-	require.Error(t, err)
-}
-
 func TestMapTrimSpace(t *testing.T) {
 	value := "hello, world , how are,you? "
 	parts := utils.Map(strings.Split(value, ","), utils.TrimSpace)
diff --git a/legacy/builder/utils/utils.go b/legacy/builder/utils/utils.go

Original file line number	Diff line number	Diff line change
`@@ -16,7 +16,6 @@`
`16`	`16`	`package builder`
`17`	`17`
`18`	`18`	`import (`
`19`		`- "fmt"`
`20`	`19`	`"os"`
`21`	`20`	`"reflect"`
`22`	`21`	`"strconv"`
`@@ -159,7 +158,7 @@ func (s Preprocess) Run(ctx types.Context) error {`
`159`	`158`	`}`
`160`	`159`
`161`	`160`	`// Output arduino-preprocessed source`
`162`		`- fmt.Println(ctx.Source)`
	`161`	`+ ctx.ExecStdout.Write([]byte(ctx.Source))`
`163`	`162`	`return nil`
`164`	`163`	`}`
`165`	`164`
Original file line number	Diff line number	Diff line change
`@@ -240,8 +240,8 @@ func canExportCmakeProject(ctx *types.Context) bool {`
`240`	`240`	`return ctx.BuildProperties.Get("compiler.export_cmake") != ""`
`241`	`241`	`}`
`242`	`242`
`243`		`-func extractCompileFlags(ctx types.Context, receipe string, defines, dynamicLibs, linkerflags, linkDirectories []string, logger i18n.Logger) {`
`244`		`- command, _ := builder_utils.PrepareCommandForRecipe(ctx, ctx.BuildProperties, receipe, true)`
	`243`	`+func extractCompileFlags(ctx types.Context, recipe string, defines, dynamicLibs, linkerflags, linkDirectories []string, logger i18n.Logger) {`
	`244`	`+ command, _ := builder_utils.PrepareCommandForRecipe(ctx.BuildProperties, recipe, true)`
`245`	`245`
`246`	`246`	`for _, arg := range command.Args {`
`247`	`247`	`if strings.HasPrefix(arg, "-D") {`
Original file line number	Diff line number	Diff line change
`@@ -69,7 +69,7 @@ func prepareGCCPreprocRecipeProperties(ctx types.Context, sourceFilePath paths`
`69`	`69`	`properties.Set(constants.RECIPE_PREPROC_MACROS, GeneratePreprocPatternFromCompile(properties.Get(constants.RECIPE_CPP_PATTERN)))`
`70`	`70`	`}`
`71`	`71`
`72`		`- cmd, err := builder_utils.PrepareCommandForRecipe(ctx, properties, constants.RECIPE_PREPROC_MACROS, true)`
	`72`	`+ cmd, err := builder_utils.PrepareCommandForRecipe(properties, constants.RECIPE_PREPROC_MACROS, true)`
`73`	`73`	`if err != nil {`
`74`	`74`	`return nil, errors.WithStack(err)`
`75`	`75`	`}`
Original file line number	Diff line number	Diff line change
`@@ -112,7 +112,7 @@ func checkSize(ctx types.Context, buildProperties properties.Map) error {`
`112`	`112`	`}`
`113`	`113`
`114`	`114`	`func execSizeRecipe(ctx types.Context, properties properties.Map) (textSize int, dataSize int, eepromSize int, resErr error) {`
`115`		`- out, _, err := builder_utils.ExecRecipe(ctx, properties, constants.RECIPE_SIZE_PATTERN, false /* stdout /, utils.Capture / stderr */, utils.Show)`
	`115`	`+ _, out, _, err := builder_utils.ExecRecipe(ctx, properties, constants.RECIPE_SIZE_PATTERN, utils.Capture /* stdout /, utils.Show / stderr */)`
`116`	`116`	`if err != nil {`
`117`	`117`	`resErr = errors.New("Error while determining sketch size: " + err.Error())`
`118`	`118`	`return`