Re: [Buildroot] [PATCH v2 1/1] package/libdatachannel: new package

[Thomas Petazzoni via buildroot <buildroot@buildroot.org>]

Hello Maksim,

On Sat, 13 May 2023 18:50:30 +0300
Maksim Kiselev <bigunclemax@gmail.com> wrote:

> This patch adds a new package for libdatachannel.
> 
> libdatachannel is an open-source software library implementing WebRTC Data
> Channels, WebRTC Media Transport, and WebSockets. It is written in C++17
> and offers C bindings.
> 
> This library has a lot dependencies.
> Some of them, such as libsrtp and libnice, can be resolved with standalone
> buildroot packages if they enabled. Otherwise dependencies will be satisfy
> by git submodules. Nlohmann JSON dependency will be skipped at all because
> it is only required to build the examples.
> 
> Signed-off-by: Maksim Kiselev <bigunclemax@gmail.com>

Thanks for your patch, and sorry for the very slow feedback. See below
some comments. Could you take them into account, and send a v3 of this
patch?

> ---
>  package/Config.in                          |  1 +
>  package/libdatachannel/Config.in           | 10 +++++++
>  package/libdatachannel/libdatachannel.hash |  3 ++
>  package/libdatachannel/libdatachannel.mk   | 34 ++++++++++++++++++++++
>  4 files changed, 48 insertions(+)

Could you please add an entry to the DEVELOPERS file associating you to
this new package?

> diff --git a/package/libdatachannel/Config.in b/package/libdatachannel/Config.in
> new file mode 100644
> index 0000000000..103d933b50
> --- /dev/null
> +++ b/package/libdatachannel/Config.in
> @@ -0,0 +1,10 @@
> +config BR2_PACKAGE_LIBDATACHANNEL
> +	bool "libdatachannel"
> +	depends on BR2_TOOLCHAIN_GCC_AT_LEAST_9 # C++17
> +	depends on !BR2_STATIC_LIBS
> +	select BR2_PACKAGE_OPENSSL
> +	help
> +	  C/C++ WebRTC network library featuring Data Channels,
> +	  Media Transport, and WebSockets.
> +
> +	  https://github.com/paullouisageneau/libdatachannel

You need a Config.in comment to help the user with the dependencies:

comment "libdatachannel needs a toolchain w/ gcc >= 9, dynamic library"
	depends on !BR2_TOOLCHAIN_GCC_AT_LEAST_9 || BR2_STATIC_LIBS

> diff --git a/package/libdatachannel/libdatachannel.mk b/package/libdatachannel/libdatachannel.mk
> new file mode 100644
> index 0000000000..b8f25fc1c1
> --- /dev/null
> +++ b/package/libdatachannel/libdatachannel.mk
> @@ -0,0 +1,34 @@
> +################################################################################
> +#
> +# libdatachannel
> +#
> +################################################################################
> +
> +LIBDATACHANNEL_VERSION = v0.18.4
> +LIBDATACHANNEL_SITE_METHOD = git
> +LIBDATACHANNEL_SITE = https://github.com/paullouisageneau/libdatachannel
> +LIBDATACHANNEL_GIT_SUBMODULES = YES
> +LIBDATACHANNEL_INSTALL_STAGING = YES
> +LIBDATACHANNEL_LICENSE = MPL-2.0
> +LIBDATACHANNEL_LICENSE_FILES = LICENSE
> +
> +LIBDATACHANNEL_DEPENDENCIES = libopenssl

You should depend on "openssl", not "libopenssl". Indeed, your package
selects BR2_PACKAGE_OPENSSL, which means you need to use the "openssl"
virtual-package, whose implementation can either be libopenssl (the
original OpenSSL) or libressl.

> +
> +LIBDATACHANNEL_CONF_OPTS = -DNO_EXAMPLES=1 -DNO_TESTS=1 -DUSE_SYSTEM_JSON=0 \
> +	-DUSE_SYSTEM_JUICE=0 -DUSE_SYSTEM_PLOG=0 -DUSE_SYSTEM_USRSCTP=0

We really want to use the system version of all those libraries, by
having separate Buildroot packages for them. This will allow you to
drop LIBDATACHANNEL_GIT_SUBMODULES = YES, and generally have a nicer
packaging.

Could you have a look at doing this?

Thanks a lot!

Thomas Petazzoni
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot