Skip to contentSkip to conversation

Add CONF_ENABLE and CONF_PKG_ENABLE helper functions

PATCH New Arnout Vandecappelle (Essensium/Mind) 5 participants 12 messages Apr 28, 2012 – May 12, 2012

lore
Share & tools
lore
#0 lore raw

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

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.

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(-)
#1 lore raw

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
index c9887c2..3743db3 100644
@@ -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
#2 lore raw

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
index 32e3df7..64a83db 100644
@@ -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
#3 lore raw

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
index 55068dd..5486372 100644
@@ -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
#4 lore raw

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
#5 lore raw

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
endif

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.

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

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.

#6 lore raw

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

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
#7 lore raw

cover Add CONF_ENABLE and CONF_PKG_ENABLE helper functions

In reply to Thomas Petazzoni
Quoted 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-acl

endif

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_ENABLE and CONF_ENABLE macros 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
#8 lore raw

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
?endif

Quoted 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_ENABLE and CONF_ENABLE macros 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 += 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.

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

#9 lore raw

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
#10 lore raw

cover Add CONF_ENABLE and CONF_PKG_ENABLE helper functions

In reply to Arnout Vandecappelle

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

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
#11 lore raw

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

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