Re: [Buildroot] [PATCH 1/4] package/uutils-coreutils: new package

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

Hello,

On Tue, 14 Mar 2023 13:15:03 +0100
Sebastian Weyer <sebastian.weyer@smile.fr> wrote:

> This package is an implementation of coreutils written completely in rust.
> 
> Signed-off-by: Sebastian Weyer <sebastian.weyer@smile.fr>

Thanks for working on this!


> diff --git a/package/uutils-coreutils/0001-GNUMakefile-remove-dependency-on-build-during-instal.patch b/package/uutils-coreutils/0001-GNUMakefile-remove-dependency-on-build-during-instal.patch
> new file mode 100644
> index 0000000000..0540d9becb
> --- /dev/null
> +++ b/package/uutils-coreutils/0001-GNUMakefile-remove-dependency-on-build-during-instal.patch
> @@ -0,0 +1,29 @@
> +From cbec6b8ee3c4418f8f04760678ec655f05e79220 Mon Sep 17 00:00:00 2001
> +From: Sebastian Weyer <sebastian.weyer@smile.fr>
> +Date: Fri, 10 Mar 2023 10:52:00 +0100
> +Subject: [PATCH] GNUMakefile: remove dependency on build during install
> +
> +When we call install we already built the application so we don't want
> +to have to rebuild
> +
> +Signed-off-by: Sebastian Weyer <sebastian.weyer@smile.fr>

It would be good to add the upstream status of this patch here. Was it
sent upstream? If so, a link?


> diff --git a/package/uutils-coreutils/Config.in b/package/uutils-coreutils/Config.in
> new file mode 100644
> index 0000000000..76ce5b174d
> --- /dev/null
> +++ b/package/uutils-coreutils/Config.in
> @@ -0,0 +1,9 @@
> +config BR2_PACKAGE_UUTILS_COREUTILS
> +	bool "uutils-coreutils"
> +	depends on BR2_PACKAGE_HOST_RUSTC_TARGET_ARCH_SUPPORTS
> +	depends on BR2_PACKAGE_BUSYBOX_SHOW_OTHERS
> +	help
> +	  uutils is an attempt at writing universal (as in
> +	  cross-platform) CLI utilities in Rust. While all programs have
> +	  been implemented, some options might be missing or different
> +	  behavior might be experienced.

We like to have a blank line followed by the URL of the project
homepage, at the end of the Config.in help text.

> diff --git a/package/uutils-coreutils/uutils-coreutils.hash b/package/uutils-coreutils/uutils-coreutils.hash
> new file mode 100644
> index 0000000000..b4855222fd
> --- /dev/null
> +++ b/package/uutils-coreutils/uutils-coreutils.hash
> @@ -0,0 +1,2 @@
> +#Locally generated

Nit: space after #

> +sha256  5bc773bcbc66851aa17979df44224c66c0b5323044c3c9cefb925b44ee9cd81b  uutils-coreutils-0.0.17.tar.gz

We also need a hash for the license file.

> diff --git a/package/uutils-coreutils/uutils-coreutils.mk b/package/uutils-coreutils/uutils-coreutils.mk
> new file mode 100644
> index 0000000000..37a647a1f0
> --- /dev/null
> +++ b/package/uutils-coreutils/uutils-coreutils.mk
> @@ -0,0 +1,23 @@
> +################################################################################
> +#
> +# uutils-coreutils
> +#
> +################################################################################
> +
> +UUTILS_COREUTILS_VERSION = 0.0.17
> +UUTILS_COREUTILS_SITE = $(call github,uutils,coreutils,$(UUTILS_COREUTILS_VERSION))
> +UUTILS_COREUTILS_LICENSE = MIT
> +UUTILS_COREUTILS_LICENSE_FILES = LICENSE

There is no dependency on host-rustc ? How can the rust compiler be
available prior to building this package then ?

> +UUTILS_COREUTILS_BUILD_OPTS = PROFILE=release MULTICALL=y PREFIX=$(TARGET_DIR)
> +UUTILS_COREUTILS_INSTALL_OPTS = $(UUTILS_COREUTILS_BUILD_OPTS) install

Is PREFIX= needed at build time ?

> +
> +define UUTILS_COREUTILS_BUILD_CMDS
> +	$(TARGET_CONFIGURE_OPTS) $(MAKE) $(UUTILS_COREUTILS_BUILD_OPTS) -C $(@D)
> +endef
> +
> +define UUTILS_COREUTILS_INSTALL_TARGET_CMDS
> +	$(TARGET_ENV) $(MAKE) $(UUTILS_COREUTILS_INSTALL_OPTS) -C $(@D)

I think we will prefer to not have BUILD_OPTS/INSTALL_OPTS but instead:

UUTILS_COREUTILS_MAKE_OPTS = \
	PROFILE=release \
	MULTICALL=y

define UUTILS_COREUTILS_BUILD_CMDS
	$(TARGET_MAKE_ENV) $(TARGET_CONFIGURE_OPTS) $(MAKE) \
		$(UUTILS_COREUTILS_MAKE_OPTS) -C $(@D)
endef

define UUTILS_COREUTILS_INSTALL_TARGET_CMDS
	$(TARGET_MAKE_ENV) $(MAKE) \
		$(UUTILS_COREUTILS_MAKE_OPTS) -C $(@D) \
		PREFIX=$(TARGET_DIR) install
endef

(assuming PREFIX= is only needed at install time)

It does the same, but the above suggestion is a bit more idiomatic in
Buildroot.

Best regards,

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