| Message ID | HbmuNdRmOn2PdMjzu0i70kXEVtJ7gzFTC74uRm0Le50@cp3-web-012.plabs.ch |
|---|---|
| State | Rejected |
| Headers | show |
| Series | Improvements to skarnet/s6 packages | expand |
On 07/04/2021 00:47, Dick Olsson via buildroot wrote: > diff --git a/package/skalibs/skalibs.mk b/package/skalibs/skalibs.mk > index f92859ff26..fe042604c2 100644 > --- a/package/skalibs/skalibs.mk > +++ b/package/skalibs/skalibs.mk > @@ -10,12 +10,24 @@ SKALIBS_LICENSE = ISC > SKALIBS_LICENSE_FILES = COPYING > SKALIBS_INSTALL_STAGING = YES > > +SKALIBS_DEFAULT_PATH = $(call qstrip,$(BR2_SYSTEM_DEFAULT_PATH)) > SKALIBS_CONF_OPTS = \ > - --prefix=/usr \ > - --with-default-path=/sbin:/usr/sbin:/bin:/usr/bin \ > + --prefix=/ \ > + --libexecdir=/libexec \ > + --with-default-path=$(SKALIBS_DEFAULT_PATH) \ > --with-sysdep-devurandom=yes \ > $(SHARED_STATIC_LIBS_OPTS) > > +# This variable can be used by dependant packages. > +SHARED_SKALIBS_CONF_OPTS = \ > + --prefix=/ \ > + --with-sysdeps=$(STAGING_DIR)/lib/skalibs/sysdeps \ > + --with-include=$(STAGING_DIR)/include \ > + --with-dynlib=$(STAGING_DIR)/lib \ > + --with-lib=$(STAGING_DIR)/lib/skalibs \ > + $(if $(BR2_STATIC_LIBS),,--disable-allstatic) \ > + $(SHARED_STATIC_LIBS_OPTS) > + Hm, I'm not sure I like this... The good is obviously that it allows you to change all skarnet packages in one fell swoop when that is needed. However, there are also some disadvantages. The main disadvantage is that it's harder for people to understand how e.g. the execline package is configured. When you look at the execline.mk file, you don't see what is the contents of the SHARED_SKALIBS_CONF_OPTS variable. Another disadvantage is that you may introduce a dependency on the order in which the different mk files are included. In this case it's fine because the variable is only used inside a rule, which is always expanded after all makefiles have been processed. But in general it's risky. If there is shared stuff, then I personally prefer to make sure that it is made clear by putting the packages that "belong together" in a subdirectory, and have the shared stuff in the .mk file of that subdirectory. freescale-imx is an example of that. However, we try to avoid subdirectories because it's annoying for other reasons, so those are conflicting forces... I'm not really sure about all this, so I put the other maintainers in Cc of this mail. If this change turns out to be acceptable, there's a small nitpick I'd like to add: since SHARED_SKALIBS_CONF_OPTS is not used by this package itself, you should put it at the very end of the file, after the generic-package. Oh, and please run check-package on it. SHARED_FOO_ is not allowed, it would have to be FOO_SHARED_. Oh, and also the refactoring should be in a separate commit from the change of /usr -> / Regards, Arnout > define SKALIBS_CONFIGURE_CMDS > (cd $(@D); $(TARGET_CONFIGURE_OPTS) ./configure $(SKALIBS_CONF_OPTS)) > endef
Arnout, Dick, All, On 2021-04-07 21:02 +0200, Arnout Vandecappelle spake thusly: > On 07/04/2021 00:47, Dick Olsson via buildroot wrote: > > The s6 suite of tools are predominantly used as an init system and for > > process supervision. It is therefore preferable for these tools to be > > installed with the default prefix / (as opposed to /usr). Since one may use s6 as an init system. and now that we have had the s6 family of packages for a while, would it make sense to promote them to an init system supported by Buildroot? There was some discussion about that in the past, but I am not sure I remember the rationale for not making s6 a full-fledged init system, like we have for busybox, system V, openrc, or systemd. > > diff --git a/package/skalibs/skalibs.mk b/package/skalibs/skalibs.mk > > index f92859ff26..fe042604c2 100644 > > --- a/package/skalibs/skalibs.mk > > +++ b/package/skalibs/skalibs.mk > > @@ -10,12 +10,24 @@ SKALIBS_LICENSE = ISC > > SKALIBS_LICENSE_FILES = COPYING > > SKALIBS_INSTALL_STAGING = YES > > > > +SKALIBS_DEFAULT_PATH = $(call qstrip,$(BR2_SYSTEM_DEFAULT_PATH)) > > SKALIBS_CONF_OPTS = \ > > - --prefix=/usr \ > > - --with-default-path=/sbin:/usr/sbin:/bin:/usr/bin \ > > + --prefix=/ \ > > + --libexecdir=/libexec \ > > + --with-default-path=$(SKALIBS_DEFAULT_PATH) \ > > --with-sysdep-devurandom=yes \ > > $(SHARED_STATIC_LIBS_OPTS) > > > > +# This variable can be used by dependant packages. > > +SHARED_SKALIBS_CONF_OPTS = \ > > + --prefix=/ \ > > + --with-sysdeps=$(STAGING_DIR)/lib/skalibs/sysdeps \ > > + --with-include=$(STAGING_DIR)/include \ > > + --with-dynlib=$(STAGING_DIR)/lib \ > > + --with-lib=$(STAGING_DIR)/lib/skalibs \ > > + $(if $(BR2_STATIC_LIBS),,--disable-allstatic) \ > > + $(SHARED_STATIC_LIBS_OPTS) > > + > > Hm, I'm not sure I like this... In the state, I too am not too fond of it. However... > The good is obviously that it allows you to change all skarnet packages in one > fell swoop when that is needed. > > However, there are also some disadvantages. > > The main disadvantage is that it's harder for people to understand how e.g. the > execline package is configured. When you look at the execline.mk file, you don't > see what is the contents of the SHARED_SKALIBS_CONF_OPTS variable. > > Another disadvantage is that you may introduce a dependency on the order in > which the different mk files are included. In this case it's fine because the > variable is only used inside a rule, which is always expanded after all > makefiles have been processed. But in general it's risky. > > If there is shared stuff, then I personally prefer to make sure that it is made > clear by putting the packages that "belong together" in a subdirectory, and have > the shared stuff in the .mk file of that subdirectory. freescale-imx is an > example of that. ... *if* the ska-related packages would really benefit from having shared variables, then we could look at it like we look at qt5, or as Arnout mentions, like freescale stuff. > However, we try to avoid subdirectories because it's annoying > for other reasons, so those are conflicting forces... > > I'm not really sure about all this, so I put the other maintainers in Cc of > this mail. Neither am I; that's only 4 packages impacted, i.e. the library plus three packages. I am not sure this-few packages deserve shared variables. So, for me, that's a -1 on a [-2..2] scale... Regards, Yann E. MORIN. > If this change turns out to be acceptable, there's a small nitpick I'd like to > add: since SHARED_SKALIBS_CONF_OPTS is not used by this package itself, you > should put it at the very end of the file, after the generic-package. > > Oh, and please run check-package on it. SHARED_FOO_ is not allowed, it would > have to be FOO_SHARED_. > > Oh, and also the refactoring should be in a separate commit from the change of > /usr -> / > > > Regards, > Arnout > > > define SKALIBS_CONFIGURE_CMDS > > (cd $(@D); $(TARGET_CONFIGURE_OPTS) ./configure $(SKALIBS_CONF_OPTS)) > > endef
On 07/04/2021 22:23, Yann E. MORIN wrote: > Arnout, Dick, All, > > On 2021-04-07 21:02 +0200, Arnout Vandecappelle spake thusly: [snip] >> Hm, I'm not sure I like this... > > In the state, I too am not too fond of it. However... > >> The good is obviously that it allows you to change all skarnet packages in one >> fell swoop when that is needed. >> >> However, there are also some disadvantages. >> >> The main disadvantage is that it's harder for people to understand how e.g. the >> execline package is configured. When you look at the execline.mk file, you don't >> see what is the contents of the SHARED_SKALIBS_CONF_OPTS variable. >> >> Another disadvantage is that you may introduce a dependency on the order in >> which the different mk files are included. In this case it's fine because the >> variable is only used inside a rule, which is always expanded after all >> makefiles have been processed. But in general it's risky. >> >> If there is shared stuff, then I personally prefer to make sure that it is made >> clear by putting the packages that "belong together" in a subdirectory, and have >> the shared stuff in the .mk file of that subdirectory. freescale-imx is an >> example of that. > > ... *if* the ska-related packages would really benefit from having > shared variables, then we could look at it like we look at qt5, or as > Arnout mentions, like freescale stuff. > >> However, we try to avoid subdirectories because it's annoying >> for other reasons, so those are conflicting forces... >> >> I'm not really sure about all this, so I put the other maintainers in Cc of >> this mail. > > Neither am I; that's only 4 packages impacted, i.e. the library plus > three packages. I am not sure this-few packages deserve shared variables. > > So, for me, that's a -1 on a [-2..2] scale... Thus, I've marked this patch and 2/4 as Rejected. Regards, Arnout >> If this change turns out to be acceptable, there's a small nitpick I'd like to >> add: since SHARED_SKALIBS_CONF_OPTS is not used by this package itself, you >> should put it at the very end of the file, after the generic-package. >> >> Oh, and please run check-package on it. SHARED_FOO_ is not allowed, it would >> have to be FOO_SHARED_. >> >> Oh, and also the refactoring should be in a separate commit from the change of >> /usr -> / >> >> >> Regards, >> Arnout >> >>> define SKALIBS_CONFIGURE_CMDS >>> (cd $(@D); $(TARGET_CONFIGURE_OPTS) ./configure $(SKALIBS_CONF_OPTS)) >>> endef >
Hi Yann, Arnout, On Wednesday, April 7th, 2021 at 8:23 PM, Yann E. MORIN wrote: > Since one may use s6 as an init system. and now that we have had the s6 > family of packages for a while, would it make sense to promote them to > an init system supported by Buildroot? > > There was some discussion about that in the past, but I am not sure I > remember the rationale for not making s6 a full-fledged init system, > like we have for busybox, system V, openrc, or systemd. Yes, it definitely makes sense. I haven't read up on the past conversations but I can imagine it was not done because there a are quite a few steps involved in doing it. But I'm already working on a patchset for it! Stay tuned :) > In the state, I too am not too fond of it. However... > ... if the ska-related packages would really benefit from having > shared variables, then we could look at it like we look at qt5, or as > Arnout mentions, like freescale stuff. I do believe there would be beneficial for a few reasons... > > I'm not really sure about all this, so I put the other maintainers in Cc of > > this mail. > > Neither am I; that's only 4 packages impacted, i.e. the library plus > three packages. I am not sure this-few packages deserve shared variables. ... there are 5-6 other ska-related packages that I want to package. After I have submitted patchset for those additional packages we are looking at around 10 packages in total that all share build system. Being able easily keep them all aligned would be good. So in essence, I do think that something similar to qt5/freescale makes sense. I'll eventually get those patchsets in, then we can review and discuss further! Cheers D. Olsson PGP: 8204A8CD
diff --git a/package/execline/execline.mk b/package/execline/execline.mk index 0ab5a03f2e..f6001ed3f7 100644 --- a/package/execline/execline.mk +++ b/package/execline/execline.mk @@ -12,13 +12,12 @@ EXECLINE_INSTALL_STAGING = YES EXECLINE_DEPENDENCIES = skalibs EXECLINE_CONF_OPTS = \ - --prefix=/usr \ - --with-sysdeps=$(STAGING_DIR)/usr/lib/skalibs/sysdeps \ - --with-include=$(STAGING_DIR)/usr/include \ - --with-dynlib=$(STAGING_DIR)/usr/lib \ - --with-lib=$(STAGING_DIR)/usr/lib/skalibs \ - $(if $(BR2_STATIC_LIBS),,--disable-allstatic) \ - $(SHARED_STATIC_LIBS_OPTS) + $(SHARED_SKALIBS_CONF_OPTS) \ + --shebangdir=/bin + +# This variable can be used by dependant packages. +SHARED_EXECLINE_CONF_OPTS = \ + --with-lib=$(STAGING_DIR)/lib/execline define EXECLINE_CONFIGURE_CMDS (cd $(@D); $(TARGET_CONFIGURE_OPTS) ./configure $(EXECLINE_CONF_OPTS)) @@ -44,17 +43,12 @@ endef HOST_EXECLINE_DEPENDENCIES = host-skalibs -# Set --shebangdir to /usr/bin, as this value is used by the host variant of -# s6-rc when generating execline scripts for the target. +# The host package does not have a run-time dependency on the +# --shebangdir option. But it must align with the target variant since +# it affects the output of commands. HOST_EXECLINE_CONF_OPTS = \ - --prefix=$(HOST_DIR) \ - --shebangdir=/usr/bin \ - --with-sysdeps=$(HOST_DIR)/lib/skalibs/sysdeps \ - --with-include=$(HOST_DIR)/include \ - --with-dynlib=$(HOST_DIR)/lib \ - --disable-static \ - --enable-shared \ - --disable-allstatic + $(SHARED_HOST_SKALIBS_CONF_OPTS) \ + --shebangdir=/bin define HOST_EXECLINE_CONFIGURE_CMDS (cd $(@D); $(HOST_CONFIGURE_OPTS) ./configure $(HOST_EXECLINE_CONF_OPTS)) diff --git a/package/s6-dns/s6-dns.mk b/package/s6-dns/s6-dns.mk index 9ed75bd600..3b1047dab3 100644 --- a/package/s6-dns/s6-dns.mk +++ b/package/s6-dns/s6-dns.mk @@ -11,29 +11,18 @@ S6_DNS_LICENSE_FILES = COPYING S6_DNS_INSTALL_STAGING = YES S6_DNS_DEPENDENCIES = skalibs -S6_DNS_CONF_OPTS = \ - --prefix=/usr \ - --with-sysdeps=$(STAGING_DIR)/usr/lib/skalibs/sysdeps \ - --with-include=$(STAGING_DIR)/usr/include \ - --with-dynlib=$(STAGING_DIR)/usr/lib \ - --with-lib=$(STAGING_DIR)/usr/lib/skalibs \ - $(if $(BR2_STATIC_LIBS),,--disable-allstatic) \ - $(SHARED_STATIC_LIBS_OPTS) +# This variable can be used by dependant packages. +SHARED_S6_DNS_CONF_OPTS = \ + --with-lib=$(STAGING_DIR)/lib/s6-dns define S6_DNS_CONFIGURE_CMDS - (cd $(@D); $(TARGET_CONFIGURE_OPTS) ./configure $(S6_DNS_CONF_OPTS)) + (cd $(@D); $(TARGET_CONFIGURE_OPTS) ./configure $(SHARED_SKALIBS_CONF_OPTS)) endef define S6_DNS_BUILD_CMDS $(TARGET_MAKE_ENV) $(MAKE) -C $(@D) endef -define S6_DNS_REMOVE_STATIC_LIB_DIR - rm -rf $(TARGET_DIR)/usr/lib/s6-dns -endef - -S6_DNS_POST_INSTALL_TARGET_HOOKS += S6_DNS_REMOVE_STATIC_LIB_DIR - define S6_DNS_INSTALL_TARGET_CMDS $(TARGET_MAKE_ENV) $(MAKE) -C $(@D) DESTDIR=$(TARGET_DIR) install endef diff --git a/package/s6/s6.mk b/package/s6/s6.mk index 443ecbbffd..8068601ab5 100644 --- a/package/s6/s6.mk +++ b/package/s6/s6.mk @@ -9,17 +9,14 @@ S6_SITE = http://skarnet.org/software/s6 S6_LICENSE = ISC S6_LICENSE_FILES = COPYING S6_INSTALL_STAGING = YES -S6_DEPENDENCIES = execline +S6_DEPENDENCIES = skalibs execline S6_CONF_OPTS = \ - --prefix=/usr \ - --with-sysdeps=$(STAGING_DIR)/usr/lib/skalibs/sysdeps \ - --with-include=$(STAGING_DIR)/usr/include \ - --with-dynlib=$(STAGING_DIR)/usr/lib \ - --with-lib=$(STAGING_DIR)/usr/lib/execline \ - --with-lib=$(STAGING_DIR)/usr/lib/skalibs \ - $(if $(BR2_STATIC_LIBS),,--disable-allstatic) \ - $(SHARED_STATIC_LIBS_OPTS) + $(SHARED_SKALIBS_CONF_OPTS) \ + $(SHARED_EXECLINE_CONF_OPTS) + +SHARED_S6_CONF_OPTS = \ + --with-lib=$(STAGING_DIR)/lib/s6 define S6_CONFIGURE_CMDS (cd $(@D); $(TARGET_CONFIGURE_OPTS) ./configure $(S6_CONF_OPTS)) @@ -43,19 +40,10 @@ define S6_INSTALL_STAGING_CMDS $(TARGET_MAKE_ENV) $(MAKE) -C $(@D) DESTDIR=$(STAGING_DIR) install endef -HOST_S6_DEPENDENCIES = host-execline - -HOST_S6_CONF_OPTS = \ - --prefix=$(HOST_DIR) \ - --with-sysdeps=$(HOST_DIR)/lib/skalibs/sysdeps \ - --with-include=$(HOST_DIR)/include \ - --with-dynlib=$(HOST_DIR)/lib \ - --disable-static \ - --enable-shared \ - --disable-allstatic +HOST_S6_DEPENDENCIES = host-skalibs host-execline define HOST_S6_CONFIGURE_CMDS - (cd $(@D); $(HOST_CONFIGURE_OPTS) ./configure $(HOST_S6_CONF_OPTS)) + (cd $(@D); $(HOST_CONFIGURE_OPTS) ./configure $(SHARED_HOST_SKALIBS_CONF_OPTS)) endef define HOST_S6_BUILD_CMDS diff --git a/package/skalibs/skalibs.mk b/package/skalibs/skalibs.mk index f92859ff26..fe042604c2 100644 --- a/package/skalibs/skalibs.mk +++ b/package/skalibs/skalibs.mk @@ -10,12 +10,24 @@ SKALIBS_LICENSE = ISC SKALIBS_LICENSE_FILES = COPYING SKALIBS_INSTALL_STAGING = YES +SKALIBS_DEFAULT_PATH = $(call qstrip,$(BR2_SYSTEM_DEFAULT_PATH)) SKALIBS_CONF_OPTS = \ - --prefix=/usr \ - --with-default-path=/sbin:/usr/sbin:/bin:/usr/bin \ + --prefix=/ \ + --libexecdir=/libexec \ + --with-default-path=$(SKALIBS_DEFAULT_PATH) \ --with-sysdep-devurandom=yes \ $(SHARED_STATIC_LIBS_OPTS) +# This variable can be used by dependant packages. +SHARED_SKALIBS_CONF_OPTS = \ + --prefix=/ \ + --with-sysdeps=$(STAGING_DIR)/lib/skalibs/sysdeps \ + --with-include=$(STAGING_DIR)/include \ + --with-dynlib=$(STAGING_DIR)/lib \ + --with-lib=$(STAGING_DIR)/lib/skalibs \ + $(if $(BR2_STATIC_LIBS),,--disable-allstatic) \ + $(SHARED_STATIC_LIBS_OPTS) + define SKALIBS_CONFIGURE_CMDS (cd $(@D); $(TARGET_CONFIGURE_OPTS) ./configure $(SKALIBS_CONF_OPTS)) endef @@ -33,8 +45,25 @@ define SKALIBS_INSTALL_STAGING_CMDS $(TARGET_MAKE_ENV) $(MAKE) -C $(@D) DESTDIR=$(STAGING_DIR) install endef +# The --with-default-path option needs to match the target variant. HOST_SKALIBS_CONF_OPTS = \ --prefix=$(HOST_DIR) \ + --with-default-path=$(SKALIBS_DEFAULT_PATH) \ + --disable-static \ + --enable-shared \ + --disable-allstatic + +# This variable can be used by dependant packages. +# +# Note: Dependant host packages does not have a run-time dependencies on +# the --libexecdir option. But it must align with the target variant +# since it affects how target packages run. +SHARED_HOST_SKALIBS_CONF_OPTS = \ + --prefix=$(HOST_DIR) \ + --libexecdir=/libexec \ + --with-sysdeps=$(HOST_DIR)/lib/skalibs/sysdeps \ + --with-include=$(HOST_DIR)/include \ + --with-dynlib=$(HOST_DIR)/lib \ --disable-static \ --enable-shared \ --disable-allstatic
The s6 suite of tools are predominantly used as an init system and for process supervision. It is therefore preferable for these tools to be installed with the default prefix / (as opposed to /usr). Also improve maintainability and extensibility by introducing variables that enable dependant packages to use shared configuration options. This gives better build guarantees in different configuration scenarios such as default paths, shared or static libraries, etc. Signed-off-by: Dick Olsson <hi@senzilla.io> --- package/execline/execline.mk | 28 +++++++++++----------------- package/s6-dns/s6-dns.mk | 19 ++++--------------- package/s6/s6.mk | 28 ++++++++-------------------- package/skalibs/skalibs.mk | 33 +++++++++++++++++++++++++++++++-- 4 files changed, 54 insertions(+), 54 deletions(-)