Improved build path and project name auto-detection

This should make the upload command compatibile with all the reasonable
usages.

See #764
See #641

Author: cmaglie

Diff for: commands/upload/testdata/Blonk/Blonk.ino

@@ -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"
 )
@@ -240,43 +242,19 @@ func runProgramAction(pm *packagemanager.PackageManager,
 		uploadProperties.Set("program.verify", uploadProperties.Get("program.params.noverify"))
 	}
 
-	var importPath *paths.Path
 	if !burnBootloader {
-		if importFile != "" {
-			importFilePath := paths.New(importFile)
-			if !importFilePath.Exist() {
-				return fmt.Errorf("binary file not found in %s", importFilePath)
-			}
-			importPath = importFilePath.Parent()
-			// In general, the binary file extension (like .bin or .hex or even .zip) are already written explicitly in
-			// the core recipes inside platform.txt. This why the CLI removes it before setting the build.project_name
-			// property.
-			importFileName := strings.TrimSuffix(importFilePath.Base(), importFilePath.Ext())
-			uploadProperties.SetPath("build.path", importPath)
-			uploadProperties.Set("build.project_name", importFileName)
-		} else {
-			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)
-			}
-
-			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")
+		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", sketchName)
 	}
 
 	// If not using programmer perform some action required
@@ -463,3 +441,109 @@ 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 with a sketch
+		if sketch != nil {
+			// In this case we have both the build path and the sketch name given,
+			// so we just return them as-is
+			return paths.New(importDir), sketch.Name + ".ino", nil
+		}
+
+		// Case 3: importDir flag without a sketch
+
+		// In this case we have a build path but the sketch name is not given, we may
+		// 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 4: nothing given...
+	if sketch == nil {
+		return nil, "", fmt.Errorf("no sketch or build directory/file specified")
+	}
+
+	// Case 5: 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
+}

Diff for: commands/upload/testdata/Blonk/build/arduino.samd.mkr1000/Blonk.ino.hex

@@ -0,0 +1,106 @@
+// This file is part of arduino-cli.
+//
+// Copyright 2020 ARDUINO SA (http://www.arduino.cc/)
+//
+// This software is released under the GNU General Public License version 3,
+// which covers the main part of arduino-cli.
+// The terms of this license can be found at:
+// https://www.gnu.org/licenses/gpl-3.0.en.html
+//
+// You can be released from the requirements of the above licenses by purchasing
+// a commercial license. Buying such a license is mandatory if you want to
+// modify or otherwise use the software for commercial activities involving the
+// Arduino software without disclosing the source code of your own applications.
+// To purchase a commercial license, send an email to [email protected].
+
+package upload
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/arduino/arduino-cli/arduino/cores"
+	"github.com/arduino/arduino-cli/arduino/sketches"
+	paths "github.com/arduino/go-paths-helper"
+	"github.com/stretchr/testify/require"
+)
+
+func TestDetectSketchNameFromBuildPath(t *testing.T) {
+	sk1, err1 := detectSketchNameFromBuildPath(paths.New("testdata/build_path_1"))
+	require.NoError(t, err1)
+	require.Equal(t, "sketch.ino", sk1)
+
+	sk2, err2 := detectSketchNameFromBuildPath(paths.New("testdata/build_path_2"))
+	require.NoError(t, err2)
+	require.Equal(t, "Blink.ino", sk2)
+
+	sk3, err3 := detectSketchNameFromBuildPath(paths.New("testdata/build_path_3"))
+	require.Error(t, err3)
+	require.Equal(t, "", sk3)
+}
+
+func TestDetermineBuildPathAndSketchName(t *testing.T) {
+	type test struct {
+		importFile    string
+		importDir     string
+		sketch        *sketches.Sketch
+		fqbn          *cores.FQBN
+		resBuildPath  string
+		resSketchName string
+		hasError      bool
+	}
+
+	blonk, err := sketches.NewSketchFromPath(paths.New("testdata/Blonk"))
+	require.NoError(t, err)
+
+	fqbn, err := cores.ParseFQBN("arduino:samd:mkr1000")
+	require.NoError(t, err)
+
+	tests := []test{
+		// 00: error: no data passed in
+		{"", "", nil, nil, "<nil>", "", true},
+		// 01: use importFile to detect build.path and project_name
+		{"testdata/build_path_2/Blink.ino.hex", "", nil, nil, "testdata/build_path_2", "Blink.ino", false},
+		// 02: use importPath as build.path and project_name
+		{"", "testdata/build_path_2", nil, nil, "testdata/build_path_2", "Blink.ino", false},
+		// 03: error: used both importPath and importFile
+		{"testdata/build_path_2/Blink.ino.hex", "testdata/build_path_2", nil, nil, "<nil>", "", true},
+		// 04: error: only sketch without FQBN
+		{"", "", blonk, nil, "<nil>", "", true},
+		// 05: use importFile to detect build.path and project_name, sketch is ignored.
+		{"testdata/build_path_2/Blink.ino.hex", "", blonk, nil, "testdata/build_path_2", "Blink.ino", false},
+		// 06: use importPath as build.path and Blonk as project name (forced by the sketch)
+		{"", "testdata/build_path_2", blonk, nil, "testdata/build_path_2", "Blonk.ino", false},
+		// 07: error: used both importPath and importFile
+		{"testdata/build_path_2/Blink.ino.hex", "testdata/build_path_2", blonk, nil, "<nil>", "", true},
+
+		// 08: error: no data passed in
+		{"", "", nil, fqbn, "<nil>", "", true},
+		// 09: use importFile to detect build.path and project_name, fqbn ignored
+		{"testdata/build_path_2/Blink.ino.hex", "", nil, fqbn, "testdata/build_path_2", "Blink.ino", false},
+		// 10: use importPath as build.path and project_name, fqbn ignored
+		{"", "testdata/build_path_2", nil, fqbn, "testdata/build_path_2", "Blink.ino", false},
+		// 11: error: used both importPath and importFile
+		{"testdata/build_path_2/Blink.ino.hex", "testdata/build_path_2", nil, fqbn, "<nil>", "", true},
+		// 12: use sketch to determine project name and sketch+fqbn to determine build path
+		{"", "", blonk, fqbn, "testdata/Blonk/build/arduino.samd.mkr1000", "Blonk.ino", false},
+		// 13: use importFile to detect build.path and project_name, sketch+fqbn is ignored.
+		{"testdata/build_path_2/Blink.ino.hex", "", blonk, fqbn, "testdata/build_path_2", "Blink.ino", false},
+		// 14: use importPath as build.path and Blonk as project name (forced by the sketch), fqbn ignored
+		{"", "testdata/build_path_2", blonk, fqbn, "testdata/build_path_2", "Blonk.ino", false},
+		// 15: error: used both importPath and importFile
+		{"testdata/build_path_2/Blink.ino.hex", "testdata/build_path_2", blonk, fqbn, "<nil>", "", true},
+	}
+	for i, test := range tests {
+		t.Run(fmt.Sprintf("SubTest%02d", i), func(t *testing.T) {
+			buildPath, sketchName, err := determineBuildPathAndSketchName(test.importFile, test.importDir, test.sketch, test.fqbn)
+			if test.hasError {
+				require.Error(t, err)
+			} else {
+				require.NoError(t, err)
+			}
+			require.Equal(t, test.resBuildPath, fmt.Sprint(buildPath))
+			require.Equal(t, test.resSketchName, sketchName)
+		})
+	}
+}