Re: [Buildroot] [RFC PATCH v1 1/2] package/ck: new package

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

Hello Sergey,

On Fri, 26 May 2023 00:51:02 +0300
Sergey Romanov via buildroot <buildroot@buildroot.org> wrote:

> Concurrency primitives, safe memory reclamation
> mechanisms and non-blocking data structures
> for the research, design and implementation
> of high performance concurrent systems.
> 
> https://github.com/concurrencykit/ck.git
> 
> Signed-off-by: Sergey Romanov <svromanov@sberdevices.ru>

Sorry for the very slow feedback. I wanted to push this, but it doesn't
build, and I don't see how it can build in a cross-compilation
environment. Indeed, the configure script does this:

$CC -o .1 .1.c
COMPILER=`./.1 2> /dev/null`
r=$?
rm -f .1.c .1

if test "$r" -ne 0; then
        assert "" "update compiler"
else
        echo "success [$CC]"
fi

So it builds a program called .1 with the cross-compiler, and then it
tries to run it. This obviously will never work in a cross-compilation
context.

Some more comments below (which I had addressed locally, but had to
give up because of the above issue)

> diff --git a/.checkpackageignore b/.checkpackageignore
> index f2dea0dfd9..1be4e79a2b 100644
> --- a/.checkpackageignore
> +++ b/.checkpackageignore
> @@ -286,6 +286,7 @@ package/cgroupfs-mount/S30cgroupfs Indent Shellcheck Variables
>  package/chipmunk/0001-Fix-build-failure-on-musl.patch Upstream
>  package/chocolate-doom/0001-Remove-redundant-demoextend-definition.patch Upstream
>  package/chrony/S49chrony Indent Shellcheck Variables
> +package/ck/0001-Avoid-buildroot-s-influence-on-LDFLAGS.patch Upstream

Please add a proper Upstream: tag instead of adding an exception. We
don't want to add new entries in .checkpackageignore, only remove
existing entries.

> diff --git a/package/ck/0001-Avoid-buildroot-s-influence-on-LDFLAGS.patch b/package/ck/0001-Avoid-buildroot-s-influence-on-LDFLAGS.patch
> new file mode 100644
> index 0000000000..7e2cc85ba6
> --- /dev/null
> +++ b/package/ck/0001-Avoid-buildroot-s-influence-on-LDFLAGS.patch
> @@ -0,0 +1,191 @@
> +From 0b30b184538c0de485fe292e2ebb6bfea3c255bc Mon Sep 17 00:00:00 2001
> +From: Sergey Romanov <svromanov@sberdevices.ru>
> +Date: Thu, 25 May 2023 19:01:28 +0300
> +Subject: [PATCH] Avoid buildroot's influence on LDFLAGS
> +
> +Upstream-status: Pending

Pending where? Could you please provide some link to a pull request?

Also, could you provide some more details on why this patch is needed.
Apparently, the configure script is designed to allow passing custom
LDFLAGS through the LDFLAGS variable, so why doesn't that just work?

> diff --git a/package/ck/Config.in b/package/ck/Config.in
> new file mode 100644
> index 0000000000..d88eaa8413
> --- /dev/null
> +++ b/package/ck/Config.in
> @@ -0,0 +1,23 @@
> +config BR2_PACKAGE_CK_ARCH_SUPPORTS
> +	bool
> +	default y if ((BR2_arm && !BR2_ARM_CPU_ARMV4 && !BR2_ARM_CPU_ARMV5 && \
> +		!BR2_ARM_CPU_ARMV7M) || BR2_aarch64 || \
> +		BR2_powerpc || BR2_powerpc64 || BR2_RISCV_64 || \
> +		BR2_s390x || BR2_sparc_v9 || BR2_x86 || BR2_x86_64)

BR2_x86 doesn't exist, it's BR2_i386. Also, please reformat as such:

config BR2_PACKAGE_CK_ARCH_SUPPORTS
        bool
        default y if BR2_arm && !BR2_ARM_CPU_ARMV4 && !BR2_ARM_CPU_ARMV5 && !BR2_ARM_CPU_ARMV7M
        default y if BR2_aarch64
        default y if BR2_i386
        default y if BR2_powerpc || BR2_powerpc64
        default y if BR2_RISCV_64
        default y if BR2_s390x
        default y if BR2_sparc_v9
        default y if BR2_x86_64

> +config BR2_PACKAGE_CK_TOOLCHAIN_SUPPORTS
> +	bool
> +	default y if ((BR2_TOOLCHAIN_USES_MUSL || BR2_TOOLCHAIN_USES_GLIBC) && \
> +		!BR2_TOOLCHAIN_USES_UCLIBC)

Please drop this.

> +
> +config BR2_PACKAGE_CK
> +	bool "concurrency kit"
> +	depends on BR2_PACKAGE_CK_ARCH_SUPPORTS
> +	depends on BR2_PACKAGE_CK_TOOLCHAIN_SUPPORTS

And use:

	# some comment here to explain the issue
	depends on !BR2_TOOLCHAIN_USES_UCLIBC

> +	help
> +	  Concurrency primitives, safe memory reclamation
> +	  mechanisms and non-blocking data structures
> +	  for the research, design and implementation
> +	  of high performance concurrent systems.
> +
> +	  https://github.com/concurrencykit/ck.git

And add:

comment "ck needs a toolchain w/ glibc or musl"
        depends on BR2_PACKAGE_CK_ARCH_SUPPORTS
        depends on BR2_TOOLCHAIN_USES_UCLIBC

> +CK_INSTALL_STAGING = YES
> +
> +CK_PROFILE_PARAMS += --platform=$(BR2_ARCH)
> +CK_PROFILE_PARAMS += --prefix="/usr"

Please use:

CK_CONF_OPTS = \
	--platform=$(BR2_ARCH) \
	--prefix="/usr"

> +
> +define CK_CONFIGURE_CMDS
> +	( cd $(@D); \
> +	$(TARGET_CONFIGURE_OPTS) \
> +	$(CK_PROFILE_CONF_ENV) \

This variable doesn't exist.

> +	./configure $(CK_PROFILE_PARAMS) )

Please simplify to:

        cd $(@D); \
                $(TARGET_CONFIGURE_OPTS) \
                ./configure $(CK_CONF_OPTS)


> +endef
> +
> +define CK_BUILD_CMDS
> +	$(TARGET_CONFIGURE_OPTS) \
> +	$(CK_PROFILE_CONF_ENV) \
> +	$(MAKE) -C $(@D)

If you can simplify to:

        $(TARGET_MAKE_ENV) $(MAKE) -C $(@D)

it would be great.

> +endef
> +
> +define CK_INSTALL_TARGET_CMDS
> +	$(TARGET_CONFIGURE_OPTS) \
> +	$(CK_PROFILE_CONF_ENV) \
> +	$(MAKE) -C $(@D) DESTDIR=$(TARGET_DIR) install

Simplify to:

        $(TARGET_MAKE_ENV) $(MAKE) -C $(@D) DESTDIR=$(TARGET_DIR) install

if possible.

> +endef
> +
> +define CK_INSTALL_STAGING_CMDS
> +	$(TARGET_CONFIGURE_OPTS) \
> +	$(CK_PROFILE_CONF_ENV) \
> +	$(MAKE) -C $(@D) DESTDIR=$(STAGING_DIR) install

Simplify to:

        $(TARGET_MAKE_ENV) $(MAKE) -C $(@D) DESTDIR=$(STAGING_DIR) install

if possible.

Thanks!

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