[Buildroot] [External] Re: [PATCH v1 1/1] package/redis-plus-plus: new package

[Sannachi, Prabhu Collins]

Hi Thomas Petazzoni,

Thanks for your input. I will take care your review comments in future commit.

Regards,
Prabhu

-----Original Message-----
From: Thomas Petazzoni <thomas.petazzoni at bootlin.com>
Sent: Friday, July 14, 2023 4:59 AM
To: prabhu sannachi via buildroot <buildroot at buildroot.org>
Cc: Sannachi, Prabhu Collins <prabhu.sannachi at collins.com>
Subject: [External] Re: [Buildroot] [PATCH v1 1/1] package/redis-plus-plus: new package

Hello Prabhu,

After many months, I finally took the time to look at your patch and apply it. A number of changes where needed, see below.

On Fri,  7 Oct 2022 16:19:32 -0500
prabhu sannachi via buildroot <buildroot at buildroot.org> wrote:

> From: prabhusannachi <prabhu.sannachi at collins.com>

The "From:" should match the Signed-off-by line, so I fixed that up.

> +N:      Prabhu Sannachi <prabhu.sannachi at collins.com>
> +F:      package/redis-plus-plus/

Indentation between N:/F: and the value is one tab.

> diff --git a/package/redis-plus-plus/Config.in
> b/package/redis-plus-plus/Config.in
> new file mode 100644
> index 0000000..3c23dc5
> --- /dev/null
> +++ b/package/redis-plus-plus/Config.in
> @@ -0,0 +1,11 @@
> +config BR2_PACKAGE_REDIS_PLUS_PLUS
> +     bool "redis-plus-plus"
> +     select BR2_PACKAGE_HIREDIS

Missing depends on BR2_INSTALL_LIBSTDCPP, since this software is written in C++. And also the corresponding Config.in comment.

> +REDIS_PLUS_PLUS_VERSION =  1.3.5

Only one space after =

> +REDIS_PLUS_PLUS_SITE = $(call
> +github,sewenew,redis-plus-plus,$(REDIS_PLUS_PLUS_VERSION))
> +REDIS_PLUS_PLUS_LICENSE =  Apache-2.0 license

Only one space after =, just "Apache-2.0", without "license"
afterwards. We use SPDX license codes.

> +REDIS_PLUS_PLUS_LICENSE_FILES = LICENSE
> +REDIS_PLUS_PLUS_INSTALL_STAGING = YES REDIS_PLUS_PLUS_DEPENDENCIES +=
> +hiredis

Just = instead of +=.

> +# since version 1.3.0, by default, redis-plus-plus is built with the
> +-std=c++17 standard ifneq ($(BR2_TOOLCHAIN_GCC_AT_LEAST_8),y)

ifeq ($(BR2_TOOLCHAIN_GCC_AT_LEAST_8),)

is more commonly used in Buildroot.

> +  ifeq ($(BR2_TOOLCHAIN_GCC_AT_LEAST_5),y)
> +    REDIS_PLUS_PLUS_CONF_OPTS += -DREDIS_PLUS_PLUS_CXX_STANDARD=14
> +  else
> +    REDIS_PLUS_PLUS_CONF_OPTS += -DREDIS_PLUS_PLUS_CXX_STANDARD=11
> +  endif
> +endif

We don't typically indent such code blocks.

> +
> +ifneq ($(BR2_TOOLCHAIN_GCC_AT_LEAST_4_9),y)
> +  REDIS_PLUS_PLUS_CONF_OPTS += -DREDIS_PLUS_PLUS_BUILD_TEST=OFF endif

We disable tests unconditionally usually, so I've done that as well.

Final commit at:

  https://urldefense.com/v3/__https://gitlab.com/buildroot.org/buildroot/-/commit/c46df9de21e718a9ed6a2df2bb945c386695d3bf__;!!MvWE!Ctrbmd8RE2sbwEAMgJQIE4dlllBaphGkzTDc_NaL0tCtQ4Vdm43DiI3oDMCjCXSb87Qq625Ux-j_NfMaIj8H5iCTbEsqqmlh7w$

Thanks a lot for your contribution!

Thomas
--
Thomas Petazzoni, co-owner and CEO, Bootlin Embedded Linux and Kernel engineering and training https://urldefense.com/v3/__https://bootlin.com__;!!MvWE!Ctrbmd8RE2sbwEAMgJQIE4dlllBaphGkzTDc_NaL0tCtQ4Vdm43DiI3oDMCjCXSb87Qq625Ux-j_NfMaIj8H5iCTbEu9EopxSw$