Patchwork [10/11] toolchain/common: introduce blind options BR2_NEEDS_GETTEXT{, _IF_LOCALE}

login
register
mail settings
Submitter Yann E. MORIN
Date Sept. 16, 2012, 10:57 p.m.
Message ID <1347836276-24262-11-git-send-email-yann.morin.1998@free.fr>
Download mbox | patch
Permalink /patch/184173/
State Superseded
Headers show

Comments

Yann E. MORIN - Sept. 16, 2012, 10:57 p.m.
Introduce two new blind config options:
  - BR2_NEEDS_GETTEXT
        selects the gettext package if the toolchain does not provide it
  - BR2_NEEDS_GETTEXT_IF_LOCALE
        ditto, but only if locales are enabled

Packages can then select either if they require gettext (resp. if locales
are enabled).

This will simplify the packages Config.in by no longer requiring that the
'select' be conditional, thus hiding the gory details out of packages,
which don't really need to know about those details.

Also, introduce four new Makefile variables:
  - $(gettext)
        contains the needed dependencies for pacakges that need gettext
        functioanlity: 'gettext' if the gettext pacakge is needed, empty
        otherwise
  - $(gettext-if-locale)
        ditto, but only if locales are enabled
  - $(gettext-LDFLAGS)
        contains the required LDFLAGS ("-lintl") if gettext is provided by
        the gettext package, empty otherwise
  - $(gettext-LDFLAGS-if-locale)
        ditto, but only if locales are enabled

Packages can then add either variable to their own LDFLAGS.

Note: those new options and variables are going to be used in the next
patch, for now they are a no-op.

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
CC: Samuel Martin <s.martin49@gmail.com>
---
 Makefile                                |    1 +
 docs/manual/adding-packages-gettext.txt |   55 +++++++++++++++++++++++-------
 toolchain/toolchain-common.in           |   10 +++++-
 toolchain/toolchain-common.mk           |   35 +++++++++++++++++++
 4 files changed, 87 insertions(+), 14 deletions(-)
 create mode 100644 toolchain/toolchain-common.mk
Thomas Petazzoni - Sept. 18, 2012, 5:55 p.m.
Dear Yann E. MORIN,

On Mon, 17 Sep 2012 00:57:55 +0200, Yann E. MORIN wrote:
> Introduce two new blind config options:
>   - BR2_NEEDS_GETTEXT
>         selects the gettext package if the toolchain does not provide it
>   - BR2_NEEDS_GETTEXT_IF_LOCALE
>         ditto, but only if locales are enabled
> 
> Packages can then select either if they require gettext (resp. if locales
> are enabled).
> 
> This will simplify the packages Config.in by no longer requiring that the
> 'select' be conditional, thus hiding the gory details out of packages,
> which don't really need to know about those details.

Nice.

> Also, introduce four new Makefile variables:
>   - $(gettext)
>         contains the needed dependencies for pacakges that need gettext
>         functioanlity: 'gettext' if the gettext pacakge is needed, empty
>         otherwise
>   - $(gettext-if-locale)
>         ditto, but only if locales are enabled
>   - $(gettext-LDFLAGS)
>         contains the required LDFLAGS ("-lintl") if gettext is provided by
>         the gettext package, empty otherwise
>   - $(gettext-LDFLAGS-if-locale)
>         ditto, but only if locales are enabled
> 
> Packages can then add either variable to their own LDFLAGS.

I am happy about the entire patch set, except those $(gettext),
$(gettext-if-locale), $(gettext-LDFLAGS) and
$(gettext-LDFLAGS-if-locale) variables. Even though they admittedly
make the code a bit shorter, I think they needlessly hide what is
really happening. I very much prefer an explicit:

FOO_DEPENDENCIES += $(if $(BR2_NEEDS_GETTEXT),gettext)

(which doesn't use any special construct)

rather than the more strange:

FOO_DEPENDENCIES += $(gettext)

I know I'm annoying by rejecting many new additional constructs, but I
think such constructs are crossing the boundary of shortness vs.
readability.

Of course, this is only my opinion, and if others are strongly in
favor of this approach, then I'll learn to live with it, but I think
it's a dangerous direction for the readability of .mk files. If you
respin your patch set without those constructs, you'll have my Acked-by
on the whole thing.

Best regards,

Thomas
Samuel Martin - Sept. 18, 2012, 9:16 p.m.
Hi Yann, Thomas, all,

2012/9/18 Thomas Petazzoni <thomas.petazzoni@free-electrons.com>:
> Dear Yann E. MORIN,
>
> On Mon, 17 Sep 2012 00:57:55 +0200, Yann E. MORIN wrote:
[...]
>> Also, introduce four new Makefile variables:
>>   - $(gettext)
>>         contains the needed dependencies for pacakges that need gettext
>>         functioanlity: 'gettext' if the gettext pacakge is needed, empty
>>         otherwise
>>   - $(gettext-if-locale)
>>         ditto, but only if locales are enabled
>>   - $(gettext-LDFLAGS)
>>         contains the required LDFLAGS ("-lintl") if gettext is provided by
>>         the gettext package, empty otherwise
>>   - $(gettext-LDFLAGS-if-locale)
>>         ditto, but only if locales are enabled
>>
>> Packages can then add either variable to their own LDFLAGS.
>
> I am happy about the entire patch set, except those $(gettext),
> $(gettext-if-locale), $(gettext-LDFLAGS) and
> $(gettext-LDFLAGS-if-locale) variables. Even though they admittedly
> make the code a bit shorter, I think they needlessly hide what is
> really happening. I very much prefer an explicit:

I disagree with Thomas.
True, this hides a bit the details, but I do not think it that's a bad thing.
I mean gettext/locale support is somehow quite closely related to the
toolchain, so, having these variables warns a bit more about what one
is about to do.

Though, these variables hide some things, it's pretty easy to find out
their definitions; and IMHO, there are other things more hidden than
this in BR. ;-)
Besides, I think,  the variable names are clear, and won't easily lead
to mistakes or misunderstandings.


Yours,
Yann E. MORIN - Sept. 18, 2012, 9:28 p.m.
Thomas, All,

On Tuesday 18 September 2012 19:55:18 Thomas Petazzoni wrote:
> On Mon, 17 Sep 2012 00:57:55 +0200, Yann E. MORIN wrote:
> > Also, introduce four new Makefile variables:
> >   - $(gettext)
> >         contains the needed dependencies for pacakges that need gettext
> >         functioanlity: 'gettext' if the gettext pacakge is needed, empty
> >         otherwise
> >   - $(gettext-if-locale)
> >         ditto, but only if locales are enabled
> >   - $(gettext-LDFLAGS)
> >         contains the required LDFLAGS ("-lintl") if gettext is provided by
> >         the gettext package, empty otherwise
> >   - $(gettext-LDFLAGS-if-locale)
> >         ditto, but only if locales are enabled
> > 
> > Packages can then add either variable to their own LDFLAGS.
> 
> I am happy about the entire patch set, except those $(gettext),
> $(gettext-if-locale), $(gettext-LDFLAGS) and
> $(gettext-LDFLAGS-if-locale) variables. Even though they admittedly
> make the code a bit shorter, I think they needlessly hide what is
> really happening. I very much prefer an explicit:
> 
> FOO_DEPENDENCIES += $(if $(BR2_NEEDS_GETTEXT),gettext)
> 
> (which doesn't use any special construct)
> 
> rather than the more strange:
> 
> FOO_DEPENDENCIES += $(gettext)

My reasoning behind this was to keep the similar constructs in the
Config.in and the package.mk.

In Config.in, a package declares its dependency on gettext, and buildroot
does what is needed "behind the hood":
  select BR2_NEEDS_GETTEXT

Then buildroot will or will not effectively select the gettext package,
depending on the toolchain features.

(To be noted, in this case we are already /hidding/ stuff behind the
 scenes, which could well be considered "crossing the line", too.)

I wanted to introduce a similar behavior in the .mk, hence the $(gettext*)
variables, without the package to even have to deal with the depedencies by
itself. A package would just have to declare a build-time dependency on the
gettext _feature_, and buildroot would decide to build or not to build the
gettext _package_ depending on the toolchain features.

> I know I'm annoying by rejecting many new additional constructs, but I
> think such constructs are crossing the boundary of shortness vs.
> readability.

(No, you are not annoying; I do expect such discussions on such impacting
changes. Yes, it's a bit frustrating, but this is absolutely needed. :-) )

I agree that this is somewhat crossing the line. But I also believe this
change would make it easier to maintain packages, with a single, right way
of expressing the dependency, and a single place we'd have to change if
we'd have to, rather than scouting the tree for packages that needs gettext
feature (although a trivial grep would probably do the trick, granted).

Also, leaving packages do the conditional test opens the door to packages
doing it in their own way, eg:

  if BR2_NEEDS_GETTEXT
  FOO_DEPS += gettext
  endif

vs.

  FOO_DEPS += $(if $(BR2_NEEDS_GETTEXT),gettext)

This is source for confusion, and thus should be avoided.

I know that semantically, this would not change much, but what if we were
to rename those variables, so they are more in-line with the Config.in ones
such as:
  $(needs-gettext)
  $(needs-gettext-if-locale)
  $(needs-gettext_LDFLAGS)
  $(needs-gettext-if-locale_LDFLAGS)

( Yes, I'm pushing for this change as much as I can! ;-)  )

> Of course, this is only my opinion, and if others are strongly in
> favor of this approach, then I'll learn to live with it, but I think
> it's a dangerous direction for the readability of .mk files. If you
> respin your patch set without those constructs, you'll have my Acked-by
> on the whole thing.

OK, so here's what I suggest:
  - fix the 4 gettext mis-constructs in packages, as you pointed out in
    another mail,
  - split the gettext abstraction in two parts: one for the Config.in stuff,
    and a second for the .mk stuff.

This way, at least the part of the series that we all agree on can be merged,
and the litiguous parts can be refined/dropped.

Does that sound OK?

Thanks for the comments! :-)

Regards,
Yann E. MORIN.
Thomas Petazzoni - Sept. 20, 2012, 6:53 p.m.
Dear Yann E. MORIN,

On Tue, 18 Sep 2012 23:28:38 +0200, Yann E. MORIN wrote:

> I agree that this is somewhat crossing the line. But I also believe this
> change would make it easier to maintain packages, with a single, right way
> of expressing the dependency, and a single place we'd have to change if
> we'd have to, rather than scouting the tree for packages that needs gettext
> feature (although a trivial grep would probably do the trick, granted).
> 
> Also, leaving packages do the conditional test opens the door to packages
> doing it in their own way, eg:
> 
>   if BR2_NEEDS_GETTEXT
>   FOO_DEPS += gettext
>   endif
> 
> vs.
> 
>   FOO_DEPS += $(if $(BR2_NEEDS_GETTEXT),gettext)

Those two ways are identical, so I don't mind what packages do between
both cases. Depending on how the package .mk file is written, one way
or the other might make more sense.

> OK, so here's what I suggest:
>   - fix the 4 gettext mis-constructs in packages, as you pointed out in
>     another mail,
>   - split the gettext abstraction in two parts: one for the Config.in stuff,
>     and a second for the .mk stuff.
> 
> This way, at least the part of the series that we all agree on can be merged,
> and the litiguous parts can be refined/dropped.
> 
> Does that sound OK?

I am all in favor of merging your entire patch series, but I remain
unconvinced on just the .mk macros themselves. That said, it seems
other people are fine with it, so if it goes in, I will not be overly
shocked. I don't think it's a good direction, but I can live with it.

Best regards,

Thomas
Arnout Vandecappelle - Sept. 20, 2012, 10:24 p.m.
On 09/17/12 00:57, Yann E. MORIN wrote:
> Introduce two new blind config options:
>    - BR2_NEEDS_GETTEXT
>          selects the gettext package if the toolchain does not provide it
>    - BR2_NEEDS_GETTEXT_IF_LOCALE
>          ditto, but only if locales are enabled

  There are 18 packages that use NEEDS_GETTEXT_IF_LOCALE, and 11 packages that
use NEEDS_GETTEXT.  However, for most of the latter, it looks like it should
be an _IF_LOCALE dependency.  Therefore, I think it's better to remove the
BR2_NEEDS_GETTEXT support and leave it explicit for these packages.  We can
fix those 11 packages (now or later).  Here's an initial analysis:

diffutils: should be _IF_LOCALE
flex: should be _IF_LOCALE
gdk-pixbuf: indirect dependency through libglib2
glib-networking: indirect dependency through libglib2
libglib2: _really_ depends on libintl
libsoup: indirect dependency through libglib2
lshw: add -DNONLS to CFLAGS
pulseaudio: indirect dependency through libglib2
ndisc6: should be _IF_LOCALE
php: gettext module obviously requires gettext
util-linux: should be _IF_LOCALE


  So, only two packages left that need gettext even without LOCALE: libglib2 and
php.  For php: gettext doesn't make much sense without LOCALE, so we can depend
on LOCALE there.  For libglib2: if you build that kind of bloat, you can live
with the overhead of LOCALE, so I'd also depend on LOCALE there...


  My proposal, therefore:

- only introduce BR2_NEEDS_GETTEXT_IF_LOCALE> ---

- Remove the NEEDS_GETTEXT part from the documentation

- we'll fix up these packages separately

- the patches for these packages are independent from patch 10/11 and 11/11
(if the NEEDS_GETTEXT symbol is not introduced), so they can be produced
in parallel.  Note that patch 9/11 is still in the way, but that's a
mechanical change that should anyway be re-done prior to commit (in case
a new package pops up using that symbol).


  Regards,
  Arnout
Arnout Vandecappelle - Sept. 20, 2012, 10:29 p.m.
On 09/18/12 23:28, Yann E. MORIN wrote:
> I know that semantically, this would not change much, but what if we were
> to rename those variables, so they are more in-line with the Config.in ones
> such as:
>    $(needs-gettext)
>    $(needs-gettext-if-locale)
>    $(needs-gettext_LDFLAGS)
>    $(needs-gettext-if-locale_LDFLAGS)
>
> ( Yes, I'm pushing for this change as much as I can!;-)   )

  For what it's worth, I too am in favour of the $(needs-gettext*) constructs.  As
mentioned in my previous mail, $(needs-gettext) itself should be left out.  But
I would name them:
$(gettext-deps-if-locale)
$(gettext-libs-if-locale)
Although the naming will never be fully self-explanatory.

  Regards,
  Arnout
Thomas Petazzoni - Sept. 21, 2012, 5:13 a.m.
Dear Arnout Vandecappelle,

On Fri, 21 Sep 2012 00:24:06 +0200, Arnout Vandecappelle wrote:

> diffutils: should be _IF_LOCALE
> flex: should be _IF_LOCALE
> gdk-pixbuf: indirect dependency through libglib2
> glib-networking: indirect dependency through libglib2
> libglib2: _really_ depends on libintl
> libsoup: indirect dependency through libglib2
> lshw: add -DNONLS to CFLAGS
> pulseaudio: indirect dependency through libglib2
> ndisc6: should be _IF_LOCALE
> php: gettext module obviously requires gettext
> util-linux: should be _IF_LOCALE
> 
> 
>   So, only two packages left that need gettext even without LOCALE: libglib2 and
> php.  For php: gettext doesn't make much sense without LOCALE, so we can depend
> on LOCALE there.  For libglib2: if you build that kind of bloat, you can live
> with the overhead of LOCALE, so I'd also depend on LOCALE there...

Ok. Just to make it clear: php itself does not depend on gettext, it's
only the gettext module that does. So even if we adopt your
solution, it remains to built the PHP interpreter with a toolchain that
doesn't have locale support, only building the gettext module will not
be possible, which indeed makes sense.

>   My proposal, therefore:
> 
> - only introduce BR2_NEEDS_GETTEXT_IF_LOCALE> ---
> 
> - Remove the NEEDS_GETTEXT part from the documentation
> 
> - we'll fix up these packages separately
> 
> - the patches for these packages are independent from patch 10/11 and 11/11
> (if the NEEDS_GETTEXT symbol is not introduced), so they can be produced
> in parallel.  Note that patch 9/11 is still in the way, but that's a
> mechanical change that should anyway be re-done prior to commit (in case
> a new package pops up using that symbol).

A full Acked-by from me on this approach. Thanks a lot for the in-depth
analysis of the problem that leads to this simplification, really great.

Thomas

Patch

diff --git a/Makefile b/Makefile
index ba5da99..16cfdf1 100644
--- a/Makefile
+++ b/Makefile
@@ -294,6 +294,7 @@  include support/dependencies/dependencies.mk
 # We also need the various per-package makefiles, which also add
 # each selected package to TARGETS if that package was selected
 # in the .config file.
+include toolchain/toolchain-common.mk
 ifeq ($(BR2_TOOLCHAIN_BUILDROOT),y)
 include toolchain/toolchain-buildroot.mk
 else ifeq ($(BR2_TOOLCHAIN_EXTERNAL),y)
diff --git a/docs/manual/adding-packages-gettext.txt b/docs/manual/adding-packages-gettext.txt
index 71da62a..ad5b7af 100644
--- a/docs/manual/adding-packages-gettext.txt
+++ b/docs/manual/adding-packages-gettext.txt
@@ -18,23 +18,52 @@  is enabled.
 
 Therefore, Buildroot defines two configuration options:
 
-* +BR2_NEEDS_EXTERNAL_GETTEXT+, which is true as soon as the toolchain doesn't
-  provide its own gettext implementation
+* +BR2_NEEDS_GETTEXT+, which a package **must** +select+ in its +Config.in+,
+  to indicate it requires gettext functionality
 
-* +BR2_NEEDS_EXTERNAL_GETTEXT_IF_LOCALE+, which is true if the toolchain
-  doesn't provide its own gettext implementation and if locale support
-  is enabled
+* +BR2_NEEDS_GETTEXT_IF_LOCALE+, which a package **must** +select+ in its
+  +Config.in+, to indicate it requires gettext functionality if locales
+  are enabled
 
-Therefore, packages that unconditionally need gettext should:
+Buildroot also defines four +Makefile+ variables:
 
-* Use +select BR2_PACKAGE_GETTEXT if BR2_NEEDS_EXTERNAL_GETTEXT+
+* +$(gettext)+, that a package **must** add to its dependency list if it
+  requires gettext functionality
 
-* Use +$(if $(BR2_NEEDS_EXTERNAL_GETTEXT),gettext)+ in the package
-  +DEPENDENCIES+ variable
+* +$(gettext-if-locale)+, that a package **must** add to its dependency
+  list if it requires gettext functionality if locales are enabled
 
-Packages that need gettext only when locale support is enabled should:
+* +$(gettext-LDFLAGS)+, that a package ___can___ add to its +LDFLAGS+ if it
+  requires gettext functionality
 
-* Use +select BR2_PACKAGE_GETTEXT if BR2_NEEDS_EXTERNAL_GETTEXT_IF_LOCALE+
+* +$(gettext-LDFLAGS-if-locale)+, that a package ___can___ add to its
+  +LDFLAGS+ if it requires gettext functionality if locales are enabled
 
-* Use +$(if $(BR2_NEEDS_EXTERNAL_GETTEXT_IF_LOCALE),gettext)+ in the package
-  +DEPENDENCIES+ variable
+Example +Config.in+ excerpts for two packages:
+
+----
+config BR2_PACKAGE_FOO
+	bool "foo"
+	select BR2_NEEDS_GETTEXT
+----
+----
+config BR2_PACKAGE_BAR
+	bool "bar"
+	select BR2_NEEDS_GETTEXT_IF_LOCALE
+----
+
+And the corresponding excerpts from their +.mk+ files:
+
+----
+FOO_DEPENDENCIES += $(gettext)
+FOO_LDFLAGS += $(gettext-LDFLAGS)
+----
+----
+BAR_DEPENDENCIES += $(gettext-if-locale)
+BAR_CONF_ENV += LIBS="$(gettext-LDFLAGS-if-locale)"
+----
+
+___**Note:**___ The two Makefile variable +$(gettext-LDFLAGS)+ and
++$(gettext-LDFLAGS-if-locale)+ should be used **only** if the package's
+build-system does not automatically detects that linking with +-lint+ is
+needed.
diff --git a/toolchain/toolchain-common.in b/toolchain/toolchain-common.in
index 091aaa1..4c55578 100644
--- a/toolchain/toolchain-common.in
+++ b/toolchain/toolchain-common.in
@@ -80,7 +80,7 @@  config BR2_GENERATE_LOCALE
 # gettext isn't needed and shouldn't be built to avoid conflicts. Some
 # packages always need gettext, other packages only need gettext when
 # locale support is enabled. See the documentation for how packages
-# should rely on the following two options.
+# should rely on the following four options.
 
 config BR2_NEEDS_EXTERNAL_GETTEXT
 	bool
@@ -92,6 +92,14 @@  config BR2_NEEDS_EXTERNAL_GETTEXT_IF_LOCALE
 	bool
 	default y if (BR2_NEEDS_EXTERNAL_GETTEXT && BR2_ENABLE_LOCALE)
 
+config BR2_NEEDS_GETTEXT
+	bool
+	select BR2_PACKAGE_GETTEXT if BR2_NEEDS_EXTERNAL_GETTEXT
+
+config BR2_NEEDS_GETTEXT_IF_LOCALE
+	bool
+	select BR2_PACKAGE_GETTEXT if BR2_NEEDS_EXTERNAL_GETTEXT_IF_LOCALE
+
 config BR2_USE_MMU
 	bool "Enable MMU support" if BR2_arm || BR2_armeb || BR2_mips || BR2_mipsel || BR2_sh || BR2_xtensa
 	default y if !BR2_bfin
diff --git a/toolchain/toolchain-common.mk b/toolchain/toolchain-common.mk
new file mode 100644
index 0000000..6333808
--- /dev/null
+++ b/toolchain/toolchain-common.mk
@@ -0,0 +1,35 @@ 
+# Common set of variables
+
+# $(gettext)
+#  - contains the needed dependencies for packages that need gettext
+#    functioanlity: 'gettext' if the gettext package is needed
+#  - empty otherwise
+#
+# $(gettext-if-locale)
+#  - contains the needed dependencies for packages that need gettext
+#    functioanlity: 'gettext' if the gettext package is needed, and
+#    locales are enabled
+#  - empty otherwise
+ifeq ($(BR2_NEEDS_EXTERNAL_GETTEXT),y)
+gettext = $(if $(BR2_PACKAGE_GETTEXT),gettext)
+endif
+ifeq ($(BR2_NEEDS_EXTERNAL_GETTEXT_IF_LOCALE),y)
+gettext-if-locale = $(if $(BR2_PACKAGE_GETTEXT),gettext)
+endif
+
+# $(gettext-LDFLAGS)
+#  - set to "-lintl" if the toolchain does not provide gettext, and
+#    package gettext is selected
+#  - empty otherwise
+#
+# $(gettext-LDFLAGS-if-locale)
+#  - set to "-lintl" if the toolchain does not provide gettext, and
+#    locales are enabled, and package gettext is selected
+#  - empty otherwise
+#
+ifeq ($(BR2_NEEDS_EXTERNAL_GETTEXT),y)
+gettext-LDFLAGS = $(if $(BR2_PACKAGE_GETTEXT),-lintl)
+endif
+ifeq ($(BR2_NEEDS_EXTERNAL_GETTEXT_IF_LOCALE),y)
+gettext-LDFLAGS-if-locale = $(if $(BR2_PACKAGE_GETTEXT),-lintl)
+endif