| Message ID | 1347836276-24262-11-git-send-email-yann.morin.1998@free.fr |
|---|---|
| State | Superseded |
| Headers | show |
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
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,
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.
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
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
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
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
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
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