Patchwork [2/3] gettext: Rename BR2_PACKAGE_LIBINTL to BR2_PACKAGE_GETTEXT_NEEDS_BINARIES

login
register
mail settings
Submitter Samuel Martin
Date May 8, 2012, 11:19 a.m.
Message ID <1336475958-5714-2-git-send-email-s.martin49@gmail.com>
Download mbox | patch
Permalink /patch/157669/
State Superseded
Headers show

Comments

Samuel Martin - May 8, 2012, 11:19 a.m.
libintl target does nothing but removing some binaries installed by
gettext.

This patch renames the BR2_PACKAGE_LIBINTL symbol to
BR2_PACKAGE_GETTEXT_NEEDS_BINARIES and fixes the logic for packages
that may need those gettext binaries.

It also adds a warning about libintl support depending on locale enabling
in the toolchain.

Signed-off-by: Samuel Martin <s.martin49@gmail.com>
---
 package/gettext/Config.in  |   16 ++++++++--------
 package/gettext/gettext.mk |    2 +-
 package/libidn/Config.in   |    1 +
 package/php/Config.ext     |    1 +
 4 files changed, 11 insertions(+), 9 deletions(-)
Arnout Vandecappelle - May 11, 2012, 9:37 p.m.
On 05/08/12 13:19, Samuel Martin wrote:
> libintl target does nothing but removing some binaries installed by
> gettext.
>
> This patch renames the BR2_PACKAGE_LIBINTL symbol to
> BR2_PACKAGE_GETTEXT_NEEDS_BINARIES and fixes the logic for packages
> that may need those gettext binaries.

  I would call it BR2_PACKAGE_GETTEXT_BINARIES instead, or perhaps
BR_PACKAGE_GETTEXT_INSTALL_BINARIES.

  Also, the logic is inverted: _LIBINTL would remove the binaries, but
now you don't remove the binaries if _GETTEXT_BINARIES is defined.  So
this should be mentioned in the commit log.

> It also adds a warning about libintl support depending on locale enabling
> in the toolchain.
>
> Signed-off-by: Samuel Martin<s.martin49@gmail.com>
> ---
>   package/gettext/Config.in  |   16 ++++++++--------
>   package/gettext/gettext.mk |    2 +-
>   package/libidn/Config.in   |    1 +
>   package/php/Config.ext     |    1 +
>   4 files changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/package/gettext/Config.in b/package/gettext/Config.in
> index 0ee4065..461652f 100644
> --- a/package/gettext/Config.in
> +++ b/package/gettext/Config.in
> @@ -12,12 +12,12 @@ config BR2_PACKAGE_GETTEXT
>   comment "gettext requires a toolchain with WCHAR support"
>   	depends on BR2_NEEDS_GETTEXT&&  !BR2_USE_WCHAR
>
> -config BR2_PACKAGE_LIBINTL
> -	bool "libintl"
> -	depends on BR2_NEEDS_GETTEXT
> -	depends on BR2_USE_WCHAR
> -	help
> -	  Selecting this package installs all of gettext in the staging
> -	  directory and the shared library for it's use in the target.
> +comment "libintl requires a toolchain with locale/i18n support"
> +	depends on BR2_PACKAGE_GETTEXT&&  !BR2_ENABLE_LOCALE

  You just removed libintl, so you shouldn't add a comment about it.
I'm also not sure where this comes from.  If gettext/libintl requires
ENABLE_LOCALE, why did it work before?  Or didn't it work before, and
should there in fact be a 'depends on BR2_ENABLE_LOCALE' in the gettext
package (and everything that selects it)?

>
> -	  http://www.gnu.org/software/gettext/
> +config BR2_PACKAGE_GETTEXT_NEEDS_BINARIES
> +	bool
> +	depends on BR2_PACKAGE_GETTEXT
> +	help
> +	  Force to install gettext binaries (gettext, gettext.sh and gettextize)
> +	  in the target.

  I think the "Force to install" is a bit strong here.  Just say
"Install gettext binaries..."

> diff --git a/package/gettext/gettext.mk b/package/gettext/gettext.mk
> index 7c7b26c..0902edd 100644
> --- a/package/gettext/gettext.mk
> +++ b/package/gettext/gettext.mk
> @@ -17,7 +17,7 @@ define GETTEXT_REMOVE_BINARIES
>   	rm -f $(TARGET_DIR)/usr/bin/gettextize
>   endef
>
> -ifeq ($(BR2_PACKAGE_LIBINTL),y)
> +ifneq ($(BR2_PACKAGE_GETTEXT_NEEDS_BINARIES),y)
>   	GETTEXT_POST_INSTALL_TARGET_HOOKS += GETTEXT_REMOVE_BINARIES
>   endif
>
> diff --git a/package/libidn/Config.in b/package/libidn/Config.in
> index dcf9724..3e98245 100644
> --- a/package/libidn/Config.in
> +++ b/package/libidn/Config.in
> @@ -1,6 +1,7 @@
>   config BR2_PACKAGE_LIBIDN
>   	bool "libidn"
>   	select BR2_PACKAGE_GETTEXT if BR2_NEEDS_GETTEXT_IF_LOCALE
> +	select BR2_PACKAGE_GETTEXT_NEEDS_BINARIES if BR2_NEEDS_GETTEXT_IF_LOCALE

  Why does libidn need the gettext binaries?  If this is just a mechanical
update of a dependency that was in fact incorrect, why is it just for
libidn and not for the 20-ish other packages that select BR2_PACKAGE_GETTEXT?

  In fact for this one I had a quick check in the source code, and I can't
find a reference to the gettext binaries.  So I doubt it is needed.


>   	help
>   	  Libidn's purpose is to encode and decode internationalized
>   	  domain names.
> diff --git a/package/php/Config.ext b/package/php/Config.ext
> index bd630ee..2ea686c 100644
> --- a/package/php/Config.ext
> +++ b/package/php/Config.ext
> @@ -68,6 +68,7 @@ config BR2_PACKAGE_PHP_EXT_FTP
>   config BR2_PACKAGE_PHP_EXT_GETTEXT
>   	bool "gettext"
>   	select BR2_PACKAGE_GETTEXT if BR2_NEEDS_GETTEXT
> +	select BR2_PACKAGE_GETTEXT_NEEDS_BINARIES if BR2_NEEDS_GETTEXT

  Same here.  I'm not altogether sure that the gettext PHP extension
doesn't call the binary, but at first sight it looks like it's just
using the library function.

  Regards,
  Arnout

>   	depends on BR2_USE_WCHAR
>   	help
>   	  gettext support
Samuel Martin - May 11, 2012, 10:45 p.m.
Hi Arnout,

Thx for the review.

2012/5/11 Arnout Vandecappelle <arnout@mind.be>:
> On 05/08/12 13:19, Samuel Martin wrote:
>>
>> libintl target does nothing but removing some binaries installed by
>> gettext.
>>
>> This patch renames the BR2_PACKAGE_LIBINTL symbol to
>> BR2_PACKAGE_GETTEXT_NEEDS_BINARIES and fixes the logic for packages
>> that may need those gettext binaries.
>
>
>  I would call it BR2_PACKAGE_GETTEXT_BINARIES instead, or perhaps
> BR_PACKAGE_GETTEXT_INSTALL_BINARIES.
ok, why not.

>
>  Also, the logic is inverted: _LIBINTL would remove the binaries, but
> now you don't remove the binaries if _GETTEXT_BINARIES is defined.  So
> this should be mentioned in the commit log.
Next time, I will pay more attention to my commit messages.

>
>> It also adds a warning about libintl support depending on locale enabling
>> in the toolchain.
>>
>> Signed-off-by: Samuel Martin<s.martin49@gmail.com>
>> ---
>>  package/gettext/Config.in  |   16 ++++++++--------
>>  package/gettext/gettext.mk |    2 +-
>>  package/libidn/Config.in   |    1 +
>>  package/php/Config.ext     |    1 +
>>  4 files changed, 11 insertions(+), 9 deletions(-)
>>
>> diff --git a/package/gettext/Config.in b/package/gettext/Config.in
>> index 0ee4065..461652f 100644
>> --- a/package/gettext/Config.in
>> +++ b/package/gettext/Config.in
>> @@ -12,12 +12,12 @@ config BR2_PACKAGE_GETTEXT
>>  comment "gettext requires a toolchain with WCHAR support"
>>        depends on BR2_NEEDS_GETTEXT&&  !BR2_USE_WCHAR
>>
>>
>> -config BR2_PACKAGE_LIBINTL
>> -       bool "libintl"
>> -       depends on BR2_NEEDS_GETTEXT
>> -       depends on BR2_USE_WCHAR
>> -       help
>> -         Selecting this package installs all of gettext in the staging
>> -         directory and the shared library for it's use in the target.
>> +comment "libintl requires a toolchain with locale/i18n support"
>> +       depends on BR2_PACKAGE_GETTEXT&&  !BR2_ENABLE_LOCALE
>
>
>  You just removed libintl, so you shouldn't add a comment about it.

Because now libintl comes with gettext, there is no explicit target
for libintl anymore.
I add the comment about libintl because the build of packages
depending on libintl failed
if I enabled gettext, but disable locale/i18n support.

> I'm also not sure where this comes from.  If gettext/libintl requires
> ENABLE_LOCALE, why did it work before?  Or didn't it work before, and
> should there in fact be a 'depends on BR2_ENABLE_LOCALE' in the gettext
> package (and everything that selects it)?

When I tested these patches, with similar configs, the unique diff was
BR2_ENABLE_LOCALE enabled or not,
I got both libgettextlib and libintl installed in the staging dir.
when I enabled BR2_ENABLE_LOCALE,
whereas there was only libgettextlib installed if BR2_ENABLE_LOCALE
was disabled.
I cannot explain me either, hope there is some gettext guru around here...

>
>
>>
>> -         http://www.gnu.org/software/gettext/
>> +config BR2_PACKAGE_GETTEXT_NEEDS_BINARIES
>> +       bool
>> +       depends on BR2_PACKAGE_GETTEXT
>> +       help
>> +         Force to install gettext binaries (gettext, gettext.sh and
>> gettextize)
>> +         in the target.
>
>
>  I think the "Force to install" is a bit strong here.  Just say
> "Install gettext binaries..."
maybe...ok.

>
>
>> diff --git a/package/gettext/gettext.mk b/package/gettext/gettext.mk
>> index 7c7b26c..0902edd 100644
>> --- a/package/gettext/gettext.mk
>> +++ b/package/gettext/gettext.mk
>> @@ -17,7 +17,7 @@ define GETTEXT_REMOVE_BINARIES
>>        rm -f $(TARGET_DIR)/usr/bin/gettextize
>>  endef
>>
>> -ifeq ($(BR2_PACKAGE_LIBINTL),y)
>> +ifneq ($(BR2_PACKAGE_GETTEXT_NEEDS_BINARIES),y)
>>        GETTEXT_POST_INSTALL_TARGET_HOOKS += GETTEXT_REMOVE_BINARIES
>>  endif
>>
>> diff --git a/package/libidn/Config.in b/package/libidn/Config.in
>> index dcf9724..3e98245 100644
>> --- a/package/libidn/Config.in
>> +++ b/package/libidn/Config.in
>> @@ -1,6 +1,7 @@
>>  config BR2_PACKAGE_LIBIDN
>>        bool "libidn"
>>        select BR2_PACKAGE_GETTEXT if BR2_NEEDS_GETTEXT_IF_LOCALE
>> +       select BR2_PACKAGE_GETTEXT_NEEDS_BINARIES if
>> BR2_NEEDS_GETTEXT_IF_LOCALE
>
>
>  Why does libidn need the gettext binaries?  If this is just a mechanical
> update of a dependency that was in fact incorrect, why is it just for
> libidn and not for the 20-ish other packages that select
> BR2_PACKAGE_GETTEXT?
>
>  In fact for this one I had a quick check in the source code, and I can't
> find a reference to the gettext binaries.  So I doubt it is needed.
>
Actually, I followed some basic logic:
Before this patch series:
"a package depending on: gettext && libintl,  did not need gettext binaries";
this can also be rephrase:
"a package depending on: gettext && !libintl,  may need gettext binaries".

After apply this patch, because I invert (and rename) the libintl
logic, these sentences become:
"a package depending on: gettext && !need_gettext_binaries,  did not
need gettext binaries";
or:
"a package depending on: gettext && need_gettext_binaries,  (may) need
gettext binaries".

I was just enforcing the eventuality that a package may need gettext binaries.

Hope this makes things clearer.

>
>
>>        help
>>          Libidn's purpose is to encode and decode internationalized
>>          domain names.
>> diff --git a/package/php/Config.ext b/package/php/Config.ext
>> index bd630ee..2ea686c 100644
>> --- a/package/php/Config.ext
>> +++ b/package/php/Config.ext
>> @@ -68,6 +68,7 @@ config BR2_PACKAGE_PHP_EXT_FTP
>>  config BR2_PACKAGE_PHP_EXT_GETTEXT
>>        bool "gettext"
>>        select BR2_PACKAGE_GETTEXT if BR2_NEEDS_GETTEXT
>> +       select BR2_PACKAGE_GETTEXT_NEEDS_BINARIES if BR2_NEEDS_GETTEXT
>
>
>  Same here.  I'm not altogether sure that the gettext PHP extension
> doesn't call the binary, but at first sight it looks like it's just
> using the library function.

If these packages don't need gettext binaries, I'm fine with removing
these select lines.

>
>  Regards,
>  Arnout
>
>
>>        depends on BR2_USE_WCHAR
>>        help
>>          gettext support
>
>
> --
> 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


Cheers,

Sam
Arnout Vandecappelle - May 12, 2012, 2:43 p.m.
On 05/12/12 00:45, Samuel Martin wrote:
> Hi Arnout,
>
> Thx for the review.
>
> 2012/5/11 Arnout Vandecappelle<arnout@mind.be>:
>> On 05/08/12 13:19, Samuel Martin wrote:
[snip]
>>> +comment "libintl requires a toolchain with locale/i18n support"
>>> +       depends on BR2_PACKAGE_GETTEXT&&    !BR2_ENABLE_LOCALE
>>
>>
>>   You just removed libintl, so you shouldn't add a comment about it.
>
> Because now libintl comes with gettext, there is no explicit target
> for libintl anymore.
> I add the comment about libintl because the build of packages
> depending on libintl failed
> if I enabled gettext, but disable locale/i18n support.
>
>> I'm also not sure where this comes from.  If gettext/libintl requires
>> ENABLE_LOCALE, why did it work before?  Or didn't it work before, and
>> should there in fact be a 'depends on BR2_ENABLE_LOCALE' in the gettext
>> package (and everything that selects it)?
>
> When I tested these patches, with similar configs, the unique diff was
> BR2_ENABLE_LOCALE enabled or not,
> I got both libgettextlib and libintl installed in the staging dir.
> when I enabled BR2_ENABLE_LOCALE,
> whereas there was only libgettextlib installed if BR2_ENABLE_LOCALE
> was disabled.
> I cannot explain me either, hope there is some gettext guru around here...

  I just tried with the following defconfig and without these patches,
and I do get both libgettextlib and libintl on the target.

BR2_powerpc=y
BR2_TOOLCHAIN_BUILDROOT_WCHAR=y
BR2_ROOTFS_DEVICE_CREATION_DYNAMIC_DEVTMPFS=y
BR2_PACKAGE_GETTEXT=y
BR2_PACKAGE_LIBINTL=y
(BR2_ENABLE_LOCALE is not set)

[snip]
>>   Why does libidn need the gettext binaries?  If this is just a mechanical
>> update of a dependency that was in fact incorrect, why is it just for
>> libidn and not for the 20-ish other packages that select
>> BR2_PACKAGE_GETTEXT?
>>
>>   In fact for this one I had a quick check in the source code, and I can't
>> find a reference to the gettext binaries.  So I doubt it is needed.
>>
> Actually, I followed some basic logic:
> Before this patch series:
> "a package depending on: gettext&&  libintl,  did not need gettext binaries";
> this can also be rephrase:
> "a package depending on: gettext&&  !libintl,  may need gettext binaries".
>
> After apply this patch, because I invert (and rename) the libintl
> logic, these sentences become:
> "a package depending on: gettext&&  !need_gettext_binaries,  did not
> need gettext binaries";
> or:
> "a package depending on: gettext&&  need_gettext_binaries,  (may) need
> gettext binaries".
>
> I was just enforcing the eventuality that a package may need gettext binaries.
>
> Hope this makes things clearer.

  Indeed.  Sorry I wasn't paying attention :-)

  Although most likely these two packages don't need the gettext binaries on
the target, it's safer to keep the old behaviour.

  Regards,
  Arnout

Patch

diff --git a/package/gettext/Config.in b/package/gettext/Config.in
index 0ee4065..461652f 100644
--- a/package/gettext/Config.in
+++ b/package/gettext/Config.in
@@ -12,12 +12,12 @@  config BR2_PACKAGE_GETTEXT
 comment "gettext requires a toolchain with WCHAR support"
 	depends on BR2_NEEDS_GETTEXT && !BR2_USE_WCHAR
 
-config BR2_PACKAGE_LIBINTL
-	bool "libintl"
-	depends on BR2_NEEDS_GETTEXT
-	depends on BR2_USE_WCHAR
-	help
-	  Selecting this package installs all of gettext in the staging
-	  directory and the shared library for it's use in the target.
+comment "libintl requires a toolchain with locale/i18n support"
+	depends on BR2_PACKAGE_GETTEXT && !BR2_ENABLE_LOCALE
 
-	  http://www.gnu.org/software/gettext/
+config BR2_PACKAGE_GETTEXT_NEEDS_BINARIES
+	bool
+	depends on BR2_PACKAGE_GETTEXT
+	help
+	  Force to install gettext binaries (gettext, gettext.sh and gettextize)
+	  in the target.
diff --git a/package/gettext/gettext.mk b/package/gettext/gettext.mk
index 7c7b26c..0902edd 100644
--- a/package/gettext/gettext.mk
+++ b/package/gettext/gettext.mk
@@ -17,7 +17,7 @@  define GETTEXT_REMOVE_BINARIES
 	rm -f $(TARGET_DIR)/usr/bin/gettextize
 endef
 
-ifeq ($(BR2_PACKAGE_LIBINTL),y)
+ifneq ($(BR2_PACKAGE_GETTEXT_NEEDS_BINARIES),y)
 	GETTEXT_POST_INSTALL_TARGET_HOOKS += GETTEXT_REMOVE_BINARIES
 endif
 
diff --git a/package/libidn/Config.in b/package/libidn/Config.in
index dcf9724..3e98245 100644
--- a/package/libidn/Config.in
+++ b/package/libidn/Config.in
@@ -1,6 +1,7 @@ 
 config BR2_PACKAGE_LIBIDN
 	bool "libidn"
 	select BR2_PACKAGE_GETTEXT if BR2_NEEDS_GETTEXT_IF_LOCALE
+	select BR2_PACKAGE_GETTEXT_NEEDS_BINARIES if BR2_NEEDS_GETTEXT_IF_LOCALE
 	help
 	  Libidn's purpose is to encode and decode internationalized
 	  domain names.
diff --git a/package/php/Config.ext b/package/php/Config.ext
index bd630ee..2ea686c 100644
--- a/package/php/Config.ext
+++ b/package/php/Config.ext
@@ -68,6 +68,7 @@  config BR2_PACKAGE_PHP_EXT_FTP
 config BR2_PACKAGE_PHP_EXT_GETTEXT
 	bool "gettext"
 	select BR2_PACKAGE_GETTEXT if BR2_NEEDS_GETTEXT
+	select BR2_PACKAGE_GETTEXT_NEEDS_BINARIES if BR2_NEEDS_GETTEXT
 	depends on BR2_USE_WCHAR
 	help
 	  gettext support