Re: [Buildroot] [PATCH v2 2/4] package/bats-support: add bats support library

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

Hello Brandon,

Thanks for the patch!

Commit title should be:

	package/bats-support: new package

More comments below.

On Thu, 25 Apr 2024 19:58:14 +0000
Brandon Maier via buildroot <buildroot@buildroot.org> wrote:

> This library provides support functions needed by the bats-assert and
> bats-file libraries.
> 
> This library does not provide an installer. Manually install the files
> under /usr/lib/bats/bats-support which is what the Arch Linux package
> does[1]. This makes the library loadable using `bats_load_library`[2].
> 
> [1] https://gitlab.archlinux.org/archlinux/packaging/packages/bats-support/-/blob/main/PKGBUILD?ref_type=heads
> [2] https://bats-core.readthedocs.io/en/stable/writing-tests.html#bats-load-library-load-system-wide-libraries
> 
> Signed-off-by: Brandon Maier <brandon.maier@collins.com>
> ---
> Changes v1 -> v2:
> - fix older versions of `install` that don't support -D

Hu? We use the -D option of install everywhere in Buildroot. How can
your build work if you have a version of install that doesn't support
-D ?
> +define BATS_SUPPORT_INSTALL_TARGET_CMDS
> +	install -d -m 0755 $(TARGET_DIR)/usr/lib/bats/bats-support/src
> +	install -m 0644 $(@D)/*.bash -t "$(TARGET_DIR)/usr/lib/bats/bats-support"
> +	install -m 0644 $(@D)/src/*.bash -t "$(TARGET_DIR)/usr/lib/bats/bats-support/src"

Use $(INSTALL) instead of install.

You can skip the double quotes around the file paths. We don't double
quotes the paths in any other Buildroot .mk file.

Also, -m 0644 is not consistent with how the files from bats-core are
installed:

$ ls -l output/target/usr/lib/bats-core/
total 68
-rwxr-xr-x. 1 thomas thomas  7988  1 mai   23:03 common.bash
-rwxr-xr-x. 1 thomas thomas  6058  1 mai   23:03 formatter.bash
-rwxr-xr-x. 1 thomas thomas   838  1 mai   23:03 preprocessing.bash
-rwxr-xr-x. 1 thomas thomas  3722  1 mai   23:03 semaphore.bash
-rwxr-xr-x. 1 thomas thomas 16483  1 mai   23:03 test_functions.bash
-rwxr-xr-x. 1 thomas thomas 14979  1 mai   23:03 tracing.bash
-rwxr-xr-x. 1 thomas thomas  1019  1 mai   23:03 validator.bash
-rwxr-xr-x. 1 thomas thomas  1900  1 mai   23:03 warnings.bash

They are installed 0755. Should we try to be consistent with this? Or
it doesn't make sense for a specific reason?

Also, while not mandatory at all, it would be nice to have small
runtime tests for these bats-* packages. Nothing fancy, just something
stupid/simple that exercises a bit those packages.

These comments apply in a completely identical way to the other 2
patches in this series.

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