* [Buildroot] [PATCH] system: move system.mk recipes inside the "target-finalize" rule @ 2014-06-10 8:52 Fabio Porcedda 2014-06-10 20:33 ` Thomas Petazzoni 0 siblings, 1 reply; 15+ messages in thread From: Fabio Porcedda @ 2014-06-10 8:52 UTC (permalink / raw) To: buildroot Move system.mk recipes inside the "target-finalize" rule in order to: - Ensure an ordering even if top-level parallel make is being used. - Execute system.mk commands after the "target-finalize" initial message is printed so they can be clearly distinguished from packages building. Signed-off-by: Fabio Porcedda <fabio.porcedda@gmail.com> --- Makefile | 6 +++++ system/system.mk | 74 ++++++++++++++++++++++++++++---------------------------- 2 files changed, 43 insertions(+), 37 deletions(-) diff --git a/Makefile b/Makefile index dc86060..dba2083 100644 --- a/Makefile +++ b/Makefile @@ -531,6 +531,12 @@ $(TARGETS_ROOTFS): target-finalize target-finalize: $(TARGETS) @$(call MESSAGE,"Finalizing target directory") + $(TARGET_FINALIZE_GENERIC_SECURETTY) + $(TARGET_FINALIZE_GENERIC_HOSTNAME) + $(TARGET_FINALIZE_GENERIC_ISSUE) + $(TARGET_FINALIZE_ROOT_PASSWD) + $(TARGET_FINALIZE_GENERIC_GETTY) + $(TARGET_FINALIZE_GENERIC_REMOUNT_RW) $(TARGET_PURGE_LOCALES) rm -rf $(TARGET_DIR)/usr/include $(TARGET_DIR)/usr/share/aclocal \ $(TARGET_DIR)/usr/lib/pkgconfig $(TARGET_DIR)/usr/share/pkgconfig \ diff --git a/system/system.mk b/system/system.mk index 01a6c3a..756d3de 100644 --- a/system/system.mk +++ b/system/system.mk @@ -7,68 +7,68 @@ TARGET_GENERIC_GETTY_BAUDRATE = $(call qstrip,$(BR2_TARGET_GENERIC_GETTY_BAUDRAT TARGET_GENERIC_GETTY_TERM = $(call qstrip,$(BR2_TARGET_GENERIC_GETTY_TERM)) TARGET_GENERIC_GETTY_OPTIONS = $(call qstrip,$(BR2_TARGET_GENERIC_GETTY_OPTIONS)) -target-generic-securetty: +ifeq ($(BR2_TARGET_GENERIC_GETTY),y) +define TARGET_FINALIZE_GENERIC_SECURETTY grep -q '^$(TARGET_GENERIC_GETTY_PORT)$$' $(TARGET_DIR)/etc/securetty || \ echo '$(TARGET_GENERIC_GETTY_PORT)' >> $(TARGET_DIR)/etc/securetty +endef +endif -target-generic-hostname: +ifneq ($(TARGET_GENERIC_HOSTNAME),) +define TARGET_FINALIZE_GENERIC_HOSTNAME mkdir -p $(TARGET_DIR)/etc echo "$(TARGET_GENERIC_HOSTNAME)" > $(TARGET_DIR)/etc/hostname $(SED) '$$a \127.0.1.1\t$(TARGET_GENERIC_HOSTNAME)' \ -e '/^127.0.1.1/d' $(TARGET_DIR)/etc/hosts +endef +endif -target-generic-issue: +ifneq ($(TARGET_GENERIC_ISSUE),) +define TARGET_FINALIZE_GENERIC_ISSUE mkdir -p $(TARGET_DIR)/etc echo "$(TARGET_GENERIC_ISSUE)" > $(TARGET_DIR)/etc/issue +endef +endif ifneq ($(TARGET_GENERIC_ROOT_PASSWD),) -target-root-passwd: host-mkpasswd +TARGETS += host-mkpasswd endif -target-root-passwd: + +ifeq ($(BR2_ROOTFS_SKELETON_DEFAULT),y) + +define TARGET_FINALIZE_ROOT_PASSWD [ -n "$(TARGET_GENERIC_ROOT_PASSWD)" ] && \ TARGET_GENERIC_ROOT_PASSWD_HASH=$$($(MKPASSWD) -m "$(TARGET_GENERIC_PASSWD_METHOD)" "$(TARGET_GENERIC_ROOT_PASSWD)"); \ $(SED) "s,^root:[^:]*:,root:$$TARGET_GENERIC_ROOT_PASSWD_HASH:," $(TARGET_DIR)/etc/shadow +endef -target-generic-getty-busybox: - $(SED) '/# GENERIC_SERIAL$$/s~^.*#~$(TARGET_GENERIC_GETTY_PORT)::respawn:/sbin/getty -L $(TARGET_GENERIC_GETTY_OPTIONS) $(TARGET_GENERIC_GETTY_PORT) $(TARGET_GENERIC_GETTY_BAUDRATE) $(TARGET_GENERIC_GETTY_TERM) #~' \ - $(TARGET_DIR)/etc/inittab - +ifeq ($(BR2_TARGET_GENERIC_GETTY),y) +ifeq ($(BR2_PACKAGE_SYSVINIT),y) # In sysvinit inittab, the "id" must not be longer than 4 bytes, so we # skip the "tty" part and keep only the remaining. -target-generic-getty-sysvinit: +define TARGET_FINALIZE_GENERIC_GETTY $(SED) '/# GENERIC_SERIAL$$/s~^.*#~$(shell echo $(TARGET_GENERIC_GETTY_PORT) | tail -c+4)::respawn:/sbin/getty -L $(TARGET_GENERIC_GETTY_OPTIONS) $(TARGET_GENERIC_GETTY_PORT) $(TARGET_GENERIC_GETTY_BAUDRATE) $(TARGET_GENERIC_GETTY_TERM) #~' \ $(TARGET_DIR)/etc/inittab +endef +else +# Add getty to busybox inittab +define TARGET_FINALIZE_GENERIC_GETTY + $(SED) '/# GENERIC_SERIAL$$/s~^.*#~$(TARGET_GENERIC_GETTY_PORT)::respawn:/sbin/getty -L $(TARGET_GENERIC_GETTY_OPTIONS) $(TARGET_GENERIC_GETTY_PORT) $(TARGET_GENERIC_GETTY_BAUDRATE) $(TARGET_GENERIC_GETTY_TERM) #~' \ + $(TARGET_DIR)/etc/inittab +endef +endif +endif +ifeq ($(BR2_TARGET_GENERIC_REMOUNT_ROOTFS_RW),y) # Find commented line, if any, and remove leading '#'s -target-generic-do-remount-rw: +define TARGET_FINALIZE_GENERIC_REMOUNT_RW $(SED) '/^#.*# REMOUNT_ROOTFS_RW$$/s~^#\+~~' $(TARGET_DIR)/etc/inittab - +endef +else # Find uncommented line, if any, and add a leading '#' -target-generic-dont-remount-rw: +define TARGET_FINALIZE_GENERIC_REMOUNT_RW $(SED) '/^[^#].*# REMOUNT_ROOTFS_RW$$/s~^~#~' $(TARGET_DIR)/etc/inittab - -ifeq ($(BR2_TARGET_GENERIC_GETTY),y) -TARGETS += target-generic-securetty -endif - -ifneq ($(TARGET_GENERIC_HOSTNAME),) -TARGETS += target-generic-hostname -endif - -ifneq ($(TARGET_GENERIC_ISSUE),) -TARGETS += target-generic-issue +endef endif -ifeq ($(BR2_ROOTFS_SKELETON_DEFAULT),y) -TARGETS += target-root-passwd - -ifeq ($(BR2_TARGET_GENERIC_GETTY),y) -TARGETS += target-generic-getty-$(if $(BR2_PACKAGE_SYSVINIT),sysvinit,busybox) -endif - -ifeq ($(BR2_TARGET_GENERIC_REMOUNT_ROOTFS_RW),y) -TARGETS += target-generic-do-remount-rw -else -TARGETS += target-generic-dont-remount-rw -endif -endif +endif # BR2_ROOTFS_SKELETON_DEFAULT -- 1.9.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [Buildroot] [PATCH] system: move system.mk recipes inside the "target-finalize" rule 2014-06-10 8:52 [Buildroot] [PATCH] system: move system.mk recipes inside the "target-finalize" rule Fabio Porcedda @ 2014-06-10 20:33 ` Thomas Petazzoni 2014-06-11 9:14 ` Fabio Porcedda 0 siblings, 1 reply; 15+ messages in thread From: Thomas Petazzoni @ 2014-06-10 20:33 UTC (permalink / raw) To: buildroot Dear Fabio Porcedda, On Tue, 10 Jun 2014 10:52:47 +0200, Fabio Porcedda wrote: > target-finalize: $(TARGETS) > @$(call MESSAGE,"Finalizing target directory") > + $(TARGET_FINALIZE_GENERIC_SECURETTY) > + $(TARGET_FINALIZE_GENERIC_HOSTNAME) > + $(TARGET_FINALIZE_GENERIC_ISSUE) > + $(TARGET_FINALIZE_ROOT_PASSWD) > + $(TARGET_FINALIZE_GENERIC_GETTY) > + $(TARGET_FINALIZE_GENERIC_REMOUNT_RW) I agree with the principle, but I'd like to have a better implementation, which is really long overdue to stop cluttering target-finalize with more and more crap. Could we implement target-finalize as just: target-finalize: $(TARGETS) $(foreach hook,$(TARGET_FINALIZE_HOOKS),$(call $(hook),end,$(1),$($(PKG)_NAME))$(sep)) And then: > +ifeq ($(BR2_TARGET_GENERIC_GETTY),y) > +define TARGET_FINALIZE_GENERIC_SECURETTY > grep -q '^$(TARGET_GENERIC_GETTY_PORT)$$' $(TARGET_DIR)/etc/securetty || \ > echo '$(TARGET_GENERIC_GETTY_PORT)' >> $(TARGET_DIR)/etc/securetty > +endef TARGET_FINALIZE_HOOKS += TARGET_FINALIZE_GENERIC_SECURETTY Then we can also use this to move the Python, Perl and other package-specific target-finalize logic down to the specific packages. Of course, I'm not asking you to do all of this work. But at least, post a patch introducing the TARGET_FINALIZE_HOOKS, and use it for those targets you were converting in your original patch. We should go towards *removing* stuff from the main Makefile, not adding more :-) Thanks! Thomas -- Thomas Petazzoni, CTO, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com ^ permalink raw reply [flat|nested] 15+ messages in thread
* [Buildroot] [PATCH] system: move system.mk recipes inside the "target-finalize" rule 2014-06-10 20:33 ` Thomas Petazzoni @ 2014-06-11 9:14 ` Fabio Porcedda 2014-06-11 9:18 ` Thomas Petazzoni 2014-06-11 16:32 ` Arnout Vandecappelle 0 siblings, 2 replies; 15+ messages in thread From: Fabio Porcedda @ 2014-06-11 9:14 UTC (permalink / raw) To: buildroot Hi Thomas, thanks for reviewing the patch. On Tue, Jun 10, 2014 at 10:33 PM, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > Dear Fabio Porcedda, > > On Tue, 10 Jun 2014 10:52:47 +0200, Fabio Porcedda wrote: > >> target-finalize: $(TARGETS) >> @$(call MESSAGE,"Finalizing target directory") >> + $(TARGET_FINALIZE_GENERIC_SECURETTY) >> + $(TARGET_FINALIZE_GENERIC_HOSTNAME) >> + $(TARGET_FINALIZE_GENERIC_ISSUE) >> + $(TARGET_FINALIZE_ROOT_PASSWD) >> + $(TARGET_FINALIZE_GENERIC_GETTY) >> + $(TARGET_FINALIZE_GENERIC_REMOUNT_RW) > > I agree with the principle, but I'd like to have a better > implementation, which is really long overdue to stop cluttering > target-finalize with more and more crap. Could we implement > target-finalize as just: > > target-finalize: $(TARGETS) > $(foreach hook,$(TARGET_FINALIZE_HOOKS),$(call $(hook),end,$(1),$($(PKG)_NAME))$(sep)) > > And then: > >> +ifeq ($(BR2_TARGET_GENERIC_GETTY),y) >> +define TARGET_FINALIZE_GENERIC_SECURETTY >> grep -q '^$(TARGET_GENERIC_GETTY_PORT)$$' $(TARGET_DIR)/etc/securetty || \ >> echo '$(TARGET_GENERIC_GETTY_PORT)' >> $(TARGET_DIR)/etc/securetty >> +endef > > TARGET_FINALIZE_HOOKS += TARGET_FINALIZE_GENERIC_SECURETTY > > Then we can also use this to move the Python, Perl and other > package-specific target-finalize logic down to the specific packages. > Of course, I'm not asking you to do all of this work. But at least, > post a patch introducing the TARGET_FINALIZE_HOOKS, and use it for > those targets you were converting in your original patch. > > We should go towards *removing* stuff from the main Makefile, not > adding more :-) Using hooks was my initial proposition but Arnout suggested the above solution and you and Perer liked it. Have you changed your mind about using hooks? This is a copy of the previous thread: On Mon, Apr 28, 2014 at 6:10 PM, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > Peter, Arnout, > > On Mon, 28 Apr 2014 11:36:15 +0200, Peter Korsgaard wrote: > >> > My personal preference is to have a single rule (e.g. target-finalize) >> > that performs everything that is post-targets and pre-rootfs. There isn't >> > much that needs to be done so parallelisation doesn't make sense. And I >> > think it's much easier to understand which steps are executed and in >> > which order if they are all put together in a single rule rather than >> > spread out over several. >> >> > To make things more readable, we can put the commands into separate >> > variables. For instance: >> >> > define TARGET_PURGE_LOCALES >> > rm -f $(LOCALE_WHITELIST) >> > ... >> > endef >> >> > define TARGET_PURGE_DEVFILES >> > rm -rf $(TARGET_DIR)/usr/include ... >> > ... >> > endef >> >> > ifneq ($(BR2_PACKAGE_GDB),y) >> > define TARGET_PURGE_GDB >> > rm -rf $(TARGET_DIR)/usr/share/gdb >> > endef >> > endif >> >> > target-finalize: $(TARGETS) >> > $(TARGET_PURGE_LOCALES) >> > $(TARGET_PURGE_DEVFILES) >> > $(TARGET_PURGE_GDB) >> > $(TARGET_PURGE_DOC) >> > ... >> >> Yes, that looks nice and clear to me too. > > Yes, agreed, this looks a lot nicer than a long chain of targets that > simply depend on each other to avoid any parallelization. Best regards -- Fabio Porcedda ^ permalink raw reply [flat|nested] 15+ messages in thread
* [Buildroot] [PATCH] system: move system.mk recipes inside the "target-finalize" rule 2014-06-11 9:14 ` Fabio Porcedda @ 2014-06-11 9:18 ` Thomas Petazzoni 2014-06-11 9:21 ` Fabio Porcedda 2014-06-11 16:32 ` Arnout Vandecappelle 1 sibling, 1 reply; 15+ messages in thread From: Thomas Petazzoni @ 2014-06-11 9:18 UTC (permalink / raw) To: buildroot Dear Fabio Porcedda, On Wed, 11 Jun 2014 11:14:38 +0200, Fabio Porcedda wrote: > > We should go towards *removing* stuff from the main Makefile, not > > adding more :-) > > Using hooks was my initial proposition but Arnout suggested the above > solution and you and Perer liked it. > Have you changed your mind about using hooks? Ah. I don't remember. Do you have a pointer to this discussion? I'd like to see what were the arguments back then :-) Thanks, and sorry for the change in position. Thomas -- Thomas Petazzoni, CTO, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com ^ permalink raw reply [flat|nested] 15+ messages in thread
* [Buildroot] [PATCH] system: move system.mk recipes inside the "target-finalize" rule 2014-06-11 9:18 ` Thomas Petazzoni @ 2014-06-11 9:21 ` Fabio Porcedda 2014-06-11 12:54 ` Thomas Petazzoni 0 siblings, 1 reply; 15+ messages in thread From: Fabio Porcedda @ 2014-06-11 9:21 UTC (permalink / raw) To: buildroot On Wed, Jun 11, 2014 at 11:18 AM, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > Dear Fabio Porcedda, > > On Wed, 11 Jun 2014 11:14:38 +0200, Fabio Porcedda wrote: > >> > We should go towards *removing* stuff from the main Makefile, not >> > adding more :-) >> >> Using hooks was my initial proposition but Arnout suggested the above >> solution and you and Perer liked it. >> Have you changed your mind about using hooks? > > Ah. I don't remember. Do you have a pointer to this discussion? I'd > like to see what were the arguments back then :-) Sure: http://lists.busybox.net/pipermail/buildroot/2014-April/095052.html > Thanks, and sorry for the change in position. > > Thomas > -- > Thomas Petazzoni, CTO, Free Electrons > Embedded Linux, Kernel and Android engineering > http://free-electrons.com Best regards -- Fabio Porcedda ^ permalink raw reply [flat|nested] 15+ messages in thread
* [Buildroot] [PATCH] system: move system.mk recipes inside the "target-finalize" rule 2014-06-11 9:21 ` Fabio Porcedda @ 2014-06-11 12:54 ` Thomas Petazzoni 0 siblings, 0 replies; 15+ messages in thread From: Thomas Petazzoni @ 2014-06-11 12:54 UTC (permalink / raw) To: buildroot Dear Fabio Porcedda, On Wed, 11 Jun 2014 11:21:38 +0200, Fabio Porcedda wrote: > > Ah. I don't remember. Do you have a pointer to this discussion? I'd > > like to see what were the arguments back then :-) > > Sure: http://lists.busybox.net/pipermail/buildroot/2014-April/095052.html Ok, I see now. The problem I have with the reasoning we used during this discussion is that we only thought of those "global" target-finalize tasks. But there are also plenty of more package-specific ones, such as cleaning up unneeded Python things, or unneeded Perl things. A hook mechanism would allow to move these individual bits of logic down to the package needing them. It's true that I admit it would be less easy to read and understand that a complete list of all target-finalize tasks. So if Peter and Arnout prefer the explicit list, I'm fine. Thomas -- Thomas Petazzoni, CTO, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com ^ permalink raw reply [flat|nested] 15+ messages in thread
* [Buildroot] [PATCH] system: move system.mk recipes inside the "target-finalize" rule 2014-06-11 9:14 ` Fabio Porcedda 2014-06-11 9:18 ` Thomas Petazzoni @ 2014-06-11 16:32 ` Arnout Vandecappelle 2014-06-11 17:36 ` Thomas Petazzoni 2014-06-12 7:20 ` Fabio Porcedda 1 sibling, 2 replies; 15+ messages in thread From: Arnout Vandecappelle @ 2014-06-11 16:32 UTC (permalink / raw) To: buildroot On 06/11/14 11:14, Fabio Porcedda wrote: > Hi Thomas, > thanks for reviewing the patch. > > On Tue, Jun 10, 2014 at 10:33 PM, Thomas Petazzoni > <thomas.petazzoni@free-electrons.com> wrote: >> Dear Fabio Porcedda, >> >> On Tue, 10 Jun 2014 10:52:47 +0200, Fabio Porcedda wrote: >> >>> target-finalize: $(TARGETS) >>> @$(call MESSAGE,"Finalizing target directory") >>> + $(TARGET_FINALIZE_GENERIC_SECURETTY) >>> + $(TARGET_FINALIZE_GENERIC_HOSTNAME) >>> + $(TARGET_FINALIZE_GENERIC_ISSUE) >>> + $(TARGET_FINALIZE_ROOT_PASSWD) >>> + $(TARGET_FINALIZE_GENERIC_GETTY) >>> + $(TARGET_FINALIZE_GENERIC_REMOUNT_RW) >> >> I agree with the principle, but I'd like to have a better >> implementation, which is really long overdue to stop cluttering >> target-finalize with more and more crap. Could we implement >> target-finalize as just: >> >> target-finalize: $(TARGETS) >> $(foreach hook,$(TARGET_FINALIZE_HOOKS),$(call $(hook),end,$(1),$($(PKG)_NAME))$(sep)) I like this idea. Though it should actually be $(foreach hook,$(TARGET_FINALIZE_HOOKS),$($(hook))$(sep)) (the call is redundant, the end is redundant, and $(1) and $(PKG) are not available). >> >> And then: >> >>> +ifeq ($(BR2_TARGET_GENERIC_GETTY),y) >>> +define TARGET_FINALIZE_GENERIC_SECURETTY >>> grep -q '^$(TARGET_GENERIC_GETTY_PORT)$$' $(TARGET_DIR)/etc/securetty || \ >>> echo '$(TARGET_GENERIC_GETTY_PORT)' >> $(TARGET_DIR)/etc/securetty >>> +endef >> >> TARGET_FINALIZE_HOOKS += TARGET_FINALIZE_GENERIC_SECURETTY >> >> Then we can also use this to move the Python, Perl and other >> package-specific target-finalize logic down to the specific packages. >> Of course, I'm not asking you to do all of this work. But at least, >> post a patch introducing the TARGET_FINALIZE_HOOKS, and use it for >> those targets you were converting in your original patch. >> >> We should go towards *removing* stuff from the main Makefile, not >> adding more :-) > > Using hooks was my initial proposition but Arnout suggested the above > solution and you and Perer liked it. > Have you changed your mind about using hooks? Actually, your initial proposition was: -target-purgelocales: +target-purgelocales: $(filter-out target-purgelocales,$(TARGETS)) And my comment was that I don't like this splitting of target-finalize over several make targets. It is that remark that lead to the thread below. With the hooks, you'll still have a single target-finalize rule that performs all the finalization and that simply depends on $(TARGETS). Regards, Arnout > > This is a copy of the previous thread: > On Mon, Apr 28, 2014 at 6:10 PM, Thomas Petazzoni > <thomas.petazzoni@free-electrons.com> wrote: >> Peter, Arnout, >> >> On Mon, 28 Apr 2014 11:36:15 +0200, Peter Korsgaard wrote: >> >>> > My personal preference is to have a single rule (e.g. target-finalize) >>> > that performs everything that is post-targets and pre-rootfs. There isn't >>> > much that needs to be done so parallelisation doesn't make sense. And I >>> > think it's much easier to understand which steps are executed and in >>> > which order if they are all put together in a single rule rather than >>> > spread out over several. >>> >>> > To make things more readable, we can put the commands into separate >>> > variables. For instance: >>> >>> > define TARGET_PURGE_LOCALES >>> > rm -f $(LOCALE_WHITELIST) >>> > ... >>> > endef >>> >>> > define TARGET_PURGE_DEVFILES >>> > rm -rf $(TARGET_DIR)/usr/include ... >>> > ... >>> > endef >>> >>> > ifneq ($(BR2_PACKAGE_GDB),y) >>> > define TARGET_PURGE_GDB >>> > rm -rf $(TARGET_DIR)/usr/share/gdb >>> > endef >>> > endif >>> >>> > target-finalize: $(TARGETS) >>> > $(TARGET_PURGE_LOCALES) >>> > $(TARGET_PURGE_DEVFILES) >>> > $(TARGET_PURGE_GDB) >>> > $(TARGET_PURGE_DOC) >>> > ... >>> >>> Yes, that looks nice and clear to me too. >> >> Yes, agreed, this looks a lot nicer than a long chain of targets that >> simply depend on each other to avoid any parallelization. > > Best regards > -- 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: 7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F ^ permalink raw reply [flat|nested] 15+ messages in thread
* [Buildroot] [PATCH] system: move system.mk recipes inside the "target-finalize" rule 2014-06-11 16:32 ` Arnout Vandecappelle @ 2014-06-11 17:36 ` Thomas Petazzoni 2014-06-12 7:20 ` Fabio Porcedda 1 sibling, 0 replies; 15+ messages in thread From: Thomas Petazzoni @ 2014-06-11 17:36 UTC (permalink / raw) To: buildroot Dear Arnout Vandecappelle, On Wed, 11 Jun 2014 18:32:36 +0200, Arnout Vandecappelle wrote: > >> target-finalize: $(TARGETS) > >> $(foreach hook,$(TARGET_FINALIZE_HOOKS),$(call $(hook),end,$(1),$($(PKG)_NAME))$(sep)) > > I like this idea. Though it should actually be > > $(foreach hook,$(TARGET_FINALIZE_HOOKS),$($(hook))$(sep)) > > (the call is redundant, the end is redundant, and $(1) and $(PKG) are not > available). Ah, yes, indeed, I copy/pasted the wrong example and did not fix it. Indeed, my example came from pkg-generic.mk, where things are a bit more complicated. > > Using hooks was my initial proposition but Arnout suggested the above > > solution and you and Perer liked it. > > Have you changed your mind about using hooks? > > Actually, your initial proposition was: > > -target-purgelocales: > +target-purgelocales: $(filter-out target-purgelocales,$(TARGETS)) > > And my comment was that I don't like this splitting of target-finalize over > several make targets. It is that remark that lead to the thread below. > > With the hooks, you'll still have a single target-finalize rule that performs > all the finalization and that simply depends on $(TARGETS). True that was Fabio's initial proposal, but if you continue reading the thread, you'll see that the solution Fabio is submitting right now is one we suggested him to implement back then :-) Thomas -- Thomas Petazzoni, CTO, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com ^ permalink raw reply [flat|nested] 15+ messages in thread
* [Buildroot] [PATCH] system: move system.mk recipes inside the "target-finalize" rule 2014-06-11 16:32 ` Arnout Vandecappelle 2014-06-11 17:36 ` Thomas Petazzoni @ 2014-06-12 7:20 ` Fabio Porcedda 2014-06-12 7:32 ` Fabio Porcedda 1 sibling, 1 reply; 15+ messages in thread From: Fabio Porcedda @ 2014-06-12 7:20 UTC (permalink / raw) To: buildroot On Wed, Jun 11, 2014 at 6:32 PM, Arnout Vandecappelle <arnout@mind.be> wrote: > On 06/11/14 11:14, Fabio Porcedda wrote: >> Hi Thomas, >> thanks for reviewing the patch. >> >> On Tue, Jun 10, 2014 at 10:33 PM, Thomas Petazzoni >> <thomas.petazzoni@free-electrons.com> wrote: >>> Dear Fabio Porcedda, >>> >>> On Tue, 10 Jun 2014 10:52:47 +0200, Fabio Porcedda wrote: >>> >>>> target-finalize: $(TARGETS) >>>> @$(call MESSAGE,"Finalizing target directory") >>>> + $(TARGET_FINALIZE_GENERIC_SECURETTY) >>>> + $(TARGET_FINALIZE_GENERIC_HOSTNAME) >>>> + $(TARGET_FINALIZE_GENERIC_ISSUE) >>>> + $(TARGET_FINALIZE_ROOT_PASSWD) >>>> + $(TARGET_FINALIZE_GENERIC_GETTY) >>>> + $(TARGET_FINALIZE_GENERIC_REMOUNT_RW) >>> >>> I agree with the principle, but I'd like to have a better >>> implementation, which is really long overdue to stop cluttering >>> target-finalize with more and more crap. Could we implement >>> target-finalize as just: >>> >>> target-finalize: $(TARGETS) >>> $(foreach hook,$(TARGET_FINALIZE_HOOKS),$(call $(hook),end,$(1),$($(PKG)_NAME))$(sep)) > > I like this idea. Though it should actually be > > $(foreach hook,$(TARGET_FINALIZE_HOOKS),$($(hook))$(sep)) > > (the call is redundant, the end is redundant, and $(1) and $(PKG) are not > available). > >>> >>> And then: >>> >>>> +ifeq ($(BR2_TARGET_GENERIC_GETTY),y) >>>> +define TARGET_FINALIZE_GENERIC_SECURETTY >>>> grep -q '^$(TARGET_GENERIC_GETTY_PORT)$$' $(TARGET_DIR)/etc/securetty || \ >>>> echo '$(TARGET_GENERIC_GETTY_PORT)' >> $(TARGET_DIR)/etc/securetty >>>> +endef >>> >>> TARGET_FINALIZE_HOOKS += TARGET_FINALIZE_GENERIC_SECURETTY >>> >>> Then we can also use this to move the Python, Perl and other >>> package-specific target-finalize logic down to the specific packages. >>> Of course, I'm not asking you to do all of this work. But at least, >>> post a patch introducing the TARGET_FINALIZE_HOOKS, and use it for >>> those targets you were converting in your original patch. >>> >>> We should go towards *removing* stuff from the main Makefile, not >>> adding more :-) >> >> Using hooks was my initial proposition but Arnout suggested the above >> solution and you and Perer liked it. >> Have you changed your mind about using hooks? > > Actually, your initial proposition was: > > -target-purgelocales: > +target-purgelocales: $(filter-out target-purgelocales,$(TARGETS)) > > And my comment was that I don't like this splitting of target-finalize over > several make targets. It is that remark that lead to the thread below. > > With the hooks, you'll still have a single target-finalize rule that performs > all the finalization and that simply depends on $(TARGETS). You are right that my initial proposal was to use the "filter-out" function, to be precise using hooks was my third proposal, and you disliked it, see the previous thread for reference: http://lists.busybox.net/pipermail/buildroot/2014-April/095213.html On Mon, Apr 28, 2014 at 12:30 PM, Arnout Vandecappelle <arnout@mind.be> wrote: > On 28/04/14 09:58, Fabio Porcedda wrote: >> On Mon, Apr 28, 2014 at 7:49 AM, Arnout Vandecappelle <arnout@mind.be> wrote: >>> On 25/04/14 23:50, Fabio Porcedda wrote: >>>> On Thu, Apr 24, 2014 at 6:41 PM, Thomas Petazzoni >>>> <thomas.petazzoni@free-electrons.com> wrote: >>>>> Dear Fabio Porcedda, >>>>> >>>>> On Thu, 24 Apr 2014 18:24:39 +0200, Fabio Porcedda wrote: >>>>> >>>>>> -target-purgelocales: >>>>>> +target-purgelocales: $(filter-out target-purgelocales,$(TARGETS)) >>>>>> rm -f $(LOCALE_WHITELIST) >>>>>> for i in $(LOCALE_NOPURGE); do echo $$i >> $(LOCALE_WHITELIST); done >>>>> >>>>> Don't we have several other targets that need to be executed only after >>>>> all packages have been built and installed? Wouldn't it make sense to >>>>> have a common solution for these? Like maybe a dedicated target? >>>> >>>> Can you please give some examples? I know only tatget-purgelocales and >>>> target-finalize. >>>> >>>> About the common solution, i see two possible solutions of the problem: >>>> >>>> 1) all those targets must be listed in a variable like >>>> TARGETS_PRE_ROOTFS, but those targets must be able to be executed in >>>> parallel without a specific order. >>>> >>>> 2) all those targets must be converted in hooks and added to a >>>> variable like PRE_ROOTFS_HOOKS, so those steps are going to be >>>> executed in serial observing a specific order. >>>> >>>> What is the more appropriate solution? The easiest and fastest one is >>>> the first, but i'm not sure if those targets can be executed in >>>> parallel. >>> >>> My personal preference is to have a single rule (e.g. target-finalize) >>> that performs everything that is post-targets and pre-rootfs. There isn't >>> much that needs to be done so parallelisation doesn't make sense. And I >>> think it's much easier to understand which steps are executed and in >>> which order if they are all put together in a single rule rather than >>> spread out over several. >>> >>> To make things more readable, we can put the commands into separate >>> variables. For instance: >>> >>> define TARGET_PURGE_LOCALES >>> rm -f $(LOCALE_WHITELIST) >>> ... >>> endef >>> >>> define TARGET_PURGE_DEVFILES >>> rm -rf $(TARGET_DIR)/usr/include ... >>> ... >>> endef >>> >>> ifneq ($(BR2_PACKAGE_GDB),y) >>> define TARGET_PURGE_GDB >>> rm -rf $(TARGET_DIR)/usr/share/gdb >>> endef >>> endif >>> >>> target-finalize: $(TARGETS) >>> $(TARGET_PURGE_LOCALES) >>> $(TARGET_PURGE_DEVFILES) >>> $(TARGET_PURGE_GDB) >>> $(TARGET_PURGE_DOC) >>> ... >>> >>> >>> I'm giving the content of target-finalize as an example here, but it's >>> not immediately needed to convert those into variables. The different >>> target-* steps that are currently added to TARGETS are rather more important. >> >> It's fine for me. >> So you prefer to list those variables inside the target instead of >> adding them to a hook variable? like e.g. PRE_ROOTFS_HOOK? > > Indeed. The HOOKS way of working is not very clear. The code for calling > the hooks is not easy to understand at first sight, because it contains > this $(foreach ...) and $(sep) stuff. Also, the assignments to the HOOKS > variable are spread out over the Makefiles so it's hard to see which > things will get executed and in which order. And finally, it's not needed > here because we can enumerate all the possible steps - that's different > from the package infrastructure, where the HOOK contents differs from > package to package. > > > Regards, > Arnout I still prefer to use hooks so because now we all agree to use hooks it's perfect :) My first patch will be like this: diff --git a/Makefile b/Makefile index dc86060..f8e446d 100644 --- a/Makefile +++ b/Makefile @@ -525,13 +525,14 @@ define TARGET_PURGE_LOCALES done; \ done endef +TARGET_FINALIZE_HOOKS += TARGET_PURGE_LOCALES endif $(TARGETS_ROOTFS): target-finalize target-finalize: $(TARGETS) @$(call MESSAGE,"Finalizing target directory") - $(TARGET_PURGE_LOCALES) + $(foreach hook,$(TARGET_FINALIZE_HOOKS),$($(hook))$(sep)) rm -rf $(TARGET_DIR)/usr/include $(TARGET_DIR)/usr/share/aclocal \ $(TARGET_DIR)/usr/lib/pkgconfig $(TARGET_DIR)/usr/share/pkgconfig \ $(TARGET_DIR)/usr/lib/cmake $(TARGET_DIR)/usr/share/cmake I will send an updated patch set that will use hooks. Best regards -- Fabio Porcedda ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [Buildroot] [PATCH] system: move system.mk recipes inside the "target-finalize" rule 2014-06-12 7:20 ` Fabio Porcedda @ 2014-06-12 7:32 ` Fabio Porcedda 2014-06-12 7:56 ` Thomas Petazzoni 0 siblings, 1 reply; 15+ messages in thread From: Fabio Porcedda @ 2014-06-12 7:32 UTC (permalink / raw) To: buildroot On Thu, Jun 12, 2014 at 9:20 AM, Fabio Porcedda <fabio.porcedda@gmail.com> wrote: > My first patch will be like this: > > diff --git a/Makefile b/Makefile > index dc86060..f8e446d 100644 > --- a/Makefile > +++ b/Makefile > @@ -525,13 +525,14 @@ define TARGET_PURGE_LOCALES > done; \ > done > endef > +TARGET_FINALIZE_HOOKS += TARGET_PURGE_LOCALES > endif > > $(TARGETS_ROOTFS): target-finalize > > target-finalize: $(TARGETS) > @$(call MESSAGE,"Finalizing target directory") > - $(TARGET_PURGE_LOCALES) > + $(foreach hook,$(TARGET_FINALIZE_HOOKS),$($(hook))$(sep)) > rm -rf $(TARGET_DIR)/usr/include $(TARGET_DIR)/usr/share/aclocal \ > $(TARGET_DIR)/usr/lib/pkgconfig > $(TARGET_DIR)/usr/share/pkgconfig \ > $(TARGET_DIR)/usr/lib/cmake $(TARGET_DIR)/usr/share/cmake > > > I will send an updated patch set that will use hooks. I think it's best to use PURGE_LOCALES_HOOK instead of TARGET_PURGE_LOCALES. diff --git a/Makefile b/Makefile index dc86060..bb51727 100644 --- a/Makefile +++ b/Makefile @@ -513,7 +513,7 @@ ifeq ($(BR2_ENABLE_LOCALE_PURGE),y) LOCALE_WHITELIST = $(BUILD_DIR)/locales.nopurge LOCALE_NOPURGE = $(call qstrip,$(BR2_ENABLE_LOCALE_WHITELIST)) -define TARGET_PURGE_LOCALES +define PURGE_LOCALES_HOOK rm -f $(LOCALE_WHITELIST) for i in $(LOCALE_NOPURGE); do echo $$i >> $(LOCALE_WHITELIST); done @@ -525,13 +525,14 @@ define TARGET_PURGE_LOCALES done; \ done endef +TARGET_FINALIZE_HOOKS += PURGE_LOCALES_HOOK endif $(TARGETS_ROOTFS): target-finalize target-finalize: $(TARGETS) @$(call MESSAGE,"Finalizing target directory") - $(TARGET_PURGE_LOCALES) + $(foreach hook,$(TARGET_FINALIZE_HOOKS),$($(hook))$(sep)) rm -rf $(TARGET_DIR)/usr/include $(TARGET_DIR)/usr/share/aclocal \ $(TARGET_DIR)/usr/lib/pkgconfig $(TARGET_DIR)/usr/share/pkgconfig \ $(TARGET_DIR)/usr/lib/cmake $(TARGET_DIR)/usr/share/cmake Best regards -- Fabio Porcedda ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [Buildroot] [PATCH] system: move system.mk recipes inside the "target-finalize" rule 2014-06-12 7:32 ` Fabio Porcedda @ 2014-06-12 7:56 ` Thomas Petazzoni 2014-06-12 8:05 ` Fabio Porcedda 0 siblings, 1 reply; 15+ messages in thread From: Thomas Petazzoni @ 2014-06-12 7:56 UTC (permalink / raw) To: buildroot Dear Fabio Porcedda, On Thu, 12 Jun 2014 09:32:39 +0200, Fabio Porcedda wrote: > I think it's best to use PURGE_LOCALES_HOOK instead of TARGET_PURGE_LOCALES. I think one of the concern with the hook thing was that the hooks would be spread all over the Buildroot tree. To mitigate this concern, maybe we could have a naming convention for those hooks that they should all start with TARGET_FINALIZE_<something>, like TARGET_FINALIZE_PURGE_LOCALES ? This way, we can more easily grep through the tree those hooks? Thomas -- Thomas Petazzoni, CTO, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com ^ permalink raw reply [flat|nested] 15+ messages in thread
* [Buildroot] [PATCH] system: move system.mk recipes inside the "target-finalize" rule 2014-06-12 7:56 ` Thomas Petazzoni @ 2014-06-12 8:05 ` Fabio Porcedda 2014-06-12 8:23 ` Thomas Petazzoni 0 siblings, 1 reply; 15+ messages in thread From: Fabio Porcedda @ 2014-06-12 8:05 UTC (permalink / raw) To: buildroot On Thu, Jun 12, 2014 at 9:56 AM, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > Dear Fabio Porcedda, > > On Thu, 12 Jun 2014 09:32:39 +0200, Fabio Porcedda wrote: > >> I think it's best to use PURGE_LOCALES_HOOK instead of TARGET_PURGE_LOCALES. > > I think one of the concern with the hook thing was that the hooks would > be spread all over the Buildroot tree. To mitigate this concern, maybe > we could have a naming convention for those hooks that they should all > start with TARGET_FINALIZE_<something>, like > TARGET_FINALIZE_PURGE_LOCALES ? Ok fine, i like it, maybe using TARGET_FINALIZE_HOOK_PURGE_LOCALES is too verbose? > This way, we can more easily grep through the tree those hooks? Sure. Best regards -- Fabio Porcedda ^ permalink raw reply [flat|nested] 15+ messages in thread
* [Buildroot] [PATCH] system: move system.mk recipes inside the "target-finalize" rule 2014-06-12 8:05 ` Fabio Porcedda @ 2014-06-12 8:23 ` Thomas Petazzoni 2014-06-12 12:54 ` Arnout Vandecappelle 0 siblings, 1 reply; 15+ messages in thread From: Thomas Petazzoni @ 2014-06-12 8:23 UTC (permalink / raw) To: buildroot Dear Fabio Porcedda, On Thu, 12 Jun 2014 10:05:41 +0200, Fabio Porcedda wrote: > > I think one of the concern with the hook thing was that the hooks would > > be spread all over the Buildroot tree. To mitigate this concern, maybe > > we could have a naming convention for those hooks that they should all > > start with TARGET_FINALIZE_<something>, like > > TARGET_FINALIZE_PURGE_LOCALES ? > > Ok fine, i like it, maybe using TARGET_FINALIZE_HOOK_PURGE_LOCALES is > too verbose? If anything, the "HOOK" should be at the end. Not sure it's needed though if we have all variables prefixed by TARGET_FINALIZE_<something>. That being said, now that I can think of it, we can anyway always grep all the hooks by grepping for "TARGET_FINALIZE_HOOKS", which will return all files doing: TARGET_FINALIZE_HOOKS += <something> So in the end, the name of make function being registered doesn't matter much, but still for consistency, I think it's good to have a common prefix for all of them. Thomas -- Thomas Petazzoni, CTO, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com ^ permalink raw reply [flat|nested] 15+ messages in thread
* [Buildroot] [PATCH] system: move system.mk recipes inside the "target-finalize" rule 2014-06-12 8:23 ` Thomas Petazzoni @ 2014-06-12 12:54 ` Arnout Vandecappelle 2014-06-19 10:06 ` Fabio Porcedda 0 siblings, 1 reply; 15+ messages in thread From: Arnout Vandecappelle @ 2014-06-12 12:54 UTC (permalink / raw) To: buildroot On 06/12/14 10:23, Thomas Petazzoni wrote: > Dear Fabio Porcedda, > > On Thu, 12 Jun 2014 10:05:41 +0200, Fabio Porcedda wrote: > >>> I think one of the concern with the hook thing was that the hooks would >>> be spread all over the Buildroot tree. To mitigate this concern, maybe >>> we could have a naming convention for those hooks that they should all >>> start with TARGET_FINALIZE_<something>, like >>> TARGET_FINALIZE_PURGE_LOCALES ? >> >> Ok fine, i like it, maybe using TARGET_FINALIZE_HOOK_PURGE_LOCALES is >> too verbose? > > If anything, the "HOOK" should be at the end. Not sure it's needed > though if we have all variables prefixed by TARGET_FINALIZE_<something>. > > That being said, now that I can think of it, we can anyway always grep > all the hooks by grepping for "TARGET_FINALIZE_HOOKS", which will > return all files doing: > > TARGET_FINALIZE_HOOKS += <something> > > So in the end, the name of make function being registered doesn't > matter much, but still for consistency, I think it's good to have a > common prefix for all of them. Well, the main reason to use the hooks approach is to be able to spread it out over different makefiles, and in particular to be able to do package-specific target-finalize things. Therefore, the hooks themselves should be named <PKGNAME>_FOO_BAR. Also, for the hooks that are currently in use, we don't usually include HOOK in the name. Regards, Arnout -- 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: 7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F ^ permalink raw reply [flat|nested] 15+ messages in thread
* [Buildroot] [PATCH] system: move system.mk recipes inside the "target-finalize" rule 2014-06-12 12:54 ` Arnout Vandecappelle @ 2014-06-19 10:06 ` Fabio Porcedda 0 siblings, 0 replies; 15+ messages in thread From: Fabio Porcedda @ 2014-06-19 10:06 UTC (permalink / raw) To: buildroot On Thu, Jun 12, 2014 at 2:54 PM, Arnout Vandecappelle <arnout@mind.be> wrote: > On 06/12/14 10:23, Thomas Petazzoni wrote: >> Dear Fabio Porcedda, >> >> On Thu, 12 Jun 2014 10:05:41 +0200, Fabio Porcedda wrote: >> >>>> I think one of the concern with the hook thing was that the hooks would >>>> be spread all over the Buildroot tree. To mitigate this concern, maybe >>>> we could have a naming convention for those hooks that they should all >>>> start with TARGET_FINALIZE_<something>, like >>>> TARGET_FINALIZE_PURGE_LOCALES ? >>> >>> Ok fine, i like it, maybe using TARGET_FINALIZE_HOOK_PURGE_LOCALES is >>> too verbose? >> >> If anything, the "HOOK" should be at the end. Not sure it's needed >> though if we have all variables prefixed by TARGET_FINALIZE_<something>. >> >> That being said, now that I can think of it, we can anyway always grep >> all the hooks by grepping for "TARGET_FINALIZE_HOOKS", which will >> return all files doing: >> >> TARGET_FINALIZE_HOOKS += <something> >> >> So in the end, the name of make function being registered doesn't >> matter much, but still for consistency, I think it's good to have a >> common prefix for all of them. > > Well, the main reason to use the hooks approach is to be able to spread it out > over different makefiles, and in particular to be able to do package-specific > target-finalize things. Therefore, the hooks themselves should be named > <PKGNAME>_FOO_BAR. > > Also, for the hooks that are currently in use, we don't usually include HOOK in > the name. Following what we usually done for hooks, i'm going to call it: PURGE_LOCALES SYSTEM_GENERIC_SECURETTY SYSTEM_GENERIC_HOSTNAME ... BTW: Wouldn't better to change the usual name we use for hooks, maybe adding "_HOOK" at the end of name? I've found only four hooks that have "HOOK" in the name. Best regards -- Fabio Porcedda ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2014-06-19 10:06 UTC | newest] Thread overview: 15+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-06-10 8:52 [Buildroot] [PATCH] system: move system.mk recipes inside the "target-finalize" rule Fabio Porcedda 2014-06-10 20:33 ` Thomas Petazzoni 2014-06-11 9:14 ` Fabio Porcedda 2014-06-11 9:18 ` Thomas Petazzoni 2014-06-11 9:21 ` Fabio Porcedda 2014-06-11 12:54 ` Thomas Petazzoni 2014-06-11 16:32 ` Arnout Vandecappelle 2014-06-11 17:36 ` Thomas Petazzoni 2014-06-12 7:20 ` Fabio Porcedda 2014-06-12 7:32 ` Fabio Porcedda 2014-06-12 7:56 ` Thomas Petazzoni 2014-06-12 8:05 ` Fabio Porcedda 2014-06-12 8:23 ` Thomas Petazzoni 2014-06-12 12:54 ` Arnout Vandecappelle 2014-06-19 10:06 ` Fabio Porcedda
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox