[Buildroot] [PATCH 1/1] New package: https://github.com/ndilieto/uacme

[Buildroot] [PATCH 1/1] New package: https://github.com/ndilieto/uacme

[Nicola Di Lieto]

Signed-off-by: Nicola Di Lieto <nicola.dilieto@gmail.com>
---
 package/uacme/Config.in  | 34 ++++++++++++++++++++++++++++++++++
 package/uacme/uacme.hash |  3 +++
 package/uacme/uacme.mk   | 29 +++++++++++++++++++++++++++++
 3 files changed, 66 insertions(+)
 create mode 100644 package/uacme/Config.in
 create mode 100644 package/uacme/uacme.hash
 create mode 100644 package/uacme/uacme.mk

diff --git a/package/uacme/Config.in b/package/uacme/Config.in
new file mode 100644
index 0000000..a84c4a6
--- /dev/null
+++ b/package/uacme/Config.in
@@ -0,0 +1,34 @@
+config BR2_PACKAGE_UACME
+	bool "uacme"
+	depends on BR2_USE_MMU # fork()
+	select BR2_PACKAGE_LIBCURL
+	select BR2_PACKAGE_LIBCURL_TLS_SUPPORT
+	help
+	  uacme is a client for the ACMEv2 protocol described in
+	  RFC8555, written in plain C code with minimal dependencies
+	  (libcurl and GnuTLS or mbedTLS). The ACMEv2 protocol allows
+	  a Certificate Authority (https://letsencrypt.org is a
+	  popular one) and an applicant to automate the process of
+	  verification and certificate issuance.
+
+	  https://github.com/ndilieto/uacme
+
+if BR2_PACKAGE_UACME
+
+choice
+	prompt "SSL/TLS library to use"
+
+config BR2_PACKAGE_UACME_GNUTLS
+	bool "GnuTLS"
+	depends on BR2_PACKAGE_GNUTLS
+
+config BR2_PACKAGE_UACME_MBEDTLS
+	bool "mbed TLS"
+	depends on BR2_PACKAGE_MBEDTLS
+
+endchoice
+
+comment "uacme needs either GnuTLS or mbedTLS"
+	depends on !BR2_PACKAGE_GNUTLS && !BR2_PACKAGE_UACME_MBEDTLS
+
+endif
diff --git a/package/uacme/uacme.hash b/package/uacme/uacme.hash
new file mode 100644
index 0000000..d0e9fbf
--- /dev/null
+++ b/package/uacme/uacme.hash
@@ -0,0 +1,3 @@
+# Locally computed:
+sha256	7de471fc4ce5bcc7071628a2d1e992fc6e30272909d4ce18b93e036eb48dfa5e	uacme-1.0.7.tar.gz
+sha256	8ceb4b9ee5adedde47b31e975c1d90c73ad27b6b165a1dcd80c7c545eb65b903	COPYING
diff --git a/package/uacme/uacme.mk b/package/uacme/uacme.mk
new file mode 100644
index 0000000..494656e
--- /dev/null
+++ b/package/uacme/uacme.mk
@@ -0,0 +1,29 @@
+################################################################################
+#
+# uacme
+#
+################################################################################
+
+UACME_VERSION = upstream/1.0.7
+UACME_SOURCE = uacme-1.0.7.tar.gz
+UACME_SITE = git://github.com/ndilieto/uacme
+UACME_LICENSE = GPL-3.0+
+UACME_LICENSE_FILES = COPYING
+
+UACME_CONF_OPTS = --disable-maintainer-mode --disable-debug
+
+ifeq ($(BR2_PACKAGE_UACME_GNUTLS),y)
+UACME_CONF_OPTS += --with-gnutls=$(STAGING_DIR)/usr
+UACME_DEPENDENCIES += gnutls
+else
+UACME_CONF_OPTS += --without-gnutls
+endif
+
+ifeq ($(BR2_PACKAGE_UACME_MBEDTLS),y)
+UACME_CONF_OPTS += --with-mbedtls=$(STAGING_DIR)/usr
+UACME_DEPENDENCIES += mbedtls
+else
+UACME_CONF_OPTS += --without-mbedtls
+endif
+
+$(eval $(autotools-package))
-- 
2.1.4

[Buildroot] [PATCH 1/1] New package: https://github.com/ndilieto/uacme

[Nicola Di Lieto]

My finger was faster than my brain...

I forgot to add Config.in and DEVELOPERS changes.

Here is a new version of the patch. Ignore the first one please

Signed-off-by: Nicola Di Lieto <nicola.dilieto@gmail.com>
---
 DEVELOPERS               |  3 +++
 package/Config.in        |  1 +
 package/uacme/Config.in  | 34 ++++++++++++++++++++++++++++++++++
 package/uacme/uacme.hash |  3 +++
 package/uacme/uacme.mk   | 29 +++++++++++++++++++++++++++++
 5 files changed, 70 insertions(+)
 create mode 100644 package/uacme/Config.in
 create mode 100644 package/uacme/uacme.hash
 create mode 100644 package/uacme/uacme.mk

diff --git a/DEVELOPERS b/DEVELOPERS
index f83198b..e3e0b0d 100644
--- a/DEVELOPERS
+++ b/DEVELOPERS
@@ -1650,6 +1650,9 @@ N:	Naumann Andreas <ANaumann@ultratronik.de>
 F:	package/evemu/
 F:	package/libevdev/
 
+N:	Nicola Di Lieto <nicola.dilieto@gmail.com>
+F:	package/uacme/
+
 N:	Nicolas Cavallari <nicolas.cavallari@green-communications.fr>
 F:	package/libgit2/
 
diff --git a/package/Config.in b/package/Config.in
index f592e74..da30283 100644
--- a/package/Config.in
+++ b/package/Config.in
@@ -2054,6 +2054,7 @@ endif
 	source "package/transmission/Config.in"
 	source "package/tunctl/Config.in"
 	source "package/tvheadend/Config.in"
+	source "package/uacme/Config.in"
 	source "package/udpcast/Config.in"
 	source "package/uftp/Config.in"
 	source "package/uhttpd/Config.in"
diff --git a/package/uacme/Config.in b/package/uacme/Config.in
new file mode 100644
index 0000000..a84c4a6
--- /dev/null
+++ b/package/uacme/Config.in
@@ -0,0 +1,34 @@
+config BR2_PACKAGE_UACME
+	bool "uacme"
+	depends on BR2_USE_MMU # fork()
+	select BR2_PACKAGE_LIBCURL
+	select BR2_PACKAGE_LIBCURL_TLS_SUPPORT
+	help
+	  uacme is a client for the ACMEv2 protocol described in
+	  RFC8555, written in plain C code with minimal dependencies
+	  (libcurl and GnuTLS or mbedTLS). The ACMEv2 protocol allows
+	  a Certificate Authority (https://letsencrypt.org is a
+	  popular one) and an applicant to automate the process of
+	  verification and certificate issuance.
+
+	  https://github.com/ndilieto/uacme
+
+if BR2_PACKAGE_UACME
+
+choice
+	prompt "SSL/TLS library to use"
+
+config BR2_PACKAGE_UACME_GNUTLS
+	bool "GnuTLS"
+	depends on BR2_PACKAGE_GNUTLS
+
+config BR2_PACKAGE_UACME_MBEDTLS
+	bool "mbed TLS"
+	depends on BR2_PACKAGE_MBEDTLS
+
+endchoice
+
+comment "uacme needs either GnuTLS or mbedTLS"
+	depends on !BR2_PACKAGE_GNUTLS && !BR2_PACKAGE_UACME_MBEDTLS
+
+endif
diff --git a/package/uacme/uacme.hash b/package/uacme/uacme.hash
new file mode 100644
index 0000000..d0e9fbf
--- /dev/null
+++ b/package/uacme/uacme.hash
@@ -0,0 +1,3 @@
+# Locally computed:
+sha256	7de471fc4ce5bcc7071628a2d1e992fc6e30272909d4ce18b93e036eb48dfa5e	uacme-1.0.7.tar.gz
+sha256	8ceb4b9ee5adedde47b31e975c1d90c73ad27b6b165a1dcd80c7c545eb65b903	COPYING
diff --git a/package/uacme/uacme.mk b/package/uacme/uacme.mk
new file mode 100644
index 0000000..494656e
--- /dev/null
+++ b/package/uacme/uacme.mk
@@ -0,0 +1,29 @@
+################################################################################
+#
+# uacme
+#
+################################################################################
+
+UACME_VERSION = upstream/1.0.7
+UACME_SOURCE = uacme-1.0.7.tar.gz
+UACME_SITE = git://github.com/ndilieto/uacme
+UACME_LICENSE = GPL-3.0+
+UACME_LICENSE_FILES = COPYING
+
+UACME_CONF_OPTS = --disable-maintainer-mode --disable-debug
+
+ifeq ($(BR2_PACKAGE_UACME_GNUTLS),y)
+UACME_CONF_OPTS += --with-gnutls=$(STAGING_DIR)/usr
+UACME_DEPENDENCIES += gnutls
+else
+UACME_CONF_OPTS += --without-gnutls
+endif
+
+ifeq ($(BR2_PACKAGE_UACME_MBEDTLS),y)
+UACME_CONF_OPTS += --with-mbedtls=$(STAGING_DIR)/usr
+UACME_DEPENDENCIES += mbedtls
+else
+UACME_CONF_OPTS += --without-mbedtls
+endif
+
+$(eval $(autotools-package))
-- 
2.1.4

[Buildroot] [PATCH 1/1] New package: https://github.com/ndilieto/uacme

[Arnout Vandecappelle]

 Hi Nicola,

 Thank you for your submission. It doesn't completely follow the Buildroot
coding style, so I have some comments.

 First of all, the subject line of a new package is normally:

package/uacme: new package

On 29/04/2019 22:55, Nicola Di Lieto wrote:
> Signed-off-by: Nicola Di Lieto <nicola.dilieto@gmail.com>
> ---
> package/uacme/Config.in? | 34 ++++++++++++++++++++++++++++++++++
> package/uacme/uacme.hash |? 3 +++
> package/uacme/uacme.mk?? | 29 +++++++++++++++++++++++++++++

 Please also add yourself to the DEVELOPERS file. This way, you'll be put in Cc
when somebody makes modifications to the package, and you'll also get a message
when the package fails to build in the autobuilders.

 You are not adding the package to package/Config.in, so it can't be selected.
You have to choose in which menu to put it - probably the networking tools menu
is most appropriate. Make sure things stays alphabetically ordered.

> 3 files changed, 66 insertions(+)
> create mode 100644 package/uacme/Config.in
> create mode 100644 package/uacme/uacme.hash
> create mode 100644 package/uacme/uacme.mk
> 
> diff --git a/package/uacme/Config.in b/package/uacme/Config.in
> new file mode 100644
> index 0000000..a84c4a6
> --- /dev/null
> +++ b/package/uacme/Config.in
> @@ -0,0 +1,34 @@
> +config BR2_PACKAGE_UACME
> +??? bool "uacme"

 Indentation is one tab in Config.in. Please use the utils/check-package script
to verify all files you add.

> +??? depends on BR2_USE_MMU # fork()
> +??? select BR2_PACKAGE_LIBCURL
> +??? select BR2_PACKAGE_LIBCURL_TLS_SUPPORT

 You cannot select that option. Well, you can, but you shouldn't :-). It's
supposed to indicate if any package was selected that curl can use to implement
TLS support, so you're only supposed to depend on it, not select it.

 However, since you require either gnutls or mbedtls, I think you should do
something like:

	select BR2_PACKAGE_MBEDTLS if !BR2_PACKAGE_GNUTLS

 Note that this type of conditional select is tricky. If some other package does

	select BR2_PACKAGE_GNUTLS if !BR2_PACKAGE_MBEDTLS

you can get a circular dependency. Fortunately, no other package is doing
anything like that, so you can choose which one of those two you prefer. But
since gnutls has additional dependencies (!static and wchar), it's better to
prefer mbedtls.


> +??? help
> +????? uacme is a client for the ACMEv2 protocol described in
> +????? RFC8555, written in plain C code with minimal dependencies
> +????? (libcurl and GnuTLS or mbedTLS). The ACMEv2 protocol allows
> +????? a Certificate Authority (https://letsencrypt.org is a
> +????? popular one) and an applicant to automate the process of
> +????? verification and certificate issuance.
> +
> +????? https://github.com/ndilieto/uacme
> +
> +if BR2_PACKAGE_UACME
> +
> +choice
> +??? prompt "SSL/TLS library to use"
> +
> +config BR2_PACKAGE_UACME_GNUTLS
> +??? bool "GnuTLS"
> +??? depends on BR2_PACKAGE_GNUTLS

 We generally don't give a choice for something like that, but choose something
automatically.

> +
> +config BR2_PACKAGE_UACME_MBEDTLS
> +??? bool "mbed TLS"
> +??? depends on BR2_PACKAGE_MBEDTLS
> +
> +endchoice
> +
> +comment "uacme needs either GnuTLS or mbedTLS"
> +??? depends on !BR2_PACKAGE_GNUTLS && !BR2_PACKAGE_UACME_MBEDTLS

 With the conditional select I proposed this is not needed.

> +
> +endif
> diff --git a/package/uacme/uacme.hash b/package/uacme/uacme.hash
> new file mode 100644
> index 0000000..d0e9fbf
> --- /dev/null
> +++ b/package/uacme/uacme.hash
> @@ -0,0 +1,3 @@
> +# Locally computed:
> +sha256??? 7de471fc4ce5bcc7071628a2d1e992fc6e30272909d4ce18b93e036eb48dfa5e???
> uacme-1.0.7.tar.gz
> +sha256??? 8ceb4b9ee5adedde47b31e975c1d90c73ad27b6b165a1dcd80c7c545eb65b903???
> COPYING
> diff --git a/package/uacme/uacme.mk b/package/uacme/uacme.mk
> new file mode 100644
> index 0000000..494656e
> --- /dev/null
> +++ b/package/uacme/uacme.mk
> @@ -0,0 +1,29 @@
> +################################################################################
> +#
> +# uacme
> +#
> +################################################################################
> +
> +UACME_VERSION = upstream/1.0.7

 I don't understand what these upstream/ tags are, but v1.0.7 seems equally
appropriate to me.

> +UACME_SOURCE = uacme-1.0.7.tar.gz
> +UACME_SITE = git://github.com/ndilieto/uacme

 We prefer http over git to access git repositories. But for github, we prefer
to use either an uploaded tarball (not available in this case, except if we
would use the debian tarball) or the github helper. Thus, it would become
something like:

UACME_VERSION = 1.0.7
UACME_SITE = $(call github,ndilieto,uacme,upstream/$(UACME_VERSION))

or

UACME_SITE = $(call github,ndilieto,uacme,v$(UACME_VERSION))

 Note that UACME_SOURCE doesn't need to be set, it is set automatically to the
correct value (uacme-$(UACME_VERSION).tar.gz).

> +UACME_LICENSE = GPL-3.0+
> +UACME_LICENSE_FILES = COPYING
> +
> +UACME_CONF_OPTS = --disable-maintainer-mode --disable-debug

 I don't think these are needed, are they?

> +
> +ifeq ($(BR2_PACKAGE_UACME_GNUTLS),y)

 So here (assuming mbedtls is preferred), you'd have:

ifeq ($(BR2_PACKAGE_MBEDTLS),y)
...
else
# Must have gnutls selected
...
endif

 Note that the preference here does not need to correspond to the Config.in. In
other words, if the user has selected both gnutls and mbedtls, you can choose
which of the two should be used for uacme.

> +UACME_CONF_OPTS += --with-gnutls=$(STAGING_DIR)/usr

 Normally, --with-gnutls (without argument) should be enough. It is even
preferred, because then pkg-config will be used and it will set LIBS correctly.


 Regards,
 Arnout

> +UACME_DEPENDENCIES += gnutls
> +else
> +UACME_CONF_OPTS += --without-gnutls
> +endif
> +
> +ifeq ($(BR2_PACKAGE_UACME_MBEDTLS),y)
> +UACME_CONF_OPTS += --with-mbedtls=$(STAGING_DIR)/usr
> +UACME_DEPENDENCIES += mbedtls
> +else
> +UACME_CONF_OPTS += --without-mbedtls
> +endif
> +
> +$(eval $(autotools-package))

[Buildroot] [PATCH 1/1] New package: https://github.com/ndilieto/uacme

[Arnout Vandecappelle]

On 29/04/2019 23:37, Nicola Di Lieto wrote:
> My finger was faster than my brain...
> 
> I forgot to add Config.in and DEVELOPERS changes.

 So you noticed that :-)

 The rest of my review still stands, of course.

 Also, when you send an update, please:

1. add the version in the subject line (e.g. with 'git format-patch -v2')

2. Make sure the commit message is still relevant for posterity (i.e. no random
remarks like the above - those should go below the --- line).

3. Include a patch changelog [1] below a --- line below your Sob.

 Regards,
 Arnout

[1] https://buildroot.org/downloads/manual/manual.html#_patch_revision_changelog

[Buildroot] [PATCH 1/1] New package: https://github.com/ndilieto/uacme

[Arnout Vandecappelle]

On 30/04/2019 00:03, Arnout Vandecappelle wrote:
> 
> 
> On 29/04/2019 23:37, Nicola Di Lieto wrote:
>> My finger was faster than my brain...
>>
>> I forgot to add Config.in and DEVELOPERS changes.
> 
>  So you noticed that :-)
> 
>  The rest of my review still stands, of course.
> 
>  Also, when you send an update, please:
> 
> 1. add the version in the subject line (e.g. with 'git format-patch -v2')
> 
> 2. Make sure the commit message is still relevant for posterity (i.e. no random
> remarks like the above - those should go below the --- line).
> 
> 3. Include a patch changelog [1] below a --- line below your Sob.

 Also, this second iteration didn't get picked up by patchwork [2]. Please use
git send-email to make sure the patch is sent correctly.

 Regards,
 Arnout


> [1] https://buildroot.org/downloads/manual/manual.html#_patch_revision_changelog

[2] http://patchwork.ozlabs.org/project/buildroot/list

[Buildroot] [PATCH 1/1] New package: https://github.com/ndilieto/uacme

[Nicola Di Lieto]

On Mon, Apr 29, 2019 at 11:59:30PM +0200, Arnout Vandecappelle wrote:
> Hi Nicola,
>
> Thank you for your submission. It doesn't completely follow the Buildroot
>coding style, so I have some comments.
>
> First of all, the subject line of a new package is normally:
>
>package/uacme: new package

Ok, thanks and apologies for deviating from the recommended style.

>
> Please also add yourself to the DEVELOPERS file. This way, you'll be put in Cc
>when somebody makes modifications to the package, and you'll also get a message
>when the package fails to build in the autobuilders.
>
> You are not adding the package to package/Config.in, so it can't be selected.
>You have to choose in which menu to put it - probably the networking tools menu
>is most appropriate. Make sure things stays alphabetically ordered.
>

Yes, as you saw in my other message I realized these mistakes.

> Indentation is one tab in Config.in. Please use the 
> utils/check-package script to verify all files you add.

Will do

>> +??? depends on BR2_USE_MMU # fork()
>> +??? select BR2_PACKAGE_LIBCURL
>> +??? select BR2_PACKAGE_LIBCURL_TLS_SUPPORT
>
> You cannot select that option. Well, you can, but you shouldn't :-). It's
>supposed to indicate if any package was selected that curl can use to implement
>TLS support, so you're only supposed to depend on it, not select it.

The ACME protocol requires TLS and it won't speak normal HTTP.  If TLS 
support is not enabled, the software  will still build and link without 
issues, then fail at runtime. I tried depending at first to prevent this 
scenario but then uacme does not appear in the list of configurable 
packages at all until libcurl TLS support is enabled.

I saw several packages selecting libcurl instead of depending on it (for 
example collectd, gstreamer, libupnpp, kodi and others), so I assumed it 
would be OK to do the same and also selecting LIBCURL_TLS_SUPPORT. I see 
your point but I also fail to understand why it is allowed to select 
LIBCURL but not LIBCURL_TLS_SUPPORT. Can you elaborate please?

>
> However, since you require either gnutls or mbedtls, I think you should do
>something like:
>
>	select BR2_PACKAGE_MBEDTLS if !BR2_PACKAGE_GNUTLS
>
> Note that this type of conditional select is tricky. If some other package does
>
>	select BR2_PACKAGE_GNUTLS if !BR2_PACKAGE_MBEDTLS
>
>you can get a circular dependency. Fortunately, no other package is doing
>anything like that, so you can choose which one of those two you prefer. But
>since gnutls has additional dependencies (!static and wchar), it's better to
>prefer mbedtls.
>
>
>> +if BR2_PACKAGE_UACME
>> +
>> +choice
>> +??? prompt "SSL/TLS library to use"
>> +
>> +config BR2_PACKAGE_UACME_GNUTLS
>> +??? bool "GnuTLS"
>> +??? depends on BR2_PACKAGE_GNUTLS
>
> We generally don't give a choice for something like that, but choose something
>automatically.
>

I actually adopted the same TLS library selection mechanism that libcurl 
itself uses in its own Config.in. It seems to work quite well: the 
correct choice is automatically forced if only one of the libraries is 
available.  If none are available the comment shows up. Only when both 
libraries are available, can the user choose. I see your point but 
again, I fail to understand why it is OK for libcurl to give a choice 
and and not for others.  Also important in my opinion, libcurls' 
approach does not risk circular dependencies. Can you please let me know 
what you think?

>> +
>> +UACME_VERSION = upstream/1.0.7
>
> I don't understand what these upstream/ tags are, but v1.0.7 seems equally
>appropriate to me.
>

They are Debian's recommended DEP14 git branch layout

https://dep-team.pages.debian.net/deps/dep14/

I use it to avoid having to maintain a separate repository for debian 
packaging, and so that I can develop on master and automatically merge 
and release using debian's git-buildpackage tools, which rely on 
pristine-tar to efficiently store release tarballs using deltas.

>> +UACME_SOURCE = uacme-1.0.7.tar.gz
>> +UACME_SITE = git://github.com/ndilieto/uacme
>
> We prefer http over git to access git repositories. But for github, we prefer
>to use either an uploaded tarball (not available in this case, except if we
>would use the debian tarball) or the github helper. Thus, it would become
>something like:
>
>UACME_VERSION = 1.0.7
>UACME_SITE = $(call github,ndilieto,uacme,upstream/$(UACME_VERSION))
>

I tried $(call github...) but it fails because the actual released code 
is in the upstream/latest branch, tagged as upstream/1.0.7 which 
includes a slash.  The slash gets converted to an underscore and the 
github helper fails.  It's not OK to use v1.0.7 from master because 
uacme's build system uses the .tarball-version file to correctly 
generate the VERSION string for a proper release, and that file is only 
included in the release tags like upstream/1.0.7 (these tags are the 
actual contents of the tarball generated by 'make dist')

>
> Note that UACME_SOURCE doesn't need to be set, it is set automatically to the
>correct value (uacme-$(UACME_VERSION).tar.gz).
>

Yes I tried without but then the tar file name generated from the git 
tree ended up being uacme-upstream_1.0.7.tar.gz which I did not 
particularly like.  Of course it's not important, so I'll remove 
UACME_SOURCE as you suggest.

>> +UACME_LICENSE = GPL-3.0+
>> +UACME_LICENSE_FILES = COPYING
>> +
>> +UACME_CONF_OPTS = --disable-maintainer-mode --disable-debug
>
> I don't think these are needed, are they?
>

The configure and build system of uacme uses some logic (look at 
GNUmakefile, configure.ac, Makefile.am and build-aux/git-version-gen) to 
determine whether it needs regenerating the configure script/version tag 
and also whether to enable debug by default. That logic is only really 
useful to me, the developer.  Those two flags are effectively overriding 
the logic to positively ensure that a) no debug is enabled and b) the 
configure script is never autoreconf'd, which is important to preserve 
the release's VERSION string. See 

https://www.gnu.org/software/automake/manual/html_node/maintainer_002dmode.html

>
> Note that the preference here does not need to correspond to the Config.in. In
>other words, if the user has selected both gnutls and mbedtls, you can choose
>which of the two should be used for uacme.
>
>> +UACME_CONF_OPTS += --with-gnutls=$(STAGING_DIR)/usr
>
> Normally, --with-gnutls (without argument) should be enough. It is even
>preferred, because then pkg-config will be used and it will set LIBS correctly.
>

Yes, you're right, I'll change this.

Please, let me know what you think about the other points, so I can make 
a revised patch that complies with all requirements.

Regards
Nicola

[Buildroot] [PATCH 1/1] New package: https://github.com/ndilieto/uacme

[Arnout Vandecappelle]

 Hi Nicola,

On 30/04/2019 02:15, Nicola Di Lieto wrote:
> On Mon, Apr 29, 2019 at 11:59:30PM +0200, Arnout Vandecappelle wrote:
[snip]
>>> +??? select BR2_PACKAGE_LIBCURL_TLS_SUPPORT
>>
>> You cannot select that option. Well, you can, but you shouldn't :-). It's
>> supposed to indicate if any package was selected that curl can use to implement
>> TLS support, so you're only supposed to depend on it, not select it.
> 
> The ACME protocol requires TLS and it won't speak normal HTTP.? If TLS support
> is not enabled, the software? will still build and link without issues, then
> fail at runtime.

 Is that really the case? You need an actual TLS library to be able to generate
the CSR, right? Obviously, you *also* need TLS support in libcurl otherwise you
won't be able to contact the ACME server. But without gnutls or mbedtls, uacme
will simply not build, AFAICS.

> I tried depending at first to prevent this scenario but then
> uacme does not appear in the list of configurable packages at all until libcurl
> TLS support is enabled.
> 
> I saw several packages selecting libcurl instead of depending on it (for example
> collectd, gstreamer, libupnpp, kodi and others), so I assumed it would be OK to
> do the same and also selecting LIBCURL_TLS_SUPPORT. I see your point but I also
> fail to understand why it is allowed to select LIBCURL but not
> LIBCURL_TLS_SUPPORT. Can you elaborate please?

 `select LIBCURL_TLS_SUPPORT` does not actually enable TLS support, it just
tells libcurl that TLS is enabled. So by doing that select without also
selecting a TLS library, you're going to pretend that TLS is available while it
is not.

 Just try with an empty configuration and only enable the uacme package. You'll
see two things:

* the uacme TLS choice has no options;

* the libcurl TLS choice is visible, but also has no options.

 Note that LIBCURL_TLS_SUPPORT doesn't do anything except to control if
libcurl's TLS choice menu is visible.


 That's why you have to do the below, instead, to actually select a TLS library.
Note that by doing this, you'll also implicitly make sure the
LIBCURL_TLS_SUPPORT is available, and that libcurl is built with some TLS library.

>> However, since you require either gnutls or mbedtls, I think you should do
>> something like:
>>
>> ????select BR2_PACKAGE_MBEDTLS if !BR2_PACKAGE_GNUTLS
>>
>> Note that this type of conditional select is tricky. If some other package does
>>
>> ????select BR2_PACKAGE_GNUTLS if !BR2_PACKAGE_MBEDTLS
>>
>> you can get a circular dependency. Fortunately, no other package is doing
>> anything like that, so you can choose which one of those two you prefer. But
>> since gnutls has additional dependencies (!static and wchar), it's better to
>> prefer mbedtls.
>>
>>
>>> +if BR2_PACKAGE_UACME
>>> +
>>> +choice
>>> +??? prompt "SSL/TLS library to use"
>>> +
>>> +config BR2_PACKAGE_UACME_GNUTLS
>>> +??? bool "GnuTLS"
>>> +??? depends on BR2_PACKAGE_GNUTLS
>>
>> We generally don't give a choice for something like that, but choose something
>> automatically.
>>
> 
> I actually adopted the same TLS library selection mechanism that libcurl itself
> uses in its own Config.in.

 That is a special case. See the commit message of b8b78e7e6a1c for the
reasoning behind this. The choice is only relevant in case the user has enabled
more than one TLS library. Is there any reason for the user to prefer that uacme
uses mbedtls rather than gnutls if both are available?

> It seems to work quite well: the correct choice is
> automatically forced if only one of the libraries is available.? If none are
> available the comment shows up.

 In your case, the choice would still come up, but without any options. Also, it
would still be possible to enable the uacme package, but it would fail to build
for lack of TLS library.

> Only when both libraries are available, can the
> user choose. I see your point but again, I fail to understand why it is OK for
> libcurl to give a choice and and not for others.? Also important in my opinion,
> libcurls' approach does not risk circular dependencies.

 In libcurl's case, the TLS library is completely optional - it can be built
without TLS. In uacme's case, the TLS library is required. Therefore, the uacme
package *must* either select or depend on a TLS library. We prefer selecting it,
because that makes life easier for the user.

> Can you please let me
> know what you think?
> 
>>> +
>>> +UACME_VERSION = upstream/1.0.7
>>
>> I don't understand what these upstream/ tags are, but v1.0.7 seems equally
>> appropriate to me.
>>
> 
> They are Debian's recommended DEP14 git branch layout
> 
> https://dep-team.pages.debian.net/deps/dep14/
> 
> I use it to avoid having to maintain a separate repository for debian packaging,
> and so that I can develop on master and automatically merge and release using
> debian's git-buildpackage tools, which rely on pristine-tar to efficiently store
> release tarballs using deltas.
> 
>>> +UACME_SOURCE = uacme-1.0.7.tar.gz
>>> +UACME_SITE = git://github.com/ndilieto/uacme
>>
>> We prefer http over git to access git repositories. But for github, we prefer
>> to use either an uploaded tarball (not available in this case, except if we
>> would use the debian tarball) or the github helper. Thus, it would become
>> something like:
>>
>> UACME_VERSION = 1.0.7
>> UACME_SITE = $(call github,ndilieto,uacme,upstream/$(UACME_VERSION))
>>
> 
> I tried $(call github...) but it fails because the actual released code is in
> the upstream/latest branch, tagged as upstream/1.0.7 which includes a slash.?
> The slash gets converted to an underscore and the github helper fails.

 It will not work if you set VERSION to upstream/1.0.7, but it *will* work if
you set version to 1.0.7 and include the upstream/ part in the github macro
call, like I did above. Trust me, I tried it :-)

>? It's not
> OK to use v1.0.7 from master because uacme's build system uses the
> .tarball-version file to correctly generate the VERSION string for a proper
> release, and that file is only included in the release tags like upstream/1.0.7
> (these tags are the actual contents of the tarball generated by 'make dist')

 Could you include this explanation either as a comment or in the commit message?

> 
>>
>> Note that UACME_SOURCE doesn't need to be set, it is set automatically to the
>> correct value (uacme-$(UACME_VERSION).tar.gz).
>>
> 
> Yes I tried without but then the tar file name generated from the git tree ended
> up being uacme-upstream_1.0.7.tar.gz which I did not particularly like.? Of
> course it's not important, so I'll remove UACME_SOURCE as you suggest.
> 
>>> +UACME_LICENSE = GPL-3.0+
>>> +UACME_LICENSE_FILES = COPYING
>>> +
>>> +UACME_CONF_OPTS = --disable-maintainer-mode --disable-debug
>>
>> I don't think these are needed, are they?
>>
> 
> The configure and build system of uacme uses some logic (look at GNUmakefile,
> configure.ac, Makefile.am and build-aux/git-version-gen) to determine whether it
> needs regenerating the configure script/version tag and also whether to enable
> debug by default.

 I understand all that. I asked because normally these options default to off so
they don't need to be given. But indeed, here they default to on. I should have
checked :-)

 Regards,
 Arnout

> That logic is only really useful to me, the developer.? Those
> two flags are effectively overriding the logic to positively ensure that a) no
> debug is enabled and b) the configure script is never autoreconf'd, which is
> important to preserve the release's VERSION string. See
> https://www.gnu.org/software/automake/manual/html_node/maintainer_002dmode.html
> 
>>
>> Note that the preference here does not need to correspond to the Config.in. In
>> other words, if the user has selected both gnutls and mbedtls, you can choose
>> which of the two should be used for uacme.
>>
>>> +UACME_CONF_OPTS += --with-gnutls=$(STAGING_DIR)/usr
>>
>> Normally, --with-gnutls (without argument) should be enough. It is even
>> preferred, because then pkg-config will be used and it will set LIBS correctly.
>>
> 
> Yes, you're right, I'll change this.
> 
> Please, let me know what you think about the other points, so I can make a
> revised patch that complies with all requirements.
> 
> Regards
> Nicola
> 

[Buildroot] [PATCH 1/1] New package: https://github.com/ndilieto/uacme

[Nicola Di Lieto]

On Wed, May 01, 2019 at 12:49:53PM +0200, Arnout Vandecappelle wrote:
> Hi Nicola,

Hi Arnout,

many thanks for your time. I've implemented all your suggestions in a 
new version of the patch which I've posted separately. I hope I got all
correct this time.

Regards
Nicola