| Message ID | 55f81de26c9b74283245f427bc7dd7ff4db09c06.1657777745.git.baruch@tkos.co.il |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [RFC,v2] package/uacme: requires TLS support in libcurl | expand |
Hello Baruch On Thu, Jul 14, 2022 at 08:49:05AM +0300, Baruch Siach wrote: >v2: > Add dependency on crypto back end for uacme itself (Nicola Di Lieto) This should work. It's perhaps not ideal but I can't see any other way to work around the circular dependency issue.
Baruch, Nicola, All, On 2022-07-14 08:49 +0300, Baruch Siach via buildroot spake thusly: > uacme configure script fails when libcurl does not support TLS. This > means that BR2_PACKAGE_LIBCURL_TLS_NONE is incompatible with uacme. But > there is no way to change the choice to something other than > BR2_PACKAGE_LIBCURL_TLS_NONE. So instead make uacme depend on libcurl > and !BR2_PACKAGE_LIBCURL_TLS_NONE. > > As a result we can no longer select BR2_PACKAGE_OPENSSL since it causes > recursive dependency. Use 'depend on' instead, and add a comment to > explain this uncommon choice. > > Fixes: > http://autobuild.buildroot.net/results/4e16f1d958ac3d30e26e7f17bdffc47834b0e2bd/ > http://autobuild.buildroot.net/results/4e16f1d958ac3d30e26e7f17bdffc47834b0e2bd/ > http://autobuild.buildroot.net/results/25280409b32282b4dd40b1e88127051439380f3d/ > > Cc: Nicola Di Lieto <nicola.dilieto@gmail.com> > Signed-off-by: Baruch Siach <baruch@tkos.co.il> > --- > v2: > Add dependency on crypto back end for uacme itself (Nicola Di Lieto) > --- > package/uacme/Config.in | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/package/uacme/Config.in b/package/uacme/Config.in > index 58b7c534e73d..815ab5da7d61 100644 > --- a/package/uacme/Config.in > +++ b/package/uacme/Config.in > @@ -1,8 +1,9 @@ > config BR2_PACKAGE_UACME > bool "uacme" > depends on BR2_USE_MMU # fork() > - select BR2_PACKAGE_OPENSSL if !(BR2_PACKAGE_GNUTLS || BR2_PACKAGE_MBEDTLS) > - select BR2_PACKAGE_LIBCURL > + # We can not use select here as it causes recursive dependency > + depends on BR2_PACKAGE_OPENSSL || BR2_PACKAGE_GNUTLS || BR2_PACKAGE_MBEDTLS > + depends on BR2_PACKAGE_LIBCURL && !BR2_PACKAGE_LIBCURL_TLS_NONE I don't think this is correct. Indeed, even with one of those packages enabled, there is nothing that prevents libcurl to be linked with another TLS provider, as that is decided with the choice entries, not with the packages being enabled. Instead, what about: depends on BR2_PACKAGE_LIBCURL_OPENSSL \ || BR2_PACKAGE_LIBCURL_GNUTLS \ || BR2_PACKAGE_LIBCURL_MBEDTLS That way, it encodes both the fact that libcurl is enabled, *and* that is has the proper TLS support enabled. > help > uacme is a client for the ACMEv2 protocol described in > RFC8555, written in plain C with minimal dependencies > @@ -14,6 +15,13 @@ config BR2_PACKAGE_UACME > > https://github.com/ndilieto/uacme > > +comment "uacme needs one of openssl, gnutls or mbedtls" > + depends on !BR2_PACKAGE_OPENSSL && !BR2_PACKAGE_GNUTLS && !BR2_PACKAGE_MBEDTLS That's not correct. It would be better to phrase it, as Nicola suggested in their review of v1: comment "uacme needs libcurl with openssl, gnutls or mbedtls" depends on BR2_USE_MMU depends on !BR2_PACKAGE_LIBCURL_OPENSSL \ && !BR2_PACKAGE_LIBCURL_GNUTLS \ && !BR2_PACKAGE_LIBCURL_MBEDTLS > +comment "uacme needs libcurl with TLS support" > + depends on BR2_USE_MMU > + depends on !BR2_PACKAGE_LIBCURL || BR2_PACKAGE_LIBCURL_TLS_NONE ... then this comment is no longer needed. Also, comments about packages being not available should go either before the main symbol, or after the conditional options. Otherwise, the sub-options are not indented below the main symbol. With your code: [*] uacme [ ] enable ualpn while we want: [*] uacme [ ] enable ualpn I'd have fixed that when applying, but I prefer to get some feedback about my proposal on the dependendcy condition. Regards, Yann E. MORIN. > if BR2_PACKAGE_UACME > > config BR2_PACKAGE_UACME_UALPN > -- > 2.35.1 > > _______________________________________________ > buildroot mailing list > buildroot@buildroot.org > https://lists.buildroot.org/mailman/listinfo/buildroot
Hi Yann, On Sun, Jul 17 2022, Yann E. MORIN wrote: > On 2022-07-14 08:49 +0300, Baruch Siach via buildroot spake thusly: >> uacme configure script fails when libcurl does not support TLS. This >> means that BR2_PACKAGE_LIBCURL_TLS_NONE is incompatible with uacme. But >> there is no way to change the choice to something other than >> BR2_PACKAGE_LIBCURL_TLS_NONE. So instead make uacme depend on libcurl >> and !BR2_PACKAGE_LIBCURL_TLS_NONE. >> >> As a result we can no longer select BR2_PACKAGE_OPENSSL since it causes >> recursive dependency. Use 'depend on' instead, and add a comment to >> explain this uncommon choice. >> >> Fixes: >> http://autobuild.buildroot.net/results/4e16f1d958ac3d30e26e7f17bdffc47834b0e2bd/ >> http://autobuild.buildroot.net/results/4e16f1d958ac3d30e26e7f17bdffc47834b0e2bd/ >> http://autobuild.buildroot.net/results/25280409b32282b4dd40b1e88127051439380f3d/ >> >> Cc: Nicola Di Lieto <nicola.dilieto@gmail.com> >> Signed-off-by: Baruch Siach <baruch@tkos.co.il> >> --- >> v2: >> Add dependency on crypto back end for uacme itself (Nicola Di Lieto) >> --- >> package/uacme/Config.in | 12 ++++++++++-- >> 1 file changed, 10 insertions(+), 2 deletions(-) >> >> diff --git a/package/uacme/Config.in b/package/uacme/Config.in >> index 58b7c534e73d..815ab5da7d61 100644 >> --- a/package/uacme/Config.in >> +++ b/package/uacme/Config.in >> @@ -1,8 +1,9 @@ >> config BR2_PACKAGE_UACME >> bool "uacme" >> depends on BR2_USE_MMU # fork() >> - select BR2_PACKAGE_OPENSSL if !(BR2_PACKAGE_GNUTLS || BR2_PACKAGE_MBEDTLS) >> - select BR2_PACKAGE_LIBCURL >> + # We can not use select here as it causes recursive dependency >> + depends on BR2_PACKAGE_OPENSSL || BR2_PACKAGE_GNUTLS || BR2_PACKAGE_MBEDTLS >> + depends on BR2_PACKAGE_LIBCURL && !BR2_PACKAGE_LIBCURL_TLS_NONE > > I don't think this is correct. Indeed, even with one of those packages > enabled, there is nothing that prevents libcurl to be linked with > another TLS provider, as that is decided with the choice entries, not > with the packages being enabled. > > Instead, what about: > > depends on BR2_PACKAGE_LIBCURL_OPENSSL \ > || BR2_PACKAGE_LIBCURL_GNUTLS \ > || BR2_PACKAGE_LIBCURL_MBEDTLS > > That way, it encodes both the fact that libcurl is enabled, *and* that > is has the proper TLS support enabled. As Nicola explained on v1, uacme does not care which crypto back end libcurl uses, as long as there is one. https://lore.kernel.org/all/Ys5vPCrxDXWvj+ok@einstein.dilieto.eu/ Regardless of that, uacme requires one of these crypt back ends for its own use. So I think these dependencies are correct. >> help >> uacme is a client for the ACMEv2 protocol described in >> RFC8555, written in plain C with minimal dependencies >> @@ -14,6 +15,13 @@ config BR2_PACKAGE_UACME >> >> https://github.com/ndilieto/uacme >> >> +comment "uacme needs one of openssl, gnutls or mbedtls" >> + depends on !BR2_PACKAGE_OPENSSL && !BR2_PACKAGE_GNUTLS && !BR2_PACKAGE_MBEDTLS > > That's not correct. It would be better to phrase it, as Nicola suggested > in their review of v1: > > comment "uacme needs libcurl with openssl, gnutls or mbedtls" > depends on BR2_USE_MMU > depends on !BR2_PACKAGE_LIBCURL_OPENSSL \ > && !BR2_PACKAGE_LIBCURL_GNUTLS \ > && !BR2_PACKAGE_LIBCURL_MBEDTLS This is overly restrictive. See above. >> +comment "uacme needs libcurl with TLS support" >> + depends on BR2_USE_MMU >> + depends on !BR2_PACKAGE_LIBCURL || BR2_PACKAGE_LIBCURL_TLS_NONE > > ... then this comment is no longer needed. > > Also, comments about packages being not available should go either > before the main symbol, or after the conditional options. Otherwise, the > sub-options are not indented below the main symbol. With your code: > > [*] uacme > [ ] enable ualpn > > while we want: > > [*] uacme > [ ] enable ualpn > > I'd have fixed that when applying, but I prefer to get some feedback > about my proposal on the dependendcy condition. I'll fix that if I send another iteration. baruch
Baruch, All, On 2022-07-17 12:09 +0300, Baruch Siach spake thusly: > On Sun, Jul 17 2022, Yann E. MORIN wrote: > > On 2022-07-14 08:49 +0300, Baruch Siach via buildroot spake thusly: > >> uacme configure script fails when libcurl does not support TLS. This > >> means that BR2_PACKAGE_LIBCURL_TLS_NONE is incompatible with uacme. But > >> there is no way to change the choice to something other than > >> BR2_PACKAGE_LIBCURL_TLS_NONE. So instead make uacme depend on libcurl > >> and !BR2_PACKAGE_LIBCURL_TLS_NONE. > >> > >> As a result we can no longer select BR2_PACKAGE_OPENSSL since it causes > >> recursive dependency. Use 'depend on' instead, and add a comment to > >> explain this uncommon choice. [--SNIP--] > >> --- a/package/uacme/Config.in > >> +++ b/package/uacme/Config.in > >> @@ -1,8 +1,9 @@ > >> config BR2_PACKAGE_UACME > >> bool "uacme" > >> depends on BR2_USE_MMU # fork() > >> - select BR2_PACKAGE_OPENSSL if !(BR2_PACKAGE_GNUTLS || BR2_PACKAGE_MBEDTLS) > >> - select BR2_PACKAGE_LIBCURL > >> + # We can not use select here as it causes recursive dependency > >> + depends on BR2_PACKAGE_OPENSSL || BR2_PACKAGE_GNUTLS || BR2_PACKAGE_MBEDTLS > >> + depends on BR2_PACKAGE_LIBCURL && !BR2_PACKAGE_LIBCURL_TLS_NONE > > I don't think this is correct. Indeed, even with one of those packages > > enabled, there is nothing that prevents libcurl to be linked with > > another TLS provider, as that is decided with the choice entries, not > > with the packages being enabled. > > Instead, what about: > > depends on BR2_PACKAGE_LIBCURL_OPENSSL \ > > || BR2_PACKAGE_LIBCURL_GNUTLS \ > > || BR2_PACKAGE_LIBCURL_MBEDTLS > > That way, it encodes both the fact that libcurl is enabled, *and* that > > is has the proper TLS support enabled. > As Nicola explained on v1, uacme does not care which crypto back end > libcurl uses, as long as there is one. > https://lore.kernel.org/all/Ys5vPCrxDXWvj+ok@einstein.dilieto.eu/ > Regardless of that, uacme requires one of these crypt back ends for its > own use. So I think these dependencies are correct. Doh... I somehow got it backwards... Then you want to also hide away the first comment behind "depends on BR2_USE_MMU" too. [--SNIP--] > > Also, comments about packages being not available should go either > > before the main symbol, or after the conditional options. Otherwise, the > > sub-options are not indented below the main symbol. With your code: > > [*] uacme > > [ ] enable ualpn > > while we want: > > [*] uacme > > [ ] enable ualpn > > I'd have fixed that when applying, but I prefer to get some feedback > > about my proposal on the dependendcy condition. > I'll fix that if I send another iteration. Thanks! Regards, Yann E. MORIN.
diff --git a/package/uacme/Config.in b/package/uacme/Config.in index 58b7c534e73d..815ab5da7d61 100644 --- a/package/uacme/Config.in +++ b/package/uacme/Config.in @@ -1,8 +1,9 @@ config BR2_PACKAGE_UACME bool "uacme" depends on BR2_USE_MMU # fork() - select BR2_PACKAGE_OPENSSL if !(BR2_PACKAGE_GNUTLS || BR2_PACKAGE_MBEDTLS) - select BR2_PACKAGE_LIBCURL + # We can not use select here as it causes recursive dependency + depends on BR2_PACKAGE_OPENSSL || BR2_PACKAGE_GNUTLS || BR2_PACKAGE_MBEDTLS + depends on BR2_PACKAGE_LIBCURL && !BR2_PACKAGE_LIBCURL_TLS_NONE help uacme is a client for the ACMEv2 protocol described in RFC8555, written in plain C with minimal dependencies @@ -14,6 +15,13 @@ config BR2_PACKAGE_UACME https://github.com/ndilieto/uacme +comment "uacme needs one of openssl, gnutls or mbedtls" + depends on !BR2_PACKAGE_OPENSSL && !BR2_PACKAGE_GNUTLS && !BR2_PACKAGE_MBEDTLS + +comment "uacme needs libcurl with TLS support" + depends on BR2_USE_MMU + depends on !BR2_PACKAGE_LIBCURL || BR2_PACKAGE_LIBCURL_TLS_NONE + if BR2_PACKAGE_UACME config BR2_PACKAGE_UACME_UALPN
uacme configure script fails when libcurl does not support TLS. This means that BR2_PACKAGE_LIBCURL_TLS_NONE is incompatible with uacme. But there is no way to change the choice to something other than BR2_PACKAGE_LIBCURL_TLS_NONE. So instead make uacme depend on libcurl and !BR2_PACKAGE_LIBCURL_TLS_NONE. As a result we can no longer select BR2_PACKAGE_OPENSSL since it causes recursive dependency. Use 'depend on' instead, and add a comment to explain this uncommon choice. Fixes: http://autobuild.buildroot.net/results/4e16f1d958ac3d30e26e7f17bdffc47834b0e2bd/ http://autobuild.buildroot.net/results/4e16f1d958ac3d30e26e7f17bdffc47834b0e2bd/ http://autobuild.buildroot.net/results/25280409b32282b4dd40b1e88127051439380f3d/ Cc: Nicola Di Lieto <nicola.dilieto@gmail.com> Signed-off-by: Baruch Siach <baruch@tkos.co.il> --- v2: Add dependency on crypto back end for uacme itself (Nicola Di Lieto) --- package/uacme/Config.in | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)