Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cli code refactoring: argument parsing/handling should not access the arduino package directly #1948

Closed
16 tasks done
cmaglie opened this issue Oct 26, 2022 · 2 comments · Fixed by #2296 or #2335
Closed
16 tasks done
Assignees
Labels
criticality: low Of low impact priority: high Resolution is a high priority topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project

Comments

@cmaglie
Copy link
Member

cmaglie commented Oct 26, 2022

Describe the problem

This is an architectural issue in the golang implementation of the CLI.
The cli package should not use directly the arduino package but it should use only the functions available through gRPC, implemented in the commands package to avoid code duplication and re-implementation of the same functionality over and over.

To reproduce

The current instances of the above issue are here:

internal/cli/arguments/completion.go access the package arduino/cores:

internal/cli/arguments/fqbn.go access the packages arduino/cores and arduino/sketch

internal/cli/arguments/port.go access arduino/discovery and arduino/sketch

  • imports:
    "github.com/arduino/arduino-cli/arduino"
    "github.com/arduino/arduino-cli/arduino/discovery"
    "github.com/arduino/arduino-cli/arduino/sketch"
  • use of sketch:
    func (p *Port) GetPortAddressAndProtocol(instance *rpc.Instance, sk *sketch.Sketch) (string, string, error) {
  • use of sketch:
    func (p *Port) GetPort(instance *rpc.Instance, sk *sketch.Sketch) (*discovery.Port, error) {
  • it completely duplicates a discovery-manager run loop to discover the ports:
    // FIXME: We must not access PackageManager directly here but use one of the commands.* functions
    pme, release := commands.GetPackageManagerExplorer(&rpc.BoardListAllRequest{Instance: instance})
    if pme == nil {
    return nil, &arduino.InvalidInstanceError{}
    }
    defer release()
    dm := pme.DiscoveryManager()
    watcher, err := dm.Watch()
    if err != nil {
    return nil, err
    }
    defer watcher.Close()
    deadline := time.After(p.timeout.Get())
    for {
    select {
    case portEvent := <-watcher.Feed():
    if portEvent.Type != "add" {
    continue
    }
    port := portEvent.Port
    if (protocol == "" || protocol == port.Protocol) && address == port.Address {
    return port, nil
    }
    case <-deadline:
    // No matching port found
    if protocol == "" {
    return &discovery.Port{
    Address: address,
    Protocol: "serial",
    }, nil
    }
    return nil, fmt.Errorf(tr("port not found: %[1]s %[2]s"), address, protocol)
    }
    }
    }

cli/arguments/reference.go uses arduino to get an error object:

cli/arguments/sketch.go uses arduino/sketch:

Expected behavior

Remove all direct access to the arduino package using the gRPC commads implementation in commands.

Arduino CLI version

nightly build

Operating system

N/A

Operating system version

Additional context

No response

Issue checklist

  • I searched for previous reports in the issue tracker
  • I verified the problem still occurs when using the nightly build
  • My report contains all necessary details
@cmaglie cmaglie added the type: imperfection Perceived defect in any part of project label Oct 26, 2022
@cmaglie cmaglie self-assigned this Oct 26, 2022
@cmaglie cmaglie added topic: code Related to content of the project itself criticality: low Of low impact priority: high Resolution is a high priority labels Oct 26, 2022
@cmaglie cmaglie added this to the Arduino CLI 1.0 milestone Oct 26, 2022
@cmaglie
Copy link
Member Author

cmaglie commented Jun 16, 2023

Partially solved by #2217

@cmaglie
Copy link
Member Author

cmaglie commented Sep 22, 2023

We forgot to remove the discovery loop in

// FIXME: We must not access PackageManager directly here but use one of the commands.* functions
pme, release := commands.GetPackageManagerExplorer(&rpc.BoardListAllRequest{Instance: instance})
if pme == nil {
return nil, &arduino.InvalidInstanceError{}
}
defer release()
dm := pme.DiscoveryManager()
watcher, err := dm.Watch()
if err != nil {
return nil, err
}
defer watcher.Close()
deadline := time.After(p.timeout.Get())
for {
select {
case portEvent := <-watcher.Feed():
if portEvent.Type != "add" {
continue
}
port := portEvent.Port
if (protocol == "" || protocol == port.Protocol) && address == port.Address {
return port, nil
}
case <-deadline:
// No matching port found
if protocol == "" {
return &discovery.Port{
Address: address,
Protocol: "serial",
}, nil
}
return nil, fmt.Errorf(tr("port not found: %[1]s %[2]s"), address, protocol)
}
}
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
criticality: low Of low impact priority: high Resolution is a high priority topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project
Projects
None yet
2 participants