Buildroot Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [Buildroot] [PATCH 1/9] core: sort packages and eliminate duplicates before building
  2015-09-30 21:54 [Buildroot] [PATCH 0/9] fs: cleanups and enhancements (branch yem/fs) Yann E. MORIN
@ 2015-09-30 21:54 ` Yann E. MORIN
  2015-10-01  6:19   ` Thomas Petazzoni
  2015-09-30 21:54 ` [Buildroot] [PATCH 2/9] linux: split overly-long dependency line for readability Yann E. MORIN
                   ` (7 subsequent siblings)
  8 siblings, 1 reply; 17+ messages in thread
From: Yann E. MORIN @ 2015-09-30 21:54 UTC (permalink / raw)
  To: buildroot

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).

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Arnout Vandecappelle <arnout@mind.be>
Cc: Peter Korsgaard <jacmet@uclibc.org>
---
 Makefile | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Makefile b/Makefile
index fdbca02..b3f519e 100644
--- a/Makefile
+++ b/Makefile
@@ -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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [Buildroot] [PATCH 2/9] linux: split overly-long dependency line for readability
  2015-09-30 21:54 [Buildroot] [PATCH 0/9] fs: cleanups and enhancements (branch yem/fs) Yann E. MORIN
  2015-09-30 21:54 ` [Buildroot] [PATCH 1/9] core: sort packages and eliminate duplicates before building Yann E. MORIN
@ 2015-09-30 21:54 ` Yann E. MORIN
  2015-09-30 21:54 ` [Buildroot] [PATCH 3/9] linux: meddle not in the affairs of filesystems, for you are tasty with bacon Yann E. MORIN
                   ` (6 subsequent siblings)
  8 siblings, 0 replies; 17+ messages in thread
From: Yann E. MORIN @ 2015-09-30 21:54 UTC (permalink / raw)
  To: buildroot

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Arnout Vandecappelle <arnout@mind.be>

---
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(-)

diff --git a/linux/linux.mk b/linux/linux.mk
index bbcc54b..316f973 100644
--- a/linux/linux.mk
+++ b/linux/linux.mk
@@ -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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [Buildroot] [PATCH 3/9] linux: meddle not in the affairs of filesystems, for you are tasty with bacon
  2015-09-30 21:54 [Buildroot] [PATCH 0/9] fs: cleanups and enhancements (branch yem/fs) Yann E. MORIN
  2015-09-30 21:54 ` [Buildroot] [PATCH 1/9] core: sort packages and eliminate duplicates before building Yann E. MORIN
  2015-09-30 21:54 ` [Buildroot] [PATCH 2/9] linux: split overly-long dependency line for readability Yann E. MORIN
@ 2015-09-30 21:54 ` Yann E. MORIN
  2015-10-01  6:20   ` Thomas Petazzoni
  2015-09-30 21:54 ` [Buildroot] [PATCH 4/9] fs/initramfs: cleanup and enhance comments Yann E. MORIN
                   ` (5 subsequent siblings)
  8 siblings, 1 reply; 17+ messages in thread
From: Yann E. MORIN @ 2015-09-30 21:54 UTC (permalink / raw)
  To: buildroot

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.

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Arnout Vandecappelle <arnout@mind.be>

---
This, too, will be useful for the initramfs patch coming next.
---
 linux/linux.mk | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/linux/linux.mk b/linux/linux.mk
index 316f973..864ccbf 100644
--- a/linux/linux.mk
+++ b/linux/linux.mk
@@ -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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [Buildroot] [PATCH 4/9] fs/initramfs: cleanup and enhance comments
  2015-09-30 21:54 [Buildroot] [PATCH 0/9] fs: cleanups and enhancements (branch yem/fs) Yann E. MORIN
                   ` (2 preceding siblings ...)
  2015-09-30 21:54 ` [Buildroot] [PATCH 3/9] linux: meddle not in the affairs of filesystems, for you are tasty with bacon Yann E. MORIN
@ 2015-09-30 21:54 ` Yann E. MORIN
  2015-09-30 21:54 ` [Buildroot] [PATCH 5/9] fs/ext2: use a post-gen hook rather than a post-target rule Yann E. MORIN
                   ` (4 subsequent siblings)
  8 siblings, 0 replies; 17+ messages in thread
From: Yann E. MORIN @ 2015-09-30 21:54 UTC (permalink / raw)
  To: buildroot

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.

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 fs/initramfs/initramfs.mk | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/fs/initramfs/initramfs.mk b/fs/initramfs/initramfs.mk
index db50812..9aca9c8 100644
--- a/fs/initramfs/initramfs.mk
+++ b/fs/initramfs/initramfs.mk
@@ -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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [Buildroot] [PATCH 0/9] fs: cleanups and enhancements (branch yem/fs)
@ 2015-09-30 21:54 Yann E. MORIN
  2015-09-30 21:54 ` [Buildroot] [PATCH 1/9] core: sort packages and eliminate duplicates before building Yann E. MORIN
                   ` (8 more replies)
  0 siblings, 9 replies; 17+ messages in thread
From: Yann E. MORIN @ 2015-09-30 21:54 UTC (permalink / raw)
  To: buildroot

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.  |
'------------------------------^-------^------------------^--------------------'

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [Buildroot] [PATCH 5/9] fs/ext2: use a post-gen hook rather than a post-target rule
  2015-09-30 21:54 [Buildroot] [PATCH 0/9] fs: cleanups and enhancements (branch yem/fs) Yann E. MORIN
                   ` (3 preceding siblings ...)
  2015-09-30 21:54 ` [Buildroot] [PATCH 4/9] fs/initramfs: cleanup and enhance comments Yann E. MORIN
@ 2015-09-30 21:54 ` Yann E. MORIN
  2015-09-30 21:54 ` [Buildroot] [PATCH 6/9] fs/cpio: " Yann E. MORIN
                   ` (3 subsequent siblings)
  8 siblings, 0 replies; 17+ messages in thread
From: Yann E. MORIN @ 2015-09-30 21:54 UTC (permalink / raw)
  To: buildroot

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.

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Arnout Vandecappelle <arnout@mind.be>
---
 fs/ext2/ext2.mk | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/fs/ext2/ext2.mk b/fs/ext2/ext2.mk
index cab66a5..312b7a0 100644
--- a/fs/ext2/ext2.mk
+++ b/fs/ext2/ext2.mk
@@ -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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [Buildroot] [PATCH 6/9] fs/cpio: use a post-gen hook rather than a post-target rule
  2015-09-30 21:54 [Buildroot] [PATCH 0/9] fs: cleanups and enhancements (branch yem/fs) Yann E. MORIN
                   ` (4 preceding siblings ...)
  2015-09-30 21:54 ` [Buildroot] [PATCH 5/9] fs/ext2: use a post-gen hook rather than a post-target rule Yann E. MORIN
@ 2015-09-30 21:54 ` Yann E. MORIN
  2015-09-30 21:54 ` [Buildroot] [PATCH 7/9] fs/common: get rid of post-target rules Yann E. MORIN
                   ` (2 subsequent siblings)
  8 siblings, 0 replies; 17+ messages in thread
From: Yann E. MORIN @ 2015-09-30 21:54 UTC (permalink / raw)
  To: buildroot

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.

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Arnout Vandecappelle <arnout@mind.be>
---
 fs/cpio/cpio.mk | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/fs/cpio/cpio.mk b/fs/cpio/cpio.mk
index e82167e..c68e0bf 100644
--- a/fs/cpio/cpio.mk
+++ b/fs/cpio/cpio.mk
@@ -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) $@.uboot
+endef
+ROOTFS_CPIO_POST_GEN_HOOKS += ROOTFS_CPIO_UBOOT_MKIMAGE
 endif
 
 $(eval $(call ROOTFS_TARGET,cpio))
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [Buildroot] [PATCH 7/9] fs/common: get rid of post-target rules
  2015-09-30 21:54 [Buildroot] [PATCH 0/9] fs: cleanups and enhancements (branch yem/fs) Yann E. MORIN
                   ` (5 preceding siblings ...)
  2015-09-30 21:54 ` [Buildroot] [PATCH 6/9] fs/cpio: " Yann E. MORIN
@ 2015-09-30 21:54 ` Yann E. MORIN
  2015-09-30 21:54 ` [Buildroot] [PATCH 8/9] fs/common: move actions common to all filesystems to their own rule Yann E. MORIN
  2015-09-30 21:54 ` [Buildroot] [PATCH 9/9] core: finalise target in its own location Yann E. MORIN
  8 siblings, 0 replies; 17+ messages in thread
From: Yann E. MORIN @ 2015-09-30 21:54 UTC (permalink / raw)
  To: buildroot

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.

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Arnout Vandecappelle <arnout@mind.be>
---
 fs/common.mk | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/fs/common.mk b/fs/common.mk
index 528e194..28b6021 100644
--- a/fs/common.mk
+++ b/fs/common.mk
@@ -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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [Buildroot] [PATCH 8/9] fs/common: move actions common to all filesystems to their own rule
  2015-09-30 21:54 [Buildroot] [PATCH 0/9] fs: cleanups and enhancements (branch yem/fs) Yann E. MORIN
                   ` (6 preceding siblings ...)
  2015-09-30 21:54 ` [Buildroot] [PATCH 7/9] fs/common: get rid of post-target rules Yann E. MORIN
@ 2015-09-30 21:54 ` Yann E. MORIN
  2015-09-30 21:54 ` [Buildroot] [PATCH 9/9] core: finalise target in its own location Yann E. MORIN
  8 siblings, 0 replies; 17+ messages in thread
From: Yann E. MORIN @ 2015-09-30 21:54 UTC (permalink / raw)
  To: buildroot

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.

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

---
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(-)

diff --git a/fs/common.mk b/fs/common.mk
index 28b6021..9b544eb 100644
--- a/fs/common.mk
+++ b/fs/common.mk
@@ -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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [Buildroot] [PATCH 9/9] core: finalise target in its own location
  2015-09-30 21:54 [Buildroot] [PATCH 0/9] fs: cleanups and enhancements (branch yem/fs) Yann E. MORIN
                   ` (7 preceding siblings ...)
  2015-09-30 21:54 ` [Buildroot] [PATCH 8/9] fs/common: move actions common to all filesystems to their own rule Yann E. MORIN
@ 2015-09-30 21:54 ` Yann E. MORIN
  2015-10-01 21:44   ` Thomas Petazzoni
  8 siblings, 1 reply; 17+ messages in thread
From: Yann E. MORIN @ 2015-09-30 21:54 UTC (permalink / raw)
  To: buildroot

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.

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 Makefile     | 28 +++++++++++++++++++++++++++-
 fs/common.mk |  4 ++--
 2 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/Makefile b/Makefile
index b3f519e..985ed02 100644
--- a/Makefile
+++ b/Makefile
@@ -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 \
diff --git a/fs/common.mk b/fs/common.mk
index 9b544eb..f35bbd6 100644
--- a/fs/common.mk
+++ b/fs/common.mk
@@ -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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [Buildroot] [PATCH 1/9] core: sort packages and eliminate duplicates before building
  2015-09-30 21:54 ` [Buildroot] [PATCH 1/9] core: sort packages and eliminate duplicates before building Yann E. MORIN
@ 2015-10-01  6:19   ` Thomas Petazzoni
  2015-10-01 16:32     ` Yann E. MORIN
  0 siblings, 1 reply; 17+ messages in thread
From: Thomas Petazzoni @ 2015-10-01  6:19 UTC (permalink / raw)
  To: buildroot

Yann,

On Wed, 30 Sep 2015 23:54:44 +0200, Yann E. MORIN wrote:
> 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.

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [Buildroot] [PATCH 3/9] linux: meddle not in the affairs of filesystems, for you are tasty with bacon
  2015-09-30 21:54 ` [Buildroot] [PATCH 3/9] linux: meddle not in the affairs of filesystems, for you are tasty with bacon Yann E. MORIN
@ 2015-10-01  6:20   ` Thomas Petazzoni
  2015-10-01 16:41     ` Yann E. MORIN
  0 siblings, 1 reply; 17+ messages in thread
From: Thomas Petazzoni @ 2015-10-01  6:20 UTC (permalink / raw)
  To: buildroot

Yann,

On Wed, 30 Sep 2015 23:54:46 +0200, Yann E. MORIN wrote:
> 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.
> 
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Arnout Vandecappelle <arnout@mind.be>
> 
> ---
> This, too, will be useful for the initramfs patch coming next.
> ---
>  linux/linux.mk | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/linux/linux.mk b/linux/linux.mk
> index 316f973..864ccbf 100644
> --- a/linux/linux.mk
> +++ b/linux/linux.mk
> @@ -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

Are you sure this is OK ? It creates a dependency of a "real target" on
a "phony target", which means that the "real target" will *always* be
rebuilt, at every invocation of "make". Is this what we want?

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [Buildroot] [PATCH 1/9] core: sort packages and eliminate duplicates before building
  2015-10-01  6:19   ` Thomas Petazzoni
@ 2015-10-01 16:32     ` Yann E. MORIN
  0 siblings, 0 replies; 17+ messages in thread
From: Yann E. MORIN @ 2015-10-01 16:32 UTC (permalink / raw)
  To: buildroot

Thomas, All,

On 2015-10-01 08:19 +0200, Thomas Petazzoni spake thusly:
> On Wed, 30 Sep 2015 23:54:44 +0200, Yann E. MORIN wrote:
> > 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.
> 
> Is this a problem? I don't think "show-targets" aims at guaranteeing
> that each target will only be listed once.

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.  |
'------------------------------^-------^------------------^--------------------'

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [Buildroot] [PATCH 3/9] linux: meddle not in the affairs of filesystems, for you are tasty with bacon
  2015-10-01  6:20   ` Thomas Petazzoni
@ 2015-10-01 16:41     ` Yann E. MORIN
  0 siblings, 0 replies; 17+ messages in thread
From: Yann E. MORIN @ 2015-10-01 16:41 UTC (permalink / raw)
  To: buildroot

Thomas, All,

On 2015-10-01 08:20 +0200, Thomas Petazzoni spake thusly:
> On Wed, 30 Sep 2015 23:54:46 +0200, Yann E. MORIN wrote:
> > 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.
> > 
> > Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> > Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> > Cc: Arnout Vandecappelle <arnout@mind.be>
> > 
> > ---
> > This, too, will be useful for the initramfs patch coming next.
> > ---
> >  linux/linux.mk | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/linux/linux.mk b/linux/linux.mk
> > index 316f973..864ccbf 100644
> > --- a/linux/linux.mk
> > +++ b/linux/linux.mk
> > @@ -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
> 
> Are you sure this is OK ? It creates a dependency of a "real target" on
> a "phony target", which means that the "real target" will *always* be
> rebuilt, at every invocation of "make". Is this what we want?

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.  |
'------------------------------^-------^------------------^--------------------'

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [Buildroot] [PATCH 9/9] core: finalise target in its own location
  2015-09-30 21:54 ` [Buildroot] [PATCH 9/9] core: finalise target in its own location Yann E. MORIN
@ 2015-10-01 21:44   ` Thomas Petazzoni
  2015-10-02  7:46     ` Yann E. MORIN
  0 siblings, 1 reply; 17+ messages in thread
From: Thomas Petazzoni @ 2015-10-01 21:44 UTC (permalink / raw)
  To: buildroot

Hello,

On Wed, 30 Sep 2015 23:54:52 +0200, Yann E. MORIN wrote:
> 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.
> 
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [Buildroot] [PATCH 9/9] core: finalise target in its own location
  2015-10-01 21:44   ` Thomas Petazzoni
@ 2015-10-02  7:46     ` Yann E. MORIN
  2015-10-02  8:21       ` Thomas Petazzoni
  0 siblings, 1 reply; 17+ messages in thread
From: Yann E. MORIN @ 2015-10-02  7:46 UTC (permalink / raw)
  To: buildroot

Thomas, All,

On 2015-10-01 23:44 +0200, Thomas Petazzoni spake thusly:
> On Wed, 30 Sep 2015 23:54:52 +0200, Yann E. MORIN wrote:
> > 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 ).
[--SNIP--]
> 
> 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.

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.

> 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

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.

>  - "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.

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.

> 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.

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.  |
'------------------------------^-------^------------------^--------------------'

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [Buildroot] [PATCH 9/9] core: finalise target in its own location
  2015-10-02  7:46     ` Yann E. MORIN
@ 2015-10-02  8:21       ` Thomas Petazzoni
  0 siblings, 0 replies; 17+ messages in thread
From: Thomas Petazzoni @ 2015-10-02  8:21 UTC (permalink / raw)
  To: buildroot

Hello,

On Fri, 2 Oct 2015 09:46:33 +0200, Yann E. MORIN wrote:

> > 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.
> 
> 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.

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.

> 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

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.

> 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.

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.

> >  - "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.
> 
> 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.

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2015-10-02  8:21 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-09-30 21:54 [Buildroot] [PATCH 0/9] fs: cleanups and enhancements (branch yem/fs) Yann E. MORIN
2015-09-30 21:54 ` [Buildroot] [PATCH 1/9] core: sort packages and eliminate duplicates before building Yann E. MORIN
2015-10-01  6:19   ` Thomas Petazzoni
2015-10-01 16:32     ` Yann E. MORIN
2015-09-30 21:54 ` [Buildroot] [PATCH 2/9] linux: split overly-long dependency line for readability Yann E. MORIN
2015-09-30 21:54 ` [Buildroot] [PATCH 3/9] linux: meddle not in the affairs of filesystems, for you are tasty with bacon Yann E. MORIN
2015-10-01  6:20   ` Thomas Petazzoni
2015-10-01 16:41     ` Yann E. MORIN
2015-09-30 21:54 ` [Buildroot] [PATCH 4/9] fs/initramfs: cleanup and enhance comments Yann E. MORIN
2015-09-30 21:54 ` [Buildroot] [PATCH 5/9] fs/ext2: use a post-gen hook rather than a post-target rule Yann E. MORIN
2015-09-30 21:54 ` [Buildroot] [PATCH 6/9] fs/cpio: " Yann E. MORIN
2015-09-30 21:54 ` [Buildroot] [PATCH 7/9] fs/common: get rid of post-target rules Yann E. MORIN
2015-09-30 21:54 ` [Buildroot] [PATCH 8/9] fs/common: move actions common to all filesystems to their own rule Yann E. MORIN
2015-09-30 21:54 ` [Buildroot] [PATCH 9/9] core: finalise target in its own location Yann E. MORIN
2015-10-01 21:44   ` Thomas Petazzoni
2015-10-02  7:46     ` Yann E. MORIN
2015-10-02  8:21       ` Thomas Petazzoni

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox