Merge ~fisterra-team/fisterra/+git/nitrogen6x-gadget:18-classic into ~fisterra-team/fisterra/+git/nitrogen6x-gadget:18-classic-mp

I am slightly confused here.

First of all, and this regards both this 18-classic and the 18 branch of the gadget tree, but I was wondering if it was really needed to have different code for classic and core?
So currently we do something like that for the pi3, but this is a left-over from the times where the snappy team was working on the gadgets. Ideally we'd want to have one repository and branch for both core and classic. Meaning, one gadget code that could be built with `make` (or `snapcraft prime`) to get a compiled gadget tree to be used for building a classic image and, in the same way, just running `snapcraft` to build a gadget snap to be used for core. Currently we have those two separate branches because we're still working on 'integrating' those two into one code-base. It's a bit difficult with pi3 because we already have users using the old gadget snaps, and due to some design decisions, switching both core and classic to one common gadget requires some migration effort.

But for new platforms, I would prefer if we could do it the 'right way' and have just one gadget repository that would work for both core and classic cases. Would that be possible?

Also, it feels to me that these branches might be missing something! So for instance for a 'classic' gadget tree, I would expect to find a Makefile. In the best case I would like to see both the core and classic gadget code to provide a Makefile and use it to pull all the needed packages from the archives and install them in the right places. But this branch for instance has no Makefile. The snapcraft.yaml doesn't seem to have the 'gadget' target here for instance.

So basically, in the perfect scenario, we would want to have one gadget repository/branch that would work and have all that's needed for core and classic. Once one pulls the branch, as mentioned earlier, a `snapcraft` call on it should create a working snap and, for now, a call to `make` should result in a built gadget tree for ubuntu-image to consume. Basically `snapcraft` should use the Makefile as well + some additional steps that would be needed to get the snap prepared.
I understand the biggest challenge might be having one bootscript (uboot.env or boot.scr) that would support both core and classic. Dave from our team is working on that for pi3, so best if you could poke him for some advice. If it's not easily possible for some reason, I guess we could provide both boot.scr and uboot.env in the gadget repository and then just decide which one to use depending whether snapcraft is run or not.

Hi Lukasz,

As previous talk with Dave on IRC, Dave plans to send a PR for integrating pi3 gadge snap to https://github.com/snapcore/pi3-gadget recently. We are integrating the uboot script first and waiting for Dave's PR as the reference method to improve this merge proposal.

Hi Lukasz, Dave,

Hope everything is well with you.
I'm working on integrating gadget snap.
I have a question that is why the boot file only supports 'uboot.env' if bootloader is uboot?
(https://github.com/snapcore/snapd/blob/3e442d10db595c15400c9b574318ed700d5ad4cf/bootloader/uboot.go#L64)
For instance, the gadget snap of classic is use 'boot.scr' as boot script and I would like to use 'boot.scr' for both classic and core, but it will fail on core because of missing kernel arguments (snap_core, snap_kernel and snap_menuentry).

Hey Jeremy! I have sent the link of this MP to Dave so that he can review it as well. Dave might be able to answer your question regarding using boot.scr for both core and classic. Ideally we'd just want to have boot.scr doing all the work, without the need for uboot.env, but from what I remember there's still something that needs to be changed in snapd to make this transition possible. Anyway, hopefully Dave will provide more input.

In the meantime, I'll review the current state of the MP shortly.

Just a few possible comments on the boot scripts; I can add more detail on the core/classic boot script situation but have to dash out right now - will reply further when I'm back!

> I have a question that is why the boot file only supports 'uboot.env' if
> bootloader is uboot?
> (https://github.com/snapcore/snapd/blob/3e442d10db595c15400c9b574318ed700d5ad4
> cf/bootloader/uboot.go#L64)
> For instance, the gadget snap of classic is use 'boot.scr' as boot script and
> I would like to use 'boot.scr' for both classic and core, but it will fail on
> core because of missing kernel arguments (snap_core, snap_kernel and
> snap_menuentry).

Good question: at present, the A/B boot support in snapd only supports the uboot.env style of boot script, not boot.scr. Hence Core images must use uboot.env, while Classic images can use either style. However, we'd generally prefer Classic use the boot.scr style as firstly that's what Debian upstream use exclusively and what Ubuntu uses for the vast majority of its boot scripts (on Classic).

Ideally, we want to update snapd (during the Core 20 cycle) to support the boot.scr style so that we can move the Core over to using the boot.scr style. This is partially because we'd like to merge the Core and Classic gadgets, but also because I'm uncomfortable with the way the A/B support with uboot is handled (the environment rewrites itself during the boot sequence and I'd much rather have a static boot script with a separate mutable state file). The theoretical boot script would detect whether it's booting a Core or Classic image (most likely by looking for the existence of the aforementioned A/B boot state), and act accordingly (using snap_core et al in the Core case and a fixed boot sequence in the Classic case).

I've got a prototype of this that theoretically works for the Pi, but it's largely untested (and probably won't get any serious attention until we look at the required snapd changes).

Anyway, for the time being I'm afraid that means you've got a choice with regards to this gadget snap:

1) Have a uboot.env script for the Core build, and a boot.scr script for the Classic build. Annoying as this is, this is the option I'd currently pick (and have picked for the pi images).

2) Have a uboot.env script for both Core and Classic. While this might seem easier *now*, I'd advise against this as it'll just mean more work when we start looking at snapd's A/B boot configuration.

Hi Dave,

> Ideally, we want to update snapd (during the Core 20 cycle) to support the boot.scr style so that we can move the Core over to using the boot.scr style.
Good news!

> also because I'm uncomfortable with the way the A/B support with uboot is handled (the environment rewrites itself during the boot sequence and I'd much rather have a static boot script with a separate mutable state file).
Yes, that's what I prefer also.
Thus, the boot.scr.in as this MP is designed for both Core and Classic. It *should* works but not yet to test because snapd only supports appending related kernel arguments in "uboot.env.in" (the kernel arguments won't be appended if file not found).

> The theoretical boot script would detect whether it's booting a Core or Classic image
Could you please share more details about how did you implemented it?
I used an ugly condition as "if test -e ${devtype} ${devnum}:2 /boot/${initrd_image}; then" line#124 in "boot.scr.in".

> 1) Have a uboot.env script for the Core build, and a boot.scr script for the Classic build. Annoying as this is, this is the option I'd currently pick (and have picked for the pi images).
Yes, that is what I did as this MP. (I handled both case separately in snapcrafy.yaml)

However, I remove some unnecessary parts as you commented before.
Please kindly to help to review it.
Many thanks!

> > The theoretical boot script would detect whether it's booting a Core or
> Classic image
> Could you please share more details about how did you implemented it?
> I used an ugly condition as "if test -e ${devtype} ${devnum}:2
> /boot/${initrd_image}; then" line#124 in "boot.scr.in".

I'm afraid my method is about as elegant :). Though, instead of testing for the initrd.img I instead test for the existence of my (not-yet-implemented) boot.dat file which (theoretically) holds the boot state (snappy_kernel etc). If that file exists, it goes the Core route, reading that file and re-writing it accordingly, otherwise it goes the Classic route and just boots vmlinuz (after optional gunzipping).

> > 1) Have a uboot.env script for the Core build, and a boot.scr script for the
> Classic build. Annoying as this is, this is the option I'd currently pick (and
> have picked for the pi images).
> Yes, that is what I did as this MP. (I handled both case separately in
> snapcrafy.yaml)

Sorry I didn't get to this last night - will take a look now!

The code looks much better, no major objections from me on that. However, I did attempt to prime the gadget as a quick experiment and it wound up throwing an error regarding conflicting boot.scr outputs; more detail in the diff comment below.

Hi Dave,

For your error, I comment it below.
It seems like your test command is 'sudo snapcraft --target-arch armhf prime'.

For currently design, I use a script to build it separately as README mentioned.
If we don't want to separate the build method for both classic and core then we will need to put everything in one gadget snap together.
(such as cloud-init config and device-tree (device-tree provided by kernel for core) )

I'd leave it as a open question and would like to know your opinion about that.

Sorry, I'd managed to completely miss the README (too used to dealing with the Pi's images)! The solution for dealing with the disparity between the core and classic images sounds good (for now, as you note; in future once snapd supports boot scripts we can move them closer and potentially remove the requirement to specify a particular part).

Anyway, that resolves the last query I had, so thumbs up from me!

initrd_high and fdt_high are required for booting
Add initrd_high into boot.scr.in and uboot.env.in.