Reintroduce the `--input-file` flag for the `upload` command (#777)

* Remove deprecation from importFile param * Implement --input-file flag * Add comment in --input-file splitting step * Add e2e test for --input-x flags * Refine upload flags testing * Add --input-file file existence check * Use TrimSuffix instead of replace * Restore -i shorthand flag for --input-file and add CLI checkFlagsConflicts function * Improved build path and project name auto-detection This should make the upload command compatibile with all the reasonable usages. See #764 See #641 * Made UploadTest more resilient * upload: sketch is ignored if input-dir or input-file is specified There is no point in overriding the sketch name if the user explicitly give it via command line. * Update go-paths-helper to version 1.3.2 fixes EquivalentTo when used with abs paths * fix TestGetCommandLine * 100% coverage on detectSketchNameFromBuildPath function * Do not git-ignore all *.bin but just inside the client_example folder * slightly simplified function signature (cosmetic) Co-authored-by: Cristian Maglie <[email protected]>
arduino · Sep 7, 2020 · f1877efe1c9b9c6a16833fe9a49bc55dc87efb65 · f1877ef
1 parent 5045656
commit f1877efe1c9b9c6a16833fe9a49bc55dc87efb65
diff --git a/.gitignore b/.gitignore
@@ -15,8 +15,8 @@ venv
 
 # gRPC client example folder
 /client_example/client_example
-*.bin
-*.elf
+/client_example/**/*.bin
+/client_example/**/*.elf
 
 # Misc.
 .DS_Store

diff --git a/arduino/sketches/sketches.go b/arduino/sketches/sketches.go
@@ -20,6 +20,7 @@ import (
 	"fmt"
 
 	"github.com/arduino/go-paths-helper"
+	"github.com/pkg/errors"
 )
 
 // Sketch is a sketch for Arduino
@@ -43,9 +44,17 @@ type BoardMetadata struct {
 
 // NewSketchFromPath loads a sketch from the specified path
 func NewSketchFromPath(path *paths.Path) (*Sketch, error) {
+	path, err := path.Abs()
+	if err != nil {
+		return nil, errors.Errorf("getting sketch path: %s", err)
+	}
 	if !path.IsDir() {
 		path = path.Parent()
 	}
+	sketchFile := path.Join(path.Base() + ".ino")
+	if !sketchFile.Exist() {
+		return nil, errors.Errorf("no valid sketch found in %s: missing %s", path, sketchFile.Base())
+	}
 	sketch := &Sketch{
 		FullPath: path,
 		Name:     path.Base(),

diff --git a/cli/upload/upload.go b/cli/upload/upload.go
@@ -36,6 +36,7 @@ var (
 	verbose    bool
 	verify     bool
 	importDir  string
+	importFile string
 	programmer string
 )
 
@@ -47,19 +48,28 @@ func NewCommand() *cobra.Command {
 		Long:    "Upload Arduino sketches. This does NOT compile the sketch prior to upload.",
 		Example: "  " + os.Args[0] + " upload /home/user/Arduino/MySketch",
 		Args:    cobra.MaximumNArgs(1),
+		PreRun:  checkFlagsConflicts,
 		Run:     run,
 	}
 
 	uploadCommand.Flags().StringVarP(&fqbn, "fqbn", "b", "", "Fully Qualified Board Name, e.g.: arduino:avr:uno")
 	uploadCommand.Flags().StringVarP(&port, "port", "p", "", "Upload port, e.g.: COM10 or /dev/ttyACM0")
 	uploadCommand.Flags().StringVarP(&importDir, "input-dir", "", "", "Directory containing binaries to upload.")
+	uploadCommand.Flags().StringVarP(&importFile, "input-file", "i", "", "Binary file to upload.")
 	uploadCommand.Flags().BoolVarP(&verify, "verify", "t", false, "Verify uploaded binary after the upload.")
 	uploadCommand.Flags().BoolVarP(&verbose, "verbose", "v", false, "Optional, turns on verbose mode.")
 	uploadCommand.Flags().StringVarP(&programmer, "programmer", "P", "", "Optional, use the specified programmer to upload or 'list' to list supported programmers.")
 
 	return uploadCommand
 }
 
+func checkFlagsConflicts(command *cobra.Command, args []string) {
+	if importFile != "" && importDir != "" {
+		feedback.Errorf("error: --input-file and --input-dir flags cannot be used together")
+		os.Exit(errorcodes.ErrBadArgument)
+	}
+}
+
 func run(command *cobra.Command, args []string) {
 	instance, err := instance.CreateInstance()
 	if err != nil {
@@ -80,6 +90,7 @@ func run(command *cobra.Command, args []string) {
 		Port:       port,
 		Verbose:    verbose,
 		Verify:     verify,
+		ImportFile: importFile,
 		ImportDir:  importDir,
 		Programmer: programmer,
 	}, os.Stdout, os.Stderr); err != nil {

diff --git a/commands/debug/debug_test.go b/commands/debug/debug_test.go
@@ -26,14 +26,16 @@ import (
 	dbg "github.com/arduino/arduino-cli/rpc/debug"
 	"github.com/arduino/go-paths-helper"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
-var customHardware = paths.New("testdata", "custom_hardware")
-var dataDir = paths.New("testdata", "data_dir", "packages")
-var sketch = "hello"
-var sketchPath = paths.New("testdata", sketch)
-
 func TestGetCommandLine(t *testing.T) {
+	customHardware := paths.New("testdata", "custom_hardware")
+	dataDir := paths.New("testdata", "data_dir", "packages")
+	sketch := "hello"
+	sketchPath := paths.New("testdata", sketch)
+	require.NoError(t, sketchPath.ToAbs())
+
 	pm := packagemanager.NewPackageManager(nil, nil, nil, nil)
 	pm.LoadHardwareFromDirectory(customHardware)
 	pm.LoadHardwareFromDirectory(dataDir)
@@ -59,9 +61,9 @@ func TestGetCommandLine(t *testing.T) {
 		fmt.Sprintf(" -c \"gdb_port pipe\" -c \"telnet_port 0\" -c init -c halt %s/build/arduino-test.samd.arduino_zero_edbg/hello.ino.elf", sketchPath)
 
 	command, err := getCommandLine(req, pm)
-	assert.Nil(t, err)
+	require.Nil(t, err)
 	commandToTest := strings.Join(command[:], " ")
-	assert.Equal(t, filepath.FromSlash(goldCommand), filepath.FromSlash(commandToTest))
+	require.Equal(t, filepath.FromSlash(goldCommand), filepath.FromSlash(commandToTest))
 
 	// Other samd boards such as mkr1000 can be debugged using an external tool such as Atmel ICE connected to
 	// the board debug port
@@ -83,5 +85,4 @@ func TestGetCommandLine(t *testing.T) {
 	assert.Nil(t, err)
 	commandToTest2 := strings.Join(command2[:], " ")
 	assert.Equal(t, filepath.FromSlash(goldCommand2), filepath.FromSlash(commandToTest2))
-
 }
diff --git a/commands/debug/testdata/hello/hello.ino b/commands/debug/testdata/hello/hello.ino
diff --git a/commands/upload/burnbootloader.go b/commands/upload/burnbootloader.go
@@ -37,6 +37,7 @@ func BurnBootloader(ctx context.Context, req *rpc.BurnBootloaderReq, outStream i
 	err := runProgramAction(
 		pm,
 		nil, // sketch
+		"",  // importFile
 		"",  // importDir
 		req.GetFqbn(),
 		req.GetPort(),

diff --git a/commands/upload/testdata/Blonk/Blonk.ino b/commands/upload/testdata/Blonk/Blonk.ino
diff --git a/commands/upload/testdata/Blonk/build/arduino.samd.mkr1000/Blonk.ino.bin b/commands/upload/testdata/Blonk/build/arduino.samd.mkr1000/Blonk.ino.bin
diff --git a/commands/upload/testdata/Blonk/build/arduino.samd.mkr1000/Blonk.ino.elf b/commands/upload/testdata/Blonk/build/arduino.samd.mkr1000/Blonk.ino.elf
diff --git a/commands/upload/testdata/Blonk/build/arduino.samd.mkr1000/Blonk.ino.hex b/commands/upload/testdata/Blonk/build/arduino.samd.mkr1000/Blonk.ino.hex
diff --git a/commands/upload/testdata/Blonk/build/arduino.samd.mkr1000/Blonk.ino.map b/commands/upload/testdata/Blonk/build/arduino.samd.mkr1000/Blonk.ino.map
diff --git a/commands/upload/testdata/Blonk/build/arduino.samd.mkr1000/Blonk.ino.with_bootloader.bin b/commands/upload/testdata/Blonk/build/arduino.samd.mkr1000/Blonk.ino.with_bootloader.bin
diff --git a/commands/upload/testdata/Blonk/build/arduino.samd.mkr1000/Blonk.ino.with_bootloader.hex b/commands/upload/testdata/Blonk/build/arduino.samd.mkr1000/Blonk.ino.with_bootloader.hex
diff --git a/commands/upload/testdata/build_path_1/sketch.ino.bin b/commands/upload/testdata/build_path_1/sketch.ino.bin
diff --git a/commands/upload/testdata/build_path_2/Blink.ino.bin b/commands/upload/testdata/build_path_2/Blink.ino.bin
diff --git a/commands/upload/testdata/build_path_2/Blink.ino.elf b/commands/upload/testdata/build_path_2/Blink.ino.elf
diff --git a/commands/upload/testdata/build_path_2/Blink.ino.hex b/commands/upload/testdata/build_path_2/Blink.ino.hex
diff --git a/commands/upload/testdata/build_path_2/Blink.ino.map b/commands/upload/testdata/build_path_2/Blink.ino.map
diff --git a/commands/upload/testdata/build_path_2/Blink.ino.with_bootloader.bin b/commands/upload/testdata/build_path_2/Blink.ino.with_bootloader.bin
diff --git a/commands/upload/testdata/build_path_2/Blink.ino.with_bootloader.hex b/commands/upload/testdata/build_path_2/Blink.ino.with_bootloader.hex
diff --git a/commands/upload/testdata/build_path_3/AnotherSketch.ino.bin b/commands/upload/testdata/build_path_3/AnotherSketch.ino.bin
diff --git a/commands/upload/testdata/build_path_3/Blink.ino.bin b/commands/upload/testdata/build_path_3/Blink.ino.bin
diff --git a/commands/upload/testdata/build_path_3/Blink.ino.elf b/commands/upload/testdata/build_path_3/Blink.ino.elf
diff --git a/commands/upload/testdata/build_path_3/Blink.ino.hex b/commands/upload/testdata/build_path_3/Blink.ino.hex
diff --git a/commands/upload/testdata/build_path_3/Blink.ino.map b/commands/upload/testdata/build_path_3/Blink.ino.map
diff --git a/commands/upload/testdata/build_path_3/Blink.ino.with_bootloader.bin b/commands/upload/testdata/build_path_3/Blink.ino.with_bootloader.bin
diff --git a/commands/upload/testdata/build_path_3/Blink.ino.with_bootloader.hex b/commands/upload/testdata/build_path_3/Blink.ino.with_bootloader.hex
diff --git a/commands/upload/testdata/build_path_4/some_other_files.txt b/commands/upload/testdata/build_path_4/some_other_files.txt
diff --git a/commands/upload/upload.go b/commands/upload/upload.go
@@ -20,6 +20,7 @@ import (
 	"fmt"
 	"io"
 	"net/url"
+	"path/filepath"
 	"strings"
 	"time"
 
@@ -32,6 +33,7 @@ import (
 	rpc "github.com/arduino/arduino-cli/rpc/commands"
 	paths "github.com/arduino/go-paths-helper"
 	properties "github.com/arduino/go-properties-orderedmap"
+	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 	"go.bug.st/serial"
 )
@@ -42,12 +44,9 @@ func Upload(ctx context.Context, req *rpc.UploadReq, outStream io.Writer, errStr
 
 	// TODO: make a generic function to extract sketch from request
 	// and remove duplication in commands/compile.go
-	if req.GetSketchPath() == "" {
-		return nil, fmt.Errorf("missing sketchPath")
-	}
 	sketchPath := paths.New(req.GetSketchPath())
 	sketch, err := sketches.NewSketchFromPath(sketchPath)
-	if err != nil {
+	if err != nil && req.GetImportDir() == "" && req.GetImportFile() == "" {
 		return nil, fmt.Errorf("opening sketch: %s", err)
 	}
 
@@ -56,6 +55,7 @@ func Upload(ctx context.Context, req *rpc.UploadReq, outStream io.Writer, errStr
 	err = runProgramAction(
 		pm,
 		sketch,
+		req.GetImportFile(),
 		req.GetImportDir(),
 		req.GetFqbn(),
 		req.GetPort(),
@@ -73,10 +73,11 @@ func Upload(ctx context.Context, req *rpc.UploadReq, outStream io.Writer, errStr
 }
 
 func runProgramAction(pm *packagemanager.PackageManager,
-	sketch *sketches.Sketch, importDir string, fqbnIn string, port string,
+	sketch *sketches.Sketch,
+	importFile, importDir, fqbnIn, port string,
 	programmerID string,
 	verbose, verify, burnBootloader bool,
-	outStream io.Writer, errStream io.Writer) error {
+	outStream, errStream io.Writer) error {
 
 	if burnBootloader && programmerID == "" {
 		return fmt.Errorf("no programmer specified for burning bootloader")
@@ -239,30 +240,19 @@ func runProgramAction(pm *packagemanager.PackageManager,
 		uploadProperties.Set("program.verify", uploadProperties.Get("program.params.noverify"))
 	}
 
-	var importPath *paths.Path
 	if !burnBootloader {
-		if sketch == nil {
-			return fmt.Errorf(("no sketch specified"))
-		}
-
-		if importDir != "" {
-			importPath = paths.New(importDir)
-		} else {
-			// TODO: Create a function to obtain importPath from sketch
-			importPath = sketch.FullPath
-			// Add FQBN (without configs part) to export path
-			fqbnSuffix := strings.Replace(fqbn.StringWithoutConfig(), ":", ".", -1)
-			importPath = importPath.Join("build").Join(fqbnSuffix)
+		importPath, sketchName, err := determineBuildPathAndSketchName(importFile, importDir, sketch, fqbn)
+		if err != nil {
+			return errors.Errorf("retrieving build artifacts: %s", err)
 		}
-
 		if !importPath.Exist() {
 			return fmt.Errorf("compiled sketch not found in %s", importPath)
 		}
 		if !importPath.IsDir() {
 			return fmt.Errorf("expected compiled sketch in directory %s, but is a file instead", importPath)
 		}
 		uploadProperties.SetPath("build.path", importPath)
-		uploadProperties.Set("build.project_name", sketch.Name+".ino")
+		uploadProperties.Set("build.project_name", sketchName)
 	}
 
 	// If not using programmer perform some action required
@@ -449,3 +439,102 @@ func waitForNewSerialPort() (string, error) {
 
 	return "", nil
 }
+
+func determineBuildPathAndSketchName(importFile, importDir string, sketch *sketches.Sketch, fqbn *cores.FQBN) (*paths.Path, string, error) {
+	// In general, compiling a sketch will produce a set of files that are
+	// named as the sketch but have different extensions, for example Sketch.ino
+	// may produce: Sketch.ino.bin; Sketch.ino.hex; Sketch.ino.zip; etc...
+	// These files are created together in the build directory and anyone of
+	// them may be required for upload.
+
+	// The upload recipes are already written using the 'build.project_name' property
+	// concatenated with an explicit extension. To perform the upload we should now
+	// determine the project name (e.g. 'sketch.ino) and set the 'build.project_name'
+	// property accordingly, together with the 'build.path' property to point to the
+	// directory containing the build artifacts.
+
+	// Case 1: importFile flag has been specified
+	if importFile != "" {
+		if importDir != "" {
+			return nil, "", fmt.Errorf("importFile and importDir cannot be used together")
+		}
+
+		// We have a path like "path/to/my/build/SketchName.ino.bin". We are going to
+		// ignore the extension and set:
+		// - "build.path" as "path/to/my/build"
+		// - "build.project_name" as "SketchName.ino"
+
+		importFilePath := paths.New(importFile)
+		if !importFilePath.Exist() {
+			return nil, "", fmt.Errorf("binary file not found in %s", importFilePath)
+		}
+		return importFilePath.Parent(), strings.TrimSuffix(importFilePath.Base(), importFilePath.Ext()), nil
+	}
+
+	if importDir != "" {
+		// Case 2: importDir flag has been specified
+
+		// In this case we have a build path but ignore the sketch name, we'll
+		// try to determine the sketch name by applying some euristics to the build folder.
+		// - "build.path" as importDir
+		// - "build.project_name" after trying to autodetect it from the build folder.
+		buildPath := paths.New(importDir)
+		sketchName, err := detectSketchNameFromBuildPath(buildPath)
+		if err != nil {
+			return nil, "", errors.Errorf("autodetect build artifact: %s", err)
+		}
+		return buildPath, sketchName, nil
+	}
+
+	// Case 3: nothing given...
+	if sketch == nil {
+		return nil, "", fmt.Errorf("no sketch or build directory/file specified")
+	}
+
+	// Case 4: only sketch specified. In this case we use the default sketch build path
+	// and the given sketch name.
+
+	// TODO: Create a function to obtain importPath from sketch
+	// Add FQBN (without configs part) to export path
+	if fqbn == nil {
+		return nil, "", fmt.Errorf("missing FQBN")
+	}
+	fqbnSuffix := strings.Replace(fqbn.StringWithoutConfig(), ":", ".", -1)
+	return sketch.FullPath.Join("build").Join(fqbnSuffix), sketch.Name + ".ino", nil
+}
+
+func detectSketchNameFromBuildPath(buildPath *paths.Path) (string, error) {
+	files, err := buildPath.ReadDir()
+	if err != nil {
+		return "", err
+	}
+
+	candidateName := ""
+	var candidateFile *paths.Path
+	for _, file := range files {
+		// Build artifacts are usually names as "Blink.ino.hex" or "Blink.ino.bin".
+		// Extract the "Blink.ino" part
+		name := strings.TrimSuffix(file.Base(), file.Ext())
+
+		// Sometimes we may have particular files like:
+		// Blink.ino.with_bootloader.bin
+		if filepath.Ext(name) != ".ino" {
+			// just ignore those files
+			continue
+		}
+
+		if candidateName == "" {
+			candidateName = name
+			candidateFile = file
+		}
+
+		if candidateName != name {
+			return "", errors.Errorf("multiple build artifacts found: '%s' and '%s'", candidateFile, file)
+		}
+	}
+
+	if candidateName == "" {
+		return "", errors.New("could not find a valid build artifact")
+	}
+	return candidateName, nil
+}