| Message ID | 577822438ab319654ee5eaa73ec0b4f96b3a8434.1504993178.git.yann.morin.1998@free.fr |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [01/49,v2] package/asterisk: new package | expand |
On 09-09-17 23:39, Yann E. MORIN wrote: > Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr> > --- > package/libpjsip/Config.in | 12 ++++++++++++ > package/libpjsip/libpjsip.mk | 14 +++++++++++++- > 2 files changed, 25 insertions(+), 1 deletion(-) > > diff --git a/package/libpjsip/Config.in b/package/libpjsip/Config.in > index 727d2ec3d0..ce15a20567 100644 > --- a/package/libpjsip/Config.in > +++ b/package/libpjsip/Config.in > @@ -10,5 +10,17 @@ config BR2_PACKAGE_LIBPJSIP > > http://www.pjsip.org > > +if BR2_PACKAGE_LIBPJSIP > + > +config BR2_PACKAGE_LIBPJSIP_CODEC_GSM > + bool "GSM codec" > + depends on !BR2_STATIC_LIBS # libgsm > + select BR2_PACKAGE_LIBGSM Is there a good reason to make this an explicit suboption rather than an automatic one like we usually do? I would be more inclined to add something like this to the pjsip help text: pjsip will include the following codecs if the corresponding package is enabled in Buildroot: GSM, Speex, iLBC. Regards, Arnout > + > +comment "GSM codec needs a toolchain w/ shared libraries" > + depends on BR2_STATIC_LIBS > + > +endif # BR2_PACKAGE_LIBPJSIP > + > comment "libpjsip needs a toolchain w/ C++, threads" > depends on !BR2_INSTALL_LIBSTDCPP || !BR2_TOOLCHAIN_HAS_THREADS > diff --git a/package/libpjsip/libpjsip.mk b/package/libpjsip/libpjsip.mk > index 4e2da7d32d..9b25dad2f8 100644 > --- a/package/libpjsip/libpjsip.mk > +++ b/package/libpjsip/libpjsip.mk > @@ -26,7 +26,6 @@ LIBPJSIP_CONF_ENV = \ > > LIBPJSIP_CONF_OPTS = \ > --disable-sound \ > - --disable-gsm-codec \ > --disable-speex-codec \ > --disable-speex-aec \ > --disable-resample \ > @@ -70,4 +69,17 @@ ifeq ($(BR2_PACKAGE_UTIL_LINUX_LIBUUID),y) > LIBPJSIP_DEPENDENCIES += util-linux > endif > > +# Codecs can only be disabled. If explictly enabled, the check is > +# omitted (but successful), and there is no configure trace "Checking > +# if [codec] codec is disabled...no". So we only explicitly disable it > +# and we do not explictly enable it, so we get the configure log in > +# both cases. > + > +ifeq ($(BR2_PACKAGE_LIBPJSIP_CODEC_GSM),y) > +LIBPJSIP_DEPENDENCIES += libgsm > +LIBPJSIP_CONF_OPTS += --with-external-gsm > +else > +LIBPJSIP_CONF_OPTS += --disable-gsm-codec > +endif > + > $(eval $(autotools-package)) >
Arnout, All, On 2017-09-23 19:05 +0200, Arnout Vandecappelle spake thusly: > On 09-09-17 23:39, Yann E. MORIN wrote: > > Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr> > > --- > > package/libpjsip/Config.in | 12 ++++++++++++ > > package/libpjsip/libpjsip.mk | 14 +++++++++++++- > > 2 files changed, 25 insertions(+), 1 deletion(-) > > > > diff --git a/package/libpjsip/Config.in b/package/libpjsip/Config.in > > index 727d2ec3d0..ce15a20567 100644 > > --- a/package/libpjsip/Config.in > > +++ b/package/libpjsip/Config.in > > @@ -10,5 +10,17 @@ config BR2_PACKAGE_LIBPJSIP > > > > http://www.pjsip.org > > > > +if BR2_PACKAGE_LIBPJSIP > > + > > +config BR2_PACKAGE_LIBPJSIP_CODEC_GSM > > + bool "GSM codec" > > + depends on !BR2_STATIC_LIBS # libgsm > > + select BR2_PACKAGE_LIBGSM > > Is there a good reason to make this an explicit suboption rather than an > automatic one like we usually do? The issue I see is that the handling of codecs is not orthogonal (accounting for your reply to the G711 codec): - codecs without external dependency will always be built, - codecs with external dependencies need those dependencies to be manually selected. By adding a config for each codec, the behaviour is then the same for all codecs, so the user experiences a _consistent_ behaviour. > I would be more inclined to add something like this to the pjsip help text: > > pjsip will include the following codecs if the corresponding package is enabled > in Buildroot: GSM, Speex, iLBC. Well, that is what I originally thought, yes, but as explained above, I still believe that usrr experience should be more consistent. Regards, Yann E. MORIN. > Regards, > Arnout > > > + > > +comment "GSM codec needs a toolchain w/ shared libraries" > > + depends on BR2_STATIC_LIBS > > + > > +endif # BR2_PACKAGE_LIBPJSIP > > + > > comment "libpjsip needs a toolchain w/ C++, threads" > > depends on !BR2_INSTALL_LIBSTDCPP || !BR2_TOOLCHAIN_HAS_THREADS > > diff --git a/package/libpjsip/libpjsip.mk b/package/libpjsip/libpjsip.mk > > index 4e2da7d32d..9b25dad2f8 100644 > > --- a/package/libpjsip/libpjsip.mk > > +++ b/package/libpjsip/libpjsip.mk > > @@ -26,7 +26,6 @@ LIBPJSIP_CONF_ENV = \ > > > > LIBPJSIP_CONF_OPTS = \ > > --disable-sound \ > > - --disable-gsm-codec \ > > --disable-speex-codec \ > > --disable-speex-aec \ > > --disable-resample \ > > @@ -70,4 +69,17 @@ ifeq ($(BR2_PACKAGE_UTIL_LINUX_LIBUUID),y) > > LIBPJSIP_DEPENDENCIES += util-linux > > endif > > > > +# Codecs can only be disabled. If explictly enabled, the check is > > +# omitted (but successful), and there is no configure trace "Checking > > +# if [codec] codec is disabled...no". So we only explicitly disable it > > +# and we do not explictly enable it, so we get the configure log in > > +# both cases. > > + > > +ifeq ($(BR2_PACKAGE_LIBPJSIP_CODEC_GSM),y) > > +LIBPJSIP_DEPENDENCIES += libgsm > > +LIBPJSIP_CONF_OPTS += --with-external-gsm > > +else > > +LIBPJSIP_CONF_OPTS += --disable-gsm-codec > > +endif > > + > > $(eval $(autotools-package)) > > > > -- > 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
On 25-09-17 18:19, Yann E. MORIN wrote: > Arnout, All, > > On 2017-09-23 19:05 +0200, Arnout Vandecappelle spake thusly: >> On 09-09-17 23:39, Yann E. MORIN wrote: >>> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr> >>> --- >>> package/libpjsip/Config.in | 12 ++++++++++++ >>> package/libpjsip/libpjsip.mk | 14 +++++++++++++- >>> 2 files changed, 25 insertions(+), 1 deletion(-) >>> >>> diff --git a/package/libpjsip/Config.in b/package/libpjsip/Config.in >>> index 727d2ec3d0..ce15a20567 100644 >>> --- a/package/libpjsip/Config.in >>> +++ b/package/libpjsip/Config.in >>> @@ -10,5 +10,17 @@ config BR2_PACKAGE_LIBPJSIP >>> >>> http://www.pjsip.org >>> >>> +if BR2_PACKAGE_LIBPJSIP >>> + >>> +config BR2_PACKAGE_LIBPJSIP_CODEC_GSM >>> + bool "GSM codec" >>> + depends on !BR2_STATIC_LIBS # libgsm >>> + select BR2_PACKAGE_LIBGSM >> Is there a good reason to make this an explicit suboption rather than an >> automatic one like we usually do? > The issue I see is that the handling of codecs is not orthogonal > (accounting for your reply to the G711 codec): > > - codecs without external dependency will always be built, > > - codecs with external dependencies need those dependencies to be > manually selected. > > By adding a config for each codec, the behaviour is then the same for > all codecs, so the user experiences a _consistent_ behaviour. So how does this differ with e.g. freeswitch, where e.g. speex is enabled if it is enabled, while zrtp (with no external dependencies) is always enabled? I'm taking this example because it is one I could fine where we explicitly pass --enable-zrtp from Buildroot, but there are many others where we don't pass anything explicitly. The general guideline is: enable everything if it doesn't increase the size much, automatically enable things with external dependencies if it doesn't increase the size much, and only add explicit options if there is a good reason, like it would increase the size too much, or the external dependency is not obvious, or something exceptional. Clearly, we have (many) exceptions to these principles. I just want to make sure that we spend conscious thought on such exceptions and don't do it randomly. Note that personally I have nothing against adding config options all over the place. I just want to make sure we follow consistent principles. Regards, Arnout
diff --git a/package/libpjsip/Config.in b/package/libpjsip/Config.in index 727d2ec3d0..ce15a20567 100644 --- a/package/libpjsip/Config.in +++ b/package/libpjsip/Config.in @@ -10,5 +10,17 @@ config BR2_PACKAGE_LIBPJSIP http://www.pjsip.org +if BR2_PACKAGE_LIBPJSIP + +config BR2_PACKAGE_LIBPJSIP_CODEC_GSM + bool "GSM codec" + depends on !BR2_STATIC_LIBS # libgsm + select BR2_PACKAGE_LIBGSM + +comment "GSM codec needs a toolchain w/ shared libraries" + depends on BR2_STATIC_LIBS + +endif # BR2_PACKAGE_LIBPJSIP + comment "libpjsip needs a toolchain w/ C++, threads" depends on !BR2_INSTALL_LIBSTDCPP || !BR2_TOOLCHAIN_HAS_THREADS diff --git a/package/libpjsip/libpjsip.mk b/package/libpjsip/libpjsip.mk index 4e2da7d32d..9b25dad2f8 100644 --- a/package/libpjsip/libpjsip.mk +++ b/package/libpjsip/libpjsip.mk @@ -26,7 +26,6 @@ LIBPJSIP_CONF_ENV = \ LIBPJSIP_CONF_OPTS = \ --disable-sound \ - --disable-gsm-codec \ --disable-speex-codec \ --disable-speex-aec \ --disable-resample \ @@ -70,4 +69,17 @@ ifeq ($(BR2_PACKAGE_UTIL_LINUX_LIBUUID),y) LIBPJSIP_DEPENDENCIES += util-linux endif +# Codecs can only be disabled. If explictly enabled, the check is +# omitted (but successful), and there is no configure trace "Checking +# if [codec] codec is disabled...no". So we only explicitly disable it +# and we do not explictly enable it, so we get the configure log in +# both cases. + +ifeq ($(BR2_PACKAGE_LIBPJSIP_CODEC_GSM),y) +LIBPJSIP_DEPENDENCIES += libgsm +LIBPJSIP_CONF_OPTS += --with-external-gsm +else +LIBPJSIP_CONF_OPTS += --disable-gsm-codec +endif + $(eval $(autotools-package))
Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr> --- package/libpjsip/Config.in | 12 ++++++++++++ package/libpjsip/libpjsip.mk | 14 +++++++++++++- 2 files changed, 25 insertions(+), 1 deletion(-)