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

["Yann E. MORIN" <yann.morin.1998@free.fr>]

Maxim, All,

On 2023-04-26 19:14 +0300, Maksim Kiselev spake thusly:
> 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.
> 
> Signed-off-by: Maksim Kiselev <bigunclemax@gmail.com>

Thanks for this new package. Please see below for a quick review...

[--SNIP--]
> diff --git a/package/Config.in b/package/Config.in
> index eaac32a01a..c323409e37 100644
> --- a/package/Config.in
> +++ b/package/Config.in
> @@ -1408,6 +1408,7 @@ menu "Audio/Sound"
>  	source "package/libcodec2/Config.in"
>  	source "package/libcue/Config.in"
>  	source "package/libcuefile/Config.in"
> +	source "package/libdatachannel/Config.in"

I would have added that in the network category, rather than the
audio/sound one.

[--SNIP--]
> diff --git a/package/libdatachannel/libdatachannel.mk b/package/libdatachannel/libdatachannel.mk
> new file mode 100644
> index 0000000000..d86eed9bef
> --- /dev/null
> +++ b/package/libdatachannel/libdatachannel.mk
> @@ -0,0 +1,32 @@
> +################################################################################
> +#
> +# libdatachannel
> +#
> +################################################################################
> +
> +LIBDATACHANNEL_VERSION = v0.18.3
> +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_CONF_OPTS = -DPREFER_SYSTEM_LIB=1

I think I understand what this means, but please explain it the commit
log why we need to do so. Also, as the name if "prefer", what happens if
a system dependency is not present? Does it fallback to the bundled one?

So, it would be better to explicitly disable all options that use
bundled dependencies (e.g. juice, et al.).

Bulding tests is not necessary either, so they should be disable
(-DNO_TESTS=1)

> +ifeq ($(BR2_PACKAGE_JSON_FOR_MODERN_CPP),y)
> +LIBDATACHANNEL_CONF_OPTS += -DUSE_SYSTEM_JSON=1
> +LIBDATACHANNEL_DEPENDENCIES += json-for-modern-cpp
> +endif

Unlesss this is not supported by the package buildsystem, we want to
pass explicit disabling of options:

    ifeq ($(BR2_PACKAGE_JSON_FOR_MODERN_CPP),y)
    LIBDATACHANNEL_CONF_OPTS += -DUSE_SYSTEM_JSON=1
    LIBDATACHANNEL_DEPENDENCIES += json-for-modern-cpp
    else
    LIBDATACHANNEL_CONF_OPTS += -DUSE_SYSTEM_JSON=0
    endif

(and so on for the other two).

Care to fix and respin, please?

Regards,
Yann E. MORIN.

> +ifeq ($(BR2_PACKAGE_LIBNICE),y)
> +LIBDATACHANNEL_CONF_OPTS += -DUSE_NICE=1
> +LIBDATACHANNEL_DEPENDENCIES += libnice
> +endif
> +
> +ifeq ($(BR2_PACKAGE_LIBSRTP),y)
> +LIBDATACHANNEL_CONF_OPTS += -DUSE_SYSTEM_SRTP=1
> +LIBDATACHANNEL_DEPENDENCIES += libsrtp
> +endif
> +
> +$(eval $(cmake-package))
> -- 
> 2.39.2
> 
> _______________________________________________
> buildroot mailing list
> buildroot@buildroot.org
> https://lists.buildroot.org/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot