[Buildroot] [PATCH 1/1] New package: https://github.com/ndilieto/uacme
Nicola Di Lieto
nicola.dilieto at gmail.com
Tue Apr 30 00:15:18 UTC 2019
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.
>> + 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
> 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
>> +if BR2_PACKAGE_UACME
>> + 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
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
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
>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
> 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.
More information about the buildroot