-
-
Notifications
You must be signed in to change notification settings - Fork 403
Enhances tracking of installed platforms #998
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
Conversation
@silvanocerza the
The expected output:
|
Uhu, this one is interesting. I'll take a look, thanks @per1234. |
@per1234 @silvanocerza |
I don't, I get the correct |
I only get it on Windows Bash. I don't get it on Windows cmd or Bash on my Linux machine. However, Windows Bash can print this character:
|
@per1234 it is a confusing world 😀 |
Should have fixed the issue reported by @per1234, I've added a test for it too. |
9e8a0df
to
5f3a429
Compare
5f3a429
to
f2bd18a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tried removing cores, json files, entries from the config and mix and matching such occurrences without being able to reproduce the no-index issue
works for me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good! Nice one.
I've added some small refactoring suggestions.
I think that moving the Board -> index.json
conversion subroutines inside the packageindex
module may avoid us to make the index structures public and will keep them together with the subrotuine that do the opposite way (i.e. converts index.json -> Board
).
return nil | ||
} | ||
|
||
func platformReleaseToIndex(pr *cores.PlatformRelease) packageindex.IndexPlatformRelease { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move this function into arduino/cores/packageindex
and avoid to make the packageindex
structs public?
URL: pr.Resource.URL, | ||
ArchiveFileName: pr.Resource.ArchiveFileName, | ||
Checksum: pr.Resource.Checksum, | ||
Size: json.Number(strconv.FormatInt(pr.Resource.Size, 10)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't a fmt.Sprintf("%d", pr.Resource.Size)
works too?
require.Contains(t, indexPackageTools, &packageindex.IndexToolRelease{ | ||
Name: "arm-none-eabi-gcc", | ||
Version: semver.ParseRelaxed("4.8.3-2014q1"), | ||
Systems: []packageindex.IndexToolReleaseFlavour{ | ||
{ | ||
OS: "arm-linux-gnueabihf", | ||
URL: "http://downloads.arduino.cc/gcc-arm-none-eabi-4.8.3-2014q1-arm.tar.bz2", | ||
ArchiveFileName: "gcc-arm-none-eabi-4.8.3-2014q1-arm.tar.bz2", | ||
Size: "44423906", | ||
Checksum: "SHA-256:ebe96b34c4f434667cab0187b881ed585e7c7eb990fe6b69be3c81ec7e11e845", | ||
}, | ||
{ | ||
OS: "i686-mingw32", | ||
URL: "http://downloads.arduino.cc/gcc-arm-none-eabi-4.8.3-2014q1-windows.tar.gz", | ||
ArchiveFileName: "gcc-arm-none-eabi-4.8.3-2014q1-windows.tar.gz", | ||
Size: "84537449", | ||
Checksum: "SHA-256:fd8c111c861144f932728e00abd3f7d1107e186eb9cd6083a54c7236ea78b7c2", | ||
}, | ||
{ | ||
OS: "x86_64-apple-darwin", | ||
URL: "http://downloads.arduino.cc/gcc-arm-none-eabi-4.8.3-2014q1-mac.tar.gz", | ||
ArchiveFileName: "gcc-arm-none-eabi-4.8.3-2014q1-mac.tar.gz", | ||
Size: "52518522", | ||
Checksum: "SHA-256:3598acf21600f17a8e4a4e8e193dc422b894dc09384759b270b2ece5facb59c2", | ||
}, | ||
{ | ||
OS: "x86_64-pc-linux-gnu", | ||
URL: "http://downloads.arduino.cc/gcc-arm-none-eabi-4.8.3-2014q1-linux64.tar.gz", | ||
ArchiveFileName: "gcc-arm-none-eabi-4.8.3-2014q1-linux64.tar.gz", | ||
Size: "51395093", | ||
Checksum: "SHA-256:d23f6626148396d6ec42a5b4d928955a703e0757829195fa71a939e5b86eecf6", | ||
}, | ||
{ | ||
OS: "i686-pc-linux-gnu", | ||
URL: "http://downloads.arduino.cc/gcc-arm-none-eabi-4.8.3-2014q1-linux32.tar.gz", | ||
ArchiveFileName: "gcc-arm-none-eabi-4.8.3-2014q1-linux32.tar.gz", | ||
Size: "51029223", | ||
Checksum: "SHA-256:ba1994235f69c526c564f65343f22ddbc9822b2ea8c5ee07dd79d89f6ace2498", | ||
}, | ||
}}) | ||
|
||
indexPlatformRelease := indexPackage.Platforms[0] | ||
require.Equal(t, "Arduino AVR Boards", indexPlatformRelease.Name) | ||
require.Equal(t, "avr", indexPlatformRelease.Architecture) | ||
require.Equal(t, "1.6.23", indexPlatformRelease.Version.String()) | ||
require.Equal(t, "Arduino", indexPlatformRelease.Category) | ||
require.Equal(t, "http://www.arduino.cc/en/Reference/HomePage", indexPlatformRelease.Help.Online) | ||
require.Equal(t, "http://downloads.arduino.cc/cores/avr-1.6.23.tar.bz2", indexPlatformRelease.URL) | ||
require.Equal(t, "avr-1.6.23.tar.bz2", indexPlatformRelease.ArchiveFileName) | ||
require.Equal(t, "SHA-256:18618d7f256f26cd77c35f4c888d5d1b2334f07925094fdc99ac3188722284aa", indexPlatformRelease.Checksum) | ||
require.Equal(t, json.Number("5001988"), indexPlatformRelease.Size) | ||
require.Contains(t, indexPlatformRelease.Boards, packageindex.IndexBoard{Name: "Arduino Yún"}) | ||
require.Contains(t, indexPlatformRelease.Boards, packageindex.IndexBoard{Name: "Arduino/Genuino Uno"}) | ||
require.Contains(t, indexPlatformRelease.Boards, packageindex.IndexBoard{Name: "Arduino Uno WiFi"}) | ||
require.Contains(t, indexPlatformRelease.Boards, packageindex.IndexBoard{Name: "Arduino Diecimila"}) | ||
require.Contains(t, indexPlatformRelease.Boards, packageindex.IndexBoard{Name: "Arduino Nano"}) | ||
require.Contains(t, indexPlatformRelease.Boards, packageindex.IndexBoard{Name: "Arduino/Genuino Mega"}) | ||
require.Contains(t, indexPlatformRelease.Boards, packageindex.IndexBoard{Name: "Arduino MegaADK"}) | ||
require.Contains(t, indexPlatformRelease.Boards, packageindex.IndexBoard{Name: "Arduino Leonardo"}) | ||
require.Contains(t, indexPlatformRelease.Boards, packageindex.IndexBoard{Name: "Arduino Leonardo Ethernet"}) | ||
require.Contains(t, indexPlatformRelease.Boards, packageindex.IndexBoard{Name: "Arduino/Genuino Micro"}) | ||
require.Contains(t, indexPlatformRelease.Boards, packageindex.IndexBoard{Name: "Arduino Esplora"}) | ||
require.Contains(t, indexPlatformRelease.Boards, packageindex.IndexBoard{Name: "Arduino Mini"}) | ||
require.Contains(t, indexPlatformRelease.Boards, packageindex.IndexBoard{Name: "Arduino Ethernet"}) | ||
require.Contains(t, indexPlatformRelease.Boards, packageindex.IndexBoard{Name: "Arduino Fio"}) | ||
require.Contains(t, indexPlatformRelease.Boards, packageindex.IndexBoard{Name: "Arduino BT"}) | ||
require.Contains(t, indexPlatformRelease.Boards, packageindex.IndexBoard{Name: "Arduino LilyPadUSB"}) | ||
require.Contains(t, indexPlatformRelease.Boards, packageindex.IndexBoard{Name: "Arduino Lilypad"}) | ||
require.Contains(t, indexPlatformRelease.Boards, packageindex.IndexBoard{Name: "Arduino Pro"}) | ||
require.Contains(t, indexPlatformRelease.Boards, packageindex.IndexBoard{Name: "Arduino ATMegaNG"}) | ||
require.Contains(t, indexPlatformRelease.Boards, packageindex.IndexBoard{Name: "Arduino Robot Control"}) | ||
require.Contains(t, indexPlatformRelease.Boards, packageindex.IndexBoard{Name: "Arduino Robot Motor"}) | ||
require.Contains(t, indexPlatformRelease.Boards, packageindex.IndexBoard{Name: "Arduino Gemma"}) | ||
require.Contains(t, indexPlatformRelease.Boards, packageindex.IndexBoard{Name: "Adafruit Circuit Playground"}) | ||
require.Contains(t, indexPlatformRelease.Boards, packageindex.IndexBoard{Name: "Arduino Yún Mini"}) | ||
require.Contains(t, indexPlatformRelease.Boards, packageindex.IndexBoard{Name: "Arduino Industrial 101"}) | ||
require.Contains(t, indexPlatformRelease.Boards, packageindex.IndexBoard{Name: "Linino One"}) | ||
require.Contains(t, indexPlatformRelease.ToolDependencies, | ||
packageindex.IndexToolDependency{ | ||
Packager: "arduino", | ||
Name: "avr-gcc", | ||
Version: semver.ParseRelaxed("5.4.0-atmel3.6.1-arduino2"), | ||
}) | ||
require.Contains(t, indexPlatformRelease.ToolDependencies, | ||
packageindex.IndexToolDependency{ | ||
Packager: "arduino", | ||
Name: "avrdude", | ||
Version: semver.ParseRelaxed("6.3.0-arduino14"), | ||
}) | ||
require.Contains(t, indexPlatformRelease.ToolDependencies, | ||
packageindex.IndexToolDependency{ | ||
Packager: "arduino", | ||
Name: "arduinoOTA", | ||
Version: semver.ParseRelaxed("1.2.1"), | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's ok-ish doing this way but, there must be a better way to check for matching JSONs :-)
@cmaglie made the changes you requested, let me know if it's ok right now. |
import ( | ||
"encoding/json" | ||
"testing" | ||
|
||
"github.com/arduino/arduino-cli/arduino/cores/packageindex" | ||
"github.com/arduino/arduino-cli/arduino/cores/packagemanager" | ||
"github.com/arduino/arduino-cli/cli/output" | ||
"github.com/arduino/arduino-cli/commands" | ||
"github.com/arduino/go-paths-helper" | ||
"github.com/stretchr/testify/require" | ||
semver "go.bug.st/relaxed-semver" | ||
) | ||
|
||
func TestInstallPlatform(t *testing.T) { | ||
dataDir := paths.New("testdata", "data_dir_1") | ||
packageDir := paths.TempDir().Join("test", "packages") | ||
downloadDir := paths.TempDir().Join("test", "staging") | ||
tmpDir := paths.TempDir().Join("test", "tmp") | ||
packageDir.MkdirAll() | ||
downloadDir.MkdirAll() | ||
tmpDir.MkdirAll() | ||
defer paths.TempDir().Join("test").RemoveAll() | ||
|
||
pm := packagemanager.NewPackageManager(dataDir, packageDir, downloadDir, tmpDir) | ||
pm.LoadPackageIndexFromFile(dataDir.Join("package_index.json")) | ||
|
||
platformRelease, tools, err := pm.FindPlatformReleaseDependencies(&packagemanager.PlatformReference{ | ||
Package: "arduino", | ||
PlatformArchitecture: "avr", | ||
PlatformVersion: semver.MustParse("1.6.23"), | ||
}) | ||
require.NotNil(t, platformRelease) | ||
require.NotNil(t, tools) | ||
require.Nil(t, err) | ||
|
||
downloaderConfig, err := commands.GetDownloaderConfig() | ||
require.NotNil(t, downloaderConfig) | ||
require.Nil(t, err) | ||
downloader, err := pm.DownloadPlatformRelease(platformRelease, downloaderConfig) | ||
require.NotNil(t, downloader) | ||
require.Nil(t, err) | ||
err = commands.Download(downloader, platformRelease.String(), output.NewNullDownloadProgressCB()) | ||
require.Nil(t, err) | ||
|
||
err = pm.InstallPlatform(platformRelease) | ||
require.Nil(t, err) | ||
|
||
destDir := packageDir.Join("arduino", "hardware", "avr", "1.6.23") | ||
require.True(t, destDir.IsDir()) | ||
|
||
installedJSON := destDir.Join("installed.json") | ||
require.True(t, installedJSON.Exist()) | ||
|
||
bt, err := installedJSON.ReadFile() | ||
require.Nil(t, err) | ||
|
||
index := &packageindex.Index{} | ||
err = json.Unmarshal(bt, index) | ||
require.Nil(t, err) | ||
|
||
expectedJSON := paths.New("testdata", "installed.json") | ||
expectedBt, err := expectedJSON.ReadFile() | ||
require.Nil(t, err) | ||
|
||
expectedIndex := &packageindex.Index{} | ||
err = json.Unmarshal(expectedBt, expectedIndex) | ||
require.Nil(t, err) | ||
|
||
require.Equal(t, expectedIndex.IsTrusted, index.IsTrusted) | ||
require.Equal(t, len(expectedIndex.Packages), len(index.Packages)) | ||
|
||
for i := range expectedIndex.Packages { | ||
expectedPackage := expectedIndex.Packages[i] | ||
indexPackage := index.Packages[i] | ||
require.Equal(t, expectedPackage.Name, indexPackage.Name) | ||
require.Equal(t, expectedPackage.Maintainer, indexPackage.Maintainer) | ||
require.Equal(t, expectedPackage.WebsiteURL, indexPackage.WebsiteURL) | ||
require.Equal(t, expectedPackage.Email, indexPackage.Email) | ||
require.Equal(t, expectedPackage.Help.Online, indexPackage.Help.Online) | ||
require.Equal(t, len(expectedPackage.Tools), len(indexPackage.Tools)) | ||
require.ElementsMatch(t, expectedPackage.Tools, indexPackage.Tools) | ||
|
||
require.Equal(t, len(expectedPackage.Platforms), len(indexPackage.Platforms)) | ||
for n := range expectedPackage.Platforms { | ||
expectedPlatform := expectedPackage.Platforms[n] | ||
indexPlatform := indexPackage.Platforms[n] | ||
require.Equal(t, expectedPlatform.Name, indexPlatform.Name) | ||
require.Equal(t, expectedPlatform.Architecture, indexPlatform.Architecture) | ||
require.Equal(t, expectedPlatform.Version.String(), indexPlatform.Version.String()) | ||
require.Equal(t, expectedPlatform.Category, indexPlatform.Category) | ||
require.Equal(t, expectedPlatform.Help.Online, indexPlatform.Help.Online) | ||
require.Equal(t, expectedPlatform.URL, indexPlatform.URL) | ||
require.Equal(t, expectedPlatform.ArchiveFileName, indexPlatform.ArchiveFileName) | ||
require.Equal(t, expectedPlatform.Checksum, indexPlatform.Checksum) | ||
require.Equal(t, expectedPlatform.Size, indexPlatform.Size) | ||
require.ElementsMatch(t, expectedPlatform.Boards, indexPlatform.Boards) | ||
require.ElementsMatch(t, expectedPlatform.ToolDependencies, indexPlatform.ToolDependencies) | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This now looks more like an integration test... I guess we can split all of this in two parts:
- A test that verifies if the written
installed.json
contains the correct information: this one is a unit-test that should be placed inpackageindex
. In the test you can create a fakecores.Package
and try to convert it tojson
. - An integration test that:
- installs a platform and checks that the installed.json exists
- remove the index relative to the previously installed platform and checks that querying the platform will still produce the correct output (in particular
name
andtool dependencies
)
IMHO 1. can be omitted if 2. is done propertly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It works great in all my tests Silvano. Thanks!
I was having trouble testing this previously because even before this change Arduino CLI didn't have the primary issues that inspired #252.
However, there was one condition under which I suspected Arduino CLI would still have an issue, but was unable to test because of a bug. Now that the bug has been fixed by #1020, I was able to confirm that Arduino CLI does indeed misbehave under those conditions, but that this PR fixes that.
Roberto had requested that I share the procedure I used to test for this, so I give you a long-winded approval:
I created a package index for the foo vendor and saved it to directories.data
:
{
"packages": [
{
"name": "foo",
"maintainer": "Arduino",
"websiteURL": "http://www.arduino.cc/",
"email": "[email protected]",
"help": {
"online": "http://www.arduino.cc/en/Reference/HomePage"
},
"platforms": [
{
"name": "Arduino megaAVR Boards",
"architecture": "megaavr",
"version": "1.8.3",
"category": "Arduino",
"help": {
"online": "http://www.arduino.cc/en/Reference/HomePage"
},
"url": "http://downloads.arduino.cc/cores/megaavr-1.8.3.tar.bz2",
"archiveFileName": "megaavr-1.8.3.tar.bz2",
"checksum": "SHA-256:9791d62551f1e6539769a026f903082344b8ae8b921c1796de94a2af368d866d",
"size": "856037",
"boards": [
{
"name": "Arduino Uno WiFi Rev2"
},
{
"name": "Arduino Nano Every"
}
],
"toolsDependencies": [
{
"packager": "arduino",
"name": "avr-gcc",
"version": "7.3.0-atmel3.6.1-arduino5"
},
{
"packager": "arduino",
"name": "avrdude",
"version": "6.3.0-arduino16"
},
{
"packager": "arduino",
"name": "arduinoOTA",
"version": "1.3.0"
}
]
},
{
"name": "Arduino megaAVR Boards",
"architecture": "megaavr",
"version": "1.8.4",
"category": "Arduino",
"help": {
"online": "http://www.arduino.cc/en/Reference/HomePage"
},
"url": "http://downloads.arduino.cc/cores/megaavr-1.8.4.tar.bz2",
"archiveFileName": "megaavr-1.8.4.tar.bz2",
"checksum": "SHA-256:ff25944bc6f7b766f8a71b836763517de3b12008b0b08cf24460b8af5a05abd1",
"size": "855455",
"boards": [
{
"name": "Arduino Uno WiFi Rev2"
},
{
"name": "Arduino Nano Every"
}
],
"toolsDependencies": [
{
"packager": "arduino",
"name": "avr-gcc",
"version": "7.3.0-atmel3.6.1-arduino5"
},
{
"packager": "arduino",
"name": "avrdude",
"version": "6.3.0-arduino16"
},
{
"packager": "arduino",
"name": "arduinoOTA",
"version": "1.3.0"
}
]
},
{
"name": "Arduino megaAVR Boards",
"architecture": "megaavr",
"version": "1.8.5",
"category": "Arduino",
"help": {
"online": "http://www.arduino.cc/en/Reference/HomePage"
},
"url": "http://downloads.arduino.cc/cores/megaavr-1.8.5.tar.bz2",
"archiveFileName": "megaavr-1.8.5.tar.bz2",
"checksum": "SHA-256:d35ca76fa081fb3a282584d498fadb5712a4330fa39cde560e31d72185c940d8",
"size": "876954",
"boards": [
{
"name": "Arduino Uno WiFi Rev2"
},
{
"name": "Arduino Nano Every"
}
],
"toolsDependencies": [
{
"packager": "arduino",
"name": "avr-gcc",
"version": "7.3.0-atmel3.6.1-arduino5"
},
{
"packager": "arduino",
"name": "avrdude",
"version": "6.3.0-arduino17"
},
{
"packager": "arduino",
"name": "arduinoOTA",
"version": "1.3.0"
}
]
}
],
"tools": []
}
]
}
I then added a fake package index URL so the package_foo_index.json
file I added to directories.data
would be recognized:
export ARDUINO_BOARD_MANAGER_ADDITIONAL_URLS=https://example.com/package_foo_index.json
Now I install the platforms:
$ ./arduino-cli version
arduino-cli.exe Version: 0.0.0-git Commit: 10d07906
$ ./arduino-cli core install foo:[email protected]
$ ./arduino-cli core install arduino:[email protected]
Both foo:[email protected]
and arduino:[email protected]
have a dependency on the arduino:[email protected]
tool, so when I uninstall arduino:avr
, that tool should not be removed:
$ ./arduino-cli core uninstall arduino:avr
Uninstalling arduino:[email protected]...
arduino:[email protected] uninstalled
Uninstalling arduino:[email protected], tool is no more required...
arduino:[email protected] uninstalled
Now I remove the foo:[email protected] entry from its package index. This might happen if the platform author "tidies up" the package index by removing all the entries for previous versions, perhaps because they no longer wish to support them. An example is all the pre-2.3.0 versions of esp8266:esp8266
being pulled from their package index.
Now install and uninstall arduino:[email protected]
again:
$ ./arduino-cli core install arduino:[email protected]
./arduino-cli core uninstall arduino:avr
Uninstalling arduino:[email protected]...
arduino:[email protected] uninstalled
Uninstalling arduino:[email protected], tool is no more required...
arduino:[email protected] uninstalled
Uninstalling arduino:[email protected], tool is no more required...
arduino:[email protected] uninstalled
Uninstalling arduino:[email protected], tool is no more required...
arduino:[email protected] uninstalled
You can see that this time the arduino:[email protected]
tool dependency of foo:[email protected]
was removed, breaking the platform.
I tried the same process again with Arduino CLI from this PR, but rebased on master
(21cc1297) and the tool dependency is not removed when arduino:avr
is uninstalled, leaving foo:megaavr
intact!
I'm thinking it's not necessary since this is almost entirely an internal implementation detail. The only concern I had was that the The only possible issue I can see is if there was a collision with a file of the same name placed there by the platform author. That's not very likely and it shouldn't cause any problems anyway. |
2dd2089
to
4cb12fc
Compare
4cb12fc
to
4ae7551
Compare
4ae7551
to
ad59b73
Compare
Please check if the PR fulfills these requirements
before creating one)
our contributing guidelines
Enhances tracking of installed platforms.
Platforms' information is lost if the index and its related URL are removed, this means that compilation might fail too.
Platforms' information is saved at install time in
installed.json
, the file is saved in the platform installation directory.No.
Fixes Track installed cores with a dedicated index file #252.
@per1234 should we mention this new behaviour in the documenation?
See how to contribute