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)"?
-
Julius Werner (Code Review)