[Buildroot] [PATCH 1/1] package/transmission: fix gtk support

[Buildroot] [PATCH 1/1] package/transmission: fix gtk support

[Bernd Kuhls]

Gtk support is controlled by ARG_WITH since
https://github.com/transmission/transmission/commit/2ccc2bbbfe2e4a26dfeaa13b56c412ea0af4ebe4

Fixes a build error if libgtk2/3 was built before transmission:
http://autobuild.buildroot.net/results/6b6/6b6ce352a9edfe3aaba82be143092a878e7715ed/

Signed-off-by: Bernd Kuhls <bernd.kuhls@t-online.de>
---
 package/transmission/transmission.mk | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/package/transmission/transmission.mk b/package/transmission/transmission.mk
index f8abb2b8b..d84e5cb77 100644
--- a/package/transmission/transmission.mk
+++ b/package/transmission/transmission.mk
@@ -82,10 +82,10 @@ TRANSMISSION_CONF_OPTS += --disable-remote
 endif
 
 ifeq ($(BR2_PACKAGE_TRANSMISSION_GTK),y)
-TRANSMISSION_CONF_OPTS += --enable-gtk
+TRANSMISSION_CONF_OPTS += --with-gtk
 TRANSMISSION_DEPENDENCIES += libgtk2
 else
-TRANSMISSION_CONF_OPTS += --disable-gtk
+TRANSMISSION_CONF_OPTS += --without-gtk
 endif
 
 $(eval $(autotools-package))
-- 
2.11.0

[Buildroot] [PATCH 1/1] package/transmission: fix gtk support

[Peter Korsgaard]

>>>>> "Bernd" == Bernd Kuhls <bernd.kuhls@t-online.de> writes:

 > Gtk support is controlled by ARG_WITH since
 > https://github.com/transmission/transmission/commit/2ccc2bbbfe2e4a26dfeaa13b56c412ea0af4ebe4

 > Fixes a build error if libgtk2/3 was built before transmission:
 > http://autobuild.buildroot.net/results/6b6/6b6ce352a9edfe3aaba82be143092a878e7715ed/

 > Signed-off-by: Bernd Kuhls <bernd.kuhls@t-online.de>

So this has been broken since (atleast) the version bump in early 2016?
Wow.

Committed, thanks.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH 1/1] package/transmission: fix gtk support

[Thomas Petazzoni]

Hello,

On Tue, 11 Jul 2017 21:37:15 +0200, Peter Korsgaard wrote:

>  > Gtk support is controlled by ARG_WITH since
>  > https://github.com/transmission/transmission/commit/2ccc2bbbfe2e4a26dfeaa13b56c412ea0af4ebe4  
> 
>  > Fixes a build error if libgtk2/3 was built before transmission:
>  > http://autobuild.buildroot.net/results/6b6/6b6ce352a9edfe3aaba82be143092a878e7715ed/  
> 
>  > Signed-off-by: Bernd Kuhls <bernd.kuhls@t-online.de>  
> 
> So this has been broken since (atleast) the version bump in early 2016?
> Wow.

Maybe not:

configure: error: "The gtk client cannot be built without nls support.  Try adding either --enable-nls or --without-gtk" 

Until the recent gettext revamp we were passing --disable-nls only when
BR2_ENABLE_LOCALE was disabled. However, the transmission-gtk support has:

	depends on BR2_PACKAGE_LIBGTK2 && BR2_ENABLE_LOCALE

Therefore, we were never building transmission with --disable-nls. With
the gettext revamp, we now pass --disable-nls to all packages, unless
BR2_SYSTEM_ENABLE_NLS is enabled (which it isn't by default).

So I am not sure the fix is complete. Indeed the error says that the
gtk client cannot be built without nls support. So I guess that if you
have BR2_PACKCAGE_TRANSMISSION_GTK=y, but BR2_SYSTEM_ENABLE_NLS
disabled it still fails to build.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[Buildroot] [PATCH 1/1] package/transmission: fix gtk support

[Arnout Vandecappelle]

On 11-07-17 23:17, Thomas Petazzoni wrote:
> Hello,
> 
> On Tue, 11 Jul 2017 21:37:15 +0200, Peter Korsgaard wrote:
> 
>>  > Gtk support is controlled by ARG_WITH since
>>  > https://github.com/transmission/transmission/commit/2ccc2bbbfe2e4a26dfeaa13b56c412ea0af4ebe4  
>>
>>  > Fixes a build error if libgtk2/3 was built before transmission:
>>  > http://autobuild.buildroot.net/results/6b6/6b6ce352a9edfe3aaba82be143092a878e7715ed/  
>>
>>  > Signed-off-by: Bernd Kuhls <bernd.kuhls@t-online.de>  
>>
>> So this has been broken since (atleast) the version bump in early 2016?
>> Wow.
> 
> Maybe not:
> 
> configure: error: "The gtk client cannot be built without nls support.  Try adding either --enable-nls or --without-gtk" 
> 
> Until the recent gettext revamp we were passing --disable-nls only when
> BR2_ENABLE_LOCALE was disabled. However, the transmission-gtk support has:
> 
> 	depends on BR2_PACKAGE_LIBGTK2 && BR2_ENABLE_LOCALE

 So in fact this dependency on LOCALE is wrong now. Indeed, the commit message
that added it (d6dfc2109c7149a795f7bda963a9a583685dec3f) says:

    Otherwise configure errors out with:

    configure: error: "The gtk client cannot be built without nls support.
    Try adding either --enable-nls or --disable-gtk"

> Therefore, we were never building transmission with --disable-nls. With
> the gettext revamp, we now pass --disable-nls to all packages, unless
> BR2_SYSTEM_ENABLE_NLS is enabled (which it isn't by default).
> 
> So I am not sure the fix is complete. Indeed the error says that the
> gtk client cannot be built without nls support. So I guess that if you
> have BR2_PACKCAGE_TRANSMISSION_GTK=y, but BR2_SYSTEM_ENABLE_NLS
> disabled it still fails to build.

 But only because we pass --disable-nls. I expect that passing --enable-nls
should be sufficient to fix the build again. Indeed, all libcs not have libintl
stubs built in so there is no reason why it shouldn't just work when you pass
--enable-nls.

 Regards,
 Arnout

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
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:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [PATCH 1/1] package/transmission: fix gtk support

[Peter Korsgaard]

>>>>> "Arnout" == Arnout Vandecappelle <arnout@mind.be> writes:

Hi,

 >> So I am not sure the fix is complete. Indeed the error says that the
 >> gtk client cannot be built without nls support. So I guess that if you
 >> have BR2_PACKCAGE_TRANSMISSION_GTK=y, but BR2_SYSTEM_ENABLE_NLS
 >> disabled it still fails to build.

 >  But only because we pass --disable-nls. I expect that passing --enable-nls
 > should be sufficient to fix the build again. Indeed, all libcs not have libintl
 > stubs built in so there is no reason why it shouldn't just work when you pass
 > --enable-nls.

Yes, makes sense with the latest changes. Care to give it a try / send a
patch?

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH 1/1] package/transmission: fix gtk support

[Thomas Petazzoni]

Hello,

On Tue, 11 Jul 2017 23:42:24 +0200, Arnout Vandecappelle wrote:

> > configure: error: "The gtk client cannot be built without nls support.  Try adding either --enable-nls or --without-gtk" 
> > 
> > Until the recent gettext revamp we were passing --disable-nls only when
> > BR2_ENABLE_LOCALE was disabled. However, the transmission-gtk support has:
> > 
> > 	depends on BR2_PACKAGE_LIBGTK2 && BR2_ENABLE_LOCALE  
> 
>  So in fact this dependency on LOCALE is wrong now. Indeed, the commit message
> that added it (d6dfc2109c7149a795f7bda963a9a583685dec3f) says:

Agreed, the BR2_ENABLE_LOCALE dependency is now wrong.

> > Therefore, we were never building transmission with --disable-nls. With
> > the gettext revamp, we now pass --disable-nls to all packages, unless
> > BR2_SYSTEM_ENABLE_NLS is enabled (which it isn't by default).
> > 
> > So I am not sure the fix is complete. Indeed the error says that the
> > gtk client cannot be built without nls support. So I guess that if you
> > have BR2_PACKCAGE_TRANSMISSION_GTK=y, but BR2_SYSTEM_ENABLE_NLS
> > disabled it still fails to build.  
> 
>  But only because we pass --disable-nls. I expect that passing --enable-nls
> should be sufficient to fix the build again. Indeed, all libcs not have libintl

"not -> now" I guess, correct?

> stubs built in so there is no reason why it shouldn't just work when you pass
> --enable-nls.

Why should we randomly pass --enable-nls to some packages? Why should
this package depends on BR2_SYSTEM_ENABLE_NLS, if it needs NLS support?

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[Buildroot] [PATCH 1/1] package/transmission: fix gtk support

[Thomas Petazzoni]

Hello,

On Wed, 12 Jul 2017 09:49:24 +0200, Thomas Petazzoni wrote:

> Why should we randomly pass --enable-nls to some packages? Why should
> this package depends on BR2_SYSTEM_ENABLE_NLS, if it needs NLS support?

I meant to say:

==
Why *shouldn't* this package depends on BR2_SYSTEM_ENABLE_NLS, if it
needs NLS support?
==

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[Buildroot] [PATCH 1/1] package/transmission: fix gtk support

[Arnout Vandecappelle]

On 12-07-17 09:49, Thomas Petazzoni wrote:
> Hello,
> 
> On Tue, 11 Jul 2017 23:42:24 +0200, Arnout Vandecappelle wrote:
> 
>>> configure: error: "The gtk client cannot be built without nls support.  Try adding either --enable-nls or --without-gtk" 
>>>
>>> Until the recent gettext revamp we were passing --disable-nls only when
>>> BR2_ENABLE_LOCALE was disabled. However, the transmission-gtk support has:
>>>
>>> 	depends on BR2_PACKAGE_LIBGTK2 && BR2_ENABLE_LOCALE  
>>
>>  So in fact this dependency on LOCALE is wrong now. Indeed, the commit message
>> that added it (d6dfc2109c7149a795f7bda963a9a583685dec3f) says:
> 
> Agreed, the BR2_ENABLE_LOCALE dependency is now wrong.
> 
>>> Therefore, we were never building transmission with --disable-nls. With
>>> the gettext revamp, we now pass --disable-nls to all packages, unless
>>> BR2_SYSTEM_ENABLE_NLS is enabled (which it isn't by default).
>>>
>>> So I am not sure the fix is complete. Indeed the error says that the
>>> gtk client cannot be built without nls support. So I guess that if you
>>> have BR2_PACKCAGE_TRANSMISSION_GTK=y, but BR2_SYSTEM_ENABLE_NLS
>>> disabled it still fails to build.  
>>
>>  But only because we pass --disable-nls. I expect that passing --enable-nls
>> should be sufficient to fix the build again. Indeed, all libcs not have libintl
> 
> "not -> now" I guess, correct?

 Yes of course.

> 
>> stubs built in so there is no reason why it shouldn't just work when you pass
>> --enable-nls.
> 
> Why should we randomly pass --enable-nls to some packages? Why should
> this package depends on BR2_SYSTEM_ENABLE_NLS, if it needs NLS support?

 Because BR2_SYSTEM_ENABLE_NLS does not really say that NLS *support* is
enabled, it says that NLS *installation* is enabled. NLS support is always
"enabled" through the stubs in musl/uClibc.

 As far as I understand, the configure error is just there because the
transmission guys are too lazy to have conditional use of the intl functions. I
don't see how there could be a hard requirement on non-stub implementations of
the intl functions - except if they start calling internals. So the better
solution, actually, would be to patch out this check in configure and instead
check for intl and error out if that is not available, independent on the
--enable-nls option - just like most packages do, I think.

 If you're not actually interested in translations, there is no reason really
why you should pull them in just because this package is calling gettext stuff
unconditionally.

 That said, probably nobody really cares, we're just fixing build failures here.
So in the end I don't really mind if we depend on BR2_SYSTEM_ENABLE_NLS as the
easy way out. Though then the commit message should at least provide the full
story, as a hint for future contributors who want to fix it properly.

 Regards,
 Arnout
-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
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:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [PATCH 1/1] package/transmission: fix gtk support

[Peter Korsgaard]

>>>>> "Bernd" == Bernd Kuhls <bernd.kuhls@t-online.de> writes:

 > Gtk support is controlled by ARG_WITH since
 > https://github.com/transmission/transmission/commit/2ccc2bbbfe2e4a26dfeaa13b56c412ea0af4ebe4

 > Fixes a build error if libgtk2/3 was built before transmission:
 > http://autobuild.buildroot.net/results/6b6/6b6ce352a9edfe3aaba82be143092a878e7715ed/

 > Signed-off-by: Bernd Kuhls <bernd.kuhls@t-online.de>
 > ---
 >  package/transmission/transmission.mk | 4 ++--
 >  1 file changed, 2 insertions(+), 2 deletions(-)

 > diff --git a/package/transmission/transmission.mk b/package/transmission/transmission.mk
 > index f8abb2b8b..d84e5cb77 100644
 > --- a/package/transmission/transmission.mk
 > +++ b/package/transmission/transmission.mk
 > @@ -82,10 +82,10 @@ TRANSMISSION_CONF_OPTS += --disable-remote
 >  endif
 
 >  ifeq ($(BR2_PACKAGE_TRANSMISSION_GTK),y)
 > -TRANSMISSION_CONF_OPTS += --enable-gtk
 > +TRANSMISSION_CONF_OPTS += --with-gtk
 >  TRANSMISSION_DEPENDENCIES += libgtk2

Committed, thanks. I also pushed two followup patches to actually get it
to build:

- transmission-gtk needs libgtk3 instead of libgtk2 since 2.61
  (E.G. your version bump in 2013)

- Adjusted NLS dependencies after the recent NLS rework
  (BR2_SYSTEM_ENABLE_NLS).

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH 1/1] package/transmission: fix gtk support

[Peter Korsgaard]

>>>>> "Arnout" == Arnout Vandecappelle <arnout@mind.be> writes:

Hi,

 >> Why should we randomly pass --enable-nls to some packages? Why should
 >> this package depends on BR2_SYSTEM_ENABLE_NLS, if it needs NLS support?

 >  Because BR2_SYSTEM_ENABLE_NLS does not really say that NLS *support* is
 > enabled, it says that NLS *installation* is enabled. NLS support is always
 > "enabled" through the stubs in musl/uClibc.

 >  As far as I understand, the configure error is just there because the
 > transmission guys are too lazy to have conditional use of the intl functions. I
 > don't see how there could be a hard requirement on non-stub implementations of
 > the intl functions - except if they start calling internals. So the better
 > solution, actually, would be to patch out this check in configure and instead
 > check for intl and error out if that is not available, independent on the
 > --enable-nls option - just like most packages do, I think.

 >  If you're not actually interested in translations, there is no reason really
 > why you should pull them in just because this package is calling gettext stuff
 > unconditionally.

 >  That said, probably nobody really cares, we're just fixing build failures here.
 > So in the end I don't really mind if we depend on BR2_SYSTEM_ENABLE_NLS as the
 > easy way out. Though then the commit message should at least provide the full
 > story, as a hint for future contributors who want to fix it properly.

Correct. I went with depending on BR2_SYSTEM_ENABLE_NLS, but I added a
comment explaining that we could also explictly pass --enable-nls.

-- 
Bye, Peter Korsgaard