Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

Commit e63c798

Browse files
umbynosper1234
andauthored
[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 <accounts@perglass.com> * 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 <accounts@perglass.com>
1 parent 12adc53 commit e63c798

File tree

9 files changed

+145
-23
lines changed

9 files changed

+145
-23
lines changed

‎arduino/errors.go‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ package arduino
1717

1818
import (
1919
"fmt"
20+
"strings"
2021

2122
"github.com/arduino/arduino-cli/arduino/discovery"
2223
"github.com/arduino/arduino-cli/i18n"
@@ -715,3 +716,24 @@ func (e *SignatureVerificationFailedError) Unwrap() error {
715716
func (e *SignatureVerificationFailedError) ToRPCStatus() *status.Status {
716717
return status.New(codes.Unavailable, e.Error())
717718
}
719+
720+
// MultiplePlatformsError is returned when trying to detect
721+
// the Platform the user is trying to interact with and
722+
// and multiple results are found.
723+
type MultiplePlatformsError struct {
724+
Platforms []string
725+
UserPlatform string
726+
}
727+
728+
func (e *MultiplePlatformsError) Error() string {
729+
return tr("Found %d platform for reference \"%s\":\n%s",
730+
len(e.Platforms),
731+
e.UserPlatform,
732+
strings.Join(e.Platforms, "\n"),
733+
)
734+
}
735+
736+
// ToRPCStatus converts the error into a *status.Status
737+
func (e *MultiplePlatformsError) ToRPCStatus() *status.Status {
738+
return status.New(codes.InvalidArgument, e.Error())
739+
}

‎cli/arguments/reference.go‎

Lines changed: 54 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,12 @@ package arguments
1818
import (
1919
"fmt"
2020
"strings"
21+
22+
"github.com/arduino/arduino-cli/arduino"
23+
"github.com/arduino/arduino-cli/cli/instance"
24+
"github.com/arduino/arduino-cli/commands/core"
25+
rpc "github.com/arduino/arduino-cli/rpc/cc/arduino/cli/commands/v1"
26+
"github.com/sirupsen/logrus"
2127
)
2228

2329
// Reference represents a reference item (core or library) passed to the CLI
@@ -37,10 +43,10 @@ func (r *Reference) String() string {
3743

3844
// ParseReferences is a convenient wrapper that operates on a slice of strings and
3945
// calls ParseReference for each of them. It returns at the first invalid argument.
40-
func ParseReferences(args []string, parseArchbool) ([]*Reference, error) {
46+
func ParseReferences(args []string) ([]*Reference, error) {
4147
ret := []*Reference{}
4248
for _, arg := range args {
43-
reference, err := ParseReference(arg, parseArch)
49+
reference, err := ParseReference(arg)
4450
if err != nil {
4551
return nil, err
4652
}
@@ -49,10 +55,13 @@ func ParseReferences(args []string, parseArch bool) ([]*Reference, error) {
4955
return ret, nil
5056
}
5157

52-
// ParseReference parses a string and returns a Reference object. If `parseArch` is passed,
53-
// the method also tries to parse the architecture bit, i.e. string must be in the form
54-
// "packager:arch@version", useful to represent a platform (or core) name.
55-
func ParseReference(arg string, parseArch bool) (*Reference, error) {
58+
// ParseReference parses a string and returns a Reference object.
59+
// It tries to infer the platform the user is asking for.
60+
// To achieve that, it tries to use github.com/arduino/arduino-cli/commands/core.GetPlatform
61+
// Note that the Reference is returned rightaway if the arg inserted by the user matches perfectly one in the response of core.GetPlatform
62+
// A MultiplePlatformsError is returned if the platform searched by the user matches multiple platforms
63+
func ParseReference(arg string) (*Reference, error) {
64+
logrus.Infof("Parsing reference %s", arg)
5665
ret := &Reference{}
5766
if arg == "" {
5867
return nil, fmt.Errorf(tr("invalid empty core argument"))
@@ -69,20 +78,49 @@ func ParseReference(arg string, parseArch bool) (*Reference, error) {
6978
ret.Version = toks[1]
7079
}
7180

72-
if parseArch {
73-
toks = strings.Split(ret.PackageName, ":")
74-
if len(toks) != 2 {
75-
return nil, fmt.Errorf(tr("invalid item %s"), arg)
81+
toks = strings.Split(ret.PackageName, ":")
82+
if len(toks) != 2 {
83+
return nil, fmt.Errorf(tr("invalid item %s"), arg)
84+
}
85+
if toks[0] == "" {
86+
return nil, fmt.Errorf(tr("invalid empty core name '%s'"), arg)
87+
}
88+
ret.PackageName = toks[0]
89+
if toks[1] == "" {
90+
return nil, fmt.Errorf(tr("invalid empty core architecture '%s'"), arg)
91+
}
92+
ret.Architecture = toks[1]
93+
94+
// Now that we have the required informations in `ret` we can
95+
// try to use core.GetPlatforms to optimize what the user typed
96+
// (by replacing the PackageName and Architecture in ret with the content of core.GetPlatform())
97+
platforms, _ := core.GetPlatforms(&rpc.PlatformListRequest{
98+
Instance: instance.CreateAndInit(),
99+
UpdatableOnly: false,
100+
All: true, // this is true because we want also the installable platforms
101+
})
102+
foundPlatforms := []string{}
103+
for _, platform := range platforms {
104+
platformID := platform.GetId()
105+
platformUser := ret.PackageName + ":" + ret.Architecture
106+
// At first we check if the platform the user is searching for matches an available one,
107+
// this way we do not need to adapt the casing and we can return it directly
108+
if platformUser == platformID {
109+
return ret, nil
76110
}
77-
if toks[0] == "" {
78-
return nil, fmt.Errorf(tr("invalid empty core name '%s'"), arg)
111+
if strings.EqualFold(platformUser, platformID) {
112+
logrus.Infof("Found possible match for reference %s -> %s", platformUser, platformID)
113+
toks = strings.Split(platformID, ":")
114+
foundPlatforms = append(foundPlatforms, platformID)
79115
}
116+
}
117+
// replace the returned Reference only if only one occurrence is found,
118+
// otherwise return an error to the user because we don't know on which platform operate
119+
if len(foundPlatforms) == 1 {
80120
ret.PackageName = toks[0]
81-
if toks[1] == "" {
82-
return nil, fmt.Errorf(tr("invalid empty core architecture '%s'"), arg)
83-
}
84121
ret.Architecture = toks[1]
122+
} else {
123+
return nil, &arduino.MultiplePlatformsError{Platforms: foundPlatforms, UserPlatform: arg}
85124
}
86-
87125
return ret, nil
88126
}

‎cli/arguments/reference_test.go‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import (
1919
"testing"
2020

2121
"github.com/arduino/arduino-cli/cli/arguments"
22+
"github.com/arduino/arduino-cli/configuration"
2223
"github.com/stretchr/testify/assert"
2324
"github.com/stretchr/testify/require"
2425
)
@@ -45,6 +46,10 @@ var badCores = []struct {
4546
{"", nil},
4647
}
4748

49+
func init() {
50+
configuration.Settings = configuration.Init("")
51+
}
52+
4853
func TestArgsStringify(t *testing.T) {
4954
for _, core := range goodCores {
5055
require.Equal(t, core.in, core.expected.String())
@@ -53,13 +58,13 @@ func TestArgsStringify(t *testing.T) {
5358

5459
func TestParseReferenceCores(t *testing.T) {
5560
for _, tt := range goodCores {
56-
actual, err := arguments.ParseReference(tt.in, true)
61+
actual, err := arguments.ParseReference(tt.in)
5762
assert.Nil(t, err)
5863
assert.Equal(t, tt.expected, actual)
5964
}
6065

6166
for _, tt := range badCores {
62-
actual, err := arguments.ParseReference(tt.in, true)
67+
actual, err := arguments.ParseReference(tt.in)
6368
require.NotNil(t, err, "Testing bad core '%s'", tt.in)
6469
require.Equal(t, tt.expected, actual, "Testing bad core '%s'", tt.in)
6570
}
@@ -71,7 +76,7 @@ func TestParseArgs(t *testing.T) {
7176
input = append(input, tt.in)
7277
}
7378

74-
refs, err := arguments.ParseReferences(input, true)
79+
refs, err := arguments.ParseReferences(input)
7580
assert.Nil(t, err)
7681
assert.Equal(t, len(goodCores), len(refs))
7782

‎cli/core/download.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ func runDownloadCommand(cmd *cobra.Command, args []string) {
5353

5454
logrus.Info("Executing `arduino-cli core download`")
5555

56-
platformsRefs, err := arguments.ParseReferences(args, true)
56+
platformsRefs, err := arguments.ParseReferences(args)
5757
if err != nil {
5858
feedback.Errorf(tr("Invalid argument passed: %v"), err)
5959
os.Exit(errorcodes.ErrBadArgument)

‎cli/core/install.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ func runInstallCommand(cmd *cobra.Command, args []string) {
6161
inst := instance.CreateAndInit()
6262
logrus.Info("Executing `arduino-cli core install`")
6363

64-
platformsRefs, err := arguments.ParseReferences(args, true)
64+
platformsRefs, err := arguments.ParseReferences(args)
6565
if err != nil {
6666
feedback.Errorf(tr("Invalid argument passed: %v"), err)
6767
os.Exit(errorcodes.ErrBadArgument)

‎cli/core/uninstall.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ func runUninstallCommand(cmd *cobra.Command, args []string) {
5050
inst := instance.CreateAndInit()
5151
logrus.Info("Executing `arduino-cli core uninstall`")
5252

53-
platformsRefs, err := arguments.ParseReferences(args, true)
53+
platformsRefs, err := arguments.ParseReferences(args)
5454
if err != nil {
5555
feedback.Errorf(tr("Invalid argument passed: %v"), err)
5656
os.Exit(errorcodes.ErrBadArgument)

‎cli/core/upgrade.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ func runUpgradeCommand(cmd *cobra.Command, args []string) {
7676

7777
// proceed upgrading, if anything is upgradable
7878
exitErr := false
79-
platformsRefs, err := arguments.ParseReferences(args, true)
79+
platformsRefs, err := arguments.ParseReferences(args)
8080
if err != nil {
8181
feedback.Errorf(tr("Invalid argument passed: %v"), err)
8282
os.Exit(errorcodes.ErrBadArgument)

‎docs/UPGRADING.md‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,19 @@
22

33
Here you can find a list of migration guides to handle breaking changes between releases of the CLI.
44

5+
## Unreleased
6+
7+
### `github.com/arduino/arduino-cli/cli/arguments.ParseReferences` function change
8+
9+
The `parseArch` parameter was removed since it was unused and was always true. This means that the architecture gets
10+
always parsed by the function.
11+
12+
### `github.com/arduino/arduino-cli/cli/arguments.ParseReference` function change
13+
14+
The `parseArch` parameter was removed since it was unused and was always true. This means that the architecture gets
15+
always parsed by the function. Furthermore the function now should also correctly interpret `packager:arch` spelled with
16+
the wrong casing.
17+
518
## 0.20.0
619

720
### `board details` arguments change

‎test/test_core.py‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -240,6 +240,10 @@ def test_core_download(run_command, downloads_dir):
240240
result = run_command(["core", "download", "bananas:avr"])
241241
assert result.failed
242242

243+
# Wrong casing
244+
result = run_command(["core", "download", "Arduino:Samd@1.8.12"])
245+
assert os.path.exists(os.path.join(downloads_dir, "packages", "core-ArduinoCore-samd-1.8.12.tar.bz2"))
246+
243247

244248
def _in(jsondata, name, version=None):
245249
installed_cores = json.loads(jsondata)
@@ -685,6 +689,46 @@ def test_core_list_platform_without_platform_txt(run_command, data_dir):
685689
assert core["name"] == "some-packager-some-arch"
686690

687691

692+
@pytest.mark.skipif(
693+
platform.system() in ["Darwin", "Windows"],
694+
reason="macOS by default is case insensitive https://github.com/actions/virtual-environments/issues/865 "
695+
+ "Windows too is case insensitive"
696+
+ "https://stackoverflow.com/questions/7199039/file-paths-in-windows-environment-not-case-sensitive",
697+
)
698+
def test_core_download_multiple_platforms(run_command, data_dir):
699+
assert run_command(["update"])
700+
701+
# Verifies no core is installed
702+
res = run_command(["core", "list", "--format", "json"])
703+
assert res.ok
704+
cores = json.loads(res.stdout)
705+
assert len(cores) == 0
706+
707+
# Simulates creation of two new cores in the sketchbook hardware folder
708+
test_boards_txt = Path(__file__).parent / "testdata" / "boards.local.txt"
709+
boards_txt = Path(data_dir, "packages", "PACKAGER", "hardware", "ARCH", "1.0.0", "boards.txt")
710+
boards_txt.parent.mkdir(parents=True, exist_ok=True)
711+
boards_txt.touch()
712+
assert boards_txt.write_bytes(test_boards_txt.read_bytes())
713+
714+
boards_txt1 = Path(data_dir, "packages", "packager", "hardware", "arch", "1.0.0", "boards.txt")
715+
boards_txt1.parent.mkdir(parents=True, exist_ok=True)
716+
boards_txt1.touch()
717+
assert boards_txt1.write_bytes(test_boards_txt.read_bytes())
718+
719+
# Verifies the two cores are detected
720+
res = run_command(["core", "list", "--format", "json"])
721+
assert res.ok
722+
cores = json.loads(res.stdout)
723+
assert len(cores) == 2
724+
725+
# Try to do an operation on the fake cores.
726+
# The cli should not allow it since optimizing the casing results in finding two cores
727+
res = run_command(["core", "upgrade", "Packager:Arch"])
728+
assert res.failed
729+
assert "Invalid argument passed: Found 2 platform for reference" in res.stderr
730+
731+
688732
def test_core_with_wrong_custom_board_options_is_loaded(run_command, data_dir):
689733
test_platform_name = "platform_with_wrong_custom_board_options"
690734
platform_install_dir = Path(data_dir, "hardware", "arduino-beta-dev", test_platform_name)

0 commit comments

Comments
(0)

AltStyle によって変換されたページ (->オリジナル) /