[breaking] Optimize core operations, improving on the user input (#1574)

* [breaking] remove `parseArch` var since it is always true

* [breaking] make packages and platform case insensitive
using the `core.GetPlatform()` approach

* enhance comments and do not optimize if results are != 1

* add logging

* add simple test, install, uninstall etc are already covered since they use the same piece of logic (`ParseReference()`)

* Apply suggestions from code review

Co-authored-by: per1234 <[email protected]>

* add new error to handle multiple platform found, return res if the string the user is trying to operate matches perfectly one of the available platforms, optimize the code

* enhance comment describing what the function does

* add test to verify that an operation on two fake cores is not possible

* skip test failing on macOS and on win and optimize the test

Co-authored-by: per1234 <[email protected]>

Author: umbynos

arduino/errors.go

@@ -17,6 +17,7 @@ package arduino
 
 import (
 	"fmt"
+	"strings"
 
 	"github.com/arduino/arduino-cli/arduino/discovery"
 	"github.com/arduino/arduino-cli/i18n"
@@ -715,3 +716,24 @@ func (e *SignatureVerificationFailedError) Unwrap() error {
 func (e *SignatureVerificationFailedError) ToRPCStatus() *status.Status {
 	return status.New(codes.Unavailable, e.Error())
 }
+
+// MultiplePlatformsError is returned when trying to detect
+// the Platform the user is trying to interact with and
+// and multiple results are found.
+type MultiplePlatformsError struct {
+	Platforms    []string
+	UserPlatform string
+}
+
+func (e *MultiplePlatformsError) Error() string {
+	return tr("Found %d platform for reference \"%s\":\n%s",
+		len(e.Platforms),
+		e.UserPlatform,
+		strings.Join(e.Platforms, "\n"),
+	)
+}
+
+// ToRPCStatus converts the error into a *status.Status
+func (e *MultiplePlatformsError) ToRPCStatus() *status.Status {
+	return status.New(codes.InvalidArgument, e.Error())
+}

cli/arguments/reference.go

@@ -18,6 +18,12 @@ package arguments
 import (
 	"fmt"
 	"strings"
+
+	"github.com/arduino/arduino-cli/arduino"
+	"github.com/arduino/arduino-cli/cli/instance"
+	"github.com/arduino/arduino-cli/commands/core"
+	rpc "github.com/arduino/arduino-cli/rpc/cc/arduino/cli/commands/v1"
+	"github.com/sirupsen/logrus"
 )
 
 // Reference represents a reference item (core or library) passed to the CLI
@@ -37,10 +43,10 @@ func (r *Reference) String() string {
 
 // ParseReferences is a convenient wrapper that operates on a slice of strings and
 // calls ParseReference for each of them. It returns at the first invalid argument.
-func ParseReferences(args []string, parseArch bool) ([]*Reference, error) {
+func ParseReferences(args []string) ([]*Reference, error) {
 	ret := []*Reference{}
 	for _, arg := range args {
-		reference, err := ParseReference(arg, parseArch)
+		reference, err := ParseReference(arg)
 		if err != nil {
 			return nil, err
 		}
@@ -49,10 +55,13 @@ func ParseReferences(args []string, parseArch bool) ([]*Reference, error) {
 	return ret, nil
 }
 
-// ParseReference parses a string and returns a Reference object. If `parseArch` is passed,
-// the method also tries to parse the architecture bit, i.e. string must be in the form
-// "packager:arch@version", useful to represent a platform (or core) name.
-func ParseReference(arg string, parseArch bool) (*Reference, error) {
+// ParseReference parses a string and returns a Reference object.
+// It tries to infer the platform the user is asking for.
+// To achieve that, it tries to use github.com/arduino/arduino-cli/commands/core.GetPlatform
+// Note that the Reference is returned rightaway if the arg inserted by the user matches perfectly one in the response of core.GetPlatform
+// A MultiplePlatformsError is returned if the platform searched by the user matches multiple platforms
+func ParseReference(arg string) (*Reference, error) {
+	logrus.Infof("Parsing reference %s", arg)
 	ret := &Reference{}
 	if arg == "" {
 		return nil, fmt.Errorf(tr("invalid empty core argument"))
@@ -69,20 +78,49 @@ func ParseReference(arg string, parseArch bool) (*Reference, error) {
 		ret.Version = toks[1]
 	}
 
-	if parseArch {
-		toks = strings.Split(ret.PackageName, ":")
-		if len(toks) != 2 {
-			return nil, fmt.Errorf(tr("invalid item %s"), arg)
+	toks = strings.Split(ret.PackageName, ":")
+	if len(toks) != 2 {
+		return nil, fmt.Errorf(tr("invalid item %s"), arg)
+	}
+	if toks[0] == "" {
+		return nil, fmt.Errorf(tr("invalid empty core name '%s'"), arg)
+	}
+	ret.PackageName = toks[0]
+	if toks[1] == "" {
+		return nil, fmt.Errorf(tr("invalid empty core architecture '%s'"), arg)
+	}
+	ret.Architecture = toks[1]
+
+	// Now that we have the required informations in `ret` we can
+	// try to use core.GetPlatforms to optimize what the user typed
+	// (by replacing the PackageName and Architecture in ret with the content of core.GetPlatform())
+	platforms, _ := core.GetPlatforms(&rpc.PlatformListRequest{
+		Instance:      instance.CreateAndInit(),
+		UpdatableOnly: false,
+		All:           true, // this is true because we want also the installable platforms
+	})
+	foundPlatforms := []string{}
+	for _, platform := range platforms {
+		platformID := platform.GetId()
+		platformUser := ret.PackageName + ":" + ret.Architecture
+		// At first we check if the platform the user is searching for matches an available one,
+		// this way we do not need to adapt the casing and we can return it directly
+		if platformUser == platformID {
+			return ret, nil
 		}
-		if toks[0] == "" {
-			return nil, fmt.Errorf(tr("invalid empty core name '%s'"), arg)
+		if strings.EqualFold(platformUser, platformID) {
+			logrus.Infof("Found possible match for reference %s -> %s", platformUser, platformID)
+			toks = strings.Split(platformID, ":")
+			foundPlatforms = append(foundPlatforms, platformID)
 		}
+	}
+	// replace the returned Reference only if only one occurrence is found,
+	// otherwise return an error to the user because we don't know on which platform operate
+	if len(foundPlatforms) == 1 {
 		ret.PackageName = toks[0]
-		if toks[1] == "" {
-			return nil, fmt.Errorf(tr("invalid empty core architecture '%s'"), arg)
-		}
 		ret.Architecture = toks[1]
+	} else {
+		return nil, &arduino.MultiplePlatformsError{Platforms: foundPlatforms, UserPlatform: arg}
 	}
-
 	return ret, nil
 }

cli/arguments/reference_test.go

@@ -19,6 +19,7 @@ import (
 	"testing"
 
 	"github.com/arduino/arduino-cli/cli/arguments"
+	"github.com/arduino/arduino-cli/configuration"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -45,6 +46,10 @@ var badCores = []struct {
 	{"", nil},
 }
 
+func init() {
+	configuration.Settings = configuration.Init("")
+}
+
 func TestArgsStringify(t *testing.T) {
 	for _, core := range goodCores {
 		require.Equal(t, core.in, core.expected.String())
@@ -53,13 +58,13 @@ func TestArgsStringify(t *testing.T) {
 
 func TestParseReferenceCores(t *testing.T) {
 	for _, tt := range goodCores {
-		actual, err := arguments.ParseReference(tt.in, true)
+		actual, err := arguments.ParseReference(tt.in)
 		assert.Nil(t, err)
 		assert.Equal(t, tt.expected, actual)
 	}
 
 	for _, tt := range badCores {
-		actual, err := arguments.ParseReference(tt.in, true)
+		actual, err := arguments.ParseReference(tt.in)
 		require.NotNil(t, err, "Testing bad core '%s'", tt.in)
 		require.Equal(t, tt.expected, actual, "Testing bad core '%s'", tt.in)
 	}
@@ -71,7 +76,7 @@ func TestParseArgs(t *testing.T) {
 		input = append(input, tt.in)
 	}
 
-	refs, err := arguments.ParseReferences(input, true)
+	refs, err := arguments.ParseReferences(input)
 	assert.Nil(t, err)
 	assert.Equal(t, len(goodCores), len(refs))
 

cli/core/download.go

@@ -53,7 +53,7 @@ func runDownloadCommand(cmd *cobra.Command, args []string) {
 
 	logrus.Info("Executing `arduino-cli core download`")
 
-	platformsRefs, err := arguments.ParseReferences(args, true)
+	platformsRefs, err := arguments.ParseReferences(args)
 	if err != nil {
 		feedback.Errorf(tr("Invalid argument passed: %v"), err)
 		os.Exit(errorcodes.ErrBadArgument)

cli/core/install.go

@@ -61,7 +61,7 @@ func runInstallCommand(cmd *cobra.Command, args []string) {
 	inst := instance.CreateAndInit()
 	logrus.Info("Executing `arduino-cli core install`")
 
-	platformsRefs, err := arguments.ParseReferences(args, true)
+	platformsRefs, err := arguments.ParseReferences(args)
 	if err != nil {
 		feedback.Errorf(tr("Invalid argument passed: %v"), err)
 		os.Exit(errorcodes.ErrBadArgument)

cli/core/uninstall.go

@@ -50,7 +50,7 @@ func runUninstallCommand(cmd *cobra.Command, args []string) {
 	inst := instance.CreateAndInit()
 	logrus.Info("Executing `arduino-cli core uninstall`")
 
-	platformsRefs, err := arguments.ParseReferences(args, true)
+	platformsRefs, err := arguments.ParseReferences(args)
 	if err != nil {
 		feedback.Errorf(tr("Invalid argument passed: %v"), err)
 		os.Exit(errorcodes.ErrBadArgument)

cli/core/upgrade.go

@@ -76,7 +76,7 @@ func runUpgradeCommand(cmd *cobra.Command, args []string) {
 
 	// proceed upgrading, if anything is upgradable
 	exitErr := false
-	platformsRefs, err := arguments.ParseReferences(args, true)
+	platformsRefs, err := arguments.ParseReferences(args)
 	if err != nil {
 		feedback.Errorf(tr("Invalid argument passed: %v"), err)
 		os.Exit(errorcodes.ErrBadArgument)

docs/UPGRADING.md

@@ -2,6 +2,19 @@
 
 Here you can find a list of migration guides to handle breaking changes between releases of the CLI.
 
+## Unreleased
+
+### `github.com/arduino/arduino-cli/cli/arguments.ParseReferences` function change
+
+The `parseArch` parameter was removed since it was unused and was always true. This means that the architecture gets
+always parsed by the function.
+
+### `github.com/arduino/arduino-cli/cli/arguments.ParseReference` function change
+
+The `parseArch` parameter was removed since it was unused and was always true. This means that the architecture gets
+always parsed by the function. Furthermore the function now should also correctly interpret `packager:arch` spelled with
+the wrong casing.
+
 ## 0.20.0
 
 ### `board details` arguments change

test/test_core.py

@@ -240,6 +240,10 @@ def test_core_download(run_command, downloads_dir):
     result = run_command(["core", "download", "bananas:avr"])
     assert result.failed
 
+    # Wrong casing
+    result = run_command(["core", "download", "Arduino:[email protected]"])
+    assert os.path.exists(os.path.join(downloads_dir, "packages", "core-ArduinoCore-samd-1.8.12.tar.bz2"))
+
 
 def _in(jsondata, name, version=None):
     installed_cores = json.loads(jsondata)
@@ -685,6 +689,46 @@ def test_core_list_platform_without_platform_txt(run_command, data_dir):
     assert core["name"] == "some-packager-some-arch"
 
 
+@pytest.mark.skipif(
+    platform.system() in ["Darwin", "Windows"],
+    reason="macOS by default is case insensitive https://github.com/actions/virtual-environments/issues/865 "
+    + "Windows too is case insensitive"
+    + "https://stackoverflow.com/questions/7199039/file-paths-in-windows-environment-not-case-sensitive",
+)
+def test_core_download_multiple_platforms(run_command, data_dir):
+    assert run_command(["update"])
+
+    # Verifies no core is installed
+    res = run_command(["core", "list", "--format", "json"])
+    assert res.ok
+    cores = json.loads(res.stdout)
+    assert len(cores) == 0
+
+    # Simulates creation of two new cores in the sketchbook hardware folder
+    test_boards_txt = Path(__file__).parent / "testdata" / "boards.local.txt"
+    boards_txt = Path(data_dir, "packages", "PACKAGER", "hardware", "ARCH", "1.0.0", "boards.txt")
+    boards_txt.parent.mkdir(parents=True, exist_ok=True)
+    boards_txt.touch()
+    assert boards_txt.write_bytes(test_boards_txt.read_bytes())
+
+    boards_txt1 = Path(data_dir, "packages", "packager", "hardware", "arch", "1.0.0", "boards.txt")
+    boards_txt1.parent.mkdir(parents=True, exist_ok=True)
+    boards_txt1.touch()
+    assert boards_txt1.write_bytes(test_boards_txt.read_bytes())
+
+    # Verifies the two cores are detected
+    res = run_command(["core", "list", "--format", "json"])
+    assert res.ok
+    cores = json.loads(res.stdout)
+    assert len(cores) == 2
+
+    # Try to do an operation on the fake cores.
+    # The cli should not allow it since optimizing the casing results in finding two cores
+    res = run_command(["core", "upgrade", "Packager:Arch"])
+    assert res.failed
+    assert "Invalid argument passed: Found 2 platform for reference" in res.stderr
+
+
 def test_core_with_wrong_custom_board_options_is_loaded(run_command, data_dir):
     test_platform_name = "platform_with_wrong_custom_board_options"
     platform_install_dir = Path(data_dir, "hardware", "arduino-beta-dev", test_platform_name)