| Message ID | 1413643624-14757-4-git-send-email-maxime.hadjinlian@gmail.com |
|---|---|
| State | Rejected |
| Headers | show |
Dear Maxime Hadjinlian, On Sat, 18 Oct 2014 16:47:02 +0200, Maxime Hadjinlian wrote: > +ifeq ($(BR2_INIT_SYSTEMD),y) > + $(Q)if test -n "$($(PKG)_INIT_SYSTEMD_FILES)" ; then \ > + mkdir -p $(TARGET_DIR)/etc/systemd/system/multi-user.target.wants ; \ > + for s in $($(PKG)_INIT_SYSTEMD_FILES); do \ > + f=$$(basename $${s}); \ > + $(INSTALL) -D -m 644 $${s} $(TARGET_DIR)/lib/systemd/system/$${f} ; \ > + ln -fs /lib/systemd/system/$${f} \ > + $(TARGET_DIR)/etc/systemd/system/multi-user.target.wants/$${f} ; \ > + done ; \ > + fi > + $($(PKG)_INSTALL_INIT_SYSTEMD) > +endif > +ifeq ($(BR2_INIT_SYSV)$(BR2_INIT_BUSYBOX),y) > + $(Q)if test -n "$($(PKG)_INIT_SYSV_FILES)" ; then \ > + for s in $($(PKG)_INIT_SYSV_FILES); do \ > + f=$$(basename $${s}) ; \ > + $(INSTALL) -D -m 0755 $${s} $(TARGET_DIR)/etc/init.d/$${f} ; \ > + done ; \ > + fi > + $($(PKG)_INSTALL_INIT_SYSV) > +endif Why is an explicit variable necessary ? Why not simply install all the S* and K* files from the package directory for sysvinit/busybox and all the .service (or some extension) of the package directory for systemd ? I think we discussed this at the meeting, no? Best regards, Thomas
Hi Thomas, all On Sat, Oct 18, 2014 at 6:56 PM, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > Dear Maxime Hadjinlian, > > On Sat, 18 Oct 2014 16:47:02 +0200, Maxime Hadjinlian wrote: > >> +ifeq ($(BR2_INIT_SYSTEMD),y) >> + $(Q)if test -n "$($(PKG)_INIT_SYSTEMD_FILES)" ; then \ >> + mkdir -p $(TARGET_DIR)/etc/systemd/system/multi-user.target.wants ; \ >> + for s in $($(PKG)_INIT_SYSTEMD_FILES); do \ >> + f=$$(basename $${s}); \ >> + $(INSTALL) -D -m 644 $${s} $(TARGET_DIR)/lib/systemd/system/$${f} ; \ >> + ln -fs /lib/systemd/system/$${f} \ >> + $(TARGET_DIR)/etc/systemd/system/multi-user.target.wants/$${f} ; \ >> + done ; \ >> + fi >> + $($(PKG)_INSTALL_INIT_SYSTEMD) >> +endif >> +ifeq ($(BR2_INIT_SYSV)$(BR2_INIT_BUSYBOX),y) >> + $(Q)if test -n "$($(PKG)_INIT_SYSV_FILES)" ; then \ >> + for s in $($(PKG)_INIT_SYSV_FILES); do \ >> + f=$$(basename $${s}) ; \ >> + $(INSTALL) -D -m 0755 $${s} $(TARGET_DIR)/etc/init.d/$${f} ; \ >> + done ; \ >> + fi >> + $($(PKG)_INSTALL_INIT_SYSV) >> +endif > > Why is an explicit variable necessary ? Why not simply install all the > S* and K* files from the package directory for sysvinit/busybox and all > the .service (or some extension) of the package directory for systemd ? > I think we discussed this at the meeting, no? Indeed, but as stated in the cover letter, we need to have a variable, because, some packages only install their init scripts conditionally (take ntpd for example). So you can't blindly install files only by maching their names. Also, the current trend is that applications have their init scripts in the sources, and we don't want to keep a copy if in our packages, we want to take it straight from the sources, having such a variable enable that. > > Best regards, > > Thomas > -- > Thomas Petazzoni, CTO, Free Electrons > Embedded Linux, Kernel and Android engineering > http://free-electrons.com
Dear Maxime Hadjinlian, On Sat, 18 Oct 2014 16:47:02 +0200, Maxime Hadjinlian wrote: > +ifeq ($(BR2_INIT_SYSV)$(BR2_INIT_BUSYBOX),y) > + $(Q)if test -n "$($(PKG)_INIT_SYSV_FILES)" ; then \ > + for s in $($(PKG)_INIT_SYSV_FILES); do \ > + f=$$(basename $${s}) ; \ > + $(INSTALL) -D -m 0755 $${s} $(TARGET_DIR)/etc/init.d/$${f} ; \ > + done ; \ > + fi > + $($(PKG)_INSTALL_INIT_SYSV) > +endif Another comment: you're forcefully installing the init script here, while many of our packages (but not all!) test if the script is already installed before installing it. The use case for this is to allow a custom version of the script to be part of the filesystem skeleton, and therefore to not see it being overridden by a package. I personally don't care very much about this use case, as I believe people should rather use rootfs overlays, and also because it is anyway not full-proof: while we can prevent the Buildroot package logic from overwriting files in the root filesystem, we cannot prevent the build system of each package from overwriting files. But it's something that needs to be discussed and decided. Peter ? Best regards, Thomas
Dear Maxime Hadjinlian, On Sat, 18 Oct 2014 18:59:03 +0200, Maxime Hadjinlian wrote: > > Why is an explicit variable necessary ? Why not simply install all the > > S* and K* files from the package directory for sysvinit/busybox and all > > the .service (or some extension) of the package directory for systemd ? > > I think we discussed this at the meeting, no? > Indeed, but as stated in the cover letter, we need to have a variable, > because, some packages only install their init scripts conditionally > (take ntpd for example). Well, we could by default install S* and K* files, except if the <pkg>_INIT_SYSV_FILES variable is defined, in which case we only install the ones that are mentioned. Maybe it's making the logic too complicated, I don't know. > So you can't blindly install files only by maching their names. > Also, the current trend is that applications have their init scripts > in the sources, and we don't want to keep a copy if in our packages, > we want to take it straight from the sources, having such a variable > enable that. For systemd, I would expect the package build system to install the unit files by itself, no? And for init scripts, we generally don't use the init scripts provided by the package itself, because they're often too complicated / not compatible with the simple Busybox init. But, well, maybe you're right and making things explicit is better. Thomas
Hi Thomas, all On Sat, Oct 18, 2014 at 7:06 PM, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > Dear Maxime Hadjinlian, > > On Sat, 18 Oct 2014 18:59:03 +0200, Maxime Hadjinlian wrote: > >> > Why is an explicit variable necessary ? Why not simply install all the >> > S* and K* files from the package directory for sysvinit/busybox and all >> > the .service (or some extension) of the package directory for systemd ? >> > I think we discussed this at the meeting, no? >> Indeed, but as stated in the cover letter, we need to have a variable, >> because, some packages only install their init scripts conditionally >> (take ntpd for example). > > Well, we could by default install S* and K* files, except if the > <pkg>_INIT_SYSV_FILES variable is defined, in which case we only > install the ones that are mentioned. Maybe it's making the logic too > complicated, I don't know. We could that, but as you say, maybe it makes things a little more complex than they should be. > >> So you can't blindly install files only by maching their names. >> Also, the current trend is that applications have their init scripts >> in the sources, and we don't want to keep a copy if in our packages, >> we want to take it straight from the sources, having such a variable >> enable that. > > For systemd, I would expect the package build system to install the > unit files by itself, no? I would expect that too yes. It would be quite nice actually. > > And for init scripts, we generally don't use the init scripts provided > by the package itself, because they're often too complicated / not > compatible with the simple Busybox init. > > But, well, maybe you're right and making things explicit is better. It's up for discussion as always :). The intention here, is mainly to remove duplicated code and have some enforced rules as to where the init files should be installed. > > Thomas > -- > Thomas Petazzoni, CTO, Free Electrons > Embedded Linux, Kernel and Android engineering > http://free-electrons.com
Dear Maxime Hadjinlian, On Sat, 18 Oct 2014 19:11:37 +0200, Maxime Hadjinlian wrote: > > Well, we could by default install S* and K* files, except if the > > <pkg>_INIT_SYSV_FILES variable is defined, in which case we only > > install the ones that are mentioned. Maybe it's making the logic too > > complicated, I don't know. > We could that, but as you say, maybe it makes things a little more > complex than they should be. Yeah, maybe you're right. What I'm proposing is too complicated, and what you suggest has the advantage of being completely explicit. Thomas
Hi, 2014-10-18 16:47 GMT+02:00 Maxime Hadjinlian <maxime.hadjinlian@gmail.com>: > Instead of copy pasting the install code over all our packages, you may > now define variables to specify where to take the init script files. > Each file will be installed in the default folder of the relevant init > system. > > For systemd, instead of installing the files into > $(TARGET_DIR)/etc/systemd/system, the file are now installed in > $(TARGET_DIR)/lib/systemd/system which appears to be the right things to > do. > > If you have specific operations to perform in order to install your init > scripts or any other file relevant to the init system (tmpfiles for > systemd), you can still define the usual FOO_INSTALL_INIT_SYSTEMD for > your specific actions. > > Also add the documentation explaining the mechanism. > > Signed-off-by: Maxime Hadjinlian <maxime.hadjinlian@gmail.com> > --- > docs/manual/adding-packages-generic.txt | 17 +++++++++++++++++ > package/pkg-generic.mk | 26 ++++++++++++++++++++++---- > 2 files changed, 39 insertions(+), 4 deletions(-) > > diff --git a/docs/manual/adding-packages-generic.txt b/docs/manual/adding-packages-generic.txt > index 67a7453..a0e51f2 100644 > --- a/docs/manual/adding-packages-generic.txt > +++ b/docs/manual/adding-packages-generic.txt > @@ -368,6 +368,23 @@ information is (assuming the package name is +libfoo+) : > FLAT binary format is only 4k bytes. If the application consumes more stack, > append the required number here. > > +* +LIBFOO_INIT_SYSV_FILES+ and +LIBFOO_INIT_SYSTEMD_FILES+ are > + a space-separated list of init scripts path. Respectively, for > + the systemV-like init systems (busybox, sysvinit, etc.) and for > + systemd. > + Theses init scripts files will only be installed when the relevant > + init system is installed (i.e. if systemd is selected as the init > + system in the configuration, only +LIBFOO_INIT_SYSTEMD_FILES+ will be > + installed). > + Each scripts will be installed in the default folder of the relevant > + init system. > + Note: This doesn't forbid you from defining +LIBFOO_INSTALL_INIT_SYSV+ and > + +LIBFOO_INSTALL_INIT_SYSTEMD+ if you need to perform specific actions. > + Theses variables are optionnal. > + Examples: + > + +LIBFOO_INSTALL_INIT_SYSV=package/libfoo/S99libfoo + > + +LIBFOO_INSTALL_INIT_SYSV=$(@D)/S99libfoo + > + > The recommended way to define these variables is to use the following > syntax: > > diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk > index 259ee02..1d90ae9 100644 > --- a/package/pkg-generic.mk > +++ b/package/pkg-generic.mk > @@ -229,10 +229,27 @@ $(BUILD_DIR)/%/.stamp_target_installed: > @$(call MESSAGE,"Installing to target") > $(foreach hook,$($(PKG)_PRE_INSTALL_TARGET_HOOKS),$(call $(hook))$(sep)) > +$($(PKG)_INSTALL_TARGET_CMDS) > - $(if $(BR2_INIT_SYSTEMD),\ > - $($(PKG)_INSTALL_INIT_SYSTEMD)) > - $(if $(BR2_INIT_SYSV)$(BR2_INIT_BUSYBOX),\ > - $($(PKG)_INSTALL_INIT_SYSV)) > +ifeq ($(BR2_INIT_SYSTEMD),y) > + $(Q)if test -n "$($(PKG)_INIT_SYSTEMD_FILES)" ; then \ > + mkdir -p $(TARGET_DIR)/etc/systemd/system/multi-user.target.wants ; \ A "set -e" or some "|| exit"s might be missing here. > + for s in $($(PKG)_INIT_SYSTEMD_FILES); do \ > + f=$$(basename $${s}); \ > + $(INSTALL) -D -m 644 $${s} $(TARGET_DIR)/lib/systemd/system/$${f} ; \ > + ln -fs /lib/systemd/system/$${f} \ > + $(TARGET_DIR)/etc/systemd/system/multi-user.target.wants/$${f} ; \ > + done ; \ > + fi > + $($(PKG)_INSTALL_INIT_SYSTEMD) > +endif > +ifeq ($(BR2_INIT_SYSV)$(BR2_INIT_BUSYBOX),y) > + $(Q)if test -n "$($(PKG)_INIT_SYSV_FILES)" ; then \ And here. > + for s in $($(PKG)_INIT_SYSV_FILES); do \ > + f=$$(basename $${s}) ; \ > + $(INSTALL) -D -m 0755 $${s} $(TARGET_DIR)/etc/init.d/$${f} ; \ > + done ; \ > + fi > + $($(PKG)_INSTALL_INIT_SYSV) > +endif > $(foreach hook,$($(PKG)_POST_INSTALL_TARGET_HOOKS),$(call $(hook))$(sep)) > $(Q)if test -n "$($(PKG)_CONFIG_SCRIPTS)" ; then \ > $(RM) -f $(addprefix $(TARGET_DIR)/usr/bin/,$($(PKG)_CONFIG_SCRIPTS)) ; \ > @@ -592,6 +609,7 @@ $(1)-reconfigure: $(1)-clean-for-reconfigure $(1) > # define the PKG variable for all targets, containing the > # uppercase package variable prefix > $$($(2)_TARGET_INSTALL_TARGET): PKG=$(2) > +$$($(2)_TARGET_INSTALL_TARGET): PKGDIR=$(pkgdir) > $$($(2)_TARGET_INSTALL_STAGING): PKG=$(2) > $$($(2)_TARGET_INSTALL_IMAGES): PKG=$(2) > $$($(2)_TARGET_INSTALL_HOST): PKG=$(2) > -- > 2.1.1 > > _______________________________________________ > buildroot mailing list > buildroot@busybox.net > http://lists.busybox.net/mailman/listinfo/buildroot
diff --git a/docs/manual/adding-packages-generic.txt b/docs/manual/adding-packages-generic.txt index 67a7453..a0e51f2 100644 --- a/docs/manual/adding-packages-generic.txt +++ b/docs/manual/adding-packages-generic.txt @@ -368,6 +368,23 @@ information is (assuming the package name is +libfoo+) : FLAT binary format is only 4k bytes. If the application consumes more stack, append the required number here. +* +LIBFOO_INIT_SYSV_FILES+ and +LIBFOO_INIT_SYSTEMD_FILES+ are + a space-separated list of init scripts path. Respectively, for + the systemV-like init systems (busybox, sysvinit, etc.) and for + systemd. + Theses init scripts files will only be installed when the relevant + init system is installed (i.e. if systemd is selected as the init + system in the configuration, only +LIBFOO_INIT_SYSTEMD_FILES+ will be + installed). + Each scripts will be installed in the default folder of the relevant + init system. + Note: This doesn't forbid you from defining +LIBFOO_INSTALL_INIT_SYSV+ and + +LIBFOO_INSTALL_INIT_SYSTEMD+ if you need to perform specific actions. + Theses variables are optionnal. + Examples: + + +LIBFOO_INSTALL_INIT_SYSV=package/libfoo/S99libfoo + + +LIBFOO_INSTALL_INIT_SYSV=$(@D)/S99libfoo + + The recommended way to define these variables is to use the following syntax: diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk index 259ee02..1d90ae9 100644 --- a/package/pkg-generic.mk +++ b/package/pkg-generic.mk @@ -229,10 +229,27 @@ $(BUILD_DIR)/%/.stamp_target_installed: @$(call MESSAGE,"Installing to target") $(foreach hook,$($(PKG)_PRE_INSTALL_TARGET_HOOKS),$(call $(hook))$(sep)) +$($(PKG)_INSTALL_TARGET_CMDS) - $(if $(BR2_INIT_SYSTEMD),\ - $($(PKG)_INSTALL_INIT_SYSTEMD)) - $(if $(BR2_INIT_SYSV)$(BR2_INIT_BUSYBOX),\ - $($(PKG)_INSTALL_INIT_SYSV)) +ifeq ($(BR2_INIT_SYSTEMD),y) + $(Q)if test -n "$($(PKG)_INIT_SYSTEMD_FILES)" ; then \ + mkdir -p $(TARGET_DIR)/etc/systemd/system/multi-user.target.wants ; \ + for s in $($(PKG)_INIT_SYSTEMD_FILES); do \ + f=$$(basename $${s}); \ + $(INSTALL) -D -m 644 $${s} $(TARGET_DIR)/lib/systemd/system/$${f} ; \ + ln -fs /lib/systemd/system/$${f} \ + $(TARGET_DIR)/etc/systemd/system/multi-user.target.wants/$${f} ; \ + done ; \ + fi + $($(PKG)_INSTALL_INIT_SYSTEMD) +endif +ifeq ($(BR2_INIT_SYSV)$(BR2_INIT_BUSYBOX),y) + $(Q)if test -n "$($(PKG)_INIT_SYSV_FILES)" ; then \ + for s in $($(PKG)_INIT_SYSV_FILES); do \ + f=$$(basename $${s}) ; \ + $(INSTALL) -D -m 0755 $${s} $(TARGET_DIR)/etc/init.d/$${f} ; \ + done ; \ + fi + $($(PKG)_INSTALL_INIT_SYSV) +endif $(foreach hook,$($(PKG)_POST_INSTALL_TARGET_HOOKS),$(call $(hook))$(sep)) $(Q)if test -n "$($(PKG)_CONFIG_SCRIPTS)" ; then \ $(RM) -f $(addprefix $(TARGET_DIR)/usr/bin/,$($(PKG)_CONFIG_SCRIPTS)) ; \ @@ -592,6 +609,7 @@ $(1)-reconfigure: $(1)-clean-for-reconfigure $(1) # define the PKG variable for all targets, containing the # uppercase package variable prefix $$($(2)_TARGET_INSTALL_TARGET): PKG=$(2) +$$($(2)_TARGET_INSTALL_TARGET): PKGDIR=$(pkgdir) $$($(2)_TARGET_INSTALL_STAGING): PKG=$(2) $$($(2)_TARGET_INSTALL_IMAGES): PKG=$(2) $$($(2)_TARGET_INSTALL_HOST): PKG=$(2)
Instead of copy pasting the install code over all our packages, you may now define variables to specify where to take the init script files. Each file will be installed in the default folder of the relevant init system. For systemd, instead of installing the files into $(TARGET_DIR)/etc/systemd/system, the file are now installed in $(TARGET_DIR)/lib/systemd/system which appears to be the right things to do. If you have specific operations to perform in order to install your init scripts or any other file relevant to the init system (tmpfiles for systemd), you can still define the usual FOO_INSTALL_INIT_SYSTEMD for your specific actions. Also add the documentation explaining the mechanism. Signed-off-by: Maxime Hadjinlian <maxime.hadjinlian@gmail.com> --- docs/manual/adding-packages-generic.txt | 17 +++++++++++++++++ package/pkg-generic.mk | 26 ++++++++++++++++++++++---- 2 files changed, 39 insertions(+), 4 deletions(-)