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

From: Thomas Petazzoni via buildroot <buildroot@buildroot.org>
To: prabhu sannachi via buildroot <buildroot@buildroot.org>
Cc: prabhu sannachi <prabhu.sannachi@collins.com>
Subject: Re: [Buildroot] [PATCH v1 1/1] package/redis-plus-plus: new package
Date: Fri, 14 Jul 2023 11:58:35 +0200	[thread overview]
Message-ID: <20230714115835.3d1f1b02@windsurf> (raw)
In-Reply-To: <20221007211932.481-1-prabhu.sannachi@collins.com>

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@buildroot.org> wrote:

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

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

> +N:      Prabhu Sannachi <prabhu.sannachi@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://gitlab.com/buildroot.org/buildroot/-/commit/c46df9de21e718a9ed6a2df2bb945c386695d3bf

Thanks a lot for your contribution!

Thomas
-- 
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