[Buildroot] [PATCH] package/openrc: new init package OpenRC (0.38.3)

Arnout Vandecappelle arnout at mind.be
Wed Mar 6 09:58:09 UTC 2019



On 06/03/2019 00:20, michal.lyszczek at bofc.pl wrote:
> Yann, Arnout
> 
> First, thank you for your extensive review. Now to the point.
> 
> On 2019-03-05 22:38:58, Yann E. MORIN wrote:
>> Michał, Arnout,
>>
>> On 2019-03-04 19:59 +0100, Arnout Vandecappelle spake thusly:
[snip]
>>> On 28/02/2019 20:44, Michał Łyszczek wrote:
>>>> OpenRC is a dependency based init system that works with the system
>>>> provided init program, normally located at /sbin/init. It is not a
>>>> replacement for /sbin/init.
>>>>
>>>> Signed-off-by: Michał Łyszczek <michal.lyszczek at bofc.pl>
>>>> ---
>>>> OpenRC uses own set of scripts for sysinit thus
>>>> BR2_PACKAGE_INITSCRIPTS is disabled. I decided to add new init
>>>> choice and choice which program to use as PID1 since OpenRC uses
>>>> its own scripts for services so S** scripts cannot be used. It is
>>>> possible to add simple script to local.d that would start all S**
>>>> scripts, but that would diminish OpenRC's features and usefullness,
>>>> so additional startup scripts should be written and added to
>>>> buildroot.
>>>
>>>  While this is true, I think you should still add that script so that packages
>>> which don't have an OpenRC service description will still work. (We should also
>>> have done that for systemd IMO.)
>>
>> Not needed, as it is my understanding that systemd autoimatically
>> creates units for scripts in /etc/init.d/ when there is no corresponding
>> native unit. From http://man7.org/linux/man-pages/man1/systemd.1.html :
>>
>>     systemd is compatible with the SysV init system to a large degree:
>>     SysV init scripts are supported and simply read as an alternative
>>     (though limited) configuration file format. The SysV /dev/initctl
>>     interface is provided, and compatibility implementations of the
>>     various SysV client tools are available. In addition to that, various
>>     established Unix functionality such as /etc/fstab or the utmp
>>     database are supported.

 Yes, but at the moment we don't install the sysvinit scripts so systemd is not
going to execute them.

 Ideally we'd also have a runtime test that checks if the sysvinit script indeed
gets called.

>>
>>> Then you can have something like thisin
>>> pkg-generic.mk:
>>>
>>> $(2)_INSTALL_INIT_OPENRC ?= $$($(2)_INSTALL_INIT_SYSV)
>>>
>>> so if a package defines its OpenRC file, that one will be used instead of the
>>> sysvinit script.
>>
>> Is that works so easily, then this is indeed a good idea.
>>
>>>  With that in mind, I think an OpenRC skeelton would make sense at well. It
>>> would be pretty empty :-).
>>
>> Except maybe for that one script that does the sysv-init fallback.
> 
> Ok, how about this:

 Er, the following paragraph is completely unrelated to what we were discussing
above... Did you understand what we were talking about above?


> by default OpenRC installs and enables many services that
> are not necessarily needed, like setting hwclock, configuring network
> interfaces, activating swap, setting keymaps. Some of these services fails or
> may fail due to missing network interfaces, or due to missing additional
> programs like loadkeys for setting keymaps. Also I notices that sysctl fails
> with BusyBox version since it is missing "--system" option. These are not
> critical and OpenRC still boots up and spawns tty with login - but it prints
> error and thus is ugly and may confuse users.

 For services that are provided by busybox, it will be difficult to know if a
custom busybox config enables them or not... Anyway, I think this is something
that is better solved in a follow-up patch (which you can propose in the same
series, of course).

> So my proposition would be to not perform default 'make install' but install
> only bare minimum set of init scrips, that are sure not to fail. 

 Instead of doing a custom install, I prefer to just do the default install and
remove things are not needed / wanted.

> And scripts
> that are not compatible with default buildroot installation could land in
> skeleton.

 Instead you can make the removal conditional.


> One notable example is "agetty". It's not like it's some hardcore dependency for
> OpenRC, but it uses it by default to spawn shell, and agetty is not compatible
> with busybox due to one change in order:
> 
> agetty [options] port [baud_rate...] [term]
> getty [OPTIONS] BAUD_RATE TTY [TERMTYPE]
> 
> See, baud rate is swaped with port. So I could write custom getty service for
> OpenRC and put it into skeleton. That would lift agetty dependency.

 Yeah, again, you can do this conditionally on whether agetty is enabled or not.
But indeed in that case it's better to put it in a skeleton package and not in
the openrc package itself, so you can get rid of it by using a custom skeleton.


> What might be best way to install only chosed files?

 In the .mk file (of the skeleton-init-openrc package), don't use rsync but
instead do conditional $(INSTALL) calls. Something like:

ifeq ($(BR2_PACKAGE_UTIL_LINUX_AGETTY),)
define SKELETON_INIT_OPENRC_INSTALL_GETTY
	$(INSTALL) -D -m 0644 $(@D)/getty $(TARGET_DIR)/etc/conf.d/getty
	$(RM) $(TARGET_DIR)/etc/conf.d/agetty
endef
endif

define SKELETON_INIT_OPENRC_INSTALL_TARGET_CMDS
	...
	$(SKELETON_INIT_OPENRC_INSTALL_GETTY)
endef

 You can also do it with a POST_INSTALL_HOOK if you prefer that.

> 
> - Do 'make install' to temp directory, remove what is not needed than copy to
> target?
> - Invoke "install" for each file? There are quite of them.
> - Do normal 'make install' then call 'rm' on each uneeded files in target_dir?

 Yep, that's the one.

> - or maybe there is some easy way of doing it I didn't think about?
> 
>>>> If OpenRC init is accepted into Buildroot, I will commit
>>>> some time to port existing sysvinit startup scripts to OpenRC.
>>>>
>>>> OpenRC works with default config - a single change is needed (which
>>>> init system to use, with optional PID1 change - default BusyBox). I
>>>> tested it with both BusyBox and sysvinit as PID1. Also tested with
>>>> BusyBox programs enabled and disabled (standalone apps used, none
>>>> of which was from BusyBox package). I tested it using
>>>> qemu-x86_64_defconfig and beaglebone_defconfig.
>>>
>>>  If this gets added as a full BR2_INIT_ option, you should also add the runtime
>>> tests for it (in separate patches, obviously). Look at the system tests for
                                                               ^^^^^^
                                                I meant systemd here.

>>> inspiration.
>>
>> ... located in support/testing/tests/init/.
>>
>> [--SNIP--]
>>>> +++ b/package/openrc/Config.in
>>>> @@ -0,0 +1,19 @@
>>>> +config BR2_PACKAGE_OPENRC
>>>> +	bool "OpenRC"
>>>> +	select BR2_PACKAGE_UTIL_LINUX
>>>> +	select BR2_PACKAGE_UTIL_LINUX_AGETTY
>>>
>>>  Does it really depend on agetty? Seems strange...
> 
> It does not, answer is above. I wanted to preserve as much upstream stuff as
> possible - but maybe that is not the way?

 So we found a way to get rid of the agetty dependency. Still, better do that in
a follow-up patch and keep the initial patch as simple as possible.

 For sure, it should not be the openrc package itself that selects it, because
it is only needed if you actually want to use openrc to start a getty. So it
belongs to a skeleton package instead.

> 
>>>  That said, if additional tweaks are needed to support busybox getty, that can
>>> be done in a follow-up patch.
>>
>> We have a similar workaround for systemd, but I just sent a patch to
>> actually remove it:
>>     https://patchwork.ozlabs.org/patch/1051821/
> 
> As stated above, I could create simple service that would use getty instead of
> agetty and put it in skeleon.
> 
>> If OpenRC does not require util-linux, it would be acceptable to have a
>> workaround to be able to use busybox' getty instead. But if OpenRC
>> otherwise requires util-linux (e.g. for lib"stuff"), then there is no
>> point for such a workaround.
> 
> OpenRC does not need util-linux.
> 
>>>> +	OPENRC_MAKE_OPTS += MKZSHCOMP=yes
>>>> +endif
>>>> +
>>>> +ifeq ($(BR2_SHARED_LIBS),y)
>>>> +	OPENRC_MAKE_OPTS += MKSTATICLIBS=no
>>>> +else
>>>> +	OPENRC_MAKE_OPTS += MKSTATICLIBS=yes
>>>> +endif
>>>> +
>>>> +OPENRC_MAKE_OPTS += TARGETDIR=$(TARGET_DIR)
>>
>> Arnout, TARGETDIR comes from here. Although I admit it is confusing...
> 
> This patch was introduced because OpenRC determines location for its libs,
> wheter to put them in /lib or /lib64 directory, by reading link of /lib.
> Unfortunately it has "/lib" path hardcoded and buildroot is reading that
> link from host os, so libs generated for cross x86 could land in lib64 on amd64
> hosts. I will rise this issue to OpenRC's author - maybe there is already
> solution for this and I've just invented wheel again.

 Yes but as I mentioned somewhere else in my review: if this is only used at
install time and not at build time then you should use $(DESTDIR)/$(PREFIX) and
not some new TARGETDIR variable.

 Oh and don't forget to send that patch upstream for review.

> 
>>>> +OPENRC_MAKE_OPTS += MKNET=yes
>>>> +OPENRC_MAKE_OPTS += MKPKGCONFIG=no
>>>> +OPENRC_MAKE_OPTS += MKSELINUX=no
>>>> +OPENRC_MAKE_OPTS += MKSYSVINIT=yes
>>
>> When doing a sequence, I's nicer to write:
>>
>>     OPENRC_MAKE_OPTS += \
>>         TARGETDIR=$(TARGET_DIR) \
>>         MKNET=yes \
>>         MKPKGCONFIG=no \
>>         [...]

 Actually, I disagree that it is nicer, and we already have both patterns in
Buildroot.

>>
>>>> +OPENRC_MAKE_OPTS += BRANDING=$(BR2_OPENRC_BRANDING)
>>>  We typically use an explicit qstrip and adding quotes here. That way, it works
>>> correctly even if you override it from the command line or in local.mk.
>>
>> To be homest, I don;t thionk overriding frm the command-line should be
>> suported that nicely, because it is not reproducible. It may work for a
>> quicky, but how much more complex is it to go to menuconfig, or edit
>> .config ?
>>>> +	ln -sf /etc/init.d/agetty.$(SYSTEM_GETTY_PORT) $(TARGET_DIR)/etc/runlevels/default
>>>> +	$(SED) '/#term_type=/c\term_type="$(SYSTEM_GETTY_TERM)"' $(TARGET_DIR)/etc/conf.d/agetty
>>>> +	$(SED) '/#agetty_options=/c\agetty_options="$(SYSTEM_GETTY_OPTIONS)"' $(TARGET_DIR)/etc/conf.d/agetty
>>>
>>>  We usually do this kind of thing in a single SED invocation, with -e to specify
>>> each command.
>>
>> ... except for the first, as $(SED) already ends with -e.
>>
>> (and I find it ugly indeed, because it makes the first pattern different
>> from the others... Bleark...)

 Eek, bleark indeed... Why do we have that -e in $(SED)?

>>
>>>> +config BR2_OPENRC_BRANDING
>>>> +	string "OpenRC branding"
>>>> +	default "Buildroot"
>>>> +	depends on BR2_INIT_OPENRC
>>>> +	help
>>>> +	  Custom string that will show up when OpenRC starts like:
>>>> +
>>>> +	  OpenRC 0.38.3.8ee04a5 is starting up <branding-name> (armv7l)
>>>
>>>  I really don't think it's worth to add a config option for this. Would it be
>>> possible to reuse one of the existing options, e.g. BR2_TARGET_GENERIC_HOSTNAME
>>> or BR2_TARGET_GENERIC_ISSUE? Or alternatively, just leave branding empty?
>>
>> Agreed.
>>
> 
> That field was not designed to place hostname there but os branding. For
> example, "GNU/Linux Debian" would be good example of branding, since multiple
> hardware with different hostnames can use single OS. Of course it's just for
> eye-candy but I would prefer to hardcode it to "Buildroot" (or empty) than put
> hostname or issue here.

 For gcc we have:

        --with-pkgversion="Buildroot $(BR2_VERSION_FULL)" \
        --with-bugurl="http://bugs.buildroot.net/"

so using the same for openrc seems appropriate.

 Maybe we should introduce a global config + variable to control this, i.e. the
NAME and PRETTY_NAME in /etc/os-release.


>>>  If it is a config option, I think it is more appropriate as a suboption of the
>>> openrc package. We do that for the systemd suboptions as well.
>>
>> I don't think it has to be a new option, BR2_TARGET_GENERIC_ISSUE is
>> exactly there for this kind of usage.
>>
>>>> +choice
>>>> +	prompt "PID1 to use with OpenRC"
>>>> +	default BR2_OPENRC_PID1_BUSYBOX
>>>> +	help
>>>> +	  Since OpenRC is not replacement for /sbin/init but works with
>>>> +	  it, you can select which PID1 service you want to run
>>>> +
>>>> +config BR2_OPENRC_PID1_BUSYBOX
>>>
>>>  Since this is a suboption of BR2_INIT_OPENRC, it should be called
>>> BR2_INIT_OPENRC_PID1_BUSYBOX. But I think the PID1 bit is a bit redundant.
>>
>> I think this should be a sub-option of the openrc package, not the 'init'
>> part.

 I disagree.

 I actually disagree that the various init packages depend on PID1 coming from
that particular package. There could be use cases to have a different init
system (usually _NONE) and still have the init executable there.

 For openrc, this is doubly so, because openrc is not a standalone init system.
So it can very obviously be used with a different PID1.

 So IMO this choice really belongs to the system config.


 Regards,
 Arnout


>>>> +endif # BR2_INIT_OPENRC
>>>> +
>>>>  choice
>>>>  	prompt "/dev management" if !BR2_INIT_SYSTEMD
>>>>  	default BR2_ROOTFS_DEVICE_CREATION_DYNAMIC_DEVTMPFS
>>>> @@ -337,16 +376,16 @@ config BR2_TARGET_GENERIC_GETTY_BAUDRATE
>>>>  config BR2_TARGET_GENERIC_GETTY_TERM
>>>>  	string "TERM environment variable"
>>>>  	default "vt100"
>>>> -	# currently observed only by busybox and sysvinit
>>>> -	depends on BR2_INIT_BUSYBOX || BR2_INIT_SYSV
>>>> +	# currently observed only by busybox, sysvinit and openrc
>>>> +	depends on BR2_INIT_BUSYBOX || BR2_INIT_SYSV || BR2_INIT_OPENRC
>>>
>>>  Perhaps this should be changed into !BR2_INIT_SYSTEMD - that's actually what it
>>> is at the moment.
>>>
>>>  Come to think of it, BR2_TARGET_GENERIC_GETTY should depend on !BR2_INIT_NONE
>>> as well.
>>
>> Probably, indeed... I missed that one when doing my big overhaul of that
>> section.
> 
> I agree with statements where I didn't leave comment and will apply proposed
> fixes.
> 
> Thanks again for review.
> 


More information about the buildroot mailing list