| Message ID | 1402390367-25418-1-git-send-email-fabio.porcedda@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
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
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
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
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
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
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 >
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
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
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(-)