fs: cleanups and enhancements (branch yem/fs)

Hello All!

This series introduces some cleaups and enhancements to the filesystem infrastructure.

Most notably, it gets rid of the post-target rules, i.e. rules defined by filesystems that were to be executed after the image was generated, in favour of post-gen hooks.

The reason for getting rid of post-target rules is that they were not really needed, and the way they were used was dubious when dealing with top-level parallel builds. Also, the sole user for which those rules were documented to be useful was not even using the filesystem infra.

The series also introduces the concept of a "landing" area for the target directory.

This landing area is a copy of the currently-known target/ directory, copy in which all the target-finalize step and hooks are run, leaving the original target/ directory to contain only what packages have installed.

Please read the commit log for the final patch for more on that landing area.

To be noted: patch 8/9 was the beginning of a bigger plan to rework how images are generated. However, and quite sadly, that bigger plan is now evading my recollection. It's sad to get old... ;-] Nevertheless, the patch by itself still makes sense, so I kept it.

Regards,
Yann E. MORIN.

The following changes since commit 803ed997fb2eca841ffafa047b194cf322f766df:

flashrom: fix static build (2015-09-30 22:56:41 +0200)

are available in the git repository at:

git://git.busybox.net/~ymorin/git/buildroot yem/fs

for you to fetch changes up to a594ac3858ea713de72610ad49566be4e20c176f:

core: finalise target in its own location (2015-09-30 23:53:09 +0200)

----------------------------------------------------------------
Yann E. MORIN (9):
core: sort packages and eliminate duplicates before building
linux: split overly-long dependency line for readability
linux: meddle not in the affairs of filesystems, for you are tasty with bacon fs/initramfs: cleanup and enhance comments
fs/ext2: use a post-gen hook rather than a post-target rule fs/cpio: use a post-gen hook rather than a post-target rule fs/common: get rid of post-target rules
fs/common: move actions common to all filesystems to their own rule
core: finalise target in its own location

 Makefile                  | 34 +++++++++++++++++++++++-
 fs/common.mk              | 67 +++++++++++++++++++++++++----------------------
 fs/cpio/cpio.mk           | 11 ++++----
 fs/ext2/ext2.mk           | 10 +++----
 fs/initramfs/initramfs.mk | 22 +++++++++++-----
 linux/linux.mk            |  5 +++-
 6 files changed, 97 insertions(+), 52 deletions(-)

.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

Currently, enabling more than one filesystem image will make 'show-targets' list a few host packages more than once.

This is because all filesystem images add the same set of host-packages to their dependencies, which are then added as-is to the package list.

Thus, host-fakeroot, host-makedevs and, if needed, host-mkpasswd will appear as many times as there are filesystem images enabled.

Fix that by sorting the package list, thus eliminating duplicates from that list.

Furthermore, and even though we're already sorting packages by scanning the .mk file in alphabetical order (because we $(sort) them), sorting the list itself will further guarantee the build order, thus enhancing reproducibility (and $(sort ...) is not dependent on the locale, it always sorts with the C locale).

---
 Makefile | 6 ++++++
 1 file changed, 6 insertions(+)

index fdbca02..b3f519e 100644
@@ -556,6 +556,12 @@ endif
 
 $(TARGETS_ROOTFS): target-finalize
 
+# Filesystems may add the same pacakges more than once from their
+# dependency lists. For example, all filesystems include host-fakeroot,
+# host-makedevs and, if required, host-mkpasswd. They would appear
+# multiple times in the list returned by show-target.
+PACKAGES := $(sort $(PACKAGES))
+
 target-finalize: $(PACKAGES)
 	@$(call MESSAGE,"Finalizing target directory")
 	$(foreach hook,$(TARGET_FINALIZE_HOOKS),$($(hook))$(sep))

1.9.1

Yann,

Is this a problem? I don't think "show-targets" aims at guaranteeing that each target will only be listed once.

Thomas

Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Thomas, All,

On 2015-10-01 08:19 +0200, Thomas Petazzoni spake thusly:

Well, it's not a real, hard problem. It's just not clean.

However, the part I'm most interested in is to sort the packages list, so we have ven more reproducible builds. Maybe I should have articulated the commit log to stress more on that reason rather than on show-targets.

Regards,
Yann E. MORIN.

.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

---
This is going to be useful to better understand the initramfs patch coming next.

---
 linux/linux.mk | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

index bbcc54b..316f973 100644
@@ -390,7 +390,10 @@ $(eval $(kconfig-package))
 
 # Support for rebuilding the kernel after the cpio archive has
 # been generated in $(BINARIES_DIR)/rootfs.cpio.
-$(LINUX_DIR)/.stamp_initramfs_rebuilt: $(LINUX_DIR)/.stamp_target_installed $(LINUX_DIR)/.stamp_images_installed $(BINARIES_DIR)/rootfs.cpio
+$(LINUX_DIR)/.stamp_initramfs_rebuilt: $(LINUX_DIR)/.stamp_target_installed
+$(LINUX_DIR)/.stamp_initramfs_rebuilt: $(LINUX_DIR)/.stamp_images_installed
+$(LINUX_DIR)/.stamp_initramfs_rebuilt: $(BINARIES_DIR)/rootfs.cpio
+$(LINUX_DIR)/.stamp_initramfs_rebuilt:
 	@$(call MESSAGE,"Rebuilding kernel with initramfs")
 	# Build the kernel.
 	$(LINUX_MAKE_ENV) $(MAKE) $(LINUX_MAKE_FLAGS) -C $(@D) $(LINUX_TARGET_NAME)

1.9.1

Currently, the rule to rebuild the Linux kernel with an initramfs directly depends on the filesystem image filename.

This is inherently "bad" from a purity point of view. linux.mk should not have to delve into the fs internals.

Rather, make it directly depend on the "frontal" rule that generates the cpio image.

---
This, too, will be useful for the initramfs patch coming next.

---
 linux/linux.mk | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 316f973..864ccbf 100644
@@ -392,7 +392,7 @@ $(eval $(kconfig-package))
 # been generated in $(BINARIES_DIR)/rootfs.cpio.
 $(LINUX_DIR)/.stamp_initramfs_rebuilt: $(LINUX_DIR)/.stamp_target_installed
 $(LINUX_DIR)/.stamp_initramfs_rebuilt: $(LINUX_DIR)/.stamp_images_installed
-$(LINUX_DIR)/.stamp_initramfs_rebuilt: $(BINARIES_DIR)/rootfs.cpio
+$(LINUX_DIR)/.stamp_initramfs_rebuilt: rootfs-cpio
 $(LINUX_DIR)/.stamp_initramfs_rebuilt:
 	@$(call MESSAGE,"Rebuilding kernel with initramfs")
 	# Build the kernel.

1.9.1

Yann,

Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Thomas, All,

On 2015-10-01 08:20 +0200, Thomas Petazzoni spake thusly:

Well, that's alredy the behaviour we have: we always rebuild the images on each make invocation. This patch does not change the current behaviour.

And indeed, that's what we do want. For the simple reason that he user may manually change the content of target/ and re-run make to update the images.

And anyway, the filesystem image files are already depending on a PHONY target, and thus are always rebuilt:

    fs/common.mk:
      73 $$(BINARIES_DIR)/rootfs.$(1): target-finalize $$(ROOTFS_$(2)_DEPENDENCIES)

However, you are right that the dependency of a rule target onto a PHONY target is not clean. And in fact, there is not reason for the stamp-file to begin with. We can coalesce the recipe of the stamp-file into the PHONY linux-rebuild-with-initramfs rule.

I'll update the patch to drop the stamp-file, unless you can come up with a reason for its existence. ;-)

Regards,
Yann E. MORIN.

.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

rootfs-nitramfs is not using the generic fs infrastructure, because there is virtually nothing to do to build the initramfs image: there is no actual image to be built to begin with.

The only purpose of rootfs-initramfs is to ensure the rootfs.cpio image is built and then that the Linux kernel is rebuilt with that rootfs.cpio as initramfs source.

Using variables of the fs infra like if it were used is misleading. It looked nice as long as there was the possibility that rootfs-initramfs would one day use the fs infra. But there's no way that will happen any time soon.

Furthermore, the linux' rule linux-rebuild-with-initramfs now already depends on rootfs-cpio by itself, so we need not duplicate this dependency in rootfs-initramfs.

Still, we want to advertise that the dependency is on rootfs-cpio, so we get nice dependency graphs (and not expose the internal linux-rebuild-with-initramfs rule to the users).

So, remove the variables and directly define the rules.

---
 fs/initramfs/initramfs.mk | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

index db50812..9aca9c8 100644
@@ -5,17 +5,25 @@
 #
 ################################################################################
 
-ROOTFS_INITRAMFS_DEPENDENCIES += rootfs-cpio
-
-ROOTFS_INITRAMFS_POST_TARGETS += linux-rebuild-with-initramfs
-
-
 # The generic fs infrastructure isn't very useful here.
+#
+# The initramfs image does not actually build an image; its only purpose is:
+# 1- to ensure rootfs.cpio is generated,
+# 2- to then rebuild the kernel with rootfs.cpio as initramfs
+#
+# Note: ordering of the dependencies is not guaranteed here, but in
+# linux/linux.mk, via the .stamp_initramfs_rebuilt stamp file, which depends
+# on the rootfs-cpio filesystem rule.
+#
+# Note: the trick here is that we directly depend on rebuilding the Linux
+# kernel image (which itself depends on the rootfs-cpio rule), while we
+# advertise that our dependency is on the rootfs-cpio rule, which is
+# cleaner in the dependency graph.
 
-rootfs-initramfs: $(ROOTFS_INITRAMFS_DEPENDENCIES) $(ROOTFS_INITRAMFS_POST_TARGETS)
+rootfs-initramfs: linux-rebuild-with-initramfs
 
 rootfs-initramfs-show-depends:
-	@echo $(ROOTFS_INITRAMFS_DEPENDENCIES)
+	@echo rootfs-cpio
 
 .PHONY: rootfs-initramfs rootfs-initramfs-show-depends
 

1.9.1

post-target rules are probably not resilient to parallel builds, given that they do not depend on the image being generated first.

Beside, we already have a mechanism for running stuff after the filesystem is generated, and that's called post-gen hooks.

Use those hooks.

Note: this basically reverts 75b6303 (rootfs-ext2: make the symlink as a _POST_TARGET) since we've now re-introduced post-gen hooks.

---
 fs/ext2/ext2.mk | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

index cab66a5..312b7a0 100644
@@ -32,13 +32,11 @@ define ROOTFS_EXT2_CMD
 	PATH=$(BR_PATH) mke2img -d $(TARGET_DIR) $(EXT2_OPTS) -o $@
 endef
 
-rootfs-ext2-symlink:
-	ln -sf rootfs.ext2$(ROOTFS_EXT2_COMPRESS_EXT) $(BINARIES_DIR)/rootfs.ext$(BR2_TARGET_ROOTFS_EXT2_GEN)$(ROOTFS_EXT2_COMPRESS_EXT)
-
-.PHONY: rootfs-ext2-symlink
-
 ifneq ($(BR2_TARGET_ROOTFS_EXT2_GEN),2)
-ROOTFS_EXT2_POST_TARGETS += rootfs-ext2-symlink
+define ROOTFS_EXT2_SYMLINK
+	ln -sf rootfs.ext2$(ROOTFS_EXT2_COMPRESS_EXT) $(BINARIES_DIR)/rootfs.ext$(BR2_TARGET_ROOTFS_EXT2_GEN)$(ROOTFS_EXT2_COMPRESS_EXT)
+endef
+ROOTFS_EXT2_POST_GEN_HOOKS += ROOTFS_EXT2_SYMLINK
 endif
 
 $(eval $(call ROOTFS_TARGET,ext2))

1.9.1

We already have a mechanism for running stuff after the filesystem is generated, and that's called post-gen hooks.

Use those hooks.

Note: for cpio (and unlike ext2 previously), the dependency chain was correct, in that the post-target rule correctly depended on the image rule. Nonetheless, we still want to fix it for consistency.

---
 fs/cpio/cpio.mk | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

index e82167e..c68e0bf 100644
@@ -31,12 +31,13 @@ define ROOTFS_CPIO_CMD
 	cd $(TARGET_DIR) && find . | cpio --quiet -o -H newc > $@
 endef
 
-$(BINARIES_DIR)/rootfs.cpio.uboot: $(BINARIES_DIR)/rootfs.cpio host-uboot-tools
-	$(MKIMAGE) -A $(MKIMAGE_ARCH) -T ramdisk \
-		-C none -d $<$(ROOTFS_CPIO_COMPRESS_EXT) $@
-
 ifeq ($(BR2_TARGET_ROOTFS_CPIO_UIMAGE),y)
-ROOTFS_CPIO_POST_TARGETS += $(BINARIES_DIR)/rootfs.cpio.uboot
+ROOTFS_CPIO_DEPENDENCIES += host-uboot-tools
+define ROOTFS_CPIO_UBOOT_MKIMAGE
+	$(MKIMAGE) -A $(MKIMAGE_ARCH) -T ramdisk \
+		-C none -d $@$(ROOTFS_CPIO_COMPRESS_EXT) [email protected]
+endef
+ROOTFS_CPIO_POST_GEN_HOOKS += ROOTFS_CPIO_UBOOT_MKIMAGE
 endif
 
 $(eval $(call ROOTFS_TARGET,cpio))

1.9.1

The only users of post-target rules were ext2, cpio and initramfs.

Of those, ext2 and cpio were changed to use post-gen hooks, while initramfs was not even using the generic rootfs infra and was fixed to no longer reference post-target rules.

Besides, the comment in the infra was really misleading: it referenced initramfs implying it was the sole user of that feature, even though initramfs was not using the fs infra.

Furthermore, using post-target rules was inherently broken for top-level parallel builds, because filesystems had to ensure the ordering by themselves. Of the two real users of post-target rules (cpio and ext2), one did enforce rules ordering (apparently correctly), while the other forgot to do so.

We can get rid of post-target rules altogether, now.

---
 fs/common.mk | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

index 528e194..28b6021 100644
@@ -19,10 +19,6 @@
 #  ROOTFS_$(FSTYPE)_POST_GEN_HOOKS, a list of hooks to call after
 #  generating the filesystem image
 #
-#  ROOTFS_$(FSTYPE)_POST_TARGETS, the list of targets that should be
-#  run after running the main filesystem target. This is useful for
-#  initramfs, to rebuild the kernel once the initramfs is generated.
-#
 # In terms of configuration option, this macro assumes that the
 # BR2_TARGET_ROOTFS_$(FSTYPE) config option allows to enable/disable
 # the generation of a filesystem image of a particular type. If

@@ -104,7 +100,7 @@ endif
 rootfs-$(1)-show-depends:
 	@echo $$(ROOTFS_$(2)_DEPENDENCIES)
 
-rootfs-$(1): $$(BINARIES_DIR)/rootfs.$(1) $$(ROOTFS_$(2)_POST_TARGETS)
+rootfs-$(1): $$(BINARIES_DIR)/rootfs.$(1)
 
 .PHONY: rootfs-$(1) rootfs-$(1)-show-depends
 

1.9.1

A lot of actions are common to generating the various images. Currently, they are all done for each image being generated.

However, we can do them once and for all.

Also add a sha-bang to the fakeroot script, for completeness.

---
Note: when I wrote that patch, it was only a preparatory work for something bigger and clever about how images are generated. However, I can't recall what it was... :-( Yet, that patch is still valid on its own.

---
 fs/common.mk | 57 ++++++++++++++++++++++++++++++++-------------------------
 1 file changed, 32 insertions(+), 25 deletions(-)

index 28b6021..9b544eb 100644
@@ -27,13 +27,36 @@
 # BR2_TARGET_ROOTFS_$(FSTYPE)_LZMA exist and are enabled, then the
 # macro will automatically generate a compressed filesystem image.
 
-FAKEROOT_SCRIPT = $(BUILD_DIR)/_fakeroot.fs
-FULL_DEVICE_TABLE = $(BUILD_DIR)/_device_table.txt
+FS_DIR = $(BUILD_DIR)/fs-common
+FAKEROOT_SCRIPT = $(FS_DIR)/fakeroot.common-base
+FULL_DEVICE_TABLE = $(FS_DIR)/device_table.txt
 ROOTFS_DEVICE_TABLES = $(call qstrip,$(BR2_ROOTFS_DEVICE_TABLE) \
 	$(BR2_ROOTFS_STATIC_DEVICE_TABLE))
-USERS_TABLE = $(BUILD_DIR)/_users_table.txt
+USERS_TABLE = $(FS_DIR)/users_table.txt
 ROOTFS_USERS_TABLES = $(call qstrip,$(BR2_ROOTFS_USERS_TABLES))
 
+.PHONY: rootfs-common
+rootfs-common: target-finalize
+	mkdir -p "$(FS_DIR)"
+	rm -f $(FAKEROOT_SCRIPT)
+	rm -f $(USERS_TABLE)
+	echo "#!/bin/sh" > $(FAKEROOT_SCRIPT)
+	echo "set -e" >> $(FAKEROOT_SCRIPT)
+	echo "chown -h -R 0:0 $(TARGET_DIR)" >> $(FAKEROOT_SCRIPT)
+ifneq ($(ROOTFS_DEVICE_TABLES),)
+	cat $(ROOTFS_DEVICE_TABLES) > $(FULL_DEVICE_TABLE)
+ifeq ($(BR2_ROOTFS_DEVICE_CREATION_STATIC),y)
+	printf '$(subst $(sep),\n,$(PACKAGES_DEVICES_TABLE))' >> $(FULL_DEVICE_TABLE)
+endif
+	printf '$(subst $(sep),\n,$(PACKAGES_PERMISSIONS_TABLE))' >> $(FULL_DEVICE_TABLE)
+	echo "$(HOST_DIR)/usr/bin/makedevs -d $(FULL_DEVICE_TABLE) $(TARGET_DIR)" >> $(FAKEROOT_SCRIPT)
+endif
+ifneq ($(ROOTFS_USERS_TABLES),)
+	cat $(ROOTFS_USERS_TABLES) >> $(USERS_TABLE)
+endif
+	printf '$(subst $(sep),\n,$(PACKAGES_USERS))' >> $(USERS_TABLE)
+	PATH=$(BR_PATH) $(TOPDIR)/support/scripts/mkusers $(USERS_TABLE) $(TARGET_DIR) >> $(FAKEROOT_SCRIPT)
+
 # Since this function will be called from within an $(eval ...)
 # all variable references except the arguments must be $$-quoted.
 define ROOTFS_TARGET_INTERNAL

@@ -66,32 +89,16 @@ ROOTFS_$(2)_COMPRESS_EXT = .xz
 ROOTFS_$(2)_COMPRESS_CMD = xz -9 -C crc32 -c
 endif
 
-$$(BINARIES_DIR)/rootfs.$(1): target-finalize $$(ROOTFS_$(2)_DEPENDENCIES)
+ROOTFS_$(2)_FAKEROOT_SCRIPT = $$(FS_DIR)/fakeroot.$(1)
+
+$$(BINARIES_DIR)/rootfs.$(1): rootfs-common $$(ROOTFS_$(2)_DEPENDENCIES)
 	@$$(call MESSAGE,"Generating root filesystem image rootfs.$(1)")
 	$$(foreach hook,$$(ROOTFS_$(2)_PRE_GEN_HOOKS),$$(call $$(hook))$$(sep))
-	rm -f $$(FAKEROOT_SCRIPT)
+	$$(INSTALL) -D -m 0755 $$(FAKEROOT_SCRIPT) $$(ROOTFS_$(2)_FAKEROOT_SCRIPT)
+	echo "$$(ROOTFS_$(2)_CMD)" >> $$(ROOTFS_$(2)_FAKEROOT_SCRIPT)
 	rm -f $$(TARGET_DIR_WARNING_FILE)
-	rm -f $$(USERS_TABLE)
-	echo "set -e" >> $$(FAKEROOT_SCRIPT)
-	echo "chown -h -R 0:0 $$(TARGET_DIR)" >> $$(FAKEROOT_SCRIPT)
-ifneq ($$(ROOTFS_DEVICE_TABLES),)
-	cat $$(ROOTFS_DEVICE_TABLES) > $$(FULL_DEVICE_TABLE)
-ifeq ($$(BR2_ROOTFS_DEVICE_CREATION_STATIC),y)
-	printf '$$(subst $$(sep),\n,$$(PACKAGES_DEVICES_TABLE))' >> $$(FULL_DEVICE_TABLE)
-endif
-	printf '$$(subst $$(sep),\n,$$(PACKAGES_PERMISSIONS_TABLE))' >> $$(FULL_DEVICE_TABLE)
-	echo "$$(HOST_DIR)/usr/bin/makedevs -d $$(FULL_DEVICE_TABLE) $$(TARGET_DIR)" >> $$(FAKEROOT_SCRIPT)
-endif
-ifneq ($$(ROOTFS_USERS_TABLES),)
-	cat $$(ROOTFS_USERS_TABLES) >> $$(USERS_TABLE)
-endif
-	printf '$$(subst $$(sep),\n,$$(PACKAGES_USERS))' >> $$(USERS_TABLE)
-	PATH=$$(BR_PATH) $$(TOPDIR)/support/scripts/mkusers $$(USERS_TABLE) $$(TARGET_DIR) >> $$(FAKEROOT_SCRIPT)
-	echo "$$(ROOTFS_$(2)_CMD)" >> $$(FAKEROOT_SCRIPT)
-	chmod a+x $$(FAKEROOT_SCRIPT)
-	PATH=$$(BR_PATH) $$(HOST_DIR)/usr/bin/fakeroot -- $$(FAKEROOT_SCRIPT)
+	PATH=$$(BR_PATH) $$(HOST_DIR)/usr/bin/fakeroot -- $$(ROOTFS_$(2)_FAKEROOT_SCRIPT)
 	$$(INSTALL) -m 0644 support/misc/target-dir-warning.txt $$(TARGET_DIR_WARNING_FILE)
-	- at rm -f $$(FAKEROOT_SCRIPT) $$(FULL_DEVICE_TABLE)
 ifneq ($$(ROOTFS_$(2)_COMPRESS_CMD),)
 	PATH=$$(BR_PATH) $$(ROOTFS_$(2)_COMPRESS_CMD) $$@ > $$@$$(ROOTFS_$(2)_COMPRESS_EXT)
 endif

1.9.1

Currently, after all packages have been installed into target/ , we run a sanitising pass called 'target-finalize' on that directory, to:
- apply overlays
- remove unnecessary files (man, .h, .a ...)
- strip files
as well as a few other miscellanous cleanups.

This means that target/ no longer contains only package-installed files, and that target-finalize might not be idempotent (i.e. sucessive runs of target-finalize may yield different results in target/ ). We're trying pretty hard that all the internal target-finalize hooks are idempotent, whether they are from the core (e.g. installing glibc locales) or provided by packages (e.g. cleaning up perl files).

However, that might not be the case for packages from br2-external for example, or under complex situations where a combination of packages does not yield an idempotent sequence (quoting Wikipedia: "a combination of idempotent methods or subroutines is not necessrily idempotent"; see:
https://en.wikipedia.org/wiki/Idempotence#Examples ).

Address this issue by copying target/ to a "landing" area where the finalising takes place.

This keeps the user-visible target/ directory to contain only what packages have installed, helps keeping target-finalize be idempotent by allowing packages to provide simpler target-finalize hooks.

For simplicity for packages, we have to allow them to use $(TARGET_DIR) everywhere, be it in target install commands or target finalize commands, without requiring them to know whether to use $(TARGET_DIR) or $(FINAL_TARGET_DIR). $(TARGET_DIR) always points to the directory in which to act.

So, for target-finalize, we override TARGET_DIR to point to the landing area. But since packages are dependencies of target-finalize, they would also inherit from this override. So, we over-override TARGET_DIR for packages, to point to the usual and currently used $(O)/target/ directory.

Finally, filesystem images are generated from that landing area, of course. Similarly, we need to override TARGET_DIR for them.

---
 Makefile     | 28 +++++++++++++++++++++++++++-
 fs/common.mk |  4 ++--
 2 files changed, 29 insertions(+), 3 deletions(-)

index b3f519e..985ed02 100644
@@ -191,7 +191,9 @@ BR_GRAPH_OUT := $(or $(BR2_GRAPH_OUT),pdf)
 
 BUILD_DIR := $(BASE_DIR)/build
 BINARIES_DIR := $(BASE_DIR)/images
-TARGET_DIR := $(BASE_DIR)/target
+BUILD_TARGET_DIR := $(BASE_DIR)/target
+TARGET_DIR := $(BUILD_TARGET_DIR)
+FINAL_TARGET_DIR := $(BUILD_DIR)/target-finalize
 # initial definition so that 'make clean' works for most users, even without
 # .config. HOST_DIR will be overwritten later when .config is included.
 HOST_DIR := $(BASE_DIR)/host

@@ -562,8 +564,32 @@ $(TARGETS_ROOTFS): target-finalize
 # multiple times in the list returned by show-target.
 PACKAGES := $(sort $(PACKAGES))
 
+# Finalizing the target directory involves:
+#  - applying overlays,
+#  - removing unecessary files (man, .h, .a ...)
+#  - tweaking files installed by packages (like stripping)
+# and miscellanous cleanups.
+#
+# We want to keep the $(O)/target/ directory as a perfect image of
+# what packages have actually installed, so we copy it to a landing
+# location where we'll do those cleanups.
+#
+# Still, we only document $(TARGET_DIR) so packages that want to provide
+# target-finalize hooks will be using that. Thus, we just override that
+# variable with the landing location just for target-finalize.
+#
+# However, since rule-overriden variables are inherited by the dependencies
+# of that rule, we must re-override TARGET_DIR with its original location
+# for packages.
+#
+$(PACKAGES): TARGET_DIR=$(BUILD_TARGET_DIR)
+target-finalize: TARGET_DIR=$(FINAL_TARGET_DIR)
+
 target-finalize: $(PACKAGES)
 	@$(call MESSAGE,"Finalizing target directory")
+	$(Q)rm -rf $(TARGET_DIR)
+	$(Q)cp -a $(BUILD_TARGET_DIR) $(TARGET_DIR)
+	$(Q)rm -f $(TARGET_DIR_WARNING_FILE)
 	$(foreach hook,$(TARGET_FINALIZE_HOOKS),$($(hook))$(sep))
 	rm -rf $(TARGET_DIR)/usr/include $(TARGET_DIR)/usr/share/aclocal \
 		$(TARGET_DIR)/usr/lib/pkgconfig $(TARGET_DIR)/usr/share/pkgconfig \

index 9b544eb..f35bbd6 100644
@@ -36,6 +36,7 @@ USERS_TABLE = $(FS_DIR)/users_table.txt
 ROOTFS_USERS_TABLES = $(call qstrip,$(BR2_ROOTFS_USERS_TABLES))
 
 .PHONY: rootfs-common
+rootfs-common: TARGET_DIR=$(FINAL_TARGET_DIR)
 rootfs-common: target-finalize
 	mkdir -p "$(FS_DIR)"
 	rm -f $(FAKEROOT_SCRIPT)

@@ -91,14 +92,13 @@ endif
 
 ROOTFS_$(2)_FAKEROOT_SCRIPT = $$(FS_DIR)/fakeroot.$(1)
 
+$$(BINARIES_DIR)/rootfs.$(1): TARGET_DIR=$(FINAL_TARGET_DIR)
 $$(BINARIES_DIR)/rootfs.$(1): rootfs-common $$(ROOTFS_$(2)_DEPENDENCIES)
 	@$$(call MESSAGE,"Generating root filesystem image rootfs.$(1)")
 	$$(foreach hook,$$(ROOTFS_$(2)_PRE_GEN_HOOKS),$$(call $$(hook))$$(sep))
 	$$(INSTALL) -D -m 0755 $$(FAKEROOT_SCRIPT) $$(ROOTFS_$(2)_FAKEROOT_SCRIPT)
 	echo "$$(ROOTFS_$(2)_CMD)" >> $$(ROOTFS_$(2)_FAKEROOT_SCRIPT)
-	rm -f $$(TARGET_DIR_WARNING_FILE)
 	PATH=$$(BR_PATH) $$(HOST_DIR)/usr/bin/fakeroot -- $$(ROOTFS_$(2)_FAKEROOT_SCRIPT)
-	$$(INSTALL) -m 0644 support/misc/target-dir-warning.txt $$(TARGET_DIR_WARNING_FILE)
 ifneq ($$(ROOTFS_$(2)_COMPRESS_CMD),)
 	PATH=$$(BR_PATH) $$(ROOTFS_$(2)_COMPRESS_CMD) $$@ > $$@$$(ROOTFS_$(2)_COMPRESS_EXT)
 endif

1.9.1

Hello,

To be honest, my initial reaction is: why? What is the benefit? It's additional complexity (not so much in the code), but in the user visible output directories.

Having idem-potent post-build scripts seems like a good goal to achieve. Your only arguments are:

- "this might not be the case for br2-external", but it doesn't explain why

- "under complex situations where a combination of packages does not yield an idempotent sequence", which doesn't come with a real-life example of such a situation.

So with the current explanation/motivation, I'm inclined to say no to this change. I'd be happy to revise my opinion if there are some clearer benefits to balance the drawbacks of the additional complexity.

Thanks!

Thomas

Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Thomas, All,

On 2015-10-01 23:44 +0200, Thomas Petazzoni spake thusly:

[--SNIP--]

Well, I would say that the user-visible target/ directory, the one we are currently exposing, is still user-visible, and the only one we want to expose to users. The new build/target-finalise/ directory introduced here is not meant to be user-visible. If having that directory in build/ is considered to be user-visible, then I agree this is not that good; we should really have this directory hidden to the user. It is an internal step which the user should not meddle with.

A trivial example of a non-idempotent target-finalise hook could be:

    define FOO_CREATE_MY_USER
        echo "user:x:1234:5678::/home/user:/bin/sh" >>$(TARGET_DIR)/etc/passwd
    endef
    TARGET_FINALIZE_HOOKS += FOO_CREATE_MY_USER

Yes, this is badly written, but we can't expect this kind of situation won't happen in real life, especially with br2-external stuff which we by design can not review. It has hapenned; it will happen again.

Indeed, I have no first-hand example. However, I really said "under complex situations" just because if such a situation exists, it is complex by nature and will be difficult to debug.

Sure, the changes introduced here are not trivial, by a fair margin...

Thanks!

Regards,
Yann E. MORIN.

.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

Hello,

Well I personally dislike precisely the fact that what the user sees in output/target/ is not really what goes into the root filesystem image. It will remain cluttered with header files, static libraries, locales and zillions of other crappy things. Users will wonder what's going on, etc. If there's one thing that users should not see, it is precisely that.

This is trivially fixed with:

1/ Using the proper mechanism for that: the users table.

2/ If it's not about creating a user but something else, then a simple
grep addition will make it work.

Right, but such mistakes are seen quite quickly I believe. The user will soon enough realize that his post-build script or TARGET_FINALIZE_HOOKS is executed each time "make" is called, and that they have to be idempotent.

Sorry for turning down the patch, but I am not a big fan of taking new features just because an unknown, supposedly complex situation by nature, may hypothetically exists.

Thomas

Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com