[Buildroot] [PATCH] libcurl: enable mbedtls support

[Buildroot] [PATCH] libcurl: enable mbedtls support

[Gustavo Zacarias]

Now that we've got an mbedtls package in the tree we can enable the
optional support for it in libcurl.

Signed-off-by: Gustavo Zacarias <gustavo@zacarias.com.ar>
---
 package/libcurl/libcurl.mk | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/package/libcurl/libcurl.mk b/package/libcurl/libcurl.mk
index 10514c6..8798d71 100644
--- a/package/libcurl/libcurl.mk
+++ b/package/libcurl/libcurl.mk
@@ -40,10 +40,12 @@ else ifeq ($(BR2_PACKAGE_LIBNSS),y)
 LIBCURL_CONF_OPTS += --with-nss=$(STAGING_DIR)/usr
 LIBCURL_CONF_ENV += CPPFLAGS="$(TARGET_CPPFLAGS) `$(PKG_CONFIG_HOST_BINARY) nspr nss --cflags`"
 LIBCURL_DEPENDENCIES += libnss
+else ifeq ($(BR2_PACKAGE_MBEDTLS),y)
+LIBCURL_CONF_OPTS += --with-mbedtls=$(STAGING_DIR)/usr
+LIBCURL_DEPENDENCIES += mbedtls
 else
-# polarssl support needs 1.3.x
 LIBCURL_CONF_OPTS += --without-ssl --without-gnutls \
-	--without-polarssl --without-nss
+	--without-polarssl --without-nss --without-mbedtls
 endif
 
 ifeq ($(BR2_PACKAGE_C_ARES),y)
-- 
2.4.10

[Buildroot] [PATCH] libcurl: enable mbedtls support

[Luca Ceresoli]

Hi Gustavo,

Gustavo Zacarias wrote:
> Now that we've got an mbedtls package in the tree we can enable the
> optional support for it in libcurl.
>
> Signed-off-by: Gustavo Zacarias <gustavo@zacarias.com.ar>
> ---
>   package/libcurl/libcurl.mk | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/package/libcurl/libcurl.mk b/package/libcurl/libcurl.mk
> index 10514c6..8798d71 100644
> --- a/package/libcurl/libcurl.mk
> +++ b/package/libcurl/libcurl.mk
> @@ -40,10 +40,12 @@ else ifeq ($(BR2_PACKAGE_LIBNSS),y)
>   LIBCURL_CONF_OPTS += --with-nss=$(STAGING_DIR)/usr
>   LIBCURL_CONF_ENV += CPPFLAGS="$(TARGET_CPPFLAGS) `$(PKG_CONFIG_HOST_BINARY) nspr nss --cflags`"
>   LIBCURL_DEPENDENCIES += libnss
> +else ifeq ($(BR2_PACKAGE_MBEDTLS),y)
> +LIBCURL_CONF_OPTS += --with-mbedtls=$(STAGING_DIR)/usr
> +LIBCURL_DEPENDENCIES += mbedtls
>   else
> -# polarssl support needs 1.3.x

This is unrelated to your change, it should be a separate patch. But
anyway I don't understand why it should be removed: we still package
polarssl 1.2.18, so the comment should still apply. Am I missing
something?

With that fixed (or explained):
  Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net>

And:
[Built with/without mbedtls, ran on qemu, checked that 'curl -V' shows
the SSL only with mbedtls]
Tested-by: Luca Ceresoli <luca@lucaceresoli.net>

-- 
Luca

[Buildroot] [PATCH] libcurl: enable mbedtls support

[Gustavo Zacarias]

On 01/02/16 13:27, Luca Ceresoli wrote:

> This is unrelated to your change, it should be a separate patch. But
> anyway I don't understand why it should be removed: we still package
> polarssl 1.2.18, so the comment should still apply. Am I missing
> something?
>
> With that fixed (or explained):
>   Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net>
>
> And:
> [Built with/without mbedtls, ran on qemu, checked that 'curl -V' shows
> the SSL only with mbedtls]
> Tested-by: Luca Ceresoli <luca@lucaceresoli.net>

Hi.
As you know polarssl was renamed to mbedtls when bought by ARM, this was 
circa the 1.3.x release.
Older 1.3.x releases install the library as polarssl alone, newer ones 
install dual as polarssl with symlinks to mbedtls (might be the other 
way around, i don't recall at the moment - it doesn't matter anyway), so 
first the distinction between polarssl and mbedtls is terribly blurred 
for the 1.3.x series.
Incidentally this is what makes polarssl 1.2.x being able to live 
side-by-side with mbedtls (2.x, which doesn't keep legacy handling) (i 
believe this is more product of branding/accident than careful thinking).
So now in some cases polarssl means 1.2.x, in other cases it means 
mbedtls 1.3.x, or the new version because nobody cared to rename everything.
Basically any explanation given out of context is useless without 
historical context.
Making 1.3.x live side-by-side with mbedtls (let's call it that way for 
2+) is probably possible by ditching the new naming and just keeping 
compat, i had a look but i think it was a dead end anyway, this requires 
patching the build of mbedtls/polarssl 1.3.x somewhat and that wouldn't 
be accepted upstream.
Regards.

[Buildroot] [PATCH] libcurl: enable mbedtls support

[Luca Ceresoli]

Hi Gustavo,

Gustavo Zacarias wrote:
> On 01/02/16 13:27, Luca Ceresoli wrote:
>
>> This is unrelated to your change, it should be a separate patch. But
>> anyway I don't understand why it should be removed: we still package
>> polarssl 1.2.18, so the comment should still apply. Am I missing
>> something?
>>
>> With that fixed (or explained):
>>   Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net>
>>
>> And:
>> [Built with/without mbedtls, ran on qemu, checked that 'curl -V' shows
>> the SSL only with mbedtls]
>> Tested-by: Luca Ceresoli <luca@lucaceresoli.net>
>
> Hi.
> As you know polarssl was renamed to mbedtls when bought by ARM, this was
> circa the 1.3.x release.
> Older 1.3.x releases install the library as polarssl alone, newer ones
> install dual as polarssl with symlinks to mbedtls (might be the other
> way around, i don't recall at the moment - it doesn't matter anyway), so
> first the distinction between polarssl and mbedtls is terribly blurred
> for the 1.3.x series.
> Incidentally this is what makes polarssl 1.2.x being able to live
> side-by-side with mbedtls (2.x, which doesn't keep legacy handling) (i
> believe this is more product of branding/accident than careful thinking).
> So now in some cases polarssl means 1.2.x, in other cases it means
> mbedtls 1.3.x, or the new version because nobody cared to rename
> everything.
> Basically any explanation given out of context is useless without
> historical context.
> Making 1.3.x live side-by-side with mbedtls (let's call it that way for
> 2+) is probably possible by ditching the new naming and just keeping
> compat, i had a look but i think it was a dead end anyway, this requires
> patching the build of mbedtls/polarssl 1.3.x somewhat and that wouldn't
> be accepted upstream.

Thank you for the explanation. Indeed, it got me totally puzzled! I am
no mbedtls/polarssl expert, I never used them...

If I understand correctly:
  - without this patch, the comment states that you'd need to upgrade
    polarssl to 1.3.x to have it supported in libcurl
  - with the patch libcurl uses mbedtls, which makes polarssl
    uninteresting.

Is it correct?

-- 
Luca

[Buildroot] [PATCH] libcurl: enable mbedtls support

[Gustavo Zacarias]

On 01/02/16 14:52, Luca Ceresoli wrote:

> Thank you for the explanation. Indeed, it got me totally puzzled! I am
> no mbedtls/polarssl expert, I never used them...
>
> If I understand correctly:
>   - without this patch, the comment states that you'd need to upgrade
>     polarssl to 1.3.x to have it supported in libcurl
>   - with the patch libcurl uses mbedtls, which makes polarssl
>     uninteresting.
>
> Is it correct?

Hi Luca.
Yes, the polarssl comment becomes redundant, since we can't support our 
current 1.2.x and we can't (easily) upgrade to 1.3.x, which i think 
would be pointless.
Newer openvpn can support 1.3.x but not newer, i know git master which 
is very much in a -dev state can do 2.x but i wouldn't pick that up lightly.
Regarding shairport-sync i don't know if it can work with newer 
versions, maybe it can do mbedtls directly now that it's been bumped.
Regards.

[Buildroot] [PATCH] libcurl: enable mbedtls support

[Luca Ceresoli]

Hi Gostavo,

Gustavo Zacarias wrote:
> On 01/02/16 14:52, Luca Ceresoli wrote:
>
>> Thank you for the explanation. Indeed, it got me totally puzzled! I am
>> no mbedtls/polarssl expert, I never used them...
>>
>> If I understand correctly:
>>   - without this patch, the comment states that you'd need to upgrade
>>     polarssl to 1.3.x to have it supported in libcurl
>>   - with the patch libcurl uses mbedtls, which makes polarssl
>>     uninteresting.
>>
>> Is it correct?
>
> Hi Luca.
> Yes, the polarssl comment becomes redundant, since we can't support our
> current 1.2.x and we can't (easily) upgrade to 1.3.x, which i think
> would be pointless.
> Newer openvpn can support 1.3.x but not newer, i know git master which
> is very much in a -dev state can do 2.x but i wouldn't pick that up
> lightly.
> Regarding shairport-sync i don't know if it can work with newer
> versions, maybe it can do mbedtls directly now that it's been bumped.

I see, thanks for the additional clarification. Now it makes sense.

Since it is non-obvious (at least it wasn't to me), it's worth adding a
comment, such as: "mbedtls is a successor or polarssl, thus polarssl
support is useless".

Care to resend with that?

Thanks,
-- 
Luca

[Buildroot] [PATCH] libcurl: enable mbedtls support

[Gustavo Zacarias]

On 01/02/16 15:02, Luca Ceresoli wrote:

> I see, thanks for the additional clarification. Now it makes sense.
>
> Since it is non-obvious (at least it wasn't to me), it's worth adding a
> comment, such as: "mbedtls is a successor or polarssl, thus polarssl
> support is useless".
>
> Care to resend with that?
>
> Thanks,

Does it make any sense?
It never had any polarssl/mbedtls support to begin with.
Regards.

[Buildroot] [PATCH] libcurl: enable mbedtls support

[Luca Ceresoli]

Hi Gustavo,

Gustavo Zacarias wrote:
> On 01/02/16 15:02, Luca Ceresoli wrote:
>
>> I see, thanks for the additional clarification. Now it makes sense.
>>
>> Since it is non-obvious (at least it wasn't to me), it's worth adding a
>> comment, such as: "mbedtls is a successor or polarssl, thus polarssl
>> support is useless".
>>
>> Care to resend with that?
>>
>> Thanks,
>
> Does it make any sense?
> It never had any polarssl/mbedtls support to begin with.

Well, it makes sense in the commit log IMO. It would have replied my
doubts immediately.

-- 
Luca

[Buildroot] [PATCH] libcurl: enable mbedtls support

[Gustavo Zacarias]

On 01/02/16 15:07, Luca Ceresoli wrote:

> Well, it makes sense in the commit log IMO. It would have replied my
> doubts immediately.

Which is now documented in the ML / archive :)
Regards.

[Buildroot] [PATCH] libcurl: enable mbedtls support

[Thomas Petazzoni]

Dear Gustavo Zacarias,

On Wed,  6 Jan 2016 15:53:37 -0300, Gustavo Zacarias wrote:
> Now that we've got an mbedtls package in the tree we can enable the
> optional support for it in libcurl.
> 
> Signed-off-by: Gustavo Zacarias <gustavo@zacarias.com.ar>
> ---
>  package/libcurl/libcurl.mk | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)

Applied with an improved commit log, as suggested by Luca.

Gustavo, for the record, I do agree with Luca that an additional
explanation was needed. It is a pity that you refused to send an
updated version, and simply referred to the ML archives as a way of
explaining your patches.

Best regards,

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