Skip to content

Commit

Permalink
[breaking] Add env variable to let tools know the cli version and the…
Browse files Browse the repository at this point in the history
… gRPC client version (#1640)

* Fix lint warning

* Add support for global env variable set over arduino-cli

* PackageManager now has a user-agent

* Propagate 'user-agent' info to tools via environment vars

* Allow nil PackageManager in GetEnvVarsForSpawnedProcess()

* Added docs

* Apply suggestions from code review

Co-authored-by: per1234 <accounts@perglass.com>

* Added docs for breaking change in golang API

* Fixed behaviour of Process.SetEnvironment

* Simplified some appends

Co-authored-by: per1234 <accounts@perglass.com>
  • Loading branch information
cmaglie and per1234 committed Jan 31, 2022
1 parent 085a31b commit 4256524
Show file tree
Hide file tree
Showing 29 changed files with 136 additions and 51 deletions.
2 changes: 1 addition & 1 deletion arduino/cores/packagemanager/install_uninstall.go
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ func (pm *PackageManager) RunPostInstallScript(platformRelease *cores.PlatformRe
}
postInstall := platformRelease.InstallDir.Join(postInstallFilename)
if postInstall.Exist() && postInstall.IsNotDir() {
cmd, err := executils.NewProcessFromPath(postInstall)
cmd, err := executils.NewProcessFromPath(pm.GetEnvVarsForSpawnedProcess(), postInstall)
if err != nil {
return err
}
Expand Down
2 changes: 1 addition & 1 deletion arduino/cores/packagemanager/loader_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ func TestLoadDiscoveries(t *testing.T) {
fakePath := paths.New("fake-path")

createTestPackageManager := func() *PackageManager {
packageManager := NewPackageManager(fakePath, fakePath, fakePath, fakePath)
packageManager := NewPackageManager(fakePath, fakePath, fakePath, fakePath, "test")
pack := packageManager.Packages.GetOrCreatePackage("arduino")
// ble-discovery tool
tool := pack.GetOrCreateTool("ble-discovery")
Expand Down
15 changes: 14 additions & 1 deletion arduino/cores/packagemanager/package_manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,12 +45,13 @@ type PackageManager struct {
TempDir *paths.Path
CustomGlobalProperties *properties.Map
discoveryManager *discoverymanager.DiscoveryManager
userAgent string
}

var tr = i18n.Tr

// NewPackageManager returns a new instance of the PackageManager
func NewPackageManager(indexDir, packagesDir, downloadDir, tempDir *paths.Path) *PackageManager {
func NewPackageManager(indexDir, packagesDir, downloadDir, tempDir *paths.Path, userAgent string) *PackageManager {
return &PackageManager{
Log: logrus.StandardLogger(),
Packages: cores.NewPackages(),
Expand All @@ -60,6 +61,18 @@ func NewPackageManager(indexDir, packagesDir, downloadDir, tempDir *paths.Path)
TempDir: tempDir,
CustomGlobalProperties: properties.NewMap(),
discoveryManager: discoverymanager.New(),
userAgent: userAgent,
}
}

// GetEnvVarsForSpawnedProcess produces a set of environment variables that
// must be sent to all processes spawned from the arduino-cli.
func (pm *PackageManager) GetEnvVarsForSpawnedProcess() []string {
if pm == nil {
return nil
}
return []string{
"ARDUINO_USER_AGENT=" + pm.userAgent,
}
}

Expand Down
19 changes: 10 additions & 9 deletions arduino/cores/packagemanager/package_manager_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ var dataDir1 = paths.New("testdata", "data_dir_1")
var extraHardware = paths.New("testdata", "extra_hardware")

func TestFindBoardWithFQBN(t *testing.T) {
pm := packagemanager.NewPackageManager(customHardware, customHardware, customHardware, customHardware)
pm := packagemanager.NewPackageManager(customHardware, customHardware, customHardware, customHardware, "test")
pm.LoadHardwareFromDirectory(customHardware)

board, err := pm.FindBoardWithFQBN("arduino:avr:uno")
Expand All @@ -53,7 +53,7 @@ func TestFindBoardWithFQBN(t *testing.T) {

func TestResolveFQBN(t *testing.T) {
// Pass nil, since these paths are only used for installing
pm := packagemanager.NewPackageManager(nil, nil, nil, nil)
pm := packagemanager.NewPackageManager(nil, nil, nil, nil, "test")
// Hardware from main packages directory
pm.LoadHardwareFromDirectory(dataDir1.Join("packages"))
// This contains the arduino:avr core
Expand Down Expand Up @@ -174,7 +174,7 @@ func TestResolveFQBN(t *testing.T) {
}

func TestBoardOptionsFunctions(t *testing.T) {
pm := packagemanager.NewPackageManager(customHardware, customHardware, customHardware, customHardware)
pm := packagemanager.NewPackageManager(customHardware, customHardware, customHardware, customHardware, "test")
pm.LoadHardwareFromDirectory(customHardware)

nano, err := pm.FindBoardWithFQBN("arduino:avr:nano")
Expand Down Expand Up @@ -218,6 +218,7 @@ func TestFindToolsRequiredForBoard(t *testing.T) {
configuration.PackagesDir(configuration.Settings),
paths.New(configuration.Settings.GetString("directories.Downloads")),
dataDir1,
"test",
)

loadIndex := func(addr string) {
Expand Down Expand Up @@ -301,7 +302,7 @@ func TestFindToolsRequiredForBoard(t *testing.T) {
}

func TestIdentifyBoard(t *testing.T) {
pm := packagemanager.NewPackageManager(customHardware, customHardware, customHardware, customHardware)
pm := packagemanager.NewPackageManager(customHardware, customHardware, customHardware, customHardware, "test")
pm.LoadHardwareFromDirectory(customHardware)

identify := func(vid, pid string) []*cores.Board {
Expand All @@ -325,11 +326,11 @@ func TestIdentifyBoard(t *testing.T) {

func TestPackageManagerClear(t *testing.T) {
// Create a PackageManager and load the harware
packageManager := packagemanager.NewPackageManager(customHardware, customHardware, customHardware, customHardware)
packageManager := packagemanager.NewPackageManager(customHardware, customHardware, customHardware, customHardware, "test")
packageManager.LoadHardwareFromDirectory(customHardware)

// Creates another PackageManager but don't load the hardware
emptyPackageManager := packagemanager.NewPackageManager(customHardware, customHardware, customHardware, customHardware)
emptyPackageManager := packagemanager.NewPackageManager(customHardware, customHardware, customHardware, customHardware, "test")

// Verifies they're not equal
require.NotEqual(t, packageManager, emptyPackageManager)
Expand All @@ -344,7 +345,7 @@ func TestFindToolsRequiredFromPlatformRelease(t *testing.T) {
// Create all the necessary data to load discoveries
fakePath := paths.New("fake-path")

pm := packagemanager.NewPackageManager(fakePath, fakePath, fakePath, fakePath)
pm := packagemanager.NewPackageManager(fakePath, fakePath, fakePath, fakePath, "test")
pack := pm.Packages.GetOrCreatePackage("arduino")

{
Expand Down Expand Up @@ -444,7 +445,7 @@ func TestFindToolsRequiredFromPlatformRelease(t *testing.T) {
}

func TestFindPlatformReleaseDependencies(t *testing.T) {
pm := packagemanager.NewPackageManager(nil, nil, nil, nil)
pm := packagemanager.NewPackageManager(nil, nil, nil, nil, "test")
pm.LoadPackageIndexFromFile(paths.New("testdata", "package_tooltest_index.json"))
pl, tools, err := pm.FindPlatformReleaseDependencies(&packagemanager.PlatformReference{Package: "test", PlatformArchitecture: "avr"})
require.NoError(t, err)
Expand All @@ -455,7 +456,7 @@ func TestFindPlatformReleaseDependencies(t *testing.T) {

func TestLegacyPackageConversionToPluggableDiscovery(t *testing.T) {
// Pass nil, since these paths are only used for installing
pm := packagemanager.NewPackageManager(nil, nil, nil, nil)
pm := packagemanager.NewPackageManager(nil, nil, nil, nil, "test")
// Hardware from main packages directory
pm.LoadHardwareFromDirectory(dataDir1.Join("packages"))
{
Expand Down
2 changes: 1 addition & 1 deletion arduino/discovery/discovery.go
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,7 @@ func (disc *PluggableDiscovery) sendCommand(command string) error {

func (disc *PluggableDiscovery) runProcess() error {
logrus.Infof("starting discovery %s process", disc.id)
proc, err := executils.NewProcess(disc.processArgs...)
proc, err := executils.NewProcess(nil, disc.processArgs...)
if err != nil {
return err
}
Expand Down
2 changes: 1 addition & 1 deletion arduino/discovery/discovery_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import (

func TestDiscoveryStdioHandling(t *testing.T) {
// Build `cat` helper inside testdata/cat
builder, err := executils.NewProcess("go", "build")
builder, err := executils.NewProcess(nil, "go", "build")
require.NoError(t, err)
builder.SetDir("testdata/cat")
require.NoError(t, builder.Run())
Expand Down
2 changes: 1 addition & 1 deletion arduino/monitor/monitor.go
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ func (mon *PluggableMonitor) sendCommand(command string) error {

func (mon *PluggableMonitor) runProcess() error {
mon.log.Infof("Starting monitor process")
proc, err := executils.NewProcess(mon.processArgs...)
proc, err := executils.NewProcess(nil, mon.processArgs...)
if err != nil {
return err
}
Expand Down
2 changes: 1 addition & 1 deletion arduino/monitor/monitor_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ func TestDummyMonitor(t *testing.T) {
// Build `dummy-monitor` helper inside testdata/dummy-monitor
testDataDir, err := paths.New("testdata").Abs()
require.NoError(t, err)
builder, err := executils.NewProcess("go", "install", "github.com/arduino/pluggable-monitor-protocol-handler/dummy-monitor@main")
builder, err := executils.NewProcess(nil, "go", "install", "github.com/arduino/pluggable-monitor-protocol-handler/dummy-monitor@main")
fmt.Println(testDataDir.String())
env := os.Environ()
env = append(env, "GOBIN="+testDataDir.String())
Expand Down
2 changes: 1 addition & 1 deletion commands/board/list_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ func TestBoardIdentifySorting(t *testing.T) {
defer paths.TempDir().Join("test").RemoveAll()

// We don't really care about the paths in this case
pm := packagemanager.NewPackageManager(dataDir, dataDir, dataDir, dataDir)
pm := packagemanager.NewPackageManager(dataDir, dataDir, dataDir, dataDir, "test")

// Create some boards with identical VID:PID combination
pack := pm.Packages.GetOrCreatePackage("packager")
Expand Down
12 changes: 10 additions & 2 deletions commands/daemon/daemon.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import (
rpc "github.com/arduino/arduino-cli/rpc/cc/arduino/cli/commands/v1"
"github.com/sirupsen/logrus"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/metadata"
"google.golang.org/grpc/status"
)

Expand Down Expand Up @@ -226,8 +227,15 @@ func (s *ArduinoCoreServerImpl) Upgrade(req *rpc.UpgradeRequest, stream rpc.Ardu
}

// Create FIXMEDOC
func (s *ArduinoCoreServerImpl) Create(_ context.Context, req *rpc.CreateRequest) (*rpc.CreateResponse, error) {
res, err := commands.Create(req)
func (s *ArduinoCoreServerImpl) Create(ctx context.Context, req *rpc.CreateRequest) (*rpc.CreateResponse, error) {
var userAgent []string
if md, ok := metadata.FromIncomingContext(ctx); ok {
userAgent = md.Get("user-agent")
}
if len(userAgent) == 0 {
userAgent = []string{"gRPCClientUnknown/0.0.0"}
}
res, err := commands.Create(req, userAgent...)
return res, convertErrorToRPCStatus(err)
}

Expand Down
2 changes: 1 addition & 1 deletion commands/debug/debug.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ func Debug(ctx context.Context, req *dbg.DebugConfigRequest, inStream io.Reader,
}
entry.Debug("Executing debugger")

cmd, err := executils.NewProcess(commandLine...)
cmd, err := executils.NewProcess(pm.GetEnvVarsForSpawnedProcess(), commandLine...)
if err != nil {
return nil, &arduino.FailedDebugError{Message: tr("Cannot execute debug tool"), Cause: err}
}
Expand Down
2 changes: 1 addition & 1 deletion commands/debug/debug_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ func TestGetCommandLine(t *testing.T) {
sketchPath := paths.New("testdata", sketch)
require.NoError(t, sketchPath.ToAbs())

pm := packagemanager.NewPackageManager(nil, nil, nil, nil)
pm := packagemanager.NewPackageManager(nil, nil, nil, nil, "test")
pm.LoadHardwareFromDirectory(customHardware)
pm.LoadHardwareFromDirectory(dataDir)

Expand Down
7 changes: 6 additions & 1 deletion commands/instances.go
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ func (instance *CoreInstance) installToolIfMissing(tool *cores.ToolRelease, down
}

// Create a new CoreInstance ready to be initialized, supporting directories are also created.
func Create(req *rpc.CreateRequest) (*rpc.CreateResponse, error) {
func Create(req *rpc.CreateRequest, extraUserAgent ...string) (*rpc.CreateResponse, error) {
instance := &CoreInstance{}

// Setup downloads directory
Expand All @@ -129,11 +129,16 @@ func Create(req *rpc.CreateRequest) (*rpc.CreateResponse, error) {
}

// Create package manager
userAgent := "arduino-cli/" + globals.VersionInfo.VersionString
for _, ua := range extraUserAgent {
userAgent += " " + ua
}
instance.PackageManager = packagemanager.NewPackageManager(
dataDir,
configuration.PackagesDir(configuration.Settings),
downloadsDir,
dataDir.Join("tmp"),
userAgent,
)

// Create library manager and add libraries directories
Expand Down
13 changes: 7 additions & 6 deletions commands/upload/upload.go
Original file line number Diff line number Diff line change
Expand Up @@ -531,19 +531,20 @@ func runProgramAction(pm *packagemanager.PackageManager,
}

// Run recipes for upload
toolEnv := pm.GetEnvVarsForSpawnedProcess()
if burnBootloader {
if err := runTool("erase.pattern", uploadProperties, outStream, errStream, verbose, dryRun); err != nil {
if err := runTool("erase.pattern", uploadProperties, outStream, errStream, verbose, dryRun, toolEnv); err != nil {
return &arduino.FailedUploadError{Message: tr("Failed chip erase"), Cause: err}
}
if err := runTool("bootloader.pattern", uploadProperties, outStream, errStream, verbose, dryRun); err != nil {
if err := runTool("bootloader.pattern", uploadProperties, outStream, errStream, verbose, dryRun, toolEnv); err != nil {
return &arduino.FailedUploadError{Message: tr("Failed to burn bootloader"), Cause: err}
}
} else if programmer != nil {
if err := runTool("program.pattern", uploadProperties, outStream, errStream, verbose, dryRun); err != nil {
if err := runTool("program.pattern", uploadProperties, outStream, errStream, verbose, dryRun, toolEnv); err != nil {
return &arduino.FailedUploadError{Message: tr("Failed programming"), Cause: err}
}
} else {
if err := runTool("upload.pattern", uploadProperties, outStream, errStream, verbose, dryRun); err != nil {
if err := runTool("upload.pattern", uploadProperties, outStream, errStream, verbose, dryRun, toolEnv); err != nil {
return &arduino.FailedUploadError{Message: tr("Failed uploading"), Cause: err}
}
}
Expand All @@ -552,7 +553,7 @@ func runProgramAction(pm *packagemanager.PackageManager,
return nil
}

func runTool(recipeID string, props *properties.Map, outStream, errStream io.Writer, verbose bool, dryRun bool) error {
func runTool(recipeID string, props *properties.Map, outStream, errStream io.Writer, verbose bool, dryRun bool, toolEnv []string) error {
recipe, ok := props.GetOk(recipeID)
if !ok {
return fmt.Errorf(tr("recipe not found '%s'"), recipeID)
Expand All @@ -577,7 +578,7 @@ func runTool(recipeID string, props *properties.Map, outStream, errStream io.Wri
if dryRun {
return nil
}
cmd, err := executils.NewProcess(cmdArgs...)
cmd, err := executils.NewProcess(toolEnv, cmdArgs...)
if err != nil {
return fmt.Errorf(tr("cannot execute upload tool: %s"), err)
}
Expand Down
2 changes: 1 addition & 1 deletion commands/upload/upload_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ func TestDetermineBuildPathAndSketchName(t *testing.T) {
}

func TestUploadPropertiesComposition(t *testing.T) {
pm := packagemanager.NewPackageManager(nil, nil, nil, nil)
pm := packagemanager.NewPackageManager(nil, nil, nil, nil, "test")
errs := pm.LoadHardwareFromDirectory(paths.New("testdata", "hardware"))
require.Len(t, errs, 0)
buildPath1 := paths.New("testdata", "build_path_1")
Expand Down
36 changes: 36 additions & 0 deletions docs/UPGRADING.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,27 @@ Here you can find a list of migration guides to handle breaking changes between

## Unreleased

### `packagemanager.NewPackageManager` function change

A new argument `userAgent` has been added to `packagemanager.NewPackageManager`, the new function signature is:

```go
func NewPackageManager(indexDir, packagesDir, downloadDir, tempDir *paths.Path, userAgent string) *PackageManager {
```
The userAgent string must be in the format `"ProgramName/Version"`, for example `"arduino-cli/0.20.1"`.
### `commands.Create` function change
A new argument `extraUserAgent` has been added to `commands.Create`, the new function signature is:
```go
func Create(req *rpc.CreateRequest, extraUserAgent ...string) (*rpc.CreateResponse, error) {
```
`extraUserAgent` is an array of strings, so multiple user agent may be provided. Each user agent must be in the format
`"ProgramName/Version"`, for example `"arduino-cli/0.20.1"`.
### `commands.Compile` function change
A new argument `progressCB` has been added to `commands.Compile`, the new function signature is:
Expand Down Expand Up @@ -32,6 +53,21 @@ The `parseArch` parameter was removed since it was unused and was always true. T
always parsed by the function. Furthermore the function now should also correctly interpret `packager:arch` spelled with
the wrong casing.
### `github.com/arduino/arduino-cli/executils.NewProcess` and `executils.NewProcessFromPath` function change
A new argument `extraEnv` has been added to `executils.NewProcess` and `executils.NewProcessFromPath`, the new function
signature is:
```go
func NewProcess(extraEnv []string, args ...string) (*Process, error) {
```
```go
func NewProcessFromPath(extraEnv []string, executable *paths.Path, args ...string) (*Process, error) {
```
The `extraEnv` params allow to pass environment variables (in addition to the default ones) to the spawned process.
### `github.com/arduino/arduino-cli/i18n.Init(...)` now requires an empty string to be passed for autodetection of locale
For automated detection of locale, change the call from:
Expand Down
9 changes: 9 additions & 0 deletions docs/platform-specification.md
Original file line number Diff line number Diff line change
Expand Up @@ -702,6 +702,15 @@ The tool configuration properties are available globally without the prefix. For
property can be used as **{cmd.path}** inside the recipe, and the same happens for all the other avrdude configuration
variables.

### Environment variables

All the tools launched to compile or upload a sketch will have the following environment variable set:

`ARDUINO_USER_AGENT`: contains the name and version of the client used by the user in
[HTTP user-agent format](https://en.wikipedia.org/wiki/User_agent), for example `"arduino-cli/0.21.0"`. It may also
contain multiple space-delimited entries like `"arduino-cli/0.21.0 ArduinoIDE/2.0.0-rc1"` if this information is
available.

### Pluggable discovery

Discovery tools are a special kind of tool used to find supported boards. A platform must declare one or more Pluggable
Expand Down

0 comments on commit 4256524

Please sign in to comment.