* [Buildroot] [PATCH 0/3] Add CONF_ENABLE and CONF_PKG_ENABLE helper functions
@ 2012-04-28 21:16 Arnout Vandecappelle
2012-04-28 21:16 ` [Buildroot] [PATCH 1/3] pkg-infra: add " Arnout Vandecappelle
` (3 more replies)
0 siblings, 4 replies; 12+ messages in thread
From: Arnout Vandecappelle @ 2012-04-28 21:16 UTC (permalink / raw)
To: buildroot
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(-)
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Buildroot] [PATCH 1/3] pkg-infra: add CONF_ENABLE and CONF_PKG_ENABLE helper functions
2012-04-28 21:16 [Buildroot] [PATCH 0/3] Add CONF_ENABLE and CONF_PKG_ENABLE helper functions Arnout Vandecappelle
@ 2012-04-28 21:16 ` Arnout Vandecappelle
2012-04-28 21:16 ` [Buildroot] [PATCH 2/3] avahi: use CONF_ENABLE and CONF_PKG_ENABLE Arnout Vandecappelle
` (2 subsequent siblings)
3 siblings, 0 replies; 12+ messages in thread
From: Arnout Vandecappelle @ 2012-04-28 21:16 UTC (permalink / raw)
To: buildroot
From: "Arnout Vandecappelle (Essensium/Mind)" <arnout@mind.be>
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.
Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
---
package/pkg-autotargets.mk | 57 ++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 57 insertions(+)
diff --git a/package/pkg-autotargets.mk b/package/pkg-autotargets.mk
index c9887c2..3743db3 100644
--- a/package/pkg-autotargets.mk
+++ b/package/pkg-autotargets.mk
@@ -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
--
1.7.10
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [Buildroot] [PATCH 2/3] avahi: use CONF_ENABLE and CONF_PKG_ENABLE
2012-04-28 21:16 [Buildroot] [PATCH 0/3] Add CONF_ENABLE and CONF_PKG_ENABLE helper functions Arnout Vandecappelle
2012-04-28 21:16 ` [Buildroot] [PATCH 1/3] pkg-infra: add " Arnout Vandecappelle
@ 2012-04-28 21:16 ` Arnout Vandecappelle
2012-04-28 21:16 ` [Buildroot] [PATCH 3/3] cairo: " Arnout Vandecappelle
2012-05-07 14:21 ` [Buildroot] [PATCH 0/3] Add CONF_ENABLE and CONF_PKG_ENABLE helper functions Thomas Petazzoni
3 siblings, 0 replies; 12+ messages in thread
From: Arnout Vandecappelle @ 2012-04-28 21:16 UTC (permalink / raw)
To: buildroot
From: "Arnout Vandecappelle (Essensium/Mind)" <arnout@mind.be>
Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
---
package/avahi/avahi.mk | 34 +++++-----------------------------
1 file changed, 5 insertions(+), 29 deletions(-)
diff --git a/package/avahi/avahi.mk b/package/avahi/avahi.mk
index 32e3df7..64a83db 100644
--- a/package/avahi/avahi.mk
+++ b/package/avahi/avahi.mk
@@ -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)
--
1.7.10
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [Buildroot] [PATCH 3/3] cairo: use CONF_ENABLE and CONF_PKG_ENABLE
2012-04-28 21:16 [Buildroot] [PATCH 0/3] Add CONF_ENABLE and CONF_PKG_ENABLE helper functions Arnout Vandecappelle
2012-04-28 21:16 ` [Buildroot] [PATCH 1/3] pkg-infra: add " Arnout Vandecappelle
2012-04-28 21:16 ` [Buildroot] [PATCH 2/3] avahi: use CONF_ENABLE and CONF_PKG_ENABLE Arnout Vandecappelle
@ 2012-04-28 21:16 ` Arnout Vandecappelle
2012-05-07 14:21 ` [Buildroot] [PATCH 0/3] Add CONF_ENABLE and CONF_PKG_ENABLE helper functions Thomas Petazzoni
3 siblings, 0 replies; 12+ messages in thread
From: Arnout Vandecappelle @ 2012-04-28 21:16 UTC (permalink / raw)
To: buildroot
From: "Arnout Vandecappelle (Essensium/Mind)" <arnout@mind.be>
Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
---
package/cairo/cairo.mk | 44 ++++++--------------------------------------
1 file changed, 6 insertions(+), 38 deletions(-)
diff --git a/package/cairo/cairo.mk b/package/cairo/cairo.mk
index 55068dd..5486372 100644
--- a/package/cairo/cairo.mk
+++ b/package/cairo/cairo.mk
@@ -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))
--
1.7.10
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [Buildroot] [PATCH 0/3] Add CONF_ENABLE and CONF_PKG_ENABLE helper functions
2012-04-28 21:16 [Buildroot] [PATCH 0/3] Add CONF_ENABLE and CONF_PKG_ENABLE helper functions Arnout Vandecappelle
` (2 preceding siblings ...)
2012-04-28 21:16 ` [Buildroot] [PATCH 3/3] cairo: " Arnout Vandecappelle
@ 2012-05-07 14:21 ` Thomas Petazzoni
2012-05-09 8:06 ` Thomas De Schampheleire
2012-05-11 22:29 ` Arnout Vandecappelle
3 siblings, 2 replies; 12+ messages in thread
From: Thomas Petazzoni @ 2012-05-07 14:21 UTC (permalink / raw)
To: buildroot
Hello Arnout,
Le Sat, 28 Apr 2012 23:16:18 +0200,
"Arnout Vandecappelle (Essensium/Mind)" <arnout@mind.be> a ?crit :
> 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.
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
--
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Buildroot] [PATCH 0/3] Add CONF_ENABLE and CONF_PKG_ENABLE helper functions
2012-05-07 14:21 ` [Buildroot] [PATCH 0/3] Add CONF_ENABLE and CONF_PKG_ENABLE helper functions Thomas Petazzoni
@ 2012-05-09 8:06 ` Thomas De Schampheleire
2012-05-09 9:35 ` Peter Korsgaard
2012-05-11 22:29 ` Arnout Vandecappelle
1 sibling, 1 reply; 12+ messages in thread
From: Thomas De Schampheleire @ 2012-05-09 8:06 UTC (permalink / raw)
To: buildroot
On Mon, May 7, 2012 at 4:21 PM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> Hello Arnout,
>
> Le Sat, 28 Apr 2012 23:16:18 +0200,
> "Arnout Vandecappelle (Essensium/Mind)" <arnout@mind.be> a ?crit :
>
>> ?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.
>
> 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.
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Buildroot] [PATCH 0/3] Add CONF_ENABLE and CONF_PKG_ENABLE helper functions
2012-05-09 8:06 ` Thomas De Schampheleire
@ 2012-05-09 9:35 ` Peter Korsgaard
0 siblings, 0 replies; 12+ messages in thread
From: Peter Korsgaard @ 2012-05-09 9:35 UTC (permalink / raw)
To: buildroot
>>>>> "Thomas" == Thomas De Schampheleire <patrickdepinguin+buildroot@gmail.com> writes:
Hi,
>> 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.
--
Bye, Peter Korsgaard
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Buildroot] [PATCH 0/3] Add CONF_ENABLE and CONF_PKG_ENABLE helper functions
2012-05-07 14:21 ` [Buildroot] [PATCH 0/3] Add CONF_ENABLE and CONF_PKG_ENABLE helper functions Thomas Petazzoni
2012-05-09 8:06 ` Thomas De Schampheleire
@ 2012-05-11 22:29 ` Arnout Vandecappelle
2012-05-12 9:03 ` Samuel Martin
2012-05-12 11:05 ` Thomas Petazzoni
1 sibling, 2 replies; 12+ messages in thread
From: Arnout Vandecappelle @ 2012-05-11 22:29 UTC (permalink / raw)
To: buildroot
On 05/07/12 16:21, Thomas Petazzoni wrote:
> Le Sat, 28 Apr 2012 23:16:18 +0200,
> "Arnout Vandecappelle (Essensium/Mind)"<arnout@mind.be> a ?crit :
>
>> > 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.
> 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
--
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Buildroot] [PATCH 0/3] Add CONF_ENABLE and CONF_PKG_ENABLE helper functions
2012-05-11 22:29 ` Arnout Vandecappelle
@ 2012-05-12 9:03 ` Samuel Martin
2012-05-12 11:42 ` Thomas Petazzoni
2012-05-12 11:05 ` Thomas Petazzoni
1 sibling, 1 reply; 12+ messages in thread
From: Samuel Martin @ 2012-05-12 9:03 UTC (permalink / raw)
To: buildroot
Hi Arnout, all,
2012/5/12 Arnout Vandecappelle <arnout@mind.be>:
> On 05/07/12 16:21, Thomas Petazzoni wrote:
>>
>> Le Sat, 28 Apr 2012 23:16:18 +0200,
>> "Arnout Vandecappelle (Essensium/Mind)"<arnout@mind.be> ?a ?crit :
>>
>>> > ? ?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.
>>
>> 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.
>
I agree, IMHO we should keep things as explicit as possible too.
> ?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.
>
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Buildroot] [PATCH 0/3] Add CONF_ENABLE and CONF_PKG_ENABLE helper functions
2012-05-11 22:29 ` Arnout Vandecappelle
2012-05-12 9:03 ` Samuel Martin
@ 2012-05-12 11:05 ` Thomas Petazzoni
2012-05-12 14:47 ` Arnout Vandecappelle
1 sibling, 1 reply; 12+ messages in thread
From: Thomas Petazzoni @ 2012-05-12 11:05 UTC (permalink / raw)
To: buildroot
Hello Arnout,
Le Sat, 12 May 2012 00:29:15 +0200,
Arnout Vandecappelle <arnout@mind.be> a ?crit :
> 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.
> 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
--
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Buildroot] [PATCH 0/3] Add CONF_ENABLE and CONF_PKG_ENABLE helper functions
2012-05-12 9:03 ` Samuel Martin
@ 2012-05-12 11:42 ` Thomas Petazzoni
0 siblings, 0 replies; 12+ messages in thread
From: Thomas Petazzoni @ 2012-05-12 11:42 UTC (permalink / raw)
To: buildroot
Le Sat, 12 May 2012 11:03:45 +0200,
Samuel Martin <s.martin49@gmail.com> a ?crit :
> 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
--
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Buildroot] [PATCH 0/3] Add CONF_ENABLE and CONF_PKG_ENABLE helper functions
2012-05-12 11:05 ` Thomas Petazzoni
@ 2012-05-12 14:47 ` Arnout Vandecappelle
0 siblings, 0 replies; 12+ messages in thread
From: Arnout Vandecappelle @ 2012-05-12 14:47 UTC (permalink / raw)
To: buildroot
On 05/12/12 13:05, Thomas Petazzoni wrote:
>> > 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
--
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
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2012-05-12 14:47 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-04-28 21:16 [Buildroot] [PATCH 0/3] Add CONF_ENABLE and CONF_PKG_ENABLE helper functions Arnout Vandecappelle
2012-04-28 21:16 ` [Buildroot] [PATCH 1/3] pkg-infra: add " Arnout Vandecappelle
2012-04-28 21:16 ` [Buildroot] [PATCH 2/3] avahi: use CONF_ENABLE and CONF_PKG_ENABLE Arnout Vandecappelle
2012-04-28 21:16 ` [Buildroot] [PATCH 3/3] cairo: " Arnout Vandecappelle
2012-05-07 14:21 ` [Buildroot] [PATCH 0/3] Add CONF_ENABLE and CONF_PKG_ENABLE helper functions Thomas Petazzoni
2012-05-09 8:06 ` Thomas De Schampheleire
2012-05-09 9:35 ` Peter Korsgaard
2012-05-11 22:29 ` Arnout Vandecappelle
2012-05-12 9:03 ` Samuel Martin
2012-05-12 11:42 ` Thomas Petazzoni
2012-05-12 11:05 ` Thomas Petazzoni
2012-05-12 14:47 ` Arnout Vandecappelle
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox