system: move system.mk recipes inside the "target-finalize" rule

Dear Fabio Porcedda,

Makefile $(TARGETS_ROOTFS): target-finalize · line 539

Quoted 8 lines · Fabio Porcedda

target-finalize: $(TARGETS)

      @$(call MESSAGE,"Finalizing target directory")

Makefile +6 -0

+     $(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:

system/system.mk TARGET_GENERIC_GETTY_BAUDRATE = $(call qstrip,$(BR2_TARGET_GENERIC_GETTY_BAUDRAT · line 14

Quoted 5 lines

system/system.mk +3 -0

+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 :-)

Peter, Arnout,

Quoted 33 lines · Peter Korsgaard

Quoted 32 lines

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)
...

Quoted 1 line

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.

Quoted 2 lines

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?

Dear Fabio Porcedda,

Quoted 3 lines · Fabio Porcedda

Quoted 2 lines

We should go towards *removing* stuff from the main Makefile, not adding more :-)

Quoted 3 lines

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

Quoted 2 lines

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

Hi Thomas,
thanks for reviewing the patch.

Quoted 20 lines · Thomas Petazzoni

Dear Fabio Porcedda,

Makefile $(TARGETS_ROOTFS): target-finalize · line 539

Quoted 8 lines · Fabio Porcedda

target-finalize: $(TARGETS)

      @$(call MESSAGE,"Finalizing target directory")

Makefile +6 -0

+     $(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))

Quoted 17 lines

And then:

system/system.mk TARGET_GENERIC_GETTY_BAUDRATE = $(call qstrip,$(BR2_TARGET_GENERIC_GETTY_BAUDRAT · line 14

Quoted 5 lines

system/system.mk +3 -0

+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.

Quoted 2 lines

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:

Quoted 37 lines · Thomas Petazzoni

Peter, Arnout,

Quoted 33 lines · Peter Korsgaard

Quoted 32 lines

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)
...

Quoted 4 lines

Quoted 1 line

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

Quoted 2 lines

Quoted 2 lines

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).

Quoted 3 lines

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:

(quoted diff) +1 -1

-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).

Quoted 25 lines · Fabio Porcedda

Hi Thomas,
thanks for reviewing the patch.

Quoted 20 lines · Thomas Petazzoni

Dear Fabio Porcedda,

Review

Quoted 8 lines · Fabio Porcedda

target-finalize: $(TARGETS)

      @$(call MESSAGE,"Finalizing target directory")

(quoted diff) +6 -0

+     $(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).

Quoted 17 lines

Quoted 17 lines

And then:

Review

Quoted 5 lines

(quoted diff) +3 -0

+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.

Quoted 6 lines

Quoted 2 lines

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:

(quoted diff) +1 -1

-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).

Quoted 68 lines · Fabio Porcedda

Quoted 67 lines · Arnout Vandecappelle

Quoted 24 lines · Fabio Porcedda

Quoted 4 lines · Thomas Petazzoni

Dear Fabio Porcedda,

On Thu, 24 Apr 2014 18:24:39 +0200, Fabio Porcedda wrote:

Quoted 8 lines

Review

Quoted 4 lines

(quoted diff) +1 -1

-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.

Quoted 7 lines

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)

...

Quoted 7 lines

Quoted 3 lines

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

My first patch will be like this:

Makefile +2 -1

index dc86060..f8e446d 100644
@@ -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.

Dear Fabio Porcedda,

Quoted 1 line · Fabio Porcedda

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?

Quoted 5 lines

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?

Dear Fabio Porcedda,

Quoted 8 lines · Fabio Porcedda

Quoted 5 lines

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.

Quoted 20 lines · Thomas Petazzoni

Dear Fabio Porcedda,

Quoted 8 lines · Fabio Porcedda

Quoted 5 lines

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:

Quoted 5 lines

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.