[PATCH 2/2] treewide: remove INITRAMFS check for preinit_main hook

[Elliott Mitchell]

On Fri, Sep 06, 2024 at 02:15:22PM +0200, Florian Eckert wrote:
> 
> On 2024-09-06 04:40, Elliott Mitchell wrote:
> > 
> > Examining this situation, I wonder whether this is really the right way
> > to go.
> > 
> > There are 29 files which match [789]* (excluding `70_initramfs_test`).
> > So you found this redundant check in >10% of files.  This seems a rather
> > high percentage.  Perhaps it might be better to remove the `break` from
> > `70_initramfs_test` and instead require explicitly checking $INITRAMFS ?
> 
> I thought about that too, but decided against it, as I would then have to
> adapt a lot more, as you mentioned. I have to admit that at the beginning
> I didn't understand why my script wasn't called in 'preinit_main' until
> I saw that there was a 'break' in 'boot_run_hook'.
> 
> I would not have expected that!
> 
> I would therefore also prefer as you notice the same to delete 'break' and
> check the INITRAMFS flag. That makes the whole thing more transparent and
> clear.

These haven't been through the copy/update cycle of the kernel
configuration files, so `git blame` works on them:

f43b7934d285 package/base-files/files/lib/preinit/80_mount_root
John Crispin <john at openwrt.org>  2013-03-13 11:11:19

a0b7fef0ffe4 package/utils/zyxel-bootconfig/files/95_apply_bootconfig
David Bauer <mail at david-bauer.net>  2022-07-20 12:52:06

fe0081eecf43 target/linux/bcm27xx/base-files/lib/preinit/81_set_root_part
Álvaro Fernández Rojas <noltari at gmail.com>  2024-03-04 07:28:57

The authors are all members of the project and should therefore be pretty
knowledgeable about OpenWRT.  If even members of the project are getting
it wrong, that seems strong evidence the existing approach doesn't work
well.

Add us and I think we've got a consensus the `break` really should be
removed and checking in every script should be used.  Though there are
the other alternative approaches below.


> > Perhaps the files should have a tag inside them and during build scripts
> > which require initramfs should be conditionally enabled?
> 
> What exactly do you mean by that? Should the scripts always be installed,
> but if an initramfs is built, then the hook line is deleted?

I was thinking something along the lines of having a in-comment tag for
scripts which should be omitted from initramfs.  Perhaps
"# INITRAMFS_OMIT" and "# INITRAMFS_INCLUDE"?

When building the initramfs use `grep -l` or `grep -L` to identify files
which should (not) be included.


> > Perhaps base-files should be split into base-files, base-files-initramfs
> > and base-files-noinitramfs?
> 
> This would be a big change from my point of view, because in base-files
> are not only the preinit scripts but also many other things. We would
> have to maintain this redundantly, if I understand your intention correctly

This might be too big of a change for now.  This may well be a bad idea
right now and in the future.

> > In other news 18 of the 29 [789]* files are named "79_move_config"
> > inside
> > target/linux.  This makes me wonder how similar the job they're doing
> > is.
> > Perhaps these should all merge into a single file in package/base-files.
> 
> This would make sense if the scripts were all the same, but on closer
> look they are all different and are target/subtarget specific. Therefore,
> from my point of view, it makes no sense to move them to the base-files.

Indeed.  A quick glance suggested many are pretty similar and I wonder if
it would be possible to write a single script which could replace all of
them.  I suspect most of them started as copies and they've simply been
slowly drifting apart.


-- 
(\___(\___(\______          --=> 8-) EHM <=--          ______/)___/)___/)
 \BS (    |         ehem+sigmsg at m5p.com  PGP 87145445         |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445