Make size and checksum fields of package index optional

[obra]

Describe the problem

Arduino CLI silently ignores some of my package index files. The package indexes in question are "git head" index files that let the user install the current main branch of my core without needing to publish an updated package file for every commit.

https://github.com/keyboardio/ArduinoCore-GD32-Keyboardio/blob/main/package_gd32_index.json is the URL to an example board index.

Installation of these platforms via the Arduino IDE 1.x Boards Manager works fine. This behavior of Arduino IDE 1.x is intentional: arduino/Arduino#3449

Arduino CLI version

arduino-cli alpha Version: 0.19.0 Commit: 56419ecd Date: 2021-09-02T14:47:35Z

Additional context

Is this a regression from the classic IDE's boards manager or is there some sort of a spec change? If it's the latter, I'd hope that running in --verbose mode, arduino-cli would complain about what causes it to discard the unacceptable platform.

Additional requests

• Make size and checksum fields of package index optional #1468 (comment)

[obra]

Prodding a little bit, it appears that the "size" key for a platform has become a required key. If 'size' is not included, the platform is silently dropped. That feels like a bug.

[ubidefeo]

hi @obra
Sorry for the late reply, I've been digging into this.

What is annoying is that the Java IDE is way too permissive (as it's always been), but the size and/or checksum are fields you should fill in.
Have you given a go at Arduino Lint?
If you build from master you have access to the new Package Index check as well as documentation for all the rules.
We have to finalise a few things before this gets a release tag.

What I get out of it is this

Linting package-index in package_gd32_index.json
ERROR: packages[*].platforms[*].help.online property does not have a valid URL format in platform(s):
         keyboardio:gd32@0.0.1
       See: https://arduino.github.io/arduino-cli/latest/package_index_json-specification/#platforms-definitions
       (Rule IL022)
ERROR: Missing packages[*].platforms[*].checksum property in platform(s):
         keyboardio:gd32@0.0.1
       See: https://arduino.github.io/arduino-cli/latest/package_index_json-specification/#platforms-definitions
       (Rule IL032)
ERROR: Missing packages[*].platforms[*].size property in platform(s):
         keyboardio:gd32@0.0.1
       See: https://arduino.github.io/arduino-cli/latest/package_index_json-specification/#platforms-definitions
       (Rule IL036)

Linter results for project: 3 ERRORS, 0 WARNINGS

-------------------

please try to just add the size property and see if it goes through, otherwise add the checksum.
We're still very much in the process of documenting the platform, making sure specs are followed by everyone (starting from within) and randomly opening PRs to repos we bump into which are not compliant at any level.

Let me know if you can get this to work, but we do have a couple of open tasks related to these properties of the Package Index that might get a facelift

u.

[obra]

Hi @ubidefeo - No worries!

The specific workflow I'm trying to unbreak here is to be able to define a platform/core that automatically installs the current head of the git main/master branch of the repo. GitHub provides an easy way to get a persistent URL to a zipfile of the current state of a repo. Being able to drop a platform json file into the repo pointing at that zipfile has meant that it's been easy to support folks who want to be running the current/dev version of a core but aren't themselves developers of that core. Because that zipfile is autogenerated at request time, there's no easy way to obtain a size or checksum.

With the existing IDE, this just works. With the new, stricter parser in arduino-cli, it does not.

The alternative is to manually create a zipfile on every commit and publish a new version of the package.json file with a checksum and size. If that's the only supported solution going forward, it can be made to work, but it feels like a waste of computing resources :/

I hugely appreciate the work you all are doing to modernize and document the platform and ecosystem. It is absolutely making Arduino better.

In other projects, the way I've typically dealt with new strictures in config files is with versioning, but there may be few enough arduino platforms out there that that'd be overkill here. It would be really nice to have a way to say "hey, this core file is dynamically generated, so you're not getting a checksum or size", though.

[ubidefeo]

I completely understand your motivations and your point.
I'm not 100% sure how much we enforce it at this very moment, but I'd invite you to add a bogus size and give it a shot.
I think it could be interesting to add a --developer flag to the core installation command.
I remember we did something for our FW team but they have the whole package_index_staging.json generated automatically via an action every time there's a merged PR.
Have you considered that option?

Also I think @silvanocerza can help me remember what was done for the FW team and if we can do something to aid developers whose pre-release workflow is less automated.

Hope he can chime in soon

[obra]

I have indeed done the auto-generated package on PR/commit thing. It works, but it's a bit clunky. It also means having to publish and store artifacts when we otherwise wouldn't need them.

I did indeed try the bogus size. Sadly, that breaks the classic 1.5 IDE, which does check it. If that had worked with the classic IDE, I'd have suggested documenting a size of 0 or -1 to indicate not checking the size.

Hrm.

[ubidefeo]

I did indeed try the bogus size. Sadly, that breaks the classic 1.5 IDE

why can't we have nice things...

Let's wait for @silvanocerza and @cmaglie 's feedback on this one, this is certainly very niche, but maybe they have an idea or a workaround, or maybe there's some behind-the-scenes way of getting this to work and not break specs or legacy ;)

[silvanocerza]

This is what's preventing your platform from being loaded:

arduino-cli/arduino/cores/packageindex/index.go

Lines 253 to 256 in fb8931c

	size, err := inPlatformRelease.Size.Int64()
	if err != nil {
	return fmt.Errorf(tr("invalid platform archive size: %s"), err)
	}

We could just log the error if the size is not valid and call it a day but am not sure if it's gonna break further down the line.

Also if we just log and don't fail that field becomes techically optional even if it's marked as mandatory, that's something that I would very much want to avoid.

[obra]

I 100% understand if the added complexity is not an acceptable thing for the codebase, but one option might be to add a new optional key "platformArchiveDynamicallyGenerated" that skips the Size and Checksum checks.

[silvanocerza]

That's certainly a solution but it would make both size and checksum useless, at that point what's stopping everyone to always set platformArchiveDynamicallyGenerated to true even if they could calculate the checksum and size comfortably?

Also I don't think removing the checksum is a great idea from a security standpoint.

In any case I understand the problem but don't have a good solution for this right now.

[ubidefeo]

@silvanocerza @obra
I have been giving this a thought, and I highly discourage enabling things at the index level.
An alternative (which I believe could be very beneficial to developers and testers) is to add a flag --unsafe to some workflows.
This way testers could be instructed to add it (at their own risk) in their configurations as we do with library.enable_unsafe_install.
How does that sound?

I believe it's an incredible asset for developers and we don't risk anything since user action is strictly required to achieve this