[coreboot-gerrit] Change in coreboot[master]: security/vboot: Split fwid.region build target

[Julius Werner (Code Review)]

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

Change subject: security/vboot: Split fwid.region build target
......................................................................


Patch Set 1:

(2 comments)

https://review.coreboot.org/#/c/27884/1//COMMIT_MSG
Commit Message:

https://review.coreboot.org/#/c/27884/1//COMMIT_MSG@11
PS1, Line 11: objects are not invalidated when bumping the fwid.
I don't believe this, actually. I also don't think your patch really makes a practical difference for incremental builts.

Right now, you just have a normal (non-PHONY) rule for the $(obj)/fwid.region target. Once that file exists, it will never be remade (since it doesn't have any prerequisites). So this should never run twice unless you blow away your $(obj) directory (and that makes sense, because the contents of that file only depend on Kconfig, and when you change Kconfig you generally have to blow away everything anyway).

With your change, you have the same sort of situation for the $(obj)/fwid.version target. And then $(obj)/fwid.region gets a dependency to that, so it will only be remade if $(obj)/fwid.version was updated, which will never happen unless it got deleted somehow. So, same situation, practically.

Can you clarify why you believe this target creates a problem in regards to incremental builds?


https://review.coreboot.org/#/c/27884/1/src/security/vboot/Makefile.inc
File src/security/vboot/Makefile.inc:

https://review.coreboot.org/#/c/27884/1/src/security/vboot/Makefile.inc@232
PS1, Line 232: file
Uhhh... either I'm really missing something right now, or this is calling the 'file' utility and connection your firmware version file to it through stdin. So... that makes no sense. I think you meant "$(< $(obj)/fwid.version)"?



-- 
To view, visit https://review.coreboot.org/27884
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I955106efd648a75a1311f24ede46bd238d1517e0
Gerrit-Change-Number: 27884
Gerrit-PatchSet: 1
Gerrit-Owner: Raul Rangel <rrangel at chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply at coreboot.org>
Gerrit-CC: Julius Werner <jwerner at chromium.org>
Gerrit-Comment-Date: Mon, 06 Aug 2018 22:32:32 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.coreboot.org/pipermail/coreboot-gerrit/attachments/20180806/fbec4e1e/attachment.html>