[Buildroot] [PATCH] system: move system.mk recipes inside the "target-finalize" rule

[Buildroot] [PATCH] system: move system.mk recipes inside the "target-finalize" rule

[Fabio Porcedda]

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

[Buildroot] [PATCH] system: move system.mk recipes inside the "target-finalize" rule

[Thomas Petazzoni]

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

[Buildroot] [PATCH] system: move system.mk recipes inside the "target-finalize" rule

[Fabio Porcedda]

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

[Buildroot] [PATCH] system: move system.mk recipes inside the "target-finalize" rule

[Thomas Petazzoni]

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

[Buildroot] [PATCH] system: move system.mk recipes inside the "target-finalize" rule

[Fabio Porcedda]

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

[Buildroot] [PATCH] system: move system.mk recipes inside the "target-finalize" rule

[Thomas Petazzoni]

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

[Buildroot] [PATCH] system: move system.mk recipes inside the "target-finalize" rule

[Arnout Vandecappelle]

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

[Buildroot] [PATCH] system: move system.mk recipes inside the "target-finalize" rule

[Thomas Petazzoni]

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

[Buildroot] [PATCH] system: move system.mk recipes inside the "target-finalize" rule

[Fabio Porcedda]

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

[Buildroot] [PATCH] system: move system.mk recipes inside the "target-finalize" rule

[Fabio Porcedda]

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

[Buildroot] [PATCH] system: move system.mk recipes inside the "target-finalize" rule

[Thomas Petazzoni]

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

[Buildroot] [PATCH] system: move system.mk recipes inside the "target-finalize" rule

[Fabio Porcedda]

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

[Buildroot] [PATCH] system: move system.mk recipes inside the "target-finalize" rule

[Thomas Petazzoni]

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

[Buildroot] [PATCH] system: move system.mk recipes inside the "target-finalize" rule

[Arnout Vandecappelle]

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

[Buildroot] [PATCH] system: move system.mk recipes inside the "target-finalize" rule

[Fabio Porcedda]

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