| Message ID | SN4P221MB06825C5F564A59CA4DE16C09A0729@SN4P221MB0682.NAMP221.PROD.OUTLOOK.COM |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [1/2] package/skeleton-init-systemd: copy over etc-factory content | expand |
James, All, On 2023-05-05 00:30 -0400, James Knight spake thusly: > The following commit tweaks the `skeleton-init-systemd` package to > gracefully handle if a user invokes a reinstall for the package (e.g. > when performing development tweaks to the initial skeleton). > > Signed-off-by: James Knight <james.d.knight@live.com> > --- > Only a suggestion; feel free to drop. I've spent a bit of time on this, and was about to apply... However, there are a few "issues" I noticed with it, mostly that /etc/fstab should not be used with systemd, but mount units should be used, and so /etc/fstab should only contain this one line, so it does not make much sense to have it "reinstallable". Also, with BR2_PER_PACKAGE_DIRECTORIES=y, when the "final" target is aggregated there is no guarantee that the modified file gets used in this case. Indeed, assuming the following dependency chain (simplified for the sake of the explanation): S <-- A <-- target/ `<-- Z <--' I.e. A depends on S (the skeleton), Z also depends on S, and the target is aggreagated from A and Z, and S. When you first build, Z inherits the fstab from S, and because it sorts after S, that the one that gets used. If you rebuild S, Z will not get rebuild, so will not get the new fstab. And thus when you assemble target after the rebuild, you still get the old file from Z, not the new one from S. (there are more complex cases, the above is just a trivial example). Finally, the only guarantee Buildroot really offers is fof a full build from scratch. And last, reinstalling a skeleton package is very rare, so fixing it is not worth the effort, considering all the above. So I just decided to drop it. Regards, Yann E. MORIN. > --- > .../skeleton-init-systemd.mk | 18 ++++++++++++++---- > 1 file changed, 14 insertions(+), 4 deletions(-) > > diff --git a/package/skeleton-init-systemd/skeleton-init-systemd.mk b/package/skeleton-init-systemd/skeleton-init-systemd.mk > index 4076821c0c0429cf90681f4b16be114c44bde282..18ae91f2eea30277805ee82791c62d53f24dca1e 100644 > --- a/package/skeleton-init-systemd/skeleton-init-systemd.mk > +++ b/package/skeleton-init-systemd/skeleton-init-systemd.mk > @@ -18,13 +18,23 @@ SKELETON_INIT_SYSTEMD_PROVIDES = skeleton > ifeq ($(BR2_TARGET_GENERIC_REMOUNT_ROOTFS_RW),y) > > define SKELETON_INIT_SYSTEMD_ROOT_RO_OR_RW > - echo "/dev/root / auto rw 0 1" >$(TARGET_DIR)/etc/fstab > + if ! grep -q "/dev/root" $(TARGET_DIR)/etc/fstab; then \ > + echo "/dev/root / auto rw 0 1" >$(TARGET_DIR)/etc/fstab; \ > + else \ > + sed -i "s|/dev/root.*|/dev/root / auto rw 0 1|g" \ > + $(TARGET_DIR)/etc/fstab; \ > + fi > endef > > else > > define SKELETON_INIT_SYSTEMD_ROOT_RO_OR_RW > - echo "/dev/root / auto ro 0 1" >$(TARGET_DIR)/etc/fstab > + if ! grep -q "/dev/root" $(TARGET_DIR)/etc/fstab; then \ > + echo "/dev/root / auto ro 0 1" >$(TARGET_DIR)/etc/fstab; \ > + else \ > + sed -i "s|/dev/root.*|/dev/root / auto ro 0 1|g" \ > + $(TARGET_DIR)/etc/fstab; \ > + fi > endef > > # On a R/O rootfs, /var is a tmpfs filesystem. So, at build time, we > @@ -78,9 +88,9 @@ define SKELETON_INIT_SYSTEMD_INSTALL_TARGET_CMDS > mkdir -p $(TARGET_DIR)/home > mkdir -p $(TARGET_DIR)/srv > mkdir -p $(TARGET_DIR)/var > - ln -s ../run $(TARGET_DIR)/var/run > + ln -sf ../run $(TARGET_DIR)/var/run > # prevent install scripts to create var/lock as directory > - ln -s ../run/lock $(TARGET_DIR)/var/lock > + ln -sf ../run/lock $(TARGET_DIR)/var/lock > install -D -m644 $(SKELETON_INIT_SYSTEMD_PKGDIR)/legacy.conf $(TARGET_DIR)/usr/lib/tmpfiles.d/legacy.conf > $(SKELETON_INIT_SYSTEMD_ROOT_RO_OR_RW) > endef > -- > 2.40.1.windows.1 > > _______________________________________________ > buildroot mailing list > buildroot@buildroot.org > https://lists.buildroot.org/mailman/listinfo/buildroot
On 01/10/2023 18:05, Yann E. MORIN wrote: > James, All, > > On 2023-05-05 00:30 -0400, James Knight spake thusly: >> The following commit tweaks the `skeleton-init-systemd` package to >> gracefully handle if a user invokes a reinstall for the package (e.g. >> when performing development tweaks to the initial skeleton). >> >> Signed-off-by: James Knight <james.d.knight@live.com> >> --- >> Only a suggestion; feel free to drop. > > I've spent a bit of time on this, and was about to apply... > > However, there are a few "issues" I noticed with it, mostly that > /etc/fstab should not be used with systemd, but mount units should be Really? The mount unit man page [1] says: "In general, configuring mount points through /etc/fstab is the preferred approach to manage mounts for humans. For tooling, writing mount units should be preferred over editing /etc/fstab." I'm not sure if the skeleton counts as tooling or as a human. Hm, probably tooling, indeed. > used, and so /etc/fstab should only contain this one line, so it does Well, no, if we say that the skeleton is tooling, it shouldn't contain that line at all; instead, there should be a mount unit to remount rw (and no mount unit at all for ro). > not make much sense to have it "reinstallable". [snip] >> diff --git a/package/skeleton-init-systemd/skeleton-init-systemd.mk b/package/skeleton-init-systemd/skeleton-init-systemd.mk >> index 4076821c0c0429cf90681f4b16be114c44bde282..18ae91f2eea30277805ee82791c62d53f24dca1e 100644 >> --- a/package/skeleton-init-systemd/skeleton-init-systemd.mk >> +++ b/package/skeleton-init-systemd/skeleton-init-systemd.mk >> @@ -18,13 +18,23 @@ SKELETON_INIT_SYSTEMD_PROVIDES = skeleton >> ifeq ($(BR2_TARGET_GENERIC_REMOUNT_ROOTFS_RW),y) >> >> define SKELETON_INIT_SYSTEMD_ROOT_RO_OR_RW >> - echo "/dev/root / auto rw 0 1" >$(TARGET_DIR)/etc/fstab The original here _was_ already "reinstallable" because it just overwrites fstab. If there is any other package that writes fstab, it's wrong anyway. Unless it's done in the overlay or post-build-script, but the overlay/script anyway still gets applied after rebuild. >> + if ! grep -q "/dev/root" $(TARGET_DIR)/etc/fstab; then \ >> + echo "/dev/root / auto rw 0 1" >$(TARGET_DIR)/etc/fstab; \ >> + else \ >> + sed -i "s|/dev/root.*|/dev/root / auto rw 0 1|g" \ >> + $(TARGET_DIR)/etc/fstab; \ >> + fi >> endef >> >> else >> >> define SKELETON_INIT_SYSTEMD_ROOT_RO_OR_RW >> - echo "/dev/root / auto ro 0 1" >$(TARGET_DIR)/etc/fstab >> + if ! grep -q "/dev/root" $(TARGET_DIR)/etc/fstab; then \ >> + echo "/dev/root / auto ro 0 1" >$(TARGET_DIR)/etc/fstab; \ >> + else \ >> + sed -i "s|/dev/root.*|/dev/root / auto ro 0 1|g" \ >> + $(TARGET_DIR)/etc/fstab; \ >> + fi >> endef >> >> # On a R/O rootfs, /var is a tmpfs filesystem. So, at build time, we >> @@ -78,9 +88,9 @@ define SKELETON_INIT_SYSTEMD_INSTALL_TARGET_CMDS >> mkdir -p $(TARGET_DIR)/home >> mkdir -p $(TARGET_DIR)/srv >> mkdir -p $(TARGET_DIR)/var >> - ln -s ../run $(TARGET_DIR)/var/run >> + ln -sf ../run $(TARGET_DIR)/var/run IMHO this part is simply a good idea. Regards, Arnout [1] https://www.freedesktop.org/software/systemd/man/systemd.mount.html >> # prevent install scripts to create var/lock as directory >> - ln -s ../run/lock $(TARGET_DIR)/var/lock >> + ln -sf ../run/lock $(TARGET_DIR)/var/lock >> install -D -m644 $(SKELETON_INIT_SYSTEMD_PKGDIR)/legacy.conf $(TARGET_DIR)/usr/lib/tmpfiles.d/legacy.conf >> $(SKELETON_INIT_SYSTEMD_ROOT_RO_OR_RW) >> endef >> -- >> 2.40.1.windows.1 >> >> _______________________________________________ >> buildroot mailing list >> buildroot@buildroot.org >> https://lists.buildroot.org/mailman/listinfo/buildroot >
diff --git a/package/skeleton-init-systemd/skeleton-init-systemd.mk b/package/skeleton-init-systemd/skeleton-init-systemd.mk index 4076821c0c0429cf90681f4b16be114c44bde282..18ae91f2eea30277805ee82791c62d53f24dca1e 100644 --- a/package/skeleton-init-systemd/skeleton-init-systemd.mk +++ b/package/skeleton-init-systemd/skeleton-init-systemd.mk @@ -18,13 +18,23 @@ SKELETON_INIT_SYSTEMD_PROVIDES = skeleton ifeq ($(BR2_TARGET_GENERIC_REMOUNT_ROOTFS_RW),y) define SKELETON_INIT_SYSTEMD_ROOT_RO_OR_RW - echo "/dev/root / auto rw 0 1" >$(TARGET_DIR)/etc/fstab + if ! grep -q "/dev/root" $(TARGET_DIR)/etc/fstab; then \ + echo "/dev/root / auto rw 0 1" >$(TARGET_DIR)/etc/fstab; \ + else \ + sed -i "s|/dev/root.*|/dev/root / auto rw 0 1|g" \ + $(TARGET_DIR)/etc/fstab; \ + fi endef else define SKELETON_INIT_SYSTEMD_ROOT_RO_OR_RW - echo "/dev/root / auto ro 0 1" >$(TARGET_DIR)/etc/fstab + if ! grep -q "/dev/root" $(TARGET_DIR)/etc/fstab; then \ + echo "/dev/root / auto ro 0 1" >$(TARGET_DIR)/etc/fstab; \ + else \ + sed -i "s|/dev/root.*|/dev/root / auto ro 0 1|g" \ + $(TARGET_DIR)/etc/fstab; \ + fi endef # On a R/O rootfs, /var is a tmpfs filesystem. So, at build time, we @@ -78,9 +88,9 @@ define SKELETON_INIT_SYSTEMD_INSTALL_TARGET_CMDS mkdir -p $(TARGET_DIR)/home mkdir -p $(TARGET_DIR)/srv mkdir -p $(TARGET_DIR)/var - ln -s ../run $(TARGET_DIR)/var/run + ln -sf ../run $(TARGET_DIR)/var/run # prevent install scripts to create var/lock as directory - ln -s ../run/lock $(TARGET_DIR)/var/lock + ln -sf ../run/lock $(TARGET_DIR)/var/lock install -D -m644 $(SKELETON_INIT_SYSTEMD_PKGDIR)/legacy.conf $(TARGET_DIR)/usr/lib/tmpfiles.d/legacy.conf $(SKELETON_INIT_SYSTEMD_ROOT_RO_OR_RW) endef
The following commit tweaks the `skeleton-init-systemd` package to gracefully handle if a user invokes a reinstall for the package (e.g. when performing development tweaks to the initial skeleton). Signed-off-by: James Knight <james.d.knight@live.com> --- Only a suggestion; feel free to drop. --- .../skeleton-init-systemd.mk | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-)