RFC: Getting rid of old-style hooks

Hello,

(Sorry, long presentation, the question is at the end)

I've started working on getting rid of the old-style hooks, i.e the hooks defined this way :

====================================================================
[...]
$(eval $(call AUTOTARGETS,package,bind))

$(BIND_HOOK_POST_INSTALL):

	rm -f $(TARGET_DIR)/usr/bin/isc-config.sh
	$(INSTALL) -m 0755 -D package/bind/bind.sysvinit $(TARGET_DIR)/etc/init.d/S81named
ifneq ($(BR2_PACKAGE_BIND_TOOLS),y)
	rm -rf $(addprefix $(TARGET_DIR)/usr/bin/, $(BIND_TARGET_BINS))

endif
touch $@
====================================================================

to the new style hooks :

====================================================================
[...]

define BIND_TARGET_INSTALL_FIXES

        rm -f $(TARGET_DIR)/usr/bin/isc-config.sh
        $(INSTALL) -m 0755 -D package/bind/bind.sysvinit $(TARGET_DIR)/etc/init.d/S81named

endef

BIND_POST_INSTALL_TARGET_HOOKS += BIND_TARGET_INSTALL_FIXES

define BIND_TARGET_REMOVE_TOOLS

        rm -rf $(addprefix $(TARGET_DIR)/usr/bin/, $(BIND_TARGET_BINS))

endef

ifneq ($(BR2_PACKAGE_BIND_TOOLS),y) BIND_POST_INSTALL_TARGET_HOOKS += BIND_TARGET_REMOVE_TOOLS endif
====================================================================

I've done the work for all remaining old-style hooks, which also to get rid of the support for these in the package infrastructure. This work, composed of 70+ commits, is visible at :

http://git.buildroot.net/~tpetazzoni/git/buildroot/log/?h=for-2010.11/remove-oldstyle-hooks

However, I have a question concerning these new style hooks. Since we can't use make conditionals inside variable definitions, it is not possible to write:

====================================================================
define FOOBAR_REMOVE_STUFF

	rm $(TARGET_DIR)/usr/bin/foo
ifneq($(FOOBAR_INSTALL_BAR),y)
	rm $(TARGET_DIR)/usr/bin/bar

endif
endef

FOOBAR_POST_INSTALL_TARGET_HOOKS += FOOBAR_REMOVE_STUFF
====================================================================

and we instead have to write:

====================================================================
define FOOBAR_REMOVE_FOO

	rm $(TARGET_DIR)/usr/bin/foo

endef

FOOBAR_POST_INSTALL_TARGET_HOOKS += FOOBAR_REMOVE_FOO

define FOOBAR_REMOVE_BAR

	rm $(TARGET_DIR)/usr/bin/bar

endef

ifneq($(FOOBAR_INSTALL_BAR),y) FOOBAR_POST_INSTALL_TARGET_HOOKS += FOOBAR_REMOVE_BAR endif
====================================================================

or

====================================================================
ifneq($(FOOBAR_INSTALL_BAR),y)
define FOOBAR_REMOVE_BAR

	rm $(TARGET_DIR)/usr/bin/bar

endef
endif

define FOOBAR_REMOVE_STUFF

	rm $(TARGET_DIR)/usr/bin/foo
	$(FOOBAR_REMOVE_BAR)

endef

FOOBAR_POST_INSTALL_TARGET_HOOKS += FOOBAR_REMOVE_STUFF
====================================================================

In other words, the conditional is placed on the assignment adding the hook to the <pkg>_POST_INSTALL_TARGET_HOOKS list of hooks. This is the only solution possible today with the current package infrastructure, and is the one I've used in the patch set mentioned above.

However, there is another possible solution, on which I'd like to have your opinion. Instead of having a list <pkg>_POST_INSTALL_TARGET_HOOKS which contains the list of hooks to call, we could directly make the package infrastructure call all hooks that are prefixed by <pkg>_POST_INSTALL_TARGET_HOOK_. So, the above example would become:

====================================================================
define FOOBAR_POST_INSTALL_TARGET_HOOK_REMOVE_FOO

	rm $(TARGET_DIR)/usr/bin/foo

endef

ifneq($(FOOBAR_INSTALL_BAR),y)
define FOOBAR_POST_INSTALL_TARGET_HOOK_REMOVE_BAR

	rm $(TARGET_DIR)/usr/bin/bar

endef
endif
===================================================================

Which is a little bit shorter. In terms of implementation in the package infrastructure, it is almost as simple as the current solution. For example, for the post extract hook, the change would be:

- $(foreach hook,$($(PKG)_POST_EXTRACT_HOOKS),$(call $(hook))$(sep))
+ $(foreach hook,$(filter $(PKG)_POST_EXTRACT_HOOK_%, $(.VARIABLES)),$(call $(hook))$(sep))

However, a major difference is the call order:

* With the current solution, hooks are called in the order they are defined in the .mk file

* With the new proposed solution, hooks are called in the reverse order of their definition in the .mk file. In the case of hooks having dependencies between each other, this may be strange (but do we really have such cases today, and do we want to support such strange cases ?)

Which solution do you prefer ?

Another point is that in the generic package infrastructure, when defining the commands to be executed in a step, there is also a single variable that must be assigned, so we have more or less the same problem. For example, with wpa_supplicant, I had to do something like :

===================================================================
ifeq ($(BR2_PACKAGE_LIBNL),y)

        WPA_SUPPLICANT_DEPENDENCIES += libnl
        WPA_SUPPLICANT_LIBNL_CONFIG = \
                echo "CONFIG_DRIVER_NL80211=y" >>$(WPA_SUPPLICANT_CONFIG)

endif

define WPA_SUPPLICANT_CONFIGURE_CMDS

        cp $(@D)/wpa_supplicant/defconfig $(WPA_SUPPLICANT_CONFIG)
        echo "CFLAGS += $(TARGET_CFLAGS)" >>$(WPA_SUPPLICANT_CONFIG)
        echo "LDFLAGS += $(TARGET_LDFLAGS)" >>$(WPA_SUPPLICANT_CONFIG)
        echo "CC = $(TARGET_CC)" >>$(WPA_SUPPLICANT_CONFIG)
        $(SED) "s/\/local//" $(@D)/wpa_supplicant/Makefile
        $(WPA_SUPPLICANT_LIBNL_CONFIG)

endef
===================================================================

I.e, the conditional part has to be taken care by a separate variable.

Thanks for your feedback,

Thomas

Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Is it possible to avoid conditional defines altogether by using computed variable names?

I am not an expert in this and I admit I haven't read your email entirely or understand completely the problem you're trying to solve, but in general, conditional makefile fragments can be replaced by code using computed variable names.

I will take this as an example:

I'm not an expert at this, but what about something like:

FOOBAR_INSTALL_BAR_y := rm $(TARGET_DIR)/usr/bin/bar

define FOOBAR_REMOVE_STUFF

       rm $(TARGET_DIR)/usr/bin/foo
       $(FOOBAR_INSTALL_BAR_$(FOOBAR_INSTALL_BAR))

endef

This is just a proof of concept which ignores the question of whether this is what you want to do. And the variable names I've picked are quite clumsy.

This also might mean you don't need to use defines. Instead, you can put these commands into a variable.

Hmm, water's quite deep here, I'll swim back to shore now :-)

Mitch.