| Message ID | 20170710175137.1402-1-bernd.kuhls@t-online.de |
|---|---|
| State | Accepted |
| Commit | e67fbcfa94e58a4d04e081be5e318953233492c8 |
| Headers | show |
>>>>> "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.
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
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" == 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?
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
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
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
>>>>> "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).
>>>>> "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.
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))
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(-)