Re: [Buildroot] [PATCH 1/1] package/mkbootimg: new host package

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

Hello,

Thanks for your contribution! See some comments below.

On Sun, 24 Jul 2022 16:58:18 -0700
Christian Stewart via buildroot <buildroot@buildroot.org> wrote:

> Android bootimg utilities: mkbootimg and unpackbootimg.
> 
> Signed-off-by: Christian Stewart <christian@paral.in>
> ---
>  package/Config.in.host                        |  1 +
>  ...akefile-remove-cc-and-ar-definitions.patch | 97 +++++++++++++++++++
>  package/mkbootimg/Config.in.host              |  4 +
>  package/mkbootimg/mkbootimg.hash              |  3 +
>  package/mkbootimg/mkbootimg.mk                | 26 +++++
>  5 files changed, 131 insertions(+)

Entry in the DEVELOPERS file is missing.

> diff --git a/package/mkbootimg/0001-makefile-remove-cc-and-ar-definitions.patch b/package/mkbootimg/0001-makefile-remove-cc-and-ar-definitions.patch
> new file mode 100644
> index 0000000000..d61b1e0b88
> --- /dev/null
> +++ b/package/mkbootimg/0001-makefile-remove-cc-and-ar-definitions.patch
> @@ -0,0 +1,97 @@
> +From 04a62922ce238f3106a916a30c0c2995ffa1526c Mon Sep 17 00:00:00 2001
> +From: Christian Stewart <christian@paral.in>
> +Date: Sat, 14 May 2022 20:58:14 -0700
> +Subject: [PATCH] makefile: remove cc and ar definitions
> +
> +Signed-off-by: Christian Stewart <christian@paral.in>
> +---
> + Makefile             | 26 +-------------------------
> + libmincrypt/Makefile | 27 +++++----------------------
> + 2 files changed, 6 insertions(+), 47 deletions(-)
> +
> +diff --git a/Makefile b/Makefile
> +index 6ee3a38c..39d0996b 100644
> +--- a/Makefile
> ++++ b/Makefile
> +@@ -1,29 +1,4 @@
> + # Andrew Huang <bluedrum@163.com>
> +-ifeq ($(CC),cc)
> +-CC = gcc
> +-endif
> +-AR = ar rcv
> +-ifeq ($(windir),)
> +-EXE =
> +-RM = rm -f
> +-else
> +-EXE = .exe
> +-RM = del
> +-endif
> +-
> +-CFLAGS = -ffunction-sections -O3
> +-
> +-ifneq (,$(findstring darwin,$(CROSS_COMPILE)))
> +-    UNAME_S := Darwin
> +-else
> +-    UNAME_S := $(shell uname -s)
> +-endif
> +-ifeq ($(UNAME_S),Darwin)
> +-    LDFLAGS += -Wl,-dead_strip
> +-else
> +-    LDFLAGS += -Wl,--gc-sections -s
> +-endif

This is not needed. They can be overridden on the command line, as long
as you pass them to the right side of make, i.e:

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

The CC, CFLAGS, LDFLAGS from HOST_CONFIGURE_OPTS will take precedence
over the ones defined in the Makefile.

> +-
> + all:mkbootimg$(EXE) unpackbootimg$(EXE)
> + 
> + static:
> +@@ -31,6 +6,7 @@ static:
> + 
> + libmincrypt.a:
> + 	$(MAKE) -C libmincrypt
> ++	cp ./libmincrypt/libmincrypt.a ./libmincrypt.a

This added line is not related to the patch.

> + 
> + mkbootimg$(EXE):mkbootimg.o libmincrypt.a
> + 	$(CROSS_COMPILE)$(CC) -o $@ $^ -L. -lmincrypt $(LDFLAGS)
> +diff --git a/libmincrypt/Makefile b/libmincrypt/Makefile
> +index ad482de9..c7c478ff 100755
> +--- a/libmincrypt/Makefile
> ++++ b/libmincrypt/Makefile
> +@@ -1,31 +1,14 @@
> +-ifeq ($(CC),cc)
> +-CC = gcc
> +-endif
> +-AR = ar rc
> +-ifeq ($(windir),)
> +-RM = rm -f
> +-CP = cp
> +-else
> +-RM = del
> +-CP = copy /y
> +-endif
> +-
> +-CFLAGS = -ffunction-sections -O3
> +-EXT = a
> +-LIB = libmincrypt.$(EXT)
> ++LIB = libmincrypt.a

Same comment. And EXT = a doesn't cause any problem.

> + LIB_OBJS = dsa_sig.o p256.o p256_ec.o p256_ecdsa.o rsa.o sha.o sha256.o
> + INC  = -I..
> + 
> +-all:$(LIB)
> ++all: $(LIB)

Spurious change.

> + 
> + clean:
> + 	$(RM) $(LIB_OBJS) $(LIB)
> + 
> +-$(LIB):$(LIB_OBJS)
> +-	$(CROSS_COMPILE)$(AR) $@ $^
> +-	$(CP) $@ ..
> +-
> ++$(LIB): $(LIB_OBJS)
> ++	$(CROSS_COMPILE)$(AR) rc $@ $^

Why?

> + 
> +-%.o:%.c
> ++%.o: %.c

Spurious change.

> diff --git a/package/mkbootimg/Config.in.host b/package/mkbootimg/Config.in.host
> new file mode 100644
> index 0000000000..2ca7bcae00
> --- /dev/null
> +++ b/package/mkbootimg/Config.in.host
> @@ -0,0 +1,4 @@
> +config BR2_PACKAGE_HOST_MKBOOTIMG
> +	bool "host mkbootimg"
> +	help
> +	  Android Bootimg mkbootimg and unpackbootimg utils.

Missing link to the upstream Github project.

> +define HOST_MKBOOTIMG_BUILD_CMDS
> +	$(foreach t,$(HOST_MKBOOTIMG_TARGETS),\
> +		$(HOST_MAKE_ENV) $(HOST_CONFIGURE_OPTS) $(MAKE) SRCDIR=$(@D) \

Are you sure SRCDIR is needed?

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