Change in coreboot[master]: cbfstool: Add dummy vboot callback vb2ex_printf()

[Yu-Ping Wu (Code Review)]

Yu-Ping Wu has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/39390 )

Change subject: cbfstool: Add dummy vboot callback vb2ex_printf()
......................................................................

cbfstool: Add dummy vboot callback vb2ex_printf()

With CL:2084062, macro VB2_TRY() is introduced, which is used within
various vboot2 APIs such as vb2_hash_verify(). vb2ex_printf() is used in
this macro to print error messages, but it is not defined in cbfstool.
Therefore, this patch adds a callback function vb2ex_printf(). Since
vprintk() is required to implement vb2ex_printf() and it is not
implemented in cbfstool, this callback is currently added as a dummy
function. We also don't lose anything because no messages were printed
before CL:2084062.

BRANCH=none
BUG=chromium:1049032
TEST=emerge-nami coreboot-utils

Change-Id: Ifc826896d895f53d69ea559a88f75672c2ec3146
Signed-off-by: Yu-Ping Wu 
---
M util/cbfstool/Makefile.inc
A util/cbfstool/vboot_lib.c
2 files changed, 25 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/39390/1

diff --git a/util/cbfstool/Makefile.inc b/util/cbfstool/Makefile.inc
index 356b295..e2eb154 100644
--- a/util/cbfstool/Makefile.inc
+++ b/util/cbfstool/Makefile.inc
@@ -22,6 +22,7 @@
 cbfsobj += rmodule.o
 cbfsobj += xdr.o
 cbfsobj += partitioned_file.o
+cbfsobj += vboot_lib.o
 # COMMONLIB
 cbfsobj += cbfs.o
 cbfsobj += fsp_relocate.o
@@ -77,6 +78,7 @@
 ifitobj += cbfs-mkstage.o
 ifitobj += cbfs-mkpayload.o
 ifitobj += rmodule.o
+ifitobj += vboot_lib.o
 # COMMONLIB
 ifitobj += cbfs.o
 ifitobj += mem_pool.o
diff --git a/util/cbfstool/vboot_lib.c b/util/cbfstool/vboot_lib.c
new file mode 100644
index 0000000..d8933e2
--- /dev/null
+++ b/util/cbfstool/vboot_lib.c
@@ -0,0 +1,23 @@
+/*
+ * This file is part of the coreboot project.
+ *
+ * Copyright 2020 Google Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <vb2_api.h>
+
+#include "common.h"
+#include "console/console.h"
+
+void vb2ex_printf(unused const char *func, unused const char *fmt, ...)
+{
+}

[Joel Kitching (Code Review)]

Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39390 )

Change subject: cbfstool: Add dummy vboot callback vb2ex_printf()
......................................................................

Patch Set 1:

Since this is a userspace utility, I don't think we need to worry about implementing vprintf ourselves.  There's already one use of stdarg.h's implementation in util/cbfstool/flashmap/kv_pair.c (vsnprintf).

Change in coreboot[master]: cbfstool: Add vboot callback function vb2ex_printf()

[Yu-Ping Wu (Code Review)]

Hello build bot (Jenkins), Patrick Georgi, Martin Roth,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/39390

to look at the new patch set (#2).

Change subject: cbfstool: Add vboot callback function vb2ex_printf()
......................................................................

cbfstool: Add vboot callback function vb2ex_printf()

With CL:2084062, macro VB2_TRY() is introduced, which is used within
various vboot2 APIs such as vb2_hash_verify(). vb2ex_printf() is used in
this macro to print error messages, but it is not defined in cbfstool.
Therefore, create a callback function vb2ex_printf() in a newly added
file vboot_lib.c.

BRANCH=none
BUG=chromium:1049032
TEST=emerge-nami coreboot-utils

Change-Id: Ifc826896d895f53d69ea559a88f75672c2ec3146
Signed-off-by: Yu-Ping Wu 
---
M util/cbfstool/Makefile.inc
A util/cbfstool/vboot_lib.c
2 files changed, 32 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/39390/2

Change in coreboot[master]: cbfstool: Add vboot callback function vb2ex_printf()

[Yu-Ping Wu (Code Review)]

Hello build bot (Jenkins), Joel Kitching, Patrick Georgi, Martin Roth, Julius Werner,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/39390

to look at the new patch set (#3).

Change subject: cbfstool: Add vboot callback function vb2ex_printf()
......................................................................

cbfstool: Add vboot callback function vb2ex_printf()

With CL:2084062, macro VB2_TRY() is introduced, which is used within
various vboot2 APIs such as vb2_hash_verify(). vb2ex_printf() is used
in this macro to print error messages, but it is not defined in
cbfstool. Therefore, create a callback function vb2ex_printf() in a
newly added file vboot_lib.c.

BRANCH=none
BUG=chromium:1049032
TEST=emerge-nami coreboot-utils

Change-Id: Ifc826896d895f53d69ea559a88f75672c2ec3146
Signed-off-by: Yu-Ping Wu 
---
M util/cbfstool/Makefile.inc
A util/cbfstool/vboot_lib.c
2 files changed, 32 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/39390/3

Change in coreboot[master]: cbfstool: Add vboot callback function vb2ex_printf()

[Joel Kitching (Code Review)]

Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39390 )

Change subject: cbfstool: Add vboot callback function vb2ex_printf()
......................................................................

Patch Set 3:

Heads up to Raul and Nick on this CL, which will need to be cherry-picked to our local forks in order to unblock CL:2084062.

Change in coreboot[master]: cbfstool: Add vboot callback function vb2ex_printf()

[Joel Kitching (Code Review)]

Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39390 )

Change subject: cbfstool: Add vboot callback function vb2ex_printf()
......................................................................

Patch Set 3: Code-Review+1

Change in coreboot[master]: cbfstool: Add vboot callback function vb2ex_printf()

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39390 )

Change subject: cbfstool: Add vboot callback function vb2ex_printf()
......................................................................

Patch Set 3: Code-Review-1

Actually, I think the real problem here is different. vboot already solves this problem by containing stubs for its own callbacks when built for non-firmware use (including this one). The real problem is that cbfstool doesn't actually build the vboot library, it just cherry-picks a few random files and hopes that these files will continue to work standalone without any dependencies. That's pretty brittle and I think we would be better served by changing Makefiles so that we always build a HOSTCC version of the vboot library when building any host utilities and then link against that.

Change in coreboot[master]: cbfstool: Build vboot library

[Yu-Ping Wu (Code Review)]

Hello Hung-Te Lin, build bot (Jenkins), Joel Kitching, Raul Rangel, Patrick Georgi, Martin Roth, Nick Vaccaro, Julius Werner,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/39390

to look at the new patch set (#4).

Change subject: cbfstool: Build vboot library
......................................................................

cbfstool: Build vboot library

Currently cbfstool cherry-picks a few files from vboot and hopes these
files will work standalone without any dependencies. This is pretty
brittle (for example, CL:2084062 will breaks it), and could be improved
by building the whole vboot library and then linking against it.
Therefore, this patch creates a new target $(VBOOT_LIB) and includes it
as a dependency for cbfstool and ifittool.

BRANCH=none
BUG=none
TEST=emerge-nami coreboot-utils

Change-Id: Ifc826896d895f53d69ea559a88f75672c2ec3146
Signed-off-by: Yu-Ping Wu 
---
M util/cbfstool/Makefile.inc
1 file changed, 24 insertions(+), 21 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/39390/4

Change in coreboot[master]: cbfstool: Build vboot library

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39390 )

Change subject: cbfstool: Build vboot library
......................................................................

Patch Set 4: Code-Review+2

(1 comment)

I think this is okay, but it would be nice if we could somehow combine this with the futility build. util/futility/Makefile.inc also (indirectly) builds this library already, and many builds of coreboot need both tools, so it's a waste to build it twice. It would be good to make them share a build target somehow (but need to make sure that both the building as part of coreboot and the standalone building when running 'make' in the subdirectory still work).

https://review.coreboot.org/c/coreboot/+/39390/4/util/cbfstool/Makefile.inc 
File util/cbfstool/Makefile.inc:

https://review.coreboot.org/c/coreboot/+/39390/4/util/cbfstool/Makefile.inc@... 
PS4, Line 143: 	VBOOT2="y" \
Pretty sure this doesn't do anything anymore and can be removed both here and from the firmware part.

Change in coreboot[master]: cbfstool: Build vboot library

[Yu-Ping Wu (Code Review)]

Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39390 )

Change subject: cbfstool: Build vboot library
......................................................................

Patch Set 4:
> I think this is okay, but it would be nice if we could somehow combine this with the futility build. util/futility/Makefile.inc also (indirectly) builds this library already, and many builds of coreboot need both tools, so it's a waste to build it twice.

I noticed that cbfstool builds vboot hostlib, while futility builds vboot futil, and there're no common *.c files for these 2 vboot targets, so I can't see how vboot will be built twice. Am I missing something?

Change in coreboot[master]: cbfstool: Build vboot library

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39390 )

Change subject: cbfstool: Build vboot library
......................................................................

Patch Set 4:

(2 comments)

https://review.coreboot.org/c/coreboot/+/39390/4//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/39390/4//COMMIT_MSG@11 
PS4, Line 11: will breaks 
*breaks* or *will break*

https://review.coreboot.org/c/coreboot/+/39390/4//COMMIT_MSG@13 
PS4, Line 13: Therefore, this patch creates a new target $(VBOOT_LIB) and includes it
Please add exactly one blank line between paragraphs.

Change in coreboot[master]: cbfstool: Build vboot library

[Yu-Ping Wu (Code Review)]

Hello Hung-Te Lin, build bot (Jenkins), Joel Kitching, Raul Rangel, Patrick Georgi, Martin Roth, Nick Vaccaro, Julius Werner,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/39390

to look at the new patch set (#5).

Change subject: cbfstool: Build vboot library
......................................................................

cbfstool: Build vboot library

Currently cbfstool cherry-picks a few files from vboot and hopes these
files will work standalone without any dependencies. This is pretty
brittle (for example, CL:2084062 will break it), and could be improved
by building the whole vboot library and then linking against it.
Therefore, this patch creates a new target $(VBOOT_LIB) and includes it
as a dependency for cbfstool and ifittool.

BRANCH=none
BUG=none
TEST=emerge-nami coreboot-utils

Change-Id: Ifc826896d895f53d69ea559a88f75672c2ec3146
Signed-off-by: Yu-Ping Wu 
---
M util/cbfstool/Makefile.inc
1 file changed, 23 insertions(+), 21 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/39390/5

Change in coreboot[master]: cbfstool: Build vboot library

[Yu-Ping Wu (Code Review)]

Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39390 )

Change subject: cbfstool: Build vboot library
......................................................................

Patch Set 5:

(3 comments)

https://review.coreboot.org/c/coreboot/+/39390/4//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/39390/4//COMMIT_MSG@11 
PS4, Line 11: will breaks
> *breaks* or *will break*

Done

https://review.coreboot.org/c/coreboot/+/39390/4//COMMIT_MSG@13 
PS4, Line 13: Therefore, this patch creates a new target $(VBOOT_LIB) and includes it
> Please add exactly one blank line between paragraphs.

It's the same paragraph, with "Therefore" unable to fit in the previous line.

https://review.coreboot.org/c/coreboot/+/39390/4/util/cbfstool/Makefile.inc 
File util/cbfstool/Makefile.inc:

https://review.coreboot.org/c/coreboot/+/39390/4/util/cbfstool/Makefile.inc@... 
PS4, Line 143: 	VBOOT2="y" \
> Pretty sure this doesn't do anything anymore and can be removed both here and from the firmware part […]

Done

Change in coreboot[master]: cbfstool: Build vboot library

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39390 )

Change subject: cbfstool: Build vboot library
......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39390/4//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/39390/4//COMMIT_MSG@13 
PS4, Line 13: Therefore, this patch creates a new target $(VBOOT_LIB) and includes it
> It's the same paragraph, with "Therefore" unable to fit in the previous line.

Ack

Change in coreboot[master]: cbfstool: Build vboot library

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39390 )

Change subject: cbfstool: Build vboot library
......................................................................

Patch Set 5:
> I noticed that cbfstool builds vboot hostlib, while futility builds vboot futil, and there're no common *.c files for these 2 vboot targets, so I can't see how vboot will be built twice. Am I missing something?

Futility depends on FWLIB and UTILLIB which, while separate lists of files, contain a lot of overlap with HOSTLIB files. So in the end you'll still save a lot of duplication.

Change in coreboot[master]: cbfstool: Build vboot library

[Yu-Ping Wu (Code Review)]

Hello Hung-Te Lin, build bot (Jenkins), Joel Kitching, Raul Rangel, Patrick Georgi, Martin Roth, Nick Vaccaro, Julius Werner,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/39390

to look at the new patch set (#6).

Change subject: cbfstool: Build vboot library
......................................................................

cbfstool: Build vboot library

Currently cbfstool cherry-picks a few files from vboot and hopes these
files will work standalone without any dependencies. This is pretty
brittle (for example, CL:2084062 will break it), and could be improved
by building the whole vboot library and then linking against it.
Therefore, this patch creates a new target $(VBOOT_HOSTLIB) and includes
it as a dependency for cbfstool and ifittool.

To prevent building the vboot lib twice (one for cbfstool and the other
for futility) when building coreboot tools together, add the variable
'VBOOT_BUILD' in Makefile to define a shared build path among different
tools so that vboot files don't need to be recompiled.

Also add vboot_lib/ to .gitignore.

BRANCH=none
BUG=none
TEST=make -C util/cbfstool
TEST=make -C util/futility
TEST=Run 'make tools' and make sure common files such as 2sha1.c are
     compiled only once
TEST=emerge-nami coreboot-utils

Change-Id: Ifc826896d895f53d69ea559a88f75672c2ec3146
Signed-off-by: Yu-Ping Wu 
---
M .gitignore
M Makefile
M util/cbfstool/Makefile
M util/cbfstool/Makefile.inc
M util/futility/Makefile
M util/futility/Makefile.inc
6 files changed, 37 insertions(+), 24 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/39390/6

Change in coreboot[master]: cbfstool: Build vboot library

[Yu-Ping Wu (Code Review)]

Hello Hung-Te Lin, build bot (Jenkins), Joel Kitching, Raul Rangel, Patrick Georgi, Martin Roth, Nick Vaccaro, Julius Werner,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/39390

to look at the new patch set (#7).

Change subject: cbfstool: Build vboot library
......................................................................

cbfstool: Build vboot library

Currently cbfstool cherry-picks a few files from vboot and hopes these
files will work standalone without any dependencies. This is pretty
brittle (for example, CL:2084062 will break it), and could be improved
by building the whole vboot library and then linking against it.
Therefore, this patch creates a new target $(VBOOT_HOSTLIB) and includes
it as a dependency for cbfstool and ifittool.

To prevent building the vboot lib twice (one for cbfstool and the other
for futility) when building coreboot tools together, add the variable
'VBOOT_BUILD' in Makefile to define a shared build path among different
tools so that vboot files don't need to be recompiled.

Also add vboot_lib/ to .gitignore.

BRANCH=none
BUG=none
TEST=make -C util/cbfstool
TEST=make -C util/futility
TEST=Run 'make tools' and make sure common files such as 2sha1.c are
     compiled only once
TEST=emerge-nami coreboot-utils

Change-Id: Ifc826896d895f53d69ea559a88f75672c2ec3146
Signed-off-by: Yu-Ping Wu 
---
M .gitignore
M Makefile
M util/cbfstool/Makefile
M util/cbfstool/Makefile.inc
M util/futility/Makefile
M util/futility/Makefile.inc
6 files changed, 36 insertions(+), 24 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/39390/7

Change in coreboot[master]: cbfstool: Build vboot library

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39390 )

Change subject: cbfstool: Build vboot library
......................................................................

Patch Set 7:

(3 comments)

https://review.coreboot.org/c/coreboot/+/39390/7/.gitignore 
File .gitignore:

https://review.coreboot.org/c/coreboot/+/39390/7/.gitignore@55 
PS7, Line 55: vboot_lib/
Isn't this under build/ (and therefore already excluded)?

https://review.coreboot.org/c/coreboot/+/39390/7/Makefile 
File Makefile:

https://review.coreboot.org/c/coreboot/+/39390/7/Makefile@45 
PS7, Line 45: VBOOT_BUILD
Probably better to name it VBOOT_HOST_BUILD or something to make it clear that this is separate from the vboot libraries built for firmware stages.

https://review.coreboot.org/c/coreboot/+/39390/7/util/cbfstool/Makefile.inc 
File util/cbfstool/Makefile.inc:

https://review.coreboot.org/c/coreboot/+/39390/7/util/cbfstool/Makefile.inc@... 
PS7, Line 135: VBOOTCFLAGS += -Wno-missing-field-initializers
You probably want to mirror what futility does here (unset CFLAGS LDFLAGS; and let vboot figure it out).

Change in coreboot[master]: cbfstool: Build vboot library

[Yu-Ping Wu (Code Review)]

Hello Hung-Te Lin, build bot (Jenkins), Joel Kitching, Raul Rangel, Patrick Georgi, Martin Roth, Nick Vaccaro, Julius Werner,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/39390

to look at the new patch set (#8).

Change subject: cbfstool: Build vboot library
......................................................................

cbfstool: Build vboot library

Currently cbfstool cherry-picks a few files from vboot and hopes these
files will work standalone without any dependencies. This is pretty
brittle (for example, CL:2084062 will break it), and could be improved
by building the whole vboot library and then linking against it.
Therefore, this patch creates a new target $(VBOOT_HOSTLIB) and includes
it as a dependency for cbfstool and ifittool.

To prevent building the vboot lib twice (one for cbfstool and the other
for futility) when building coreboot tools together, add the variable
'VBOOT_BUILD' in Makefile to define a shared build path among different
tools so that vboot files don't need to be recompiled.

Also ignore util/cbfstool/vboot_lib and *.a.

BRANCH=none
BUG=none
TEST=make -C util/cbfstool
TEST=make -C util/futility
TEST=Run 'make tools' and make sure common files such as 2sha1.c are
     compiled only once
TEST=emerge-nami coreboot-utils

Change-Id: Ifc826896d895f53d69ea559a88f75672c2ec3146
Signed-off-by: Yu-Ping Wu 
---
M .gitignore
M Makefile
M util/cbfstool/Makefile
M util/cbfstool/Makefile.inc
M util/futility/Makefile
M util/futility/Makefile.inc
6 files changed, 28 insertions(+), 25 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/39390/8

Change in coreboot[master]: cbfstool: Build vboot library

[Yu-Ping Wu (Code Review)]

Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39390 )

Change subject: cbfstool: Build vboot library
......................................................................

Patch Set 8:

(3 comments)

https://review.coreboot.org/c/coreboot/+/39390/7/.gitignore 
File .gitignore:

https://review.coreboot.org/c/coreboot/+/39390/7/.gitignore@55 
PS7, Line 55: vboot_lib/
> Isn't this under build/ (and therefore already excluded)?

That's true for top-most make and futility. However, when running 'make' inside util/cbfstool, the binaries will be inside util/cbfstool/vboot_lib.

https://review.coreboot.org/c/coreboot/+/39390/7/Makefile 
File Makefile:

https://review.coreboot.org/c/coreboot/+/39390/7/Makefile@45 
PS7, Line 45: VBOOT_BUILD
> Probably better to name it VBOOT_HOST_BUILD or something to make it clear that this is separate from […]

Do you mean to rename this variable for futility as well? When running make from the top-level Makefile, VBOOT_BUILD will be passed to both cbfstool/Makefile.inc and futility/Makefile.inc so that *.o files can be reused.

https://review.coreboot.org/c/coreboot/+/39390/7/util/cbfstool/Makefile.inc 
File util/cbfstool/Makefile.inc:

https://review.coreboot.org/c/coreboot/+/39390/7/util/cbfstool/Makefile.inc@... 
PS7, Line 135: VBOOTCFLAGS += -Wno-missing-field-initializers
> You probably want to mirror what futility does here (unset CFLAGS LDFLAGS; and let vboot figure it o […]

Done

Change in coreboot[master]: cbfstool: Build vboot library

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39390 )

Change subject: cbfstool: Build vboot library
......................................................................

Patch Set 8:

(2 comments)
> make[1]: execvp: /cb-build/coreboot-gerrit.0/chromeos/sharedutils/cbfstool/cbfstool: Permission denied

I am very confused why Jenkins is choking on your CL, but it is choking on something. Can you try to reproduce locally? You can run something like 'util/abuild/abuild -c max -t GOOGLE_BOB -x' to build a board similar to how Jenkins does it.

https://review.coreboot.org/c/coreboot/+/39390/7/.gitignore 
File .gitignore:

https://review.coreboot.org/c/coreboot/+/39390/7/.gitignore@55 
PS7, Line 55: vboot_lib/
> That's true for top-most make and futility. […]

Where does the rest of the build output go when you just run 'make' in util/cbfstool? Looks like it is just put in that same directory? Does anything .gitignore those files? If not, don't think we need to worry about handling this directory either.

https://review.coreboot.org/c/coreboot/+/39390/7/Makefile 
File Makefile:

https://review.coreboot.org/c/coreboot/+/39390/7/Makefile@45 
PS7, Line 45: VBOOT_BUILD
> Do you mean to rename this variable for futility as well? When running make from the top-level Makef […]

Yes, I mean rename this everywhere it is used.

Change in coreboot[master]: cbfstool: Build vboot library

[Yu-Ping Wu (Code Review)]

Hello Hung-Te Lin, build bot (Jenkins), Joel Kitching, Raul Rangel, Patrick Georgi, Martin Roth, Nick Vaccaro, Julius Werner,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/39390

to look at the new patch set (#9).

Change subject: cbfstool: Build vboot library
......................................................................

cbfstool: Build vboot library

Currently cbfstool cherry-picks a few files from vboot and hopes these
files will work standalone without any dependencies. This is pretty
brittle (for example, CL:2084062 will break it), and could be improved
by building the whole vboot library and then linking against it.
Therefore, this patch creates a new target $(VBOOT_HOSTLIB) and includes
it as a dependency for cbfstool and ifittool.

To prevent building the vboot lib twice (one for cbfstool and the other
for futility) when building coreboot tools together, add the variable
'VBOOT_BUILD' in Makefile to define a shared build path among different
tools so that vboot files don't need to be recompiled.

Also ignore *.o.d and *.a for vboot library.

BRANCH=none
BUG=none
TEST=make -C util/cbfstool
TEST=make -C util/futility
TEST=Run 'make tools' and make sure common files such as 2sha1.c are
     compiled only once
TEST=emerge-nami coreboot-utils

Change-Id: Ifc826896d895f53d69ea559a88f75672c2ec3146
Signed-off-by: Yu-Ping Wu 
---
M .gitignore
M Makefile
M util/cbfstool/Makefile
M util/cbfstool/Makefile.inc
M util/futility/Makefile
M util/futility/Makefile.inc
6 files changed, 28 insertions(+), 25 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/39390/9

Change in coreboot[master]: cbfstool: Build vboot library

[Yu-Ping Wu (Code Review)]

Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39390 )

Change subject: cbfstool: Build vboot library
......................................................................

Patch Set 9:

(2 comments)

https://review.coreboot.org/c/coreboot/+/39390/7/.gitignore 
File .gitignore:

https://review.coreboot.org/c/coreboot/+/39390/7/.gitignore@55 
PS7, Line 55: vboot_lib/
> Where does the rest of the build output go when you just run 'make' in util/cbfstool? Looks like it  […]

Compiling vboot lib results in *.o, *.o.d and *.a files, with only *.o ignored. Since the output binaries are in build/ when running 'make' and 'make -C util/futility', and we did't have *.a or *.o.d for cbfstool before, there's no problem. However, with the whole vboot lib built for cbfstool in this CL, there will be *.a and *.o.d under cbfstool/vboot_lib.

Now ignoring *.o.d and *.a instead.

https://review.coreboot.org/c/coreboot/+/39390/7/Makefile 
File Makefile:

https://review.coreboot.org/c/coreboot/+/39390/7/Makefile@45 
PS7, Line 45: VBOOT_BUILD
> Yes, I mean rename this everywhere it is used.

Ack

Change in coreboot[master]: cbfstool: Build vboot library

[Yu-Ping Wu (Code Review)]

Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39390 )

Change subject: cbfstool: Build vboot library
......................................................................

Patch Set 9:
> You can run something like 'util/abuild/abuild -c max -t GOOGLE_BOB -x' to build a board similar to how Jenkins does it.

I can't reproduce it with this command. It finishes without any errors.

Change in coreboot[master]: cbfstool: Build vboot library

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39390 )

Change subject: cbfstool: Build vboot library
......................................................................

Patch Set 9:
> I can't reproduce it with this command. It finishes without any errors.

Hmm... yeah, I don't really understand what's happening there. Maybe it's some weird race condition with parallel make?

Patrick, can you help?

Change in coreboot[master]: cbfstool: Build vboot library

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39390 )

Change subject: cbfstool: Build vboot library
......................................................................

Patch Set 9: Code-Review+2

(Looks good to me code-wise, FWIW...)

Change in coreboot[master]: cbfstool: Build vboot library

[Patrick Georgi (Code Review)]

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39390 )

Change subject: cbfstool: Build vboot library
......................................................................

Patch Set 9:
> Patch Set 9:
> Hmm... yeah, I don't really understand what's happening there. Maybe it's some weird race condition with parallel make?
>
> Patrick, can you help?

There's a race condition between creating the binary and using it. The simple fix for this is to compile to cbfstool.tmp first, then mv into the real location, because mv is an atomic operation.

But: it will likely fail because there will be multiple writers to coreboot.tmp then. abuild (which is called by the what-jenkins-does target, which is what the builder does) first runs "make tools" (with tons of options) in util/abuild/abuild:839 to avoid that issue.

For some reason this change creates a perceived need to rebuild cbfstool for every board.

Change in coreboot[master]: cbfstool: Build vboot library

[Patrick Georgi (Code Review)]

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39390 )

Change subject: cbfstool: Build vboot library
......................................................................

Patch Set 9:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39390/9/Makefile 
File Makefile:

https://review.coreboot.org/c/coreboot/+/39390/9/Makefile@45 
PS9, Line 45: VBOOT_HOST_BUILD ?= $(absobj)/vboot_lib
putting this below $(objutil) somewhere might remove the endless rebuilds of cbfstool.

Change in coreboot[master]: cbfstool: Build vboot library

[Yu-Ping Wu (Code Review)]

Hello Hung-Te Lin, build bot (Jenkins), Joel Kitching, Raul Rangel, Patrick Georgi, Martin Roth, Nick Vaccaro, Julius Werner,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/39390

to look at the new patch set (#10).

Change subject: cbfstool: Build vboot library
......................................................................

cbfstool: Build vboot library

Currently cbfstool cherry-picks a few files from vboot and hopes these
files will work standalone without any dependencies. This is pretty
brittle (for example, CL:2084062 will break it), and could be improved
by building the whole vboot library and then linking against it.
Therefore, this patch creates a new target $(VBOOT_HOSTLIB) and includes
it as a dependency for cbfstool and ifittool.

To prevent building the vboot lib twice (one for cbfstool and the other
for futility) when building coreboot tools together, add the variable
'VBOOT_BUILD' in Makefile to define a shared build path among different
tools so that vboot files don't need to be recompiled.

Also ignore *.o.d and *.a for vboot library.

BRANCH=none
BUG=none
TEST=make -C util/cbfstool
TEST=make -C util/futility
TEST=Run 'make tools' and make sure common files such as 2sha1.c are
     compiled only once
TEST=emerge-nami coreboot-utils

Change-Id: Ifc826896d895f53d69ea559a88f75672c2ec3146
Signed-off-by: Yu-Ping Wu 
---
M .gitignore
M Makefile
M util/cbfstool/Makefile
M util/cbfstool/Makefile.inc
M util/futility/Makefile
M util/futility/Makefile.inc
6 files changed, 28 insertions(+), 25 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/39390/10

Change in coreboot[master]: cbfstool: Build vboot library

[Yu-Ping Wu (Code Review)]

Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39390 )

Change subject: cbfstool: Build vboot library
......................................................................

Patch Set 10:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39390/9/Makefile 
File Makefile:

https://review.coreboot.org/c/coreboot/+/39390/9/Makefile@45 
PS9, Line 45: VBOOT_HOST_BUILD ?= $(absobj)/vboot_lib
> putting this below $(objutil) somewhere might remove the endless rebuilds of cbfstool.

Still couldn't reproduce it locally, but let's see if it works.

Change in coreboot[master]: cbfstool: Build vboot library

[Yu-Ping Wu (Code Review)]

Hello Hung-Te Lin, build bot (Jenkins), Joel Kitching, Raul Rangel, Patrick Georgi, Martin Roth, Nick Vaccaro, Julius Werner,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/39390

to look at the new patch set (#11).

Change subject: cbfstool: Build vboot library
......................................................................

cbfstool: Build vboot library

Currently cbfstool cherry-picks a few files from vboot and hopes these
files will work standalone without any dependencies. This is pretty
brittle (for example, CL:2084062 will break it), and could be improved
by building the whole vboot library and then linking against it.
Therefore, this patch creates a new target $(VBOOT_HOSTLIB) and includes
it as a dependency for cbfstool and ifittool.

To prevent building the vboot lib twice (one for cbfstool and the other
for futility) when building coreboot tools together, add the variable
'VBOOT_BUILD' in Makefile to define a shared build path among different
tools so that vboot files don't need to be recompiled.

Also ignore *.o.d and *.a for vboot library.

BRANCH=none
BUG=none
TEST=make -C util/cbfstool
TEST=make -C util/futility
TEST=Run 'make tools' and make sure common files such as 2sha1.c are
     compiled only once
TEST=emerge-nami coreboot-utils

Change-Id: Ifc826896d895f53d69ea559a88f75672c2ec3146
Signed-off-by: Yu-Ping Wu 
---
M .gitignore
M Makefile
M util/cbfstool/Makefile
M util/cbfstool/Makefile.inc
M util/futility/Makefile
M util/futility/Makefile.inc
6 files changed, 29 insertions(+), 25 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/39390/11

Change in coreboot[master]: cbfstool: Build vboot library

[Patrick Georgi (Code Review)]

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39390 )

Change subject: cbfstool: Build vboot library
......................................................................

Patch Set 11: Code-Review+2

Change in coreboot[master]: cbfstool: Build vboot library

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/39390 )

Change subject: cbfstool: Build vboot library
......................................................................

cbfstool: Build vboot library

Currently cbfstool cherry-picks a few files from vboot and hopes these
files will work standalone without any dependencies. This is pretty
brittle (for example, CL:2084062 will break it), and could be improved
by building the whole vboot library and then linking against it.
Therefore, this patch creates a new target $(VBOOT_HOSTLIB) and includes
it as a dependency for cbfstool and ifittool.

To prevent building the vboot lib twice (one for cbfstool and the other
for futility) when building coreboot tools together, add the variable
'VBOOT_BUILD' in Makefile to define a shared build path among different
tools so that vboot files don't need to be recompiled.

Also ignore *.o.d and *.a for vboot library.

BRANCH=none
BUG=none
TEST=make -C util/cbfstool
TEST=make -C util/futility
TEST=Run 'make tools' and make sure common files such as 2sha1.c are
     compiled only once
TEST=emerge-nami coreboot-utils

Change-Id: Ifc826896d895f53d69ea559a88f75672c2ec3146
Signed-off-by: Yu-Ping Wu 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/39390
Tested-by: build bot (Jenkins) 
Reviewed-by: Patrick Georgi 
---
M .gitignore
M Makefile
M util/cbfstool/Makefile
M util/cbfstool/Makefile.inc
M util/futility/Makefile
M util/futility/Makefile.inc
6 files changed, 29 insertions(+), 25 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Patrick Georgi: Looks good to me, approved

diff --git a/.gitignore b/.gitignore
index 5301a6e..ed66776 100644
--- a/.gitignore
+++ b/.gitignore
@@ -54,11 +54,13 @@
 site-local

*.#
+*.a
 *.bin
 *.debug
 !Kconfig.debug
 *.elf
 *.o
+*.o.d
 *.out
 *.pyc
 *.sw[po]
diff --git a/Makefile b/Makefile
index 3f60493..98e3eb5 100644
--- a/Makefile
+++ b/Makefile
@@ -42,6 +42,8 @@
 objk := $(objutil)/kconfig
 absobj := $(abspath $(obj))

+VBOOT_HOST_BUILD ?= $(abspath $(objutil)/vboot_lib)
+
 COREBOOT_EXPORTS := COREBOOT_EXPORTS
 COREBOOT_EXPORTS += top src srck obj objutil objk

diff --git a/util/cbfstool/Makefile b/util/cbfstool/Makefile
index d5321f6..5251b2d 100644
--- a/util/cbfstool/Makefile
+++ b/util/cbfstool/Makefile
@@ -10,6 +10,7 @@
 OBJCOPY ?= objcopy

VBOOT_SOURCE ?= $(top)/3rdparty/vboot
+VBOOT_HOST_BUILD ?= $(abspath $(objutil)/vboot_lib)

.PHONY: all
 all: cbfstool ifittool fmaptool rmodtool ifwitool cbfs-compression-tool
@@ -35,6 +36,7 @@
    $(RM) $(objutil)/cbfstool/ifwitool $(ifwiobj)
    $(RM) $(objutil)/cbfstool/ifittool $(ifitobj)
    $(RM) $(objutil)/cbfstool/cbfs-compression-tool $(cbfscompobj)
+	$(RM) -r $(VBOOT_HOST_BUILD)

linux_trampoline.c: linux_trampoline.S
    rm -f linux_trampoline.c
diff --git a/util/cbfstool/Makefile.inc b/util/cbfstool/Makefile.inc
index 356b295..f38c825 100644
--- a/util/cbfstool/Makefile.inc
+++ b/util/cbfstool/Makefile.inc
@@ -27,11 +27,6 @@
 cbfsobj += fsp_relocate.o
 cbfsobj += mem_pool.o
 cbfsobj += region.o
-# CRYPTOLIB
-cbfsobj += 2sha_utility.o
-cbfsobj += 2sha1.o
-cbfsobj += 2sha256.o
-cbfsobj += 2sha512.o
 # FMAP
 cbfsobj += fmap.o
 cbfsobj += kv_pair.o
@@ -81,11 +76,6 @@
 ifitobj += cbfs.o
 ifitobj += mem_pool.o
 ifitobj += region.o
-# CRYPTOLIB
-ifitobj += 2sha_utility.o
-ifitobj += 2sha1.o
-ifitobj += 2sha256.o
-ifitobj += 2sha512.o
 # FMAP
 ifitobj += fmap.o
 ifitobj += kv_pair.o
@@ -136,6 +126,17 @@
 TOOLCFLAGS+=-std=c11
 endif

+VBOOT_HOSTLIB = $(VBOOT_HOST_BUILD)/libvboot_host.a
+
+$(VBOOT_HOSTLIB):
+	printf "    MAKE       $(subst $(objutil)/,,$(@))\n"
+	unset CFLAGS LDFLAGS; $(MAKE) -C $(VBOOT_SOURCE) \
+		BUILD=$(VBOOT_HOST_BUILD) \
+		CC="$(HOSTCC)" \
+		$(if $(HOSTPKGCONFIG), PKG_CONFIG="$(HOSTPKGCONFIG)") \
+		V=$(V) \
+		hostlib
+
 $(objutil)/cbfstool/%.o: $(objutil)/cbfstool/%.c
    printf "    HOSTCC     $(subst $(objutil)/,,$(@))\n"
    $(HOSTCC) $(TOOLCPPFLAGS) $(TOOLCFLAGS) $(HOSTCFLAGS) -c -o $@ $<
@@ -156,10 +157,6 @@
    printf "    HOSTCC     $(subst $(objutil)/,,$(@))\n"
    $(HOSTCC) $(TOOLCPPFLAGS) $(TOOLCFLAGS) $(HOSTCFLAGS) -c -o $@ $<

-$(objutil)/cbfstool/%.o: $(VBOOT_SOURCE)/firmware/2lib/%.c
-	printf "    HOSTCC     $(subst $(objutil)/,,$(@))\n"
-	$(HOSTCC) $(TOOLCPPFLAGS) $(TOOLCFLAGS) $(HOSTCFLAGS) -c -o $@ $<
-
 $(objutil)/cbfstool/%.o: $(top)/src/commonlib/%.c
    printf "    HOSTCC     $(subst $(objutil)/,,$(@))\n"
    $(HOSTCC) $(TOOLCPPFLAGS) $(TOOLCFLAGS) $(HOSTCFLAGS) -c -o $@ $<
@@ -172,9 +169,9 @@
    printf "    HOSTCC     $(subst $(objutil)/,,$(@))\n"
    $(HOSTCC) $(TOOLCPPFLAGS) $(TOOLCFLAGS) $(HOSTCFLAGS) -c -o $@ $<

-$(objutil)/cbfstool/cbfstool: $(addprefix $(objutil)/cbfstool/,$(cbfsobj))
+$(objutil)/cbfstool/cbfstool: $(addprefix $(objutil)/cbfstool/,$(cbfsobj)) $(VBOOT_HOSTLIB)
    printf "    HOSTCC     $(subst $(objutil)/,,$(@)) (link)\n"
-	$(HOSTCC) $(TOOLLDFLAGS) -o $@ $(addprefix $(objutil)/cbfstool/,$(cbfsobj))
+	$(HOSTCC) -v $(TOOLLDFLAGS) -o $@ $(addprefix $(objutil)/cbfstool/,$(cbfsobj)) $(VBOOT_HOSTLIB)

$(objutil)/cbfstool/fmaptool: $(addprefix $(objutil)/cbfstool/,$(fmapobj))
    printf "    HOSTCC     $(subst $(objutil)/,,$(@)) (link)\n"
@@ -188,9 +185,9 @@
    printf "    HOSTCC     $(subst $(objutil)/,,$(@)) (link)\n"
    $(HOSTCC) $(TOOLLDFLAGS) -o $@ $(addprefix $(objutil)/cbfstool/,$(ifwiobj))

-$(objutil)/cbfstool/ifittool: $(addprefix $(objutil)/cbfstool/,$(ifitobj))
+$(objutil)/cbfstool/ifittool: $(addprefix $(objutil)/cbfstool/,$(ifitobj)) $(VBOOT_HOSTLIB)
    printf "    HOSTCC     $(subst $(objutil)/,,$(@)) (link)\n"
-	$(HOSTCC) $(TOOLLDFLAGS) -o $@ $(addprefix $(objutil)/cbfstool/,$(ifitobj))
+	$(HOSTCC) $(TOOLLDFLAGS) -o $@ $(addprefix $(objutil)/cbfstool/,$(ifitobj)) $(VBOOT_HOSTLIB)

$(objutil)/cbfstool/cbfs-compression-tool: $(addprefix $(objutil)/cbfstool/,$(cbfscompobj))
    printf "    HOSTCC     $(subst $(objutil)/,,$(@)) (link)\n"
@@ -208,9 +205,6 @@
 $(objutil)/cbfstool/fmd_scanner.o: TOOLCFLAGS += -Wno-unused-function
 # Tolerate lzma sdk warnings
 $(objutil)/cbfstool/LzmaEnc.o: TOOLCFLAGS += -Wno-sign-compare -Wno-cast-qual
-# Tolerate vboot warnings
-$(objutil)/cbfstool/2sha_utility.o: TOOLCFLAGS += -Wno-sign-compare
-$(objutil)/cbfstool/2sha1.o: TOOLCFLAGS += -Wno-cast-qual
 # Tolerate commonlib warnings
 $(objutil)/cbfstool/region.o: TOOLCFLAGS += -Wno-sign-compare -Wno-cast-qual
 $(objutil)/cbfstool/cbfs.o: TOOLCFLAGS += -Wno-sign-compare -Wno-cast-qual
diff --git a/util/futility/Makefile b/util/futility/Makefile
index cce5da6..2eaab3e 100644
--- a/util/futility/Makefile
+++ b/util/futility/Makefile
@@ -4,6 +4,7 @@

HOSTCC ?= $(CC)
 VBOOT_SOURCE ?= $(top)/3rdparty/vboot
+VBOOT_HOST_BUILD ?= $(abspath $(objutil)/vboot_lib)

.PHONY: all
 all: $(objutil)/futility/futility
diff --git a/util/futility/Makefile.inc b/util/futility/Makefile.inc
index 06e724c..ee4ad05 100644
--- a/util/futility/Makefile.inc
+++ b/util/futility/Makefile.inc
@@ -1,14 +1,17 @@
 additional-dirs += $(objutil)/futility

-$(objutil)/futility/build/futility/futility:
+VBOOT_FUTILITY = $(VBOOT_HOST_BUILD)/futility/futility
+
+$(VBOOT_FUTILITY):
    @printf "    MAKE       $(subst $(objutil)/,,$(@))\n"
    unset CFLAGS LDFLAGS; $(MAKE) -C $(VBOOT_SOURCE) \
-		BUILD=$(abspath $@/../..) \
+		BUILD=$(VBOOT_HOST_BUILD) \
    	CC="$(HOSTCC)" \
    	$(if $(HOSTPKGCONFIG), PKG_CONFIG="$(HOSTPKGCONFIG)") \
    	V=$(V) \
-		$(abspath $@)
+		$@

-$(objutil)/futility/futility: $(objutil)/futility/build/futility/futility
+$(objutil)/futility/futility: $(VBOOT_FUTILITY)
+	mkdir -p $(dir $@)
    cp $< $@.tmp
    mv $@.tmp $@