AsteroidOS / meta-asteroid

OpenEmbedded layer that provides the basis of AsteroidOS
GNU General Public License v2.0
78 stars 45 forks source link

Remove pkg_postinst_ontarget rule for kernel images. #176

Closed argosphil closed 1 year ago

argosphil commented 1 year ago

This should fix the recent device bricks seen. The underlying problem was identified and described by @FlorentRevest, this is merely an attempt to translate his observations into a PR.

This means you will not be able to install a new kernel with opkg without also invoking fastboot. OTOH, that never worked reliably.

Some of the removed code was dodgy, and most likely caused https://github.com/AsteroidOS/meta-asteroid/issues/175

Further code should be removed but this will happen in a separate PR; this should fix the bricking issue and remove the kernel-update functionality, creating a consistently empty package for linux-$machine-image is less of a priority.

Fixes https://github.com/AsteroidOS/meta-asteroid/issues/175.

argosphil commented 1 year ago

Marking as draft until the build finishes.

dodoradio commented 1 year ago

I think it might be a better idea to instead merge in https://github.com/shr-distribution/meta-smartphone/commit/2d0b169dd286c494b744ec3b8d58a9be70271328 which is how this was fixed 'upstream'. But if removing entirely fixes the bricks, then it's a reasonable stopgap, and we can patch back in later

FlorentRevest commented 1 year ago

I agree with dodoradio but let's take it one step at a time. I don't mind ripping this off for now and if someone decides to put it back we should properly test it and then reintroduce it

argosphil commented 1 year ago

@dodoradio I've thought about this for a bit, and while I don't feel strongly that updating the "boot" partition upon installing a package is a bad idea, I do think that in the balance of things, the convenience probably isn't worth it. The main reason for this is that for people installing packages, there's a very significant probability the new kernel won't work, and you'll have to use fastboot to fix it. That's a lot less scary, IMHO, if you've just used fastboot to break it, since you'd just be using software a second time rather than an entirely different piece of software.

I also don't trust that code. It finds a file called "boot" anywhere in /dev, checks there's a "disk" somewhere in the path, then blindly dds over it without even checking the new image will fit first. This may be crazy, but I've been playing around with LVM2 on an asteroidos-based system, and if I create an LVM2 volume called "mydisk" with a "boot" LV, that will match and might get overwritten instead. I also think it's a bad idea to overwrite the "boot" partition on first boot, since it should already contain the desired data and our write will be, at best, wastefully redundant. (Speaking of waste, the old code also left an extra copy of the kernel image in /boot. Oh, and does the code indicate to the user in any way that now would be a fabulously bad time to reset the watch? fastboot does.)

Lastly, do we really know what fastboot is doing? Some future version might save a checksum somewhere, sign the boot partition, or maybe resize it to match the image.

Sorry this got a bit long (at least people are unlikely to have to read it here :-) ). Again, I don't feel strongly about this and I realize that some of my concerns are upstream issues that I should be submitting fixes for...

dodoradio commented 1 year ago

the intention behind this is that the watch should be able to get kernel updates. This is an incredibly rare event, given how finnicky the ports are, but it's worth planning for. Relying only on fastboot is a significant inconvenience, especially on the devices where USB access requires disassembly. You definitely make a good point about fastboot being theoretically safer. In any case, kernel updates are rare, and asteroid is currently quite hands-on anyways, so relying on fastboot there isn't going to be catastrophic. We can probably do without this script for quite a while (we've obviously managed without it already) and we can probably implement something a bit more device-specific if we need to engage kernel updates in the future.