[Buildroot] [PATCH 10/11] toolchain/common: introduce blind options BR2_NEEDS_GETTEXT{, _IF_LOCALE}
Yann E. MORIN
yann.morin.1998 at free.fr
Tue Sep 18 21:28:38 UTC 2012
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.
--
.-----------------.--------------------.------------------.--------------------.
| Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ |
| +33 223 225 172 `------------.-------: X AGAINST | \e/ There is no |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. |
'------------------------------^-------^------------------^--------------------'
More information about the buildroot
mailing list