Add CONF_ENABLE and CONF_PKG_ENABLE helper functions
cover Add CONF_ENABLE and CONF_PKG_ENABLE helper functions
Many packages have the following type of construct:
ifeq ($(BR2_PACKAGE_ACL),y)
SYSTEMD_CONF_OPT += --enable-acl
SYSTEMD_DEPENDENCIES += acl
else
SYSTEMD_CONF_OPT += --disable-aclendif
The first patch in this series abbreviates that to
$(eval $(call CONF_PKG_ENABLE,SYSTEMD,acl))
It makes it possible to abbreviate similar constructs as well.
The main advantage I see is not so much that the .mk files get
shorter, but more importantly that it's easier to keep things consistent, e.g. making sure that --enable and --disable are always given explicitly.
The second and the third patch are examples of how this can be used in
practice. I've chosen two random packages that have a lot of these --enable/--disable options. Once the main implementation is accepted I'll probably start to convert some more packages.
Note: I don't particularly like the way the call statement looks, but
I couldn't find a better way to do it. Suggestions are definitely welcome!
Regards,
Arnout
----------------------------------------------------------------
Arnout Vandecappelle (Essensium/Mind) (3):
pkg-infra: add CONF_ENABLE and CONF_PKG_ENABLE helper functions
avahi: use CONF_ENABLE and CONF_PKG_ENABLE
cairo: use CONF_ENABLE and CONF_PKG_ENABLE
package/avahi/avahi.mk | 34 +++++----------------------------- package/cairo/cairo.mk | 44 ++++++-------------------------------------- package/pkg-autotargets.mk | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 68 insertions(+), 67 deletions(-)
PATCH 1/3 pkg-infra: add CONF_ENABLE and CONF_PKG_ENABLE helper functions
From: "Arnout Vandecappelle (Essensium/Mind)" <[email protected]>
CONF_ENABLE is shorthand for --enable/--disable options depending on a .config variable.
CONF_PKG_ENABLE is shorthand for --enable/--disable options and for a dependency depending on the selection of a package.
These macros make it easier to consistently add --enable and --disable options to configure.
--- package/pkg-autotargets.mk | 57 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+)
package/pkg-autotargets.mk +57 -0
@@ -37,6 +37,63 @@ define CONFIG_UPDATE endef ################################################################################ +# CONFIG_ENABLE -- generates the configure option to enable or disable a +# feature depending on a .config option. +# +# argument 1 is the uppercase package name +# argument 2 is y or empty (i.e. it is the .config option to check) +# argument 3 (optional) are additional dependencies in case the .config option +# is y. +# If the call has four arguments, argument 4 is the name that comes behind +# --enable- resp. --disable- +# If the call has five argument, argument 4 is the ensable option and +# argument 5 is the disable option. +# The call must be done within an $(eval) clause. +# +# Examples: +# $(eval $(call CONF_ENABLE,CAIRO,$(BR2_PACKAGE_CAIRO_SVG),,svg)) +# $(eval $(call CONF_ENABLE,CAIRO,$(BR2_PACKAGE_CAIRO_PNG),libpng,png)) +# $(eval $(call CONF_ENABLE,AVAHI,$(BR2_PACKAGE_AVAHI_DAEMON),expat,--with-xml=expat,--with-xml=none)) +# +################################################################################ +define CONF_ENABLE +ifeq ($(2),y) + $(1)_CONF_OPT += $(if $(5),$(4),--enable-$(4)) + $(1)_DEPENDENCIES += $(3) +else + $(1)_CONF_OPT += $(if $(5),$(5),--disable-$(4)) +endif +endef + +################################################################################ +# CONF_PKG ENABLE -- generates the configure option and dependency for another +# package, depending on whether that package is selected. If the other package +# is selected, an --enable- option and a dependency on the other package is +# added. If the other package is not selected, a --disable- option is added. +# +# argument 1 is the uppercase package name +# argument 2 is the lowercase name of the other package +# If the call has two arguments, the --enable-<pkg> configure option is added. +# If the call has three arguments, argument 3 is the name that comes after +# --enable- and --disable- (in case this differs from the buildroot package +# name). +# If the call has four arguments, argument 3 is the option to enable the +# other package and argument 4 is the option to disable it. +# +# The call must be done within an $(eval) clause. +# +# Examples: +# $(eval $(call CONF_PKG_ENABLE,AVAHI,dbus)) +# $(eval $(call CONF_PKG_ENABLE,AVAHI,libglade,gtk)) +# $(eval $(call CONF_PKG_ENABLE,AVAHI,libglib2,--enable-glib --enable-gobject,--disable-glib --disable-gobject)) +# +################################################################################ +define CONF_PKG_ENABLE +$(call CONF_ENABLE,$(1),$(BR2_PACKAGE_$(call UPPERCASE,$(2))),$(2),$(if $(3),$(3),$(2)),$(if $(4),$(4),)) +endef + + +################################################################################ # AUTOTARGETS_INNER -- defines how the configuration, compilation and # installation of an autotools package should be done, implements a # few hooks to tune the build process for autotools specifities and
Signature · 1 line
1.7.10
PATCH 2/3 avahi: use CONF_ENABLE and CONF_PKG_ENABLE
In reply to Arnout Vandecappelle (Essensium/Mind)From: "Arnout Vandecappelle (Essensium/Mind)" <[email protected]>
--- package/avahi/avahi.mk | 34 +++++----------------------------- 1 file changed, 5 insertions(+), 29 deletions(-)
package/avahi/avahi.mk +5 -29
@@ -91,31 +91,12 @@ else AVAHI_CONF_OPT += --disable-libdaemon endif -ifeq ($(BR2_PACKAGE_AVAHI_DAEMON),y) -AVAHI_DEPENDENCIES += expat -AVAHI_CONF_OPT += --with-xml=expat -else -AVAHI_CONF_OPT += --with-xml=none -endif - -ifeq ($(BR2_PACKAGE_DBUS),y) -AVAHI_DEPENDENCIES += dbus -else -AVAHI_CONF_OPT += --disable-dbus -endif - -ifeq ($(BR2_PACKAGE_LIBGLIB2),y) -AVAHI_DEPENDENCIES += libglib2 -else -AVAHI_CONF_OPT += --disable-glib --disable-gobject -endif - -ifeq ($(BR2_PACKAGE_LIBGLADE),y) -AVAHI_DEPENDENCIES += libglade -else -AVAHI_CONF_OPT += --disable-gtk -endif +$(eval $(call CONF_ENABLE,AVAHI,$(BR2_PACKAGE_AVAHI_DAEMON),expat,--with-xml=expat,--with-xml=none)) +$(eval $(call CONF_PKG_ENABLE,AVAHI,dbus)) +$(eval $(call CONF_PKG_ENABLE,AVAHI,libglib2,--enable-glib --enable-gobject,--disable-glib --disable-gobject)) +$(eval $(call CONF_PKG_ENABLE,AVAHI,libglade,gtk)) +$(eval $(call CONF_PKG_ENABLE,AVAHI,python)) ifeq ($(BR2_PACKAGE_PYTHON),y) AVAHI_CONF_ENV += am_cv_pathless_PYTHON=python \ am_cv_path_PYTHON=$(PYTHON_TARGET_BINARY) \
@@ -124,11 +105,6 @@ AVAHI_CONF_ENV += am_cv_pathless_PYTHON=python \ am_cv_python_pythondir=/usr/lib/python$(PYTHON_VERSION_MAJOR)/site-packages \ am_cv_python_pyexecdir=/usr/lib/python$(PYTHON_VERSION_MAJOR)/site-packages \ py_cv_mod_socket_=yes - -AVAHI_DEPENDENCIES += python -AVAHI_CONF_OPT += --enable-python -else -AVAHI_CONF_OPT += --disable-python endif ifeq ($(BR2_PACKAGE_LIBINTL),y)
Signature · 1 line
1.7.10
PATCH 3/3 cairo: use CONF_ENABLE and CONF_PKG_ENABLE
In reply to Arnout Vandecappelle (Essensium/Mind)From: "Arnout Vandecappelle (Essensium/Mind)" <[email protected]>
--- package/cairo/cairo.mk | 44 ++++++-------------------------------------- 1 file changed, 6 insertions(+), 38 deletions(-)
package/cairo/cairo.mk +6 -38
@@ -36,45 +36,13 @@ CAIRO_CONF_ENV = ac_cv_func_posix_getpwuid_r=yes glib_cv_stack_grows=no \ CAIRO_DEPENDENCIES = host-pkg-config fontconfig pixman -ifeq ($(BR2_PACKAGE_DIRECTFB),y) - CAIRO_CONF_OPT += --enable-directfb - CAIRO_DEPENDENCIES += directfb -else - CAIRO_CONF_OPT += --disable-directfb -endif +$(eval $(call CONF_PKG_ENABLE,CAIRO,directfb)) -ifeq ($(BR2_PACKAGE_XORG7),y) - CAIRO_CONF_OPT += --enable-xlib --with-x - CAIRO_DEPENDENCIES += xserver_xorg-server -else - CAIRO_CONF_OPT += --disable-xlib --without-x -endif +$(eval $(call CONF_ENABLE,CAIRO,$(BR2_PACKAGE_XORG7),xserver_xorg-server,--enable-xlib --with-x,--disable-xlib --without-x)) -ifeq ($(BR2_PACKAGE_CAIRO_PS),y) - CAIRO_CONF_OPT += --enable-ps - CAIRO_DEPENDENCIES += zlib -else - CAIRO_CONF_OPT += --disable-ps -endif - -ifeq ($(BR2_PACKAGE_CAIRO_PDF),y) - CAIRO_CONF_OPT += --enable-pdf - CAIRO_DEPENDENCIES += zlib -else - CAIRO_CONF_OPT += --disable-pdf -endif - -ifeq ($(BR2_PACKAGE_CAIRO_PNG),y) - CAIRO_CONF_OPT += --enable-png - CAIRO_DEPENDENCIES += libpng -else - CAIRO_CONF_OPT += --disable-png -endif - -ifeq ($(BR2_PACKAGE_CAIRO_SVG),y) - CAIRO_CONF_OPT += --enable-svg -else - CAIRO_CONF_OPT += --disable-svg -endif +$(eval $(call CONF_ENABLE,CAIRO,$(BR2_PACKAGE_CAIRO_PS),zlib,ps)) +$(eval $(call CONF_ENABLE,CAIRO,$(BR2_PACKAGE_CAIRO_PDF),zlib,pdf)) +$(eval $(call CONF_ENABLE,CAIRO,$(BR2_PACKAGE_CAIRO_PNG),libpng,png)) +$(eval $(call CONF_ENABLE,CAIRO,$(BR2_PACKAGE_CAIRO_SVG),,svg)) $(eval $(call AUTOTARGETS))
Signature · 1 line
1.7.10
cover Add CONF_ENABLE and CONF_PKG_ENABLE helper functions
In reply to Arnout Vandecappelle (Essensium/Mind)Hello Arnout,
Le Sat, 28 Apr 2012 23:16:18 +0200,
"Arnout Vandecappelle (Essensium/Mind)" <[email protected]> a ?crit :
Quoted 7 lines
Many packages have the following type of construct:
ifeq ($(BR2_PACKAGE_ACL),y)
SYSTEMD_CONF_OPT += --enable-acl SYSTEMD_DEPENDENCIES += acl else SYSTEMD_CONF_OPT += --disable-acl
Quoted 7 lines
endif
The first patch in this series abbreviates that to
$(eval $(call CONF_PKG_ENABLE,SYSTEMD,acl))
It makes it possible to abbreviate similar constructs as well.
Thanks for working on this and making a proposal.
However, on my side, I am not yet convinced that this is actually making things better than what we have now. What we have now is quite obvious for the first-time reader, which is very good. Those CONF_PKG_ENABLE and CONF_ENABLE macros make things a bit too cryptic from my point of view. This is not an absolutely strong opinion, but I have the feeling this is on the "we want to make things smaller/shorter and end up making things cryptic" side of the trade-off. Of course, others have to comment to share their views on this.
Regards,
Thomas
Signature · 4 lines
Thomas Petazzoni, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com
cover Add CONF_ENABLE and CONF_PKG_ENABLE helper functions
Quoted 22 lines · Thomas Petazzoni
Hello Arnout,
Le Sat, 28 Apr 2012 23:16:18 +0200,
"Arnout Vandecappelle (Essensium/Mind)" <[email protected]> a ?crit :Quoted 9 lines
?Many packages have the following type of construct:
ifeq ($(BR2_PACKAGE_ACL),y)
? ? ? ?SYSTEMD_CONF_OPT += --enable-acl
? ? ? ?SYSTEMD_DEPENDENCIES += acl
else
? ? ? ?SYSTEMD_CONF_OPT += --disable-acl
endifQuoted 5 lines
?The first patch in this series abbreviates that to
$(eval $(call CONF_PKG_ENABLE,SYSTEMD,acl))
?It makes it possible to abbreviate similar constructs as well.
Thanks for working on this and making a proposal.
Quoted 8 lines
However, on my side, I am not yet convinced that this is actually making things better than what we have now. What we have now is quite obvious for the first-time reader, which is very good. Those
CONF_PKG_ENABLEandCONF_ENABLEmacros make things a bit too cryptic from my point of view. This is not an absolutely strong opinion, but I have the feeling this is on the "we want to make things smaller/shorter and end up making things cryptic" side of the trade-off. Of course, others have to comment to share their views on this.
I tend to agree with Thomas that it makes things less obvious for regular users. The relation between such a CONF_PKG_ENABLE statement and the corresponding options passed to the configure script is not apparent, which confuses people.
cover Add CONF_ENABLE and CONF_PKG_ENABLE helper functions
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Thomas" == Thomas De Schampheleire <[email protected]> writes:
Hi,
Quoted 10 lines
Quoted 10 lines
Thanks for working on this and making a proposal.
However, on my side, I am not yet convinced that this is actually making things better than what we have now. What we have now is quite obvious for the first-time reader, which is very good. Those
CONF_PKG_ENABLEandCONF_ENABLEmacros make things a bit too cryptic from my point of view. This is not an absolutely strong opinion, but I have the feeling this is on the "we want to make things smaller/shorter and end up making things cryptic" side of the trade-off. Of course, others have to comment to share their views on this.
Thomas> I tend to agree with Thomas that it makes things less obvious Thomas> for regular users. The relation between such a CONF_PKG_ENABLE Thomas> statement and the corresponding options passed to the configure Thomas> script is not apparent, which confuses people.
I must say I agree.
Signature · 1 line
Bye, Peter Korsgaard
cover Add CONF_ENABLE and CONF_PKG_ENABLE helper functions
In reply to Thomas PetazzoniQuoted 16 lines · Thomas Petazzoni
Le Sat, 28 Apr 2012 23:16:18 +0200,
"Arnout Vandecappelle (Essensium/Mind)"<[email protected]> a ?crit :Quoted 13 lines
Quoted 13 lines
Many packages have the following type of construct:
ifeq ($(BR2_PACKAGE_ACL),y)
SYSTEMD_CONF_OPT += --enable-acl SYSTEMD_DEPENDENCIES += acl else SYSTEMD_CONF_OPT += --disable-aclendif
The first patch in this series abbreviates that to
$(eval $(call CONF_PKG_ENABLE,SYSTEMD,acl))
Quoted 8 lines
Quoted 1 line
Quoted 1 line
It makes it possible to abbreviate similar constructs as well.
Thanks for working on this and making a proposal.
However, on my side, I am not yet convinced that this is actually making things better than what we have now. What we have now is quite obvious for the first-time reader, which is very good. Those
CONF_PKG_ENABLEandCONF_ENABLEmacros make things a bit too cryptic from my point of view.
I agree that it looks too cryptic. Certainly with the eval and call things.
And we should indeed try to keep the package .mk files as intuitive as possible.
However, my purpose was not to save a few lines, but rather to make sure
that package contributors do things the correct way. Specifically, I want to make it easier for package contributors to explicitly specify --enable and --disable options.
I was probably just trying too hard to support all possible use cases.
What about adding something like:
SYSTEMD_AUTO_CONF_ENABLE_DEPS += acl
which would get expanded to the whole ifeq thing in the AUTOTARGETS call? This only works if the buildroot package name is the same as the --enable/--disable option, but that already catches many cases.
Regards,
Arnout
Signature · 6 lines
Arnout Vandecappelle arnout at mind be Senior Embedded Software Architect +32-16-286540 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
cover Add CONF_ENABLE and CONF_PKG_ENABLE helper functions
Hi Arnout, all,
2012/5/12 Arnout Vandecappelle <[email protected]>:
Quoted 29 lines · Thomas Petazzoni
Quoted 19 lines
Le Sat, 28 Apr 2012 23:16:18 +0200,
"Arnout Vandecappelle (Essensium/Mind)"<[email protected]> ?a ?crit :Quoted 9 lines
Quoted 9 lines
? ?Many packages have the following type of construct:
?ifeq ($(BR2_PACKAGE_ACL),y)
? ? ? ? ?SYSTEMD_CONF_OPT += --enable-acl
? ? ? ? ?SYSTEMD_DEPENDENCIES += acl
?else
? ? ? ? ?SYSTEMD_CONF_OPT += --disable-acl
?endifQuoted 5 lines
Quoted 5 lines
? ?The first patch in this series abbreviates that to
?$(eval $(call CONF_PKG_ENABLE,SYSTEMD,acl))
? ?It makes it possible to abbreviate similar constructs as well.
Quoted 7 lines
Thanks for working on this and making a proposal.
However, on my side, I am not yet convinced that this is actually making things better than what we have now. What we have now is quite obvious for the first-time reader, which is very good. Those
CONF_PKG_ENABLEandCONF_ENABLEmacros make things a bit too cryptic from my point of view.
Quoted 5 lines
?I agree that it looks too cryptic. ?Certainly with the eval and call things.
And we should indeed try to keep the package .mk files as intuitive as possible.
I agree, IMHO we should keep things as explicit as possible too.
Quoted 5 lines
?However, my purpose was not to save a few lines, but rather to make sure that package contributors do things the correct way. ?Specifically, I want to make it easier for package contributors to explicitly specify --enable and --disable options.
Quoted 9 lines
?I was probably just trying too hard to support all possible use cases. What about adding something like:
SYSTEMD_AUTO_CONF_ENABLE_DEPS+= aclwhich would get expanded to the whole ifeq thing in the AUTOTARGETS call? This only works if the buildroot package name is the same as the --enable/--disable option, but that already catches many cases.
Actually, this works well only if the --enable/--disable option has the same name as the dependency itself.
But, I prefer having to 2 lines, one for the _CONF_OPT, this other for the _DEPENDENCIES.
BTW, there is a similar issue with CMAKETARGETS.
I'd prefer having something like:SYSTEMD_CONF_OPT += $(call AUTO_ARG_ENDISABLE,acl,$(BR2_PACKAGE_ACL)) SYSTEMD_DEPENDENCIES += $(call COND_DEPS,acl,$(BR2_PACKAGE_ACL))
This kind of helpers can easily be extended to support --with/--without option for AUTOTARGETS
and all kinds of knobs existing in cmake packages.
Also, doing things like this make the COND_DEPS helper common to {AUTO,CMAKE,GEN}TARGET;
just the conditional configure helpers belongs to each specific XXXTARGET.
Regards,
Sam
cover Add CONF_ENABLE and CONF_PKG_ENABLE helper functions
Le Sat, 12 May 2012 11:03:45 +0200,
Samuel Martin <[email protected]> a ?crit :
Quoted 3 lines
I'd prefer having something like:
SYSTEMD_CONF_OPT+= $(call AUTO_ARG_ENDISABLE,acl,$(BR2_PACKAGE_ACL))SYSTEMD_DEPENDENCIES+= $(call COND_DEPS,acl,$(BR2_PACKAGE_ACL))
Is this really better than:
SYSTEMD_CONF_OPT += $(if $(BR2_PACKAGE_ACL),--enable-acl,--disable-acl)SYSTEMD_DEPENDENCIES += $(if $(BR2_PACKAGE_ACL),acl)
Thomas
Signature · 4 lines
Thomas Petazzoni, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com
cover Add CONF_ENABLE and CONF_PKG_ENABLE helper functions
In reply to Arnout VandecappelleHello Arnout,
Le Sat, 12 May 2012 00:29:15 +0200,
Arnout Vandecappelle <[email protected]> a ?crit :
Quoted 6 lines
I agree that it looks too cryptic. Certainly with the eval and call things.
And we should indeed try to keep the package .mk files as intuitive as possible.However, my purpose was not to save a few lines, but rather to make sure
that package contributors do things the correct way.
I agree it would have been nice, but there are many things that we verify during review, and this could be one of the things to verify when a new package is added, or updates to existing packages are made.
Quoted 8 lines
I was probably just trying too hard to support all possible use cases.
What about adding something like:
SYSTEMD_AUTO_CONF_ENABLE_DEPS+= aclwhich would get expanded to the whole ifeq thing in the AUTOTARGETS call? This only works if the buildroot package name is the same as the --enable/--disable option, but that already catches many cases.
I am still personally not a big fan of this. What's happening is really too magic.
Thomas
Signature · 4 lines
Thomas Petazzoni, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com
cover Add CONF_ENABLE and CONF_PKG_ENABLE helper functions
Quoted 10 lines · Thomas Petazzoni
Quoted 8 lines
Quoted 8 lines
I was probably just trying too hard to support all possible use cases.
What about adding something like:
SYSTEMD_AUTO_CONF_ENABLE_DEPS+= aclwhich would get expanded to the whole ifeq thing in the AUTOTARGETS call? This only works if the buildroot package name is the same as the --enable/--disable option, but that already catches many cases.
I am still personally not a big fan of this. What's happening is really too magic.
Okay, fair enough.
Maybe I should concentrate on the MAKETARGETS instead :-)
Regards,
Arnout
Signature · 6 lines
Arnout Vandecappelle arnout at mind be Senior Embedded Software Architect +32-16-286540 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