system: properly handle systemd as init system

Currently, defining a config fragment in the runtime test infra requires that the fragment not to be indented. This is beark, and causes grievance when looking at the code (e.g. to fix it).

Just strip out all leading spaces/tabs when writing the configuration lines into the config file, allowing in-line indented config fragments, like so:

    class TestFoo(bla):
        config = bla.config + \
            """
            FOO=y
            # BAR is not set
            """

Signed-off-by "Yann E. MORIN" <[email protected]>

Cc Thomas Petazzoni <[email protected]>

---
Changes v1 -> v2:
- add example in commit log (Thomas)

---
Note: this is only tangentially related to this series, because it is needed by the last patch in the series. It wasalready submitted before:

    https://patchwork.ozlabs.org/patch/788985/
---
 support/testing/infra/builder.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

support/testing/infra/builder.py +2 -1

index a475bb0a30..81735dec96 100644
@@ -16,7 +16,8 @@ class Builder(object):
 
         config_file = os.path.join(self.builddir, ".config")
         with open(config_file, "w+") as cf:
-            cf.write(self.config)
+            for line in self.config.splitlines():
+                cf.write("{}\n".format(line.lstrip()))
 
         cmd = ["make",
                "O={}".format(self.builddir),

Signature · 1 line

2.11.0

We currently skip the skeleton dependency by checking if the current package is the skeleton.

We are going to introduce more skeleton-related packages, so we need a way to exclude the skeleton dependency for those, or we'd get a circular dependency, for the same reason we need to skip the toolchain dependency.

Instead of checking for all the skeleton-providing packages in the core infra, add a new package options so that packages can express they do not need the dependency on the skeleton, like we have an option to avoid the depednency on the toolchain. The only packages that will use that option are probably the skeletons, so we need not document this variable, like we did not document the option to exclude the dependency on the toolchain.

Signed-off-by "Yann E. MORIN" <[email protected]>

Reviewed-by Romain Naour <[email protected]>

Quoted 10 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Arnout" == Arnout Vandecappelle <[email protected]> writes:

Instead of checking for all the skeleton-providing packages in the core infra, add a new package options so that packages can express they do not need the dependency on the skeleton, like we have an option to avoid the depednency on the toolchain. The only packages that will use that option are probably the skeletons, so we need not document this variable, like we did not document the option to exclude the dependency on the toolchain.

Quoted 2 lines

Signed-off-by "Yann E. MORIN" <[email protected]>

Reviewed-by Romain Naour <[email protected]>

I absolutely hate adding additional variables that are used for only a few
packages, but I have no better ideas, so

Quoted 12 lines

Quoted 12 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Arnout" == Arnout Vandecappelle <[email protected]> writes:

Instead of checking for all the skeleton-providing packages in the core infra, add a new package options so that packages can express they do not need the dependency on the skeleton, like we have an option to avoid the depednency on the toolchain. The only packages that will use that option are probably the skeletons, so we need not document this variable, like we did not document the option to exclude the dependency on the toolchain.

Signed-off-by "Yann E. MORIN" <[email protected]>

Reviewed-by Romain Naour <[email protected]>

Quoted 2 lines

I absolutely hate adding additional variables that are used for only a few
packages, but I have no better ideas, so

An alternative could be to enforce a naming convention for these packages, E.G. skeleton-*, and then use $(filter skeleton%,$(1)) in the check, but I'm not sure that really is much nicer.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Yann" == Yann E MORIN <[email protected]> writes:

Quoted 3 lines

An alternative could be to enforce a naming convention for these packages, E.G. skeleton-*, and then use $(filter skeleton%,$(1)) in the check, but I'm not sure that really is much nicer.

We also talked about that possibility on IRC, and came to the conclusion that it was not nice to hard-code magic values in the infra.

We currently skip the skeleton dependency by checking if the current package is the skeleton.

We are going to introduce more skeleton-related packages, so we need a way to exclude the skeleton dependency for those, or we'd get a circular dependency, for the same reason we need to skip the toolchain dependency.

Instead of checking for all the skeleton-providing packages in the core infra, add a new package options so that packages can express they do not need the dependency on the skeleton, like we have an option to avoid the depednency on the toolchain. The only packages that will use that option are probably the skeletons, so we need not document this variable, like we did not document the option to exclude the dependency on the toolchain.

Signed-off-by "Yann E. MORIN" <[email protected]>

Reviewed-by Romain Naour <[email protected]>

---
 package/pkg-generic.mk       | 7 +++++--
 package/skeleton/skeleton.mk | 1 +
 2 files changed, 6 insertions(+), 2 deletions(-)

The rsync command is currently called from a single location. In follow-up patches, it will be called from various places then.

It will be easier to update and fix this command if it is defined in a single location, rather than if duplicated in many places.

Define a macro that skeleton packages can call to install their skeleton directories. Since at least one occurence of it will want to install in staging, the macro must accept a destination path, rather than hard-code target as the destination.

Since it is to be used by other skeleton packages, define it early, outside of any conditional block.

Signed-off-by "Yann E. MORIN" <[email protected]>

Reviewed-by Romain Naour <[email protected]>

---
 package/skeleton/skeleton.mk | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

package/skeleton/skeleton.mk +1 -0

index 9b7407a236..017479a990 100644
@@ -14,6 +14,18 @@ SKELETON_ADD_SKELETON_DEPENDENCY = NO
 # The skeleton also handles the merged /usr case in the sysroot
 SKELETON_INSTALL_STAGING = YES
 
+############

package/skeleton/skeleton.mk +2 -3

+
 ifeq ($(BR2_ROOTFS_SKELETON_CUSTOM),y)
 
 SKELETON_PATH = $(call qstrip,$(BR2_ROOTFS_SKELETON_CUSTOM_PATH))

@@ -89,9 +101,7 @@ SKELETON_LIB_SYMLINK = lib32
 endif
 
 define SKELETON_INSTALL_TARGET_CMDS
-	rsync -a --ignore-times $(RSYNC_VCS_EXCLUSIONS) \
-		--chmod=u=rwX,go=rX --exclude .empty --exclude '*~' \
-		$(SKELETON_PATH)/ $(TARGET_DIR)/
+	$(call SKELETON_RSYNC,$(SKELETON_PATH),$(TARGET_DIR))
 	$(call SKELETON_USR_SYMLINKS_OR_DIRS,$(TARGET_DIR))
 	ln -snf lib $(TARGET_DIR)/$(SKELETON_LIB_SYMLINK)
 	ln -snf lib $(TARGET_DIR)/usr/$(SKELETON_LIB_SYMLINK)

Define a macro that skeleton packages can call to install their skeleton directories. Since at least one occurence of it will want to install in

staging, the macro must accept a destination path, rather than hard-code target as the destination.

Quoted 2 lines

Define a macro that skeleton packages can call to install their skeleton directories. Since at least one occurence of it will want to install in

occurrence

Quoted 2 lines

staging, the macro must accept a destination path, rather than hard-code target as the destination.

By the way, the overlay copying uses almost the same rsync arguments, so they
can be used there as well.

The rsync command is currently called from a single location. In follow-up patches, it will be called from various places then.

It will be easier to update and fix this command if it is defined in a single location, rather than if duplicated in many places.

Define a macro that skeleton packages can call to install their skeleton directories. Since at least one occurence of it will want to install in staging, the macro must accept a destination path, rather than hard-code target as the destination.

Since it is to be used by other skeleton packages, define it early, outside of any conditional block.

Signed-off-by "Yann E. MORIN" <[email protected]>

Reviewed-by Romain Naour <[email protected]>

---
 package/skeleton/skeleton.mk | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

Rather than duplicate that code in all skeleton packages, make this a macro, so that it is easier to use.

Signed-off-by "Yann E. MORIN" <[email protected]>

---
 package/skeleton/skeleton.mk | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

package/skeleton/skeleton.mk +1 -0

index 017479a990..0a8add66dc 100644
@@ -17,6 +17,7 @@ SKELETON_INSTALL_STAGING = YES
 ############
 # Macros available for use by any skeleton package:
 # - SKELETON_RSYNC
+# - SKELETON_LIB_SYMLINK
 
 # This function rsyncs the skeleton directory in $(1) to the destination
 # in $(2), which should be either $(TARTGET_DIR) or $(STAGING_DIR)

package/skeleton/skeleton.mk +10 -0

+define SKELETON_LIB_SYMLINK
+	ln -snf lib $(1)/lib64
+	ln -snf lib $(1)/usr/lib64
+endef
+else
+define SKELETON_LIB_SYMLINK
+	ln -snf lib $(1)/lib32
+	ln -snf lib $(1)/usr/lib32
+endef
+endif

package/skeleton/skeleton.mk define SKELETON_RSYNC · line 33

Quoted 4 lines

package/skeleton/skeleton.mk +4 -0

+# Make a symlink lib32->lib or lib64->lib as appropriate.
+# MIPS64/n32 requires lib32 even though it's a 64-bit arch.
+# $(1): base dir (either staging or target)
+ifeq ($(BR2_ARCH_IS_64)$(BR2_MIPS_NABI32),y)  

This makes me wonder: are we doing the right thing with NABI32? What is the
semantics of BR2_ARCH_IS_64? I would say, it doesn't specify that you have a 32-bit or 64-bit capable CPU; instead, it says that you have a 32-bit or 64-bit userspace. I.e., that void* and long are 32 resp. 64 bits. Indeed, I don't think it's really relevant for Buildroot to know about the CPU capabilities, the important thing is the userpace ABI, i.e. how we compile packages.

So, with that in mind, I think we should NOT select BR2_ARCH_IS_64 for the n32
ABI. Similarly, when we add x32, it should also not be BR2_ARCH_IS_64.

Hello,

Quoted 12 lines · Arnout Vandecappelle

package/skeleton/skeleton.mk define SKELETON_RSYNC · line 33

Quoted 4 lines

package/skeleton/skeleton.mk +4 -0

+# Make a symlink lib32->lib or lib64->lib as appropriate.
+# MIPS64/n32 requires lib32 even though it's a 64-bit arch.
+# $(1): base dir (either staging or target)
+ifeq ($(BR2_ARCH_IS_64)$(BR2_MIPS_NABI32),y)  

This makes me wonder: are we doing the right thing with NABI32? What is the
semantics of BR2_ARCH_IS_64? I would say, it doesn't specify that you have a 32-bit or 64-bit capable CPU; instead, it says that you have a 32-bit or 64-bit userspace. I.e., that void* and long are 32 resp. 64 bits. Indeed, I don't think it's really relevant for Buildroot to know about the CPU capabilities, the important thing is the userpace ABI, i.e. how we compile packages.

Quoted 2 lines

So, with that in mind, I think we should NOT select BR2_ARCH_IS_64 for the n32
ABI. Similarly, when we add x32, it should also not be BR2_ARCH_IS_64.

I see your reasoning, but I believe it might be confusing to have BR2_ARCH_IS_64 be false on a 64 bit architecture. However, we could introduce something like BR2_UABI_IS_64, to define whether the userspace ABI is 32 or 64 bits.

But doing that involves reviewing all the places where BR2_ARCH_IS_64 is used, and check if we're using it to say "the architecture is 64 bits" or "the userspace ABI is 32 bits".

Rather than duplicate that code in all skeleton packages, make this a macro, so that it is easier to use.

Quoted 2 lines

Rather than duplicate that code in all skeleton packages, make this a macro, so that it is easier to use.

Is it really going to be used in *all* skeleton packages ? Because if it is, it means it's the exact same call that will be duplicated everywhere, which would mean it should be factored out.

But in practice, it seems it's not used in all skeleton packages, but just in skeleton-common.mk and skeleton-custom.mk, correct ?

Rather than duplicate that code in all skeleton packages, make this a macro, so that it is easier to use.

Signed-off-by "Yann E. MORIN" <[email protected]>

---
 package/skeleton/skeleton.mk | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

Some macros, soon some variables, currently defined in the skeleton are going to be used by other packages.

Some of those variables will be used as Makefile conditions (e.g. in ifeq() conditiopns), so they *must* be defined before being used.

Since the skeleton package, starting with an 's', is included quite late, those variables would not be available to most packages.

Offload the existing macros into the new system/system/mk file, that is

included early, before any package is. Rename the macros to appropriate names.

Future commits will add new macros and variables in that file.

Signed-off-by "Yann E. MORIN" <[email protected]>

Currently, setting the getty is done:
- by the skeleton package when the init system is either busybox or

    sysvinit;

- by the systemd package when the init system is systemd;
both by registering a target-finalize hook.

This is not very consistent.

Move setting the getty out of the skeleton and into the package that provides the init system, by registering a per-package target-fialize

hook.

This offloads yet a bit more out of the skeleton, so that it is easier to properly separate the skeletons for the various init systems.

Signed-off-by "Yann E. MORIN" <[email protected]>

package/busybox/busybox.mk +4 -0

+define BUSYBOX_SET_GETTY
+	$(SED) '/# GENERIC_SERIAL$$/s~^.*#~$(SYSTEM_GETTY_PORT)::respawn:/sbin/getty -L $(SYSTEM_GETTY_OPTIONS) $(SYSTEM_GETTY_PORT) $(SYSTEM_GETTY_BAUDRATE) $(SYSTEM_GETTY_TERM) #~' \
+		$(TARGET_DIR)/etc/inittab
+endef

[snip]

Quoted 4 lines

package/busybox/busybox.mk +4 -0

+define BUSYBOX_SET_GETTY
+	$(SED) '/# GENERIC_SERIAL$$/s~^.*#~$(SYSTEM_GETTY_PORT)::respawn:/sbin/getty -L $(SYSTEM_GETTY_OPTIONS) $(SYSTEM_GETTY_PORT) $(SYSTEM_GETTY_BAUDRATE) $(SYSTEM_GETTY_TERM) #~' \
+		$(TARGET_DIR)/etc/inittab
+endef  

[snip]

package/sysvinit/sysvinit.mk define SYSVINIT_INSTALL_TARGET_CMDS · line 54

Quoted 4 lines

package/sysvinit/sysvinit.mk +4 -0

+define SYSVINIT_SET_GETTY
+	$(SED) '/# GENERIC_SERIAL$$/s~^.*#~$(shell echo $(SYSTEM_GETTY_PORT) | tail -c+4)::respawn:/sbin/getty -L $(SYSTEM_GETTY_OPTIONS) $(SYSTEM_GETTY_PORT) $(SYSTEM_GETTY_BAUDRATE) $(SYSTEM_GETTY_TERM) #~' \
+		$(TARGET_DIR)/etc/inittab
+endef  

The only difference between these two is that for sysvinit we have to make sure
it's only 4 characters. But it actually doesn't hurt to do the same for busybox as well. So perhaps we can factor the two in a single SYSTEM_SET_GETTY ?

Hello,

On Sat, 22 Jul 2017 15:11:57 +0200, Arnout Vandecappelle wrote:

Quoted 3 lines

The only difference between these two is that for sysvinit we have to make sure
it's only 4 characters. But it actually doesn't hurt to do the same for busybox as well. So perhaps we can factor the two in a single SYSTEM_SET_GETTY ?

I am not sure if that works, because the semantic of the first field is not the same between sysvinit and Busybox init.

For sysvinit (http://www.manpages.info/linux/inittab.5.html)

id is a unique sequence of 1-4 characters which identifies an entry
in inittab (for versions of sysvinit compiled with libraries < 5.2.18 or a.out libraries the limit is 2 characters).

Note: For gettys or other login processes, the id field should
be the tty suffix of the corresponding tty, e.g. 1 for tty1.
Otherwise, the login accounting might not work correctly.

For Busybox init:

# <id>: WARNING: This field has a non-traditional meaning for BusyBox init! #

#       The id field is used by BusyBox init to specify the controlling tty for
#       the specified process to run on.  The contents of this field are
#       appended to "/dev/" and used as-is.  There is no need for this field to
#       be unique, although if it isn't you may have strange results.  If this
#       field is left blank, then the init's stdin/out will be used.

Hence, with Busybox init, if you pass a truncated "id", the command might fail to run as it will try to open a /dev/<something> that doesn't exist. So, if you truncate ttyAMA0, it will be just ttyA, Busybox will try to open /dev/ttyA and it will fail.

Best regards,

Thomas

Currently, setting the getty is done:
- by the skeleton package when the init system is either busybox or

    sysvinit;

- by the systemd package when the init system is systemd;
both by registering a target-finalize hook.

This is not very consistent.

Move setting the getty out of the skeleton and into the package that provides the init system, by registering a per-package target-fialize hook.

This offloads yet a bit more out of the skeleton, so that it is easier to properly separate the skeletons for the various init systems.

Signed-off-by "Yann E. MORIN" <[email protected]>

---
 package/busybox/busybox.mk   | 12 +++++++++++-
 package/skeleton/skeleton.mk | 22 ----------------------
 package/sysvinit/sysvinit.mk |  8 ++++++++
 system/system.mk             | 11 +++++++++++
 4 files changed, 30 insertions(+), 23 deletions(-)

Currently, remounting / read-write (or not) is done by the skeleton package when the init system is eitehr busybox or sysvinit, by

registering a target-finalize hook; it is not done at all for systemd.

Move registering this target-finalize hook to both of busybox and sysvinit. Leave systemd alone, we'll take care of it later.

Rename the macro to a more meaningful name, and move it to system.mk with the other such macros.

Yet a little bit less init-system knowledge in the skeleton.

Signed-off-by "Yann E. MORIN" <[email protected]>

+# Provided by the skeleton package

+BUSYBOX_TARGET_FINALIZE_HOOKS += SYSTEM_REMOUNT_ROOT_INITTAB

+# Provided by the skeleton package

package/sysvinit/sysvinit.mk +2 -0

+SYSVINIT_TARGET_FINALIZE_HOOKS += SYSTEM_REMOUNT_ROOT_INITTAB
+

Currently, remounting / read-write (or not) is done by the skeleton package when the init system is eitehr busybox or sysvinit, by registering a target-finalize hook; it is not done at all for systemd.

Move registering this target-finalize hook to both of busybox and sysvinit. Leave systemd alone, we'll take care of it later.

Rename the macro to a more meaningful name, and move it to system.mk with the other such macros.

Yet a little bit less init-system knowledge in the skeleton.

Signed-off-by "Yann E. MORIN" <[email protected]>

---
 package/busybox/busybox.mk   |  3 +++
 package/skeleton/skeleton.mk | 15 ---------------
 package/sysvinit/sysvinit.mk |  3 +++
 system/system.mk             | 15 +++++++++++++++
 4 files changed, 21 insertions(+), 15 deletions(-)

When there is no init system (i.e. a custom one), our bundled default skeleton is most probably un-fit for that (non-)init system.

This will be the case when we introduce per init system skeletons. The systemd skeleton will be unfit except for a systemd init; the sysv skeleton will be unfit except for a sysv-like init system.

In case they are using no init system (really, a custom one), the user should be responsible for providing their own, custom skeleton as well.

Signed-off-by "Yann E. MORIN" <[email protected]>

---
 system/Config.in | 6 ++++++
 1 file changed, 6 insertions(+)

system/Config.in +6 -0

index 828df4217e..ed539dcbe0 100644
@@ -3,11 +3,17 @@ menu "System configuration"
 choice
 	prompt "Root FS skeleton"
 
+# When not using an init system (i.e. a custom one), our default skeleton
+# is most probably unfit; so, allow only the custom skeleton in that case.
 config BR2_ROOTFS_SKELETON_DEFAULT
 	bool "default target skeleton"
+	depends on !BR2_INIT_NONE
 	help
 	  Use default target skeleton
 
+comment "default target skeleton needs an init system"
+	depends on BR2_INIT_NONE
+
 config BR2_ROOTFS_SKELETON_CUSTOM
 	bool "custom target skeleton"
 	help

Quoted 11 lines

When there is no init system (i.e. a custom one), our bundled default skeleton is most probably un-fit for that (non-)init system.

This will be the case when we introduce per init system skeletons. The systemd skeleton will be unfit except for a systemd init; the sysv skeleton will be unfit except for a sysv-like init system.

In case they are using no init system (really, a custom one), the user should be responsible for providing their own, custom skeleton as well.

Signed-off-by "Yann E. MORIN" <[email protected]>

NACK on this one. Our skeleton (as in, the contents of system/skeleton) is
already init-agnostic. If you use another init system (shell script, or no init at all because you just use it as a chroot, or whatever), you will *still* need that skelecton. Indeed, the skeleton contains things like the /sys and /dev directories, a passwd file, ....

If there are things in that skeleton that you don't need for a custom init
system, then they probably shouldn't even be in the skeleton.

Regards,
Arnout

Quoted 27 lines

---
 system/Config.in | 6 ++++++
 1 file changed, 6 insertions(+)

system/Config.in +6 -0

index 828df4217e..ed539dcbe0 100644
@@ -3,11 +3,17 @@ menu "System configuration"
 choice
 	prompt "Root FS skeleton"
 
+# When not using an init system (i.e. a custom one), our default skeleton
+# is most probably unfit; so, allow only the custom skeleton in that case.
 config BR2_ROOTFS_SKELETON_DEFAULT
 	bool "default target skeleton"
+	depends on !BR2_INIT_NONE
 	help
 	  Use default target skeleton
 
+comment "default target skeleton needs an init system"
+	depends on BR2_INIT_NONE
+
 config BR2_ROOTFS_SKELETON_CUSTOM
 	bool "custom target skeleton"
 	help

Signature · 6 lines

Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

Ranout, All,

On 2017-07-22 15:28 +0200, Arnout Vandecappelle spake thusly:

Quoted 19 lines · Yann E. MORIN

Quoted 11 lines

When there is no init system (i.e. a custom one), our bundled default skeleton is most probably un-fit for that (non-)init system.

This will be the case when we introduce per init system skeletons. The systemd skeleton will be unfit except for a systemd init; the sysv skeleton will be unfit except for a sysv-like init system.

In case they are using no init system (really, a custom one), the user should be responsible for providing their own, custom skeleton as well.

Signed-off-by "Yann E. MORIN" <[email protected]>

NACK on this one. Our skeleton (as in, the contents of system/skeleton) is
already init-agnostic. If you use another init system (shell script, or no init at all because you just use it as a chroot, or whatever), you will *still* need that skelecton. Indeed, the skeleton contains things like the /sys and /dev directories, a passwd file, ....

Quoted 2 lines

If there are things in that skeleton that you don't need for a custom init
system, then they probably shouldn't even be in the skeleton.

This patch is reverted later in the series, once all the types of skeleton are in place. I could not see how to do it sanely and cleanly otherwise.

Quoted 25 lines

Ranout, All,

On 2017-07-22 15:28 +0200, Arnout Vandecappelle spake thusly:

Quoted 21 lines · Yann E. MORIN

Quoted 11 lines

When there is no init system (i.e. a custom one), our bundled default skeleton is most probably un-fit for that (non-)init system.

This will be the case when we introduce per init system skeletons. The systemd skeleton will be unfit except for a systemd init; the sysv skeleton will be unfit except for a sysv-like init system.

In case they are using no init system (really, a custom one), the user should be responsible for providing their own, custom skeleton as well.

Signed-off-by "Yann E. MORIN" <[email protected]>

NACK on this one. Our skeleton (as in, the contents of system/skeleton) is
already init-agnostic. If you use another init system (shell script, or no init at all because you just use it as a chroot, or whatever), you will *still* need that skelecton. Indeed, the skeleton contains things like the /sys and /dev directories, a passwd file, ....

If there are things in that skeleton that you don't need for a custom init
system, then they probably shouldn't even be in the skeleton.

Quoted 3 lines

This patch is reverted later in the series, once all the types of skeleton are in place. I could not see how to do it sanely and cleanly otherwise.

OK, in that case

Reviewed-by Arnout Vandecappelle (Essensium/Mind) <[email protected]>

I trust that you looked for a way to avoid this temporary workaround.

Regards,
Arnout

Signature · 6 lines

Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

Setting the root pasword is done in a target-finalize hook, so we do not need to enforce a dependency from the skeleton onto host-mkpasswd.

Dropping that dependency will simplify making skeleton a virtual package (in up-coming changes).

This however introduces a slight change in behaviour: previously, host-mkpasswd would only be built if we needed to hash the root password from its plain-text value. Now, host-mkpasswd is always built as soon as the root password is non-empty, even if already pre-hashed.

Since host-mkpasswd is a really tiny weeny package bundled in Buidlroot,

with only two C files, built as a single unit with a single gcc call, the overhead is really minimal. Compared to the simplifications this will allow in the skeleton packages (plural: common, sysv, systemd, custom) to come, this overhead is acceptable.

Yet another simplification, even if small, to ease providing multiple skeletons.

Signed-off-by "Yann E. MORIN" <[email protected]>

---
 package/skeleton/skeleton.mk | 1 -
 system/Config.in             | 1 +
 2 files changed, 1 insertion(+), 1 deletion(-)

package/skeleton/skeleton.mk +0 -1

index 4a66f6ceca..4ec1ecbb51 100644
@@ -121,7 +121,6 @@ SKELETON_ROOT_PASSWORD =
 else ifneq ($(filter $$1$$% $$5$$% $$6$$%,$(SKELETON_TARGET_GENERIC_ROOT_PASSWD)),)
 SKELETON_ROOT_PASSWORD = '$(SKELETON_TARGET_GENERIC_ROOT_PASSWD)'
 else
-SKELETON_DEPENDENCIES += host-mkpasswd
 # This variable will only be evaluated in the finalize stage, so we can
 # be sure that host-mkpasswd will have already been built by that time.
 SKELETON_ROOT_PASSWORD = "`$(MKPASSWD) -m "$(SKELETON_TARGET_GENERIC_PASSWD_METHOD)" "$(SKELETON_TARGET_GENERIC_ROOT_PASSWD)"`"

system/Config.in +1 -0

index ed539dcbe0..5cb921f108 100644
@@ -224,6 +224,7 @@ config BR2_ROOTFS_MERGED_USR
 config BR2_TARGET_ENABLE_ROOT_LOGIN
 	bool "Enable root login with password"
 	default y
+	select BR2_PACKAGE_HOST_MKPASSWD if BR2_TARGET_GENERIC_ROOT_PASSWD != ""
 	help
 	  Allow root to log in with a password.
 

This makes no sense to me...

Quoted 3 lines · Yann E. MORIN

Setting the root pasword is done in a target-finalize hook, so we do not need to enforce a dependency from the skeleton onto host-mkpasswd.

Quoted 2 lines

Dropping that dependency will simplify making skeleton a virtual package (in up-coming changes).

How so? What's wrong with

SKELETON_COMMON_DEPENDENCIES += host-mkpasswd

in exactly the same place?

You forgot to mention the other interesting bit: it is now selected, so we are
still sure it will get built before target-finalize.

Quoted 6 lines

This however introduces a slight change in behaviour: previously, host-mkpasswd would only be built if we needed to hash the root password from its plain-text value. Now, host-mkpasswd is always built as soon as the root password is non-empty, even if already pre-hashed.

Since host-mkpasswd is a really tiny weeny package bundled in Buidlroot,

Buildroot

Quoted 7 lines

with only two C files, built as a single unit with a single gcc call, the overhead is really minimal. Compared to the simplifications this will allow in the skeleton packages (plural: common, sysv, systemd, custom) to come, this overhead is acceptable.

Yet another simplification, even if small, to ease providing multiple skeletons.

Quoted 1 line

Signed-off-by "Yann E. MORIN" <[email protected]>

Although I don't think it's really *necessary* to remove the dependency, it is
certainly more correct. And we want to select host packages from Config.in anyway on the long run, so doing it only for not-yet-hashed passwords won't be possible anyway. Therefore:

Reviewed-by Arnout Vandecappelle (Essensium/Mind) <[email protected]>

Regards,
Arnout

Quoted 30 lines

---
 package/skeleton/skeleton.mk | 1 -
 system/Config.in             | 1 +
 2 files changed, 1 insertion(+), 1 deletion(-)

package/skeleton/skeleton.mk +0 -1

index 4a66f6ceca..4ec1ecbb51 100644
@@ -121,7 +121,6 @@ SKELETON_ROOT_PASSWORD =
 else ifneq ($(filter $$1$$% $$5$$% $$6$$%,$(SKELETON_TARGET_GENERIC_ROOT_PASSWD)),)
 SKELETON_ROOT_PASSWORD = '$(SKELETON_TARGET_GENERIC_ROOT_PASSWD)'
 else
-SKELETON_DEPENDENCIES += host-mkpasswd
 # This variable will only be evaluated in the finalize stage, so we can
 # be sure that host-mkpasswd will have already been built by that time.
 SKELETON_ROOT_PASSWORD = "`$(MKPASSWD) -m "$(SKELETON_TARGET_GENERIC_PASSWD_METHOD)" "$(SKELETON_TARGET_GENERIC_ROOT_PASSWD)"`"

system/Config.in +1 -0

index ed539dcbe0..5cb921f108 100644
@@ -224,6 +224,7 @@ config BR2_ROOTFS_MERGED_USR
 config BR2_TARGET_ENABLE_ROOT_LOGIN
 	bool "Enable root login with password"
 	default y
+	select BR2_PACKAGE_HOST_MKPASSWD if BR2_TARGET_GENERIC_ROOT_PASSWD != ""
 	help
 	  Allow root to log in with a password.
 

Signature · 6 lines

Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

Our current skeleton is tailored to sysv-like init systems; it is not fit for systemd-based systems. So, in upcoming changes, we'll add another skeleton for systemd.

This means we can no longer have the current skeleton default to 'y', or it would be enabled also for systemd, which would be incorrect.

So, we remove the default to 'y' but have it selected by the default skeleton choice.

However, we do not yet have a way to directly build (really, install) the custom skeleton, it is built (really, installed) as a dependency of the default skeleton. So we must also forcibly select the default skeleton when using a custom one.

Until we have the mean to do only one or the other; i.e. when we have a

virtual skeleton.

Signed-off-by "Yann E. MORIN" <[email protected]>

---
 package/skeleton/Config.in | 1 -
 system/Config.in           | 2 ++
 2 files changed, 2 insertions(+), 1 deletion(-)

package/skeleton/Config.in +0 -1

index d25147bd92..b22ac66b35 100644
@@ -1,5 +1,4 @@
 config BR2_PACKAGE_SKELETON
 	bool
-	default y
 	help
 	  The basic skeleton for your rootfs.

system/Config.in +2 -0

index 5cb921f108..9b42fbbbac 100644
@@ -8,6 +8,7 @@ choice
 config BR2_ROOTFS_SKELETON_DEFAULT
 	bool "default target skeleton"
 	depends on !BR2_INIT_NONE
+	select BR2_PACKAGE_SKELETON
 	help
 	  Use default target skeleton
 

@@ -16,6 +17,7 @@ comment "default target skeleton needs an init system"
 
 config BR2_ROOTFS_SKELETON_CUSTOM
 	bool "custom target skeleton"
+	select BR2_PACKAGE_SKELETON
 	help
 	  Use custom target skeleton.
 

For the custom skeleton, we practicaly do nothing, except ensure it

contains the basic, required directories, and that those are properly setup wrt. merged /usr.

Furthermore, our current skeleton is not fit for systemd, and we'll have to split things out into various skeletons.

So, off-load the custom skeleton into its own package.

Thus, the existing skeleton package is now limited to:

- when using our default skeleton, install and tweak it properly;

- when using a custom skeleton, do nothing except for depending on the skeleton-custom package.

Signed-off-by "Yann E. MORIN" <[email protected]>

package/skeleton-custom/skeleton-custom.mk new file +4 -0

+define SKELETON_CUSTOM_INSTALL_STAGING_CMDS
+	$(call SYSTEM_RSYNC,$(SKELETON_CUSTOM_PATH),$(STAGING_DIR))
+	$(call SYSTEM_LIB_SYMLINK,$(STAGING_DIR))
+endef

package/skeleton-custom/skeleton-custom.mk line 102

Quoted 3 lines

package/skeleton-custom/skeleton-custom.mk new file +3 -0

+define SKELETON_CUSTOM_INSTALL_STAGING_CMDS
+	$(call SYSTEM_RSYNC,$(SKELETON_CUSTOM_PATH),$(STAGING_DIR))
+	$(call SYSTEM_LIB_SYMLINK,$(STAGING_DIR))

Quoted 1 line

+endef

On second thought...

Quoted 3 lines · Arnout Vandecappelle

package/skeleton-custom/skeleton-custom.mk line 102

Quoted 3 lines

package/skeleton-custom/skeleton-custom.mk new file +3 -0

+define SKELETON_CUSTOM_INSTALL_STAGING_CMDS
+	$(call SYSTEM_RSYNC,$(SKELETON_CUSTOM_PATH),$(STAGING_DIR))
+	$(call SYSTEM_LIB_SYMLINK,$(STAGING_DIR))

In skeleton-common, we also have:

	$(INSTALL) -d -m 0755 $(STAGING_DIR)/usr/include

Shouldn't we do that here as well?

Regards,
Arnout

Quoted 1 line

Quoted 1 line

+endef

Signature · 6 lines

Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

Move all the handling of the default skeleton into a new package, skeleton-common.

We don;t name it skeleton-default, because it will be further split

later, into a skeleton for sysv and another for systemd, with some parts still common between the two. So just name it skeleton-common right now; this will save us a rename later.

We keep in the skeleton package all definitions of macros and variables that can be used by other skeleton packages and/or init systems, and just offload the actual feeding of the skeleton to the new package.

Signed-off-by "Yann E. MORIN" <[email protected]>

Quoted 4 lines

Move all the handling of the default skeleton into a new package, skeleton-common.

We don;t name it skeleton-default, because it will be further split

don't

Quoted 9 lines

later, into a skeleton for sysv and another for systemd, with some parts still common between the two. So just name it skeleton-common right now; this will save us a rename later.

We keep in the skeleton package all definitions of macros and variables that can be used by other skeleton packages and/or init systems, and just offload the actual feeding of the skeleton to the new package.

Signed-off-by "Yann E. MORIN" <[email protected]>

Reviewed-by Arnout Vandecappelle (Essensium/Mind) <[email protected]>

[snip]

package/skeleton-common/skeleton-common.mk line 10

Quoted 4 lines

package/skeleton-common/skeleton-common.mk new file +4 -0

+# The skeleton can't depend on the toolchain, since all packages depends on the
+# skeleton and the toolchain is a target package, as is skeleton.
+# Hence, skeleton would depends on the toolchain and the toolchain would depend
+# on skeleton.

I don't think this comment is very useful - it's pretty obvious. Actually, it
would make sense for SKELETON_COMMON_ADD_SKELETON_DEPENDENCY=NO to imply SKELETON_COMMON_ADD_TOOLCHAIN_DEPENDENCY=NO, but really we don't need to complicate the infra for that...

You can't have too many comments though, so let's not block the patch for that.

package/skeleton-common/skeleton-common.mk line 14

Quoted 4 lines

package/skeleton-common/skeleton-common.mk new file +4 -0

+SKELETON_COMMON_ADD_TOOLCHAIN_DEPENDENCY = NO
+SKELETON_COMMON_ADD_SKELETON_DEPENDENCY = NO
+
+# The skeleton also handles the merged /usr case in the sysroot

This comment, on the other hand, *is* very useful.

package/skeleton-common/skeleton-common.mk line 17

Quoted 3 lines

package/skeleton-common/skeleton-common.mk new file +3 -0

+SKELETON_COMMON_INSTALL_STAGING = YES
+
+SKELETON_COMMON_PATH = system/skeleton

Not for this patch of course, but I think it would make sense to also move that
here. But you probably thought of that already :-)

Regards,
Arnout

[snip]

Signature · 6 lines

Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

Arnout, All,

On 2017-07-23 01:06 +0200, Arnout Vandecappelle spake thusly:

Quoted 4 lines

Quoted 1 line

+SKELETON_COMMON_PATH = system/skeleton

Not for this patch of course, but I think it would make sense to also move that
here. But you probably thought of that already :-)

Yes, that's is what I did in the first iteration, but Thomas was against moving it, so I left it alone to stay there.

Quoted 5 lines

Quoted 2 lines

Not for this patch of course, but I think it would make sense to also move that
here. But you probably thought of that already :-)

Yes, that's is what I did in the first iteration, but Thomas was against moving it, so I left it alone to stay there.

Thomas, do you remember why you were against moving the skeleton to
package/skeleton-common?

Quoted 9 lines

Quoted 6 lines

Quoted 3 lines

Not for this patch of course, but I think it would make sense to also move that
here. But you probably thought of that already :-)

Yes, that's is what I did in the first iteration, but Thomas was against moving it, so I left it alone to stay there.

Thomas, do you remember why you were against moving the skeleton to
package/skeleton-common?

I honestly don't remember. I believe it might be that I thought that system/skeleton/ was a more "obvious" place to look for the skeleton, rather than buried in package/skeleton-common/. But this argument feels a bit fishy, so I'm not sure if that was that or something else.

I don't currently see a problem with having the base skeleton moved to package/skeleton-common/.

We now have two packages that can act as a skeleton, skeleton-common, also known as our default skeleton, and skeleton-custom.

This means that the skeleton package can be a standard virtual package now.

Well, except that, besides being a virtual package, it also provides variables and macros that can be used by the other skeletons and/or the init systems.

Signed-off-by "Yann E. MORIN" <[email protected]>

But here we have an exception: there are only two providers

system/Config.in +2 -0

+# and they only get selected each by separate entries in this choice.
+# So this is a safe situation.
 choice
 	prompt "Root FS skeleton"

Quoted 6 lines

We now have two packages that can act as a skeleton, skeleton-common, also known as our default skeleton, and skeleton-custom.

This means that the skeleton package can be a standard virtual package now.

Quoted 3 lines

Well, except that, besides being a virtual package, it also provides variables and macros that can be used by the other skeletons and/or the init systems.

I guess this is a left-over from when you hadn't moved everything to system.mk
yet...

Quoted 1 line

Signed-off-by "Yann E. MORIN" <[email protected]>

On this patch, I do have kind of big comments.

[snip]

package/skeleton-custom/Config.in line 6

Quoted 12 lines

package/skeleton-custom/Config.in +4 -1

index b12bd8f73c..601c3b247e 100644
@@ -1,3 +1,6 @@
 config BR2_PACKAGE_SKELETON_CUSTOM
 	bool
-	select BR2_PACKAGE_SKELETON
+	select BR2_PACKAGE_HAS_SKELETON
+
+config BR2_PACKAGE_PROVIDES_SKELETON
+	default "skeleton-custom" if BR2_PACKAGE_SKELETON_CUSTOM

In almost all virtual package providers, we do it like:

if BR2_PACKAGE_SKELETON_CUSTOM
config BR2_PACKAGE_PROVIDES_SKELETON
default "skeleton-custom"
endif

[snip]

package/skeleton-custom/Config.in

Quoted 5 lines

config BR2_PACKAGE_SKELETON

package/skeleton-custom/Config.in +1 -2

 	bool
-	help
-	  The basic skeleton for your rootfs.
+	default y

For most virtual packages, we don't define the Kconfig symbol (e.g. there is no
BR2_PACKAGE_EGL). Do we need it for skeleton?

package/skeleton-custom/Config.in

Quoted 3 lines

package/skeleton-custom/Config.in +3 -0

+
+config BR2_PACKAGE_HAS_SKELETON
+	bool

We also don't really need this. It is used by packages that need the virtual
package to check if there is a provider for it. But no package 'depends on' skeleton (or rather, all of them do), so we don't need this.

package/skeleton-custom/Config.in

Quoted 3 lines

package/skeleton-custom/Config.in +3 -0

+
+config BR2_PACKAGE_PROVIDES_SKELETON
+	string

This one we do need :-)

[snip]

Quoted 7 lines

system/Config.in +0 -0

index c27b013784..75b6a8edac 100644
@@ -1,5 +1,9 @@
 menu "System configuration"
 

system/Config.in line 4

Quoted 2 lines

system/Config.in +2 -0

+# Note: usually, it is not possible to select a provider of a virtual
+# package.

This is not true cryptodev, jpeg and mysql do exactly that. libressl/openssl
will also do it that way. In fact, a choice is in a way the better way to do it, because then you're guaranteed not to have conflicting providers.

So this entire comment is wrong and can be removed. IMO.

Regards,
Arnout

Quoted 7 lines

But here we have an exception: there are only two providers

system/Config.in +2 -0

+# and they only get selected each by separate entries in this choice.
+# So this is a safe situation.
 choice
 	prompt "Root FS skeleton"

Signature · 6 lines

Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

On 2017-07-23 01:36 +0200, Arnout Vandecappelle spake thusly:

Quoted 3 lines · Yann E. MORIN

Quoted 8 lines

package/skeleton/Config.in line 3

Quoted 5 lines

config BR2_PACKAGE_SKELETON

package/skeleton/Config.in +1 -2

 	bool
-	help
-	  The basic skeleton for your rootfs.
+	default y

For most virtual packages, we don't define the Kconfig symbol (e.g. there is no
BR2_PACKAGE_EGL). Do we need it for skeleton?

We have it already for toolchain, another 'speical' package.

Quoted 6 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+# Note: usually, it is not possible to select a provider of a virtual
+# package.

This is not true cryptodev, jpeg and mysql do exactly that. libressl/openssl
will also do it that way. In fact, a choice is in a way the better way to do it, because then you're guaranteed not to have conflicting providers.

But then you disallow implementations from a br2-external tree, for example for jpeg, a provider that has a hardware-accelerated libjpeg.

Quoted 2 lines

So this entire comment is wrong and can be removed. IMO.

No, the comment is true, when you look at a virtual package definition in the manual. The way we do it for jpeg, cryptodev and mysql is that we have a real package and a virtual package both in one, which have a choice, and thus lift the limitation.

We don't really care what's going in staging, as long as it is properly setup for merged/non-merged /usr, especially for the lib/ directory.

So we can just copy the skeleton as-is.

This simplify the maintenance, should we ever need to tweak the layout:
we'd just have to do it once in the skeleton directory to have it propagated to both target and staging.

Signed-off-by "Yann E. MORIN" <[email protected]>

---
 package/skeleton-common/skeleton-common.mk | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

package/skeleton-common/skeleton-common.mk +5 -9

index ee44eaf810..475c07e32d 100644
@@ -26,18 +26,14 @@ define SKELETON_COMMON_INSTALL_TARGET_CMDS
 		$(TARGET_DIR_WARNING_FILE)
 endef
 
-# For the staging dir, we don't really care about /bin and /sbin.
-# But for consistency with the target dir, and to simplify the code,
-# we still handle them for the merged or non-merged /usr cases.
-# Since the toolchain is not yet available, the staging is not yet
-# populated, so we need to create the directories in /usr
+# We don't care much about what goes in staging, as long as it is
+# correctly setup for merged/non-merged /usr. The simplest is to
+# fill it in with the content of the skeleton.
 define SKELETON_COMMON_INSTALL_STAGING_CMDS
-	$(INSTALL) -d -m 0755 $(STAGING_DIR)/usr/lib
-	$(INSTALL) -d -m 0755 $(STAGING_DIR)/usr/bin
-	$(INSTALL) -d -m 0755 $(STAGING_DIR)/usr/sbin
-	$(INSTALL) -d -m 0755 $(STAGING_DIR)/usr/include
+	$(call SYSTEM_RSYNC,$(SKELETON_COMMON_PATH),$(STAGING_DIR))
 	$(call SYSTEM_USR_SYMLINKS_OR_DIRS,$(STAGING_DIR))
 	$(call SYSTEM_LIB_SYMLINK,$(STAGING_DIR))
+	$(INSTALL) -d -m 0755 $(STAGING_DIR)/usr/include
 endef
 
 SKELETON_COMMON_HOSTNAME = $(call qstrip,$(BR2_TARGET_GENERIC_HOSTNAME))

Currently, we use the same skeleton for sysv-like init systems and systemd, even though systemd has some peculiarities that makes out

default skeleton unfit.

So, we'll need to provide different skeletons (really, only part of it) for sysv-like and systemd.

Introduce two new skeleton packages, aptly named skeleton-sysv and skeleton-systemd, one for each, that are both providers of the skeleton package, in lieue of the skeleton-common, which is now a simple dependency of both the two new skeletons.

Those packages are empty for now. In followup changes:
- sysv-specific stuff will be moved out of skeleton-common and into

    skeleton-sysv;

- systemd-specific stuff will be added to skeleton-systemd.

Signed-off-by "Yann E. MORIN" <[email protected]>

system/Config.in +0 -0

 	help
 	  Use default target skeleton
 

Quoted 2 lines

Currently, we use the same skeleton for sysv-like init systems and systemd, even though systemd has some peculiarities that makes out

our

Quoted 10 lines

default skeleton unfit.

So, we'll need to provide different skeletons (really, only part of it) for sysv-like and systemd.

Introduce two new skeleton packages, aptly named skeleton-sysv and skeleton-systemd, one for each, that are both providers of the skeleton package, in lieue of the skeleton-common, which is now a simple dependency of both the two new skeletons.

Quoted 6 lines

Those packages are empty for now. In followup changes:
- sysv-specific stuff will be moved out of skeleton-common and into

    skeleton-sysv;

- systemd-specific stuff will be added to skeleton-systemd.

Signed-off-by "Yann E. MORIN" <[email protected]>

Reviewed-by Arnout Vandecappelle (Essensium/Mind) <[email protected]>

But...

[snip]

package/skeleton-systemd/Config.in line 7

Quoted 7 lines

package/skeleton-systemd/Config.in new file +7 -0

+config BR2_PACKAGE_SKELETON_SYSTEMD
+	bool
+	select BR2_PACKAGE_HAS_SKELETON
+	select BR2_PACKAGE_SKELETON_COMMON
+
+config BR2_PACKAGE_PROVIDES_SKELETON
+	default "skeleton-systemd" if BR2_PACKAGE_SKELETON_SYSTEMD

Again, we usually do it with if...config...endif. Same for sysv, obviously.

[snip]

system/Config.in choice · line 17

Quoted 8 lines

system/Config.in +3 -1

@@ -12,7 +12,9 @@ choice
 config BR2_ROOTFS_SKELETON_DEFAULT
 	bool "default target skeleton"
 	depends on !BR2_INIT_NONE
-	select BR2_PACKAGE_SKELETON_COMMON
+	select BR2_PACKAGE_SKELETON_SYSV if BR2_INIT_SYSV
+	select BR2_PACKAGE_SKELETON_SYSV if BR2_INIT_BUSYBOX
+	select BR2_PACKAGE_SKELETON_SYSTEMD if BR2_INIT_SYSTEMD

This works, but I'm thinking of an alternative, let's see if it flies:

Instead of choosing the different skeletons here, we could do it in the select
of BR2_INIT_*. Indeed, you have a skeleton for each init option. Of course, all the selects there would have to look like

select BR2_PACKAGE_SKELETON_SYSV if BR2_ROOTFS_SKELETON_DEFAULT

I'm not entirely sure if it's an improvement, but I have the feeling it's
easier to follow if the different skeleton options are put together with the different init options.

systems does not like that the content of /var be symlinks to /tmp,

especially journald that starts before /tmp is mounted, and thus the journal files are hidden from view, which causes wuite a bit if fuss...

Instead, move the current /var to a sysv-only skeleton.

systemd at install time will create the /var content it needs, so we

just create an empty /var for systemd. Yet, systemd really requires a bunch of extra directories to exist, so we also create those.

Signed-off-by "Yann E. MORIN" <[email protected]>

Cc Maxime Hadjinlian <[email protected]>

---
Note: /home and /srv will be created at runtime if missing, but it is better to create them right now, to simplify supporting systemd on a RO filesystem in the (near) future.

---
 package/skeleton-systemd/skeleton-systemd.mk              | 7 +++++++
 package/skeleton-sysv/skeleton-sysv.mk                    | 4 ++++
 {system => package/skeleton-sysv}/skeleton/dev/log        | 0
 {system => package/skeleton-sysv}/skeleton/dev/pts/.empty | 0
 {system => package/skeleton-sysv}/skeleton/dev/shm/.empty | 0

 {system => package/skeleton-sysv}/skeleton/etc/fstab      | 0

Quoted 1 line · Yann E. MORIN

systems does not like that the content of /var be symlinks to /tmp,

systemd

Quoted 2 lines

especially journald that starts before /tmp is mounted, and thus the journal files are hidden from view, which causes wuite a bit if fuss...

quite

Quoted 3 lines

Instead, move the current /var to a sysv-only skeleton.

systemd at install time will create the /var content it needs, so we

I guess systemd will also create a tmpfs on /var?

Quoted 8 lines · Yann E. MORIN

Quoted 1 line

systems does not like that the content of /var be symlinks to /tmp,

systemd

Quoted 2 lines

especially journald that starts before /tmp is mounted, and thus the journal files are hidden from view, which causes wuite a bit if fuss...

quite

Quoted 4 lines

Quoted 3 lines

Instead, move the current /var to a sysv-only skeleton.

systemd at install time will create the /var content it needs, so we

I guess systemd will also create a tmpfs on /var?

Seems the following sentence got dropped:

What happens when a package installs something in /var?

Quoted 1 line

systems does not like that the content of /var be symlinks to /tmp,

systemd

Quoted 2 lines

especially journald that starts before /tmp is mounted, and thus the journal files are hidden from view, which causes wuite a bit if fuss...

quite

Quoted 4 lines

Instead, move the current /var to a sysv-only skeleton.

systemd at install time will create the /var content it needs, so we

I guess systemd will also create a tmpfs on /var?

Quoted 5 lines

just create an empty /var for systemd. Yet, systemd really requires a bunch of extra directories to exist, so we also create those.

Signed-off-by "Yann E. MORIN" <[email protected]>

Cc Maxime Hadjinlian <[email protected]>

Commit message needs to be improved (see also below), but with that

Reviewed-by Arnout Vandecappelle (Essensium/Mind) <[email protected]>

Quoted 11 lines

---
Note: /home and /srv will be created at runtime if missing, but it is better to create them right now, to simplify supporting systemd on a RO filesystem in the (near) future.

---
 package/skeleton-systemd/skeleton-systemd.mk              | 7 +++++++
 package/skeleton-sysv/skeleton-sysv.mk                    | 4 ++++
 {system => package/skeleton-sysv}/skeleton/dev/log        | 0
 {system => package/skeleton-sysv}/skeleton/dev/pts/.empty | 0
 {system => package/skeleton-sysv}/skeleton/dev/shm/.empty | 0

I guess indeed the /dev stuff is not needed for systemd because there is anyway
a devtmpfs mounted over it. Which makes me wonder: shouldn't these be moved to the device-table? Only the static devices case needs these to be present... Of course, such a change is not for this patch, but I'd say that we can just leave this /dev stuff in the common part - systemd has no less use of it than sysv.

Quoted 1 line

 {system => package/skeleton-sysv}/skeleton/etc/fstab      | 0

Why? systemd does use fstab, so it seems weird not to have an fstab in the
systemd case.

Well, I guess our skeleton fstab doesn't contain anything relevant for systemd because all of that is handled already by tmpfiles.

But the commit log should at least explain this.

Arnout, All,

On 2017-07-23 02:08 +0200, Arnout Vandecappelle spake thusly:

Quoted 1 line

But the commit log should at least explain this.

Like so, maybe?

For systemd, we only ever need an entry for /, because all the rest is totally irrelevant for systemd (it automatically mounts what it needs where it needs it).

Quoted 3 lines · Yann E. MORIN

Arnout, All,

On 2017-07-23 02:08 +0200, Arnout Vandecappelle spake thusly:

[snip]

Quoted 2 lines

Quoted 1 line

But the commit log should at least explain this.

Quoted 5 lines

Like so, maybe?

For systemd, we only ever need an entry for /, because all the rest is totally irrelevant for systemd (it automatically mounts what it needs where it needs it).

But... skeleton-common installs a lot more than just /, no? It installs
/etc/stuff, and a bunch of top-level directories...

Quoted 14 lines · Yann E. MORIN

Quoted 3 lines

Arnout, All,

On 2017-07-23 02:08 +0200, Arnout Vandecappelle spake thusly:

[snip]

Quoted 7 lines

Quoted 1 line

But the commit log should at least explain this.

Like so, maybe?

For systemd, we only ever need an entry for /, because all the rest is totally irrelevant for systemd (it automatically mounts what it needs where it needs it).

Quoted 2 lines

But... skeleton-common installs a lot more than just /, no? It installs
/etc/stuff, and a bunch of top-level directories...

Sorry, I missed that this was about fstab, I thought it was about the skeleton
itself.

fstab is not yet mentioned at all in the commit message, so this should be
rephrased a little. But content-wise this is indeed what I wanted.

Requiring that no-init system indeed provide their own custom skeletons

is a bit too-high a burden, especially for quick one-off tests when testing our packages.

Allow no-init systems to use our default skeleton, but only use the common part, for the specific sysv or systemd ones are unfit.

Reported-by Thomas Petazzoni <[email protected]>

Signed-off-by "Yann E. MORIN" <[email protected]>

---
 package/skeleton-common/Config.in | 29 +++++++++++++++++++++++++++++
 system/Config.in                  |  5 +----
 2 files changed, 30 insertions(+), 4 deletions(-)

package/skeleton-common/Config.in +7 -0

index 9abed4292a..ba0abe9d35 100644
@@ -1,2 +1,31 @@
+# Normally, skeleton-common is a dependency of either the sysv or
+# the systemd ones, so it should not declare itself as a provider
+# for the skeleton virtual package.
+#
+# However, when one uses a no-init system (BR2_INIT_NONE), neither
+# the sysv not the systemd skeleton are enabled, so skeleton-common
+# is the one and only skeleton available, so it must declare itself

Quoted 1 line

Requiring that no-init system indeed provide their own custom skeletons

systems

Quoted 5 lines

is a bit too-high a burden, especially for quick one-off tests when testing our packages.

Allow no-init systems to use our default skeleton, but only use the common part, for the specific sysv or systemd ones are unfit.

For people who use BR2_INIT_NONE to create a chroot-able rootfs, this entails
an important change: the /var stuff (e.g. /var/run symlink) is no longer there. This could have implications for some use cases. So I think a comment about this should be added to the release notes.

Quoted 20 lines

Reported-by Thomas Petazzoni <[email protected]>

Signed-off-by "Yann E. MORIN" <[email protected]>

---
 package/skeleton-common/Config.in | 29 +++++++++++++++++++++++++++++
 system/Config.in                  |  5 +----
 2 files changed, 30 insertions(+), 4 deletions(-)

package/skeleton-common/Config.in +7 -0

index 9abed4292a..ba0abe9d35 100644
@@ -1,2 +1,31 @@
+# Normally, skeleton-common is a dependency of either the sysv or
+# the systemd ones, so it should not declare itself as a provider
+# for the skeleton virtual package.
+#
+# However, when one uses a no-init system (BR2_INIT_NONE), neither
+# the sysv not the systemd skeleton are enabled, so skeleton-common
+# is the one and only skeleton available, so it must declare itself

package/skeleton-common/Config.in line 17

Quoted 10 lines

package/skeleton-common/Config.in +8 -0

+# as a provider.
+#
+# We can safely select _HAS_SKELETON, because either we are the only
+# provider (in which case we must select it) or we were selected by
+# another skeleton which would have selected it by itself (in which
+# case us selecting _HAS_SKELETON is harmless).
+#
 config BR2_PACKAGE_SKELETON_COMMON
 	bool
+	select BR2_PACKAGE_HAS_SKELETON

Instead of putting it here...

package/skeleton-common/Config.in line 26

Quoted 9 lines

package/skeleton-common/Config.in +9 -0

+
+if BR2_PACKAGE_SKELETON_COMMON
+
+config BR2_PACKAGE_SKELETON_COMMON_ONLY
+	bool
+	default y
+	depends on !BR2_PACKAGE_SKELETON_SYSV
+	depends on !BR2_PACKAGE_SKELETON_SYSTEMD
+	depends on !BR2_PACKAGE_SKELETON_CUSTOM

... put it here. Then you don't need the comment above, there will always be
just one symbol that selects skeleton.

But as I wrote in an earlier reply, we don't even need to select
BR2_PACKAGE_HAS_SKELETON, so the point is moot.

In some cases, the directory structure we want in the filesystem is not exactly what we have in target/

For example, when systemd is used on a read-only rootfs, /var must be a tmpfs. However, we may have packages that install stuff in there, and set important rights (via the permission-table). So, at build time, we need /var to be a symlink to the remanent location (/usr/share/factory) while at runtime we need /var to be a directory.

One option would have seen to have /var as a real directory even during build time, and in a target-finalize hook, move everything out of there and into the "factory" location. However, that's not possible because it's too early: some packages may want to set ownership and/or acces rights on directories or files in /var, and this is only done in the fakeroot script, which is called only later during the assembling of the filesystem images.

Also, there would have been no way to undo the tweak (i.e. we need to restore the /var symlink so that subsequent builds continue to work) if it were done as a target-finalize hook.

The only solution is to allow packages to register pre- and post-hooks that are called right before and right after the rootfs commands are executed, and inside in the fakeroot script.

We can however not re-use the BR2_ROOTFS_POST_FAKEROOT_SCRIPT feature either because it is done before the filesystem command, but there is nothing that is done after. Also, we don't want to add to, and modify a user-supplied variable.

So, we introduce two new variables that packages can set to add the commands they need to run to tweak the filesystem right at the last moment.

Those hooks are not documented on-purpose; they are probably going to only ever be used by systemd.

Signed-off-by "Yann E. MORIN" <[email protected]>

Reviewed-by Romain Naour <[email protected]>

---
Changes v2 -> v3:
- add space between >> and $$ (Romain)

---
 fs/common.mk           | 4 ++++
 package/pkg-generic.mk | 4 ++++
 2 files changed, 8 insertions(+)

fs/common.mk +0 -0

index 14e0a6b868..9a7758ff49 100644

Quoted 21 lines

In some cases, the directory structure we want in the filesystem is not exactly what we have in target/

For example, when systemd is used on a read-only rootfs, /var must be a tmpfs. However, we may have packages that install stuff in there, and set important rights (via the permission-table). So, at build time, we need /var to be a symlink to the remanent location (/usr/share/factory) while at runtime we need /var to be a directory.

One option would have seen to have /var as a real directory even during build time, and in a target-finalize hook, move everything out of there and into the "factory" location. However, that's not possible because it's too early: some packages may want to set ownership and/or acces rights on directories or files in /var, and this is only done in the fakeroot script, which is called only later during the assembling of the filesystem images.

Also, there would have been no way to undo the tweak (i.e. we need to restore the /var symlink so that subsequent builds continue to work) if it were done as a target-finalize hook.

Quoted 8 lines

The only solution is to allow packages to register pre- and post-hooks that are called right before and right after the rootfs commands are executed, and inside in the fakeroot script.

We can however not re-use the BR2_ROOTFS_POST_FAKEROOT_SCRIPT feature either because it is done before the filesystem command, but there is nothing that is done after. Also, we don't want to add to, and modify a user-supplied variable.

I completely agree with this approach.

Quoted 4 lines

So, we introduce two new variables that packages can set to add the commands they need to run to tweak the filesystem right at the last moment.

But not with this.

Quoted 2 lines

Those hooks are not documented on-purpose; they are probably going to only ever be used by systemd.

If systemd is the only one ever to use it, then the approach with a list of
hooks is just too much over the top. Why not hardcode it? Like so:

Quoted 16 lines

Signed-off-by "Yann E. MORIN" <[email protected]>

Reviewed-by Romain Naour <[email protected]>

---
Changes v2 -> v3:
- add space between >> and $$ (Romain)

---
 fs/common.mk           | 4 ++++
 package/pkg-generic.mk | 4 ++++
 2 files changed, 8 insertions(+)

fs/common.mk +0 -0

index 14e0a6b868..9a7758ff49 100644

fs/common.mk endif · line 99

Quoted 5 lines

fs/common.mk +2 -0

@@ -95,10 +95,14 @@ endif
 	$$(foreach s,$$(call qstrip,$$(BR2_ROOTFS_POST_FAKEROOT_SCRIPT)),\
 		echo "echo '$$(TERM_BOLD)>>>   Executing fakeroot script $$(s)$$(TERM_RESET)'" >> $$(FAKEROOT_SCRIPT); \
 		echo $$(s) $$(TARGET_DIR) $$(BR2_ROOTFS_POST_SCRIPT_ARGS) >> $$(FAKEROOT_SCRIPT)$$(sep))
+	$$(foreach hook,$$(ROOTFS_PRE_CMD_HOOKS),\
+		$$(call PRINTF,$$($$(hook))) >> $$(FAKEROOT_SCRIPT))

	$$(call PRINTF,$$(ROOTFS_SKELETON_PRE_CMD_HOOK)) >> $$(FAKEROOT_SCRIPT)

You may want to change the variable name, obviously.

Regards,
Arnout

[snip]

Signature · 6 lines

Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

Arnout, All,

On 2017-07-23 15:42 +0200, Arnout Vandecappelle spake thusly:

Quoted 3 lines

	$$(call PRINTF,$$(ROOTFS_SKELETON_PRE_CMD_HOOK)) >> $$(FAKEROOT_SCRIPT)

You may want to change the variable name, obviously.

And it also means that the declaration of ROOTFS_SKELETON_PRE_CMD_HOOK be conditional in the skeleton-systemd package, like:

    ifeq ($(BR2_PACKAGE_SKELETON_SYSTEMD),y)
    define ROOTFS_SKELETON_PRE_CMD_HOOK
        blabla
    endef
    endif

But I do not like these kind of constructs in the packages themselves, because they are too easy to miss...

Alternatively, I would rather move that logic into the filesystem code itself, because after all that is its responsibility, and guard its use with something like (with a proper variable name, of course):

    ifeq ($(ROOTFS_VAR_FACTORY_BLABLA),YES)
        blabla...
    endif

And then have the packages that need it just set:
ROOTFS_VAR_FACTORY_BLABLA = YES

All that, because Thomas already suggested (last year in Toulouse) that we could re-use the same factory mechanism for the other init systems as well (on read-only rootfs). In this case, the code would then be guarded by an test against an empty $(BR2_TARGET_GENERIC_REMOUNT_ROOTFS_RW), because then there would be no relation to an init system at all.

But the tmpfiles format is very specific to systemd; we'd have to write our own parser for that (not too difficult, I think, but it'd run on the target so it'd have to be really dead-simple).

So in the end, I'd remove the hooks and move the code to the fs handling, protected by a conditional that so far only systemd would set.

OK with this new approach?

Still, there is yet ine thing that I dislike in this solution, whether it be with hooks or the variable, and that noone has seen so far: it is not atomic and the target/ directory will be broken if the filesystem command fails right in the middle, before the var symlink was restored.

I have seen no solution to this issue, except doing a copy of target first, right before assembling the image(s), becasue in that case, we wouldn't care about restoring the previous status, so we would no longer need atomicity.

Quoted 3 lines

Arnout, All,

On 2017-07-23 15:42 +0200, Arnout Vandecappelle spake thusly:

[snip]

Quoted 7 lines

Quoted 3 lines

	$$(call PRINTF,$$(ROOTFS_SKELETON_PRE_CMD_HOOK)) >> $$(FAKEROOT_SCRIPT)

You may want to change the variable name, obviously.

And it also means that the declaration of ROOTFS_SKELETON_PRE_CMD_HOOK be conditional in the skeleton-systemd package, like:

Quoted 8 lines

    ifeq ($(BR2_PACKAGE_SKELETON_SYSTEMD),y)
    define ROOTFS_SKELETON_PRE_CMD_HOOK
        blabla
    endef
    endif

But I do not like these kind of constructs in the packages themselves, because they are too easy to miss...

I'm not sure if it's that hard to miss. You just grep for
ROOTFS_SKELETON_PRE_CMD_HOOK to get the context.

Quoted 4 lines

Alternatively, I would rather move that logic into the filesystem code itself, because after all that is its responsibility, and guard its use with something like (with a proper variable name, of course):

Quoted 6 lines

    ifeq ($(ROOTFS_VAR_FACTORY_BLABLA),YES)
        blabla...
    endif

And then have the packages that need it just set:
ROOTFS_VAR_FACTORY_BLABLA = YES

I'm not so sure I like that better...

Quoted 5 lines

All that, because Thomas already suggested (last year in Toulouse) that we could re-use the same factory mechanism for the other init systems as well (on read-only rootfs). In this case, the code would then be guarded by an test against an empty $(BR2_TARGET_GENERIC_REMOUNT_ROOTFS_RW), because then there would be no relation to an init system at all.

You completely lost me here.

Quoted 4 lines

But the tmpfiles format is very specific to systemd; we'd have to write our own parser for that (not too difficult, I think, but it'd run on the target so it'd have to be really dead-simple).

Quoted 4 lines

So in the end, I'd remove the hooks and move the code to the fs handling, protected by a conditional that so far only systemd would set.

OK with this new approach?

I'm not so convinced. I mean, if you're putting systemd-specific code into
ROOTFS_TARGET_INTERNAL, you might just as well make that bit depend on BR2_INIT_SYSTEMD directly.

But then it becomes really too intertwined for my
liking. I think my ROOTFS_SKELETON_PRE_CMD_HOOK proposal is a better middle ground between complexity and abstraction. If you really don't like that, I prefer the original ROOTFS_PRE_CMD_HOOKS approach. I just think that giving the possibility of defining more than one hook is useless. Particularly since the systemd hook hinges on being the very last thing before rootfs creation.

Quoted 4 lines

Still, there is yet ine thing that I dislike in this solution, whether it be with hooks or the variable, and that noone has seen so far: it is not atomic and the target/ directory will be broken if the filesystem command fails right in the middle, before the var symlink was restored.

Can we create the symlink early in the build process, every time? I.e. run
SKELETON_SYSTEMD_POST_ROOTFS_VAR somehwere at the start of the build as well? Only, I'm not sure where we could put it.

Regards,
Arnout

Quoted 4 lines

I have seen no solution to this issue, except doing a copy of target first, right before assembling the image(s), becasue in that case, we wouldn't care about restoring the previous status, so we would no longer need atomicity.

Signature · 6 lines

Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

On 2017-07-23 23:51 +0200, Arnout Vandecappelle spake thusly:

So Thomas suggested that we apply the factory feature to all init systems. With systemd, it would be built-in (by mean of tmpfiles), while for the others we'd need some extra startup scripts to mount a tmpfs on /var and populate it.

Quoted 3 lines

Can we create the symlink early in the build process, every time? I.e. run
SKELETON_SYSTEMD_POST_ROOTFS_VAR somehwere at the start of the build as well? Only, I'm not sure where we could put it.

Hmm... Good point. I'll try to find a place. But I'm not sure that would be possible.

Maybe with the step-hooks, that are called very early for all steps, and then we'd use that to create the symlink if missing. But I do not like that too much...

I'll investigate.

In the meantime, I'll drop the read-only patches from the series, to avoid too much noise for now, and so we concentrate on the skeleton stuff for this release.

With a line like:

C /var/something - - - -

it will look for /usr/share/factory/something and copy it (recursively if it is a directory) to /var/something, but only if it does not already exist there.

Note that we treat symlinks a little bit specially, by creating symlinks to the factory defaults rather than copying them.

[snip]

Quoted 7 lines

With a line like:

C /var/something - - - -

it will look for /usr/share/factory/something and copy it (recursively if it is a directory) to /var/something, but only if it does not already exist there.

So we could just have a line

C /var - - - -

and be done with it?

[snip]

Quoted 2 lines

Note that we treat symlinks a little bit specially, by creating symlinks to the factory defaults rather than copying them.

You forgot to explain why. I guess because a relative symlink that was created
based on the /usr/share/factory location wouldn't work any more.

However, it is possible that a package creates a relative symlink based on the
/var location. Most symlinks that we create in Buildroot stupidly assume that all the path components are not symlinks. So a package could for example do

ln -sf ../srv/www $DESTDIR/var/www

This will now create a broken symlink. It's fine as long as it is copied as-is to /var, but not with your approach...

[snip]

package/skeleton-systemd/skeleton-systemd.mk SKELETON_SYSTEMD_DEPENDENCIES = skeleton-common · line 35

Quoted 4 lines

package/skeleton-systemd/skeleton-systemd.mk +4 -0

+define SKELETON_SYSTEMD_ROOT_RO_OR_RW
+	mkdir -p $(TARGET_DIR)/etc/systemd/tmpfiles.d
+	mkdir -p $(TARGET_DIR)/usr/share/factory
+	ln -s usr/share/factory $(TARGET_DIR)/var

Since other stuff may also want to put things in /usr/share/factory, I think it
should be

	mkdir -p $(TARGET_DIR)/usr/share/factory/var
	ln -s usr/share/factory/var $(TARGET_DIR)/var

Regards,
Arnout

[snip]

Signature · 6 lines

Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

Arnout, All,

On 2017-07-24 00:18 +0200, Arnout Vandecappelle spake thusly:

Quoted 12 lines · Yann E. MORIN

[snip]

Quoted 7 lines

With a line like:

C /var/something - - - -

it will look for /usr/share/factory/something and copy it (recursively if it is a directory) to /var/something, but only if it does not already exist there.

So we could just have a line

Quoted 3 lines

C /var - - - -

and be done with it?

No, because systemd-tmpfiles will only act if the entry does not exist. But our case, /var does exist, and it is a mount point. We must have /var to exist, because / is read-only.

Quoted 8 lines

However, it is possible that a package creates a relative symlink based on the
/var location. Most symlinks that we create in Buildroot stupidly assume that all the path components are not symlinks. So a package could for example do

ln -sf ../srv/www $DESTDIR/var/www

This will now create a broken symlink. It's fine as long as it is copied as-is to /var, but not with your approach...

Hmm... I'll have to think about that a bit more, even though I think I have taken that into account...

At least I did see no breakage so far, but the set of packages I built was arguably rather limited...

Quoted 10 lines

[snip]

package/skeleton-systemd/skeleton-systemd.mk SKELETON_SYSTEMD_DEPENDENCIES = skeleton-common · line 35

Quoted 4 lines

package/skeleton-systemd/skeleton-systemd.mk +4 -0

+define SKELETON_SYSTEMD_ROOT_RO_OR_RW
+	mkdir -p $(TARGET_DIR)/etc/systemd/tmpfiles.d
+	mkdir -p $(TARGET_DIR)/usr/share/factory
+	ln -s usr/share/factory $(TARGET_DIR)/var

Since other stuff may also want to put things in /usr/share/factory, I think it
should be

	mkdir -p $(TARGET_DIR)/usr/share/factory/var
	ln -s usr/share/factory/var $(TARGET_DIR)/var

Not really, because systemd-tmpfile will look in exactly the factory path to find the source of the copy/link. See the exerpt I pasted above.

So, given this entry, for example:

    C /var/foo - - - -

systemd-tmpfiles will see if it can find /usr/share/factory/foo not /usr/share/factory/var/foo

And if other packages install things in factory, they would most probably install their own tmpfile description to copy it in place on boot, becasue factory *is* for factory settings.

Quoted 2 lines

On 2017-07-24 00:18 +0200, Arnout Vandecappelle spake thusly:

Quoted 1 line · Yann E. MORIN

Thinking out of the box: can we make /var a symlink to /tmp/var, and avoid
creating a tmpfs on /var at all? Since /tmp/var doesn't exist, "C! /tmp/var" should work, no? Or did that cause journald to bail out?

There is unfortunately no way to fix this. If the package is smart, it does
calculates the relative path based on the real path of the source and destination. If the package is stupid, it just assumes ../something. Whatever we do, one of them will break... The only safe approach would be to put stuff in /factory instead of /usr/share/factory.

Quoted 5 lines

Not really, because systemd-tmpfile will look in exactly the factory path to find the source of the copy/link. See the exerpt I pasted above.

So, given this entry, for example:

Quoted 4 lines

    C /var/foo - - - -

systemd-tmpfiles will see if it can find /usr/share/factory/foo not /usr/share/factory/var/foo

Weird, the way I read both the man page and the source code seem to suggest
that "C /var/something" will copy from /usr/share/factory/var/something... But I guess you tested that so I must be misreading the source code.

Arnout, All,

On 2017-07-25 00:44 +0200, Arnout Vandecappelle spake thusly:

Quoted 3 lines · Yann E. MORIN

Quoted 2 lines

On 2017-07-24 00:18 +0200, Arnout Vandecappelle spake thusly:

Quoted 1 line · Yann E. MORIN

[--SNIP--]

Quoted 3 lines

Thinking out of the box: can we make /var a symlink to /tmp/var, and avoid
creating a tmpfs on /var at all? Since /tmp/var doesn't exist, "C! /tmp/var" should work, no? Or did that cause journald to bail out?

Worth a try, of course. But still we'd need hoks of some kind to allow package to install in /var at build time, and move that to factory at the end of the build, while still preserving the ability to be able to rebuild stuff without requiring a 'make clean'.

[--SNIP symlinks in /var--]

Quoted 5 lines

There is unfortunately no way to fix this. If the package is smart, it does
calculates the relative path based on the real path of the source and destination. If the package is stupid, it just assumes ../something. Whatever we do, one of them will break... The only safe approach would be to put stuff in /factory instead of /usr/share/factory.

But that is not where systemd-tmpfiles will look at...

Unless you meant we have /factory during the build, then move it at the moment we aggregaate the images, and move it back (moving the problem from handling /var to handling /factory instead) ?

However (thinking aloud now), I have another tentative to do, which instead of creating a symlink for /var at runtime, would just creat it as a direcotry as expected.

Then the hooks would simply "mv /var /usr/share/factory/var; mkdir /var", create the tmpfiles entries, aggregate the image(s), the rm -rf /var; mv /usr/share/factory/var /var".

This is probably much simpler than moving the entries one by one.

The only drawback in this case would be a package that already creates a factory/var/foo entry. It would end up in /var/foo afterwards... :-/ So I think the /var symlink is better, in fact, so I'll keep that.

The "builtin" kernel does not boot a systemd-based system, so we resort to building the same one as currently used by our qemu_arm_vexpress_defconfig.

We test the 11 following combinations:

- busybox, read-only, without network
- busybox, read-only, with network
- busybox, read-write, without network
- busybox, read-write, with network

- basic systemd, read-only, network w/ ifupdown
- basic systemd, read-only, network w/ networkd
- basic systemd, read-write, network w/ ifupdown
- basic systemd, read-write, network w/ networkd

- full systemd, read-only, network w/ networkd
- full systemd, read-write, network w/ networkd

- no init system, read-only, without network

The tests just verify what the /sbin/init binary is, and that we were able to grab an IP address. More tests can be added later, for example to check each systemd features (journal, tmpfiles...)

Signed-off-by "Yann E. MORIN" <[email protected]>

Cc Thomas Petazzoni <[email protected]>

+ def startEmulator(self, fs_type, kernel=None, dtb=None, init=None):

support/testing/tests/init/test_none.py new file +4 -0

+class TestInitSystemNone(InitSystemBase):
+    config = infra.basetest.BASIC_TOOLCHAIN_CONFIG + \
+        """
+        BR2_INIT_NONE=y

I think this patch should go in right away, even before the skeleton/systemd
fixes. Yes, that may make some tests fail, but that's fine.

Quoted 16 lines

The "builtin" kernel does not boot a systemd-based system, so we resort to building the same one as currently used by our qemu_arm_vexpress_defconfig.

We test the 11 following combinations:

- busybox, read-only, without network
- busybox, read-only, with network
- busybox, read-write, without network
- busybox, read-write, with network

- basic systemd, read-only, network w/ ifupdown
- basic systemd, read-only, network w/ networkd
- basic systemd, read-write, network w/ ifupdown
- basic systemd, read-write, network w/ networkd

Quoted 4 lines

- full systemd, read-only, network w/ networkd
- full systemd, read-write, network w/ networkd

- no init system, read-only, without network

Given that we're testing init here, it would be nice to also add tests with the
cpio filesystem, which runs a shell script before starting /sbin/init.

It's probably sufficient to just do the maximal test with this scenario. Oh, and a test with sysvinit would also be nice...

Quoted 6 lines

The tests just verify what the /sbin/init binary is, and that we were able to grab an IP address. More tests can be added later, for example to check each systemd features (journal, tmpfiles...)

Signed-off-by "Yann E. MORIN" <[email protected]>

Cc Thomas Petazzoni <[email protected]>

I have a bunch of comments, but nothing critical so

Reviewed-by Arnout Vandecappelle (Essensium/Mind) <[email protected]>

[snip]

Quoted 1 line

+ def startEmulator(self, fs_type, kernel=None, dtb=None, init=None):

PEP8 says snake-case for functions. I'm not sure if the rest of the testing
infra satisfies this, but let's try to not make things worse.

support/testing/tests/init/base.py line 9

Quoted 2 lines

support/testing/tests/init/base.py new file +2 -0

+        img = os.path.join(self.builddir, "images", "rootfs.{}".format(fs_type))
+        subprocess.call(["truncate", "-s", "%1M", img])

It's a bit weird to do that here, but I see no better way either.

support/testing/tests/init/base.py line 18

Quoted 9 lines

support/testing/tests/init/base.py new file +9 -0

+
+        options = ["-drive",
+                   "file={},if=sd,format=raw".format(img),
+                   "-M", "vexpress-a9"]
+
+        if kernel is None:
+            kernel = "builtin"
+        else:
+            kernel = os.path.join(self.builddir, "images", kernel)

This condition should probably be refactored into emulator.boot itself -
although admittedly that doesn't have a reference to builddir at the moment.

support/testing/tests/init/base.py line 20

Quoted 2 lines

support/testing/tests/init/base.py new file +2 -0

+            options.extend(["-dtb", os.path.join(self.builddir, "images",
+                                             "{}.dtb".format(dtb))])

This should only be done if dtb is not None.

support/testing/tests/init/base.py line 29

Quoted 9 lines

support/testing/tests/init/base.py new file +9 -0

+
+        kernel_cmdline = ["root=/dev/mmcblk0",
+                          "rootfstype={}".format(fs_type),
+                          "rootwait",
+                          "ro",
+                          "console=ttyAMA0"]
+
+        if not init is None:
+            kernel_cmdline.extend(["init={}".format(init)])

Just a single element, so append() instead of extend().

support/testing/tests/init/base.py line 37

Quoted 8 lines

support/testing/tests/init/base.py new file +8 -0

+
+        self.emulator.boot(arch="armv7",
+                           kernel=kernel,
+                           kernel_cmdline=kernel_cmdline,
+                           options=options)
+
+        if init is None:
+            self.emulator.login()

This looks weird so warrants a comment: