General rework for Yocto Project integration by otavio · Pull Request #2 · nxp-imx-support/nxp-imx-signer

otavio · 2023-10-12T22:14:59Z

The two commit messages provide information about the changes made in the code:

Fix include of headers so it indicates it uses the includedir:
- This commit fixes the incorrect inclusion of headers in the code. It updates the include statements to indicate that they use the includedir.
Improve Makefile so it is overridable and provides standardized targets:
- This commit improves the Makefile by reworking the targets. It provides a standardized set of targets and allows the overriding of variables. This is done to support cross-compiling properly. Additionally, it fixes an issue related to Issue Missing install target in Makefile #1.

This commit rework the Makefile targets so it provides a standardized set of targets and allow overriding of variables so cross compiling is properly supported. Fixes: nxp-imx-support#1. Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>

Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>

utkarshguptanxp · 2023-10-30T16:09:57Z

src/Makefile

-COPTS = -g -Wall -Werror
-CFLAGS = -I../inc/.
+PREFIX   ?= /usr/local
+BINDIR   ?= $(PREFIX)/bin


Currently my yocto scripts are pulling cst-singer binary and csf config files from home directory of cst-signer tool. I will not be able to apply this change in this release.

It is recommended to use /usr/local for manually built applications, and /usr for packaged tools. The home directory is not suitable for this purpose.

utkarshguptanxp · 2023-10-30T16:30:33Z

src/Makefile

-	$(CC) $(COPTS) $(CFLAGS) -o $@ $(SRCS)
+install: cst-signer
+	install -D -m 0755 cst-signer $(DESTDIR)/$(BINDIR)/cst-signer
+	install -D -m 0755 -t $(DESTDIR)$(DATADIR)/doc/cst-signer \


I do not understand the usage of doc folder here. maybe its something I am not familiar with possibly autotools related?

The .sample configuration files go to /usr/share/doc/ as

-rw-r--r-- root/root 230 2023-09-08 16:41 ./usr/share/doc/cst-signer/csf_ahab.cfg.sample -rw-r--r-- root/root 430 2023-09-08 16:41 ./usr/share/doc/cst-signer/csf_hab4.cfg.sample

for target packages. Here for example I generated:

[0] ~/src/oe/oel/master/build/tmp/deploy/ipk/cortexa53-crypto/cst-signer-dev_0.3+0+7fa50daf32-r0_cortexa53-crypto.ipk [0] [1] ~/src/oe/oel/master/build/tmp/deploy/ipk/cortexa53-crypto/cst-signer-doc_0.3+0+7fa50daf32-r0_cortexa53-crypto.ipk [1] [2] ~/src/oe/oel/master/build/tmp/deploy/ipk/cortexa53-crypto/cst-signer_0.3+0+7fa50daf32-r0_cortexa53-crypto.ipk [2] [3] ~/src/oe/oel/master/build/tmp/deploy/ipk/cortexa53-crypto/cst-signer-src_0.3+0+7fa50daf32-r0_cortexa53-crypto.ipk [3] [4] ~/src/oe/oel/master/build/tmp/deploy/ipk/cortexa53-crypto/cst-signer-dbg_0.3+0+7fa50daf32-r0_cortexa53-crypto.ipk [4]

I understand that you use those files as templates, but in this particular case, it should be done in the native recipe. In my recipe I did this as well.

utkarshguptanxp · 2023-10-30T16:35:07Z

src/Makefile


 clean:
-	rm -rf cst_signer fdt.o
+	rm -rf cst-signer *.o


Shouldnt DESTDIR be cleaned as well?

No, 'clean' is not an uninstall target.

utkarshguptanxp

The header files are surrounded by quotes as they are not system header files. Angle brackets should be ok here even though we are using include directory as the compiler should first search in local include directory for these files.

utkarshguptanxp · 2024-01-09T00:26:18Z

Hi Otavio, I am back to working on this project. We had a new release in Q4 '23 which has gone public.

Yocto Recipe: https://github.com/nxp-imx-support/meta-nxp-security-reference-design/tree/mickledore-6.1.55-2.2.0/meta-secure-boot
Yocto Manifest File: https://github.com/nxp-imx/imx-manifest/blob/imx-linux-mickledore/imx-6.1.55-2.2.0_security-reference-design.xml

otavio · 2024-01-11T13:47:37Z

Okay, I understand. Can you tell me what is left to be done before this PR can be merged?

utkarshguptanxp · 2024-01-11T15:55:23Z

Okay, I understand. Can you tell me what is left to be done before this PR can be merged?

Please see my previous review comments from last few days.

In addition I have made following changes to the Makefiles which are adapting your changes. But please also advice on the questions above.

Author: Utkarsh Gupta <utkarsh.gupta@nxp.com>
Date:   Tue Jan 9 09:28:43 2024 -0600

    MICRSE-2103: Improve Makefile so it is overridable and provides standardized targets
    
    This commit rework the Makefile targets so it provides a standardized set of
    targets and allow overriding of variables so cross compiling is properly
    supported.
    
    Reported-by: Otavio Salvador <otavio@ossystems.com.br>
    Signed-off-by: Utkarsh Gupta <utkarsh.gupta@nxp.com>

diff --git a/Makefile b/Makefile
index c799b13..552f2d4 100644
--- a/Makefile
+++ b/Makefile
@@ -1,5 +1,5 @@
 #==============================================================================
-#    Copyright 2022-2023 NXP
+#    Copyright 2022-2024 NXP
 #
 #    SPDX-License-Identifier: GPL-2.0-or-later
 #==============================================================================
@@ -10,7 +10,9 @@ SRC_DIR := src
 
 all:
        @$(MAKE) -C $(SRC_DIR)/
-       @mv $(SRC_DIR)/cst_signer .
+
+install:
+       @$(MAKE) -C src install
 
 clean:
-       @rm -rf cst_signer src/fdt.o
+       @$(MAKE) -C src clean
diff --git a/src/Makefile b/src/Makefile
index 5e6dade..a469a3c 100644
--- a/src/Makefile
+++ b/src/Makefile
@@ -1,26 +1,37 @@
 #==============================================================================
-#    Copyright 2022-2023 NXP
+#    Copyright 2022-2024 NXP
 #
 #    SPDX-License-Identifier: GPL-2.0-or-later
 #==============================================================================
 
-CC = gcc
+CC        ?= gcc
+CFLAGS    ?= -g -Wall -Werror
+CPPFLAGS  ?=
+LDFLAGS   ?=
+INCLUDES  = -I../inc/
 
-COPTS = -g -Wall -Werror
-CFLAGS = -I../inc/.
+PREFIX   ?= /usr/local
+BINDIR   ?= $(PREFIX)/bin
+DATADIR  ?= $(PREFIX)/share
 
-DEPS = cst_signer.h cfg_parser.h mkimage_helper.h
-SRCS = cst_signer.c cfg_parser.c mkimage_helper.c fdt.o
+#DEPS = cst_signer.h cfg_parser.h mkimage_helper.h
+SRCS = cst_signer.c cfg_parser.c mkimage_helper.c fdt.c
+OBJS = $(SRCS:.c=.o)
 
-.PHONY: all clean
+.PHONY: all install clean
 
-all: cst_signer fdt.o
+all: cst_signer
 
-fdt.o: fdt.c
-       $(CC) -c -w -o $@ $< $(CFLAGS)
+%.o: %.c
+       $(CC) -c $(INCLUDES) $(CFLAGS) $(LDFLAGS) $(CPPFLAGS) $< -o $@
 
-cst_signer: cst_signer.c fdt.o
-       $(CC) $(COPTS) $(CFLAGS) -o $@ $(SRCS)
+cst_signer: $(OBJS)
+       $(CC) $(INCLUDES) $(CFLAGS) $(LDFLAGS) $(CPPFLAGS) -o $@ $^
 
+install: cst_signer
+       install -D -m 0755 cst_signer $(DESTDIR)/$(BINDIR)/cst_signer
+       install -D -m 0755 -t $(DESTDIR)$(DATADIR)/doc/cst_signer \
+            ../csf_ahab.cfg.sample \
+            ../csf_hab4.cfg.sample
 clean:
-       rm -rf cst_signer fdt.o
+       $(RM) cst_signer *.o

otavio · 2024-01-11T16:19:20Z

When including local headers, we typically use quotation marks ("). However, if you are using headers from another directory, it makes more sense to use angle brackets instead. These headers are then loaded from the search paths specified by -I../inc/. Using angle brackets indicates that they do not come from the current directory.

utkarshguptanxp · 2024-01-17T22:12:43Z

Ok sounds good. Please also comment on the patch posted above and about the install/uninstall command.

otavio added 2 commits October 12, 2023 19:11

Fix include of headers so it indicates it uses the includedir

bc8252a

Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>

utkarshguptanxp reviewed Oct 30, 2023

View reviewed changes

otavio requested a review from utkarshguptanxp October 30, 2023 18:08

utkarshguptanxp reviewed Jan 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

General rework for Yocto Project integration#2

General rework for Yocto Project integration#2
otavio wants to merge 2 commits intonxp-imx-support:masterfrom
OSSystems:topic/general-rework

otavio commented Oct 12, 2023

Uh oh!

utkarshguptanxp Oct 30, 2023

Uh oh!

otavio Oct 30, 2023

Uh oh!

utkarshguptanxp Oct 30, 2023

Uh oh!

otavio Oct 30, 2023

Uh oh!

utkarshguptanxp Oct 30, 2023

Uh oh!

otavio Oct 30, 2023

Uh oh!

utkarshguptanxp left a comment

Uh oh!

utkarshguptanxp commented Jan 9, 2024

Uh oh!

otavio commented Jan 11, 2024

Uh oh!

utkarshguptanxp commented Jan 11, 2024

Uh oh!

otavio commented Jan 11, 2024

Uh oh!

utkarshguptanxp commented Jan 17, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

otavio commented Oct 12, 2023

Uh oh!

utkarshguptanxp Oct 30, 2023

Choose a reason for hiding this comment

Uh oh!

otavio Oct 30, 2023

Choose a reason for hiding this comment

Uh oh!

utkarshguptanxp Oct 30, 2023

Choose a reason for hiding this comment

Uh oh!

otavio Oct 30, 2023

Choose a reason for hiding this comment

Uh oh!

utkarshguptanxp Oct 30, 2023

Choose a reason for hiding this comment

Uh oh!

otavio Oct 30, 2023

Choose a reason for hiding this comment

Uh oh!

utkarshguptanxp left a comment

Choose a reason for hiding this comment

Uh oh!

utkarshguptanxp commented Jan 9, 2024

Uh oh!

otavio commented Jan 11, 2024

Uh oh!

utkarshguptanxp commented Jan 11, 2024

Uh oh!

otavio commented Jan 11, 2024

Uh oh!

utkarshguptanxp commented Jan 17, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants