Martin Roth has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35531 )
Change subject: Makefile: Create the build directory before bootblock.bin ......................................................................
Makefile: Create the build directory before bootblock.bin
This was causing a failure when building platforms with no bootblock when building with make -jXX
Change-Id: Ic4cd4fe8ac82bd1e9ce114dbd53763538d125af3 Signed-off-by: Martin Roth martinroth@chromium.org --- M Makefile.inc 1 file changed, 2 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/31/35531/1
diff --git a/Makefile.inc b/Makefile.inc index 3c3088d..cf8e470 100644 --- a/Makefile.inc +++ b/Makefile.inc @@ -504,7 +504,7 @@ @printf " GEN build.h\n" mv $< $@
-build-dirs: +build-dirs $(objcbfs) $(objgenerated): mkdir -p $(objcbfs) $(objgenerated)
####################################################################### @@ -706,7 +706,7 @@ $(OBJCOPY_bootblock) -O binary $< $@
ifneq ($(CONFIG_HAVE_BOOTBLOCK),y) -$(objcbfs)/bootblock.bin: +$(objcbfs)/bootblock.bin: $(objcbfs) dd if=/dev/zero of=$@ bs=64 count=1 endif
Hello Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35531
to look at the new patch set (#2).
Change subject: Makefile: Create the build directory before bootblock.bin ......................................................................
Makefile: Create the build directory before bootblock.bin
This was causing a failure when building platforms with no bootblock when building with make -jXX
Change-Id: Ic4cd4fe8ac82bd1e9ce114dbd53763538d125af3 Signed-off-by: Martin Roth martinroth@chromium.org --- M Makefile.inc 1 file changed, 2 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/31/35531/2
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35531 )
Change subject: Makefile: Create the build directory before bootblock.bin ......................................................................
Patch Set 2: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35531 )
Change subject: Makefile: Create the build directory before bootblock.bin ......................................................................
Makefile: Create the build directory before bootblock.bin
This was causing a failure when building platforms with no bootblock when building with make -jXX
Change-Id: Ic4cd4fe8ac82bd1e9ce114dbd53763538d125af3 Signed-off-by: Martin Roth martinroth@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/35531 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Marshall Dawson marshalldawson3rd@gmail.com --- M Makefile.inc 1 file changed, 2 insertions(+), 2 deletions(-)
Approvals: build bot (Jenkins): Verified Marshall Dawson: Looks good to me, approved
diff --git a/Makefile.inc b/Makefile.inc index 3c3088d..8676404 100644 --- a/Makefile.inc +++ b/Makefile.inc @@ -504,7 +504,7 @@ @printf " GEN build.h\n" mv $< $@
-build-dirs: +build-dirs $(objcbfs) $(objgenerated): mkdir -p $(objcbfs) $(objgenerated)
####################################################################### @@ -706,7 +706,7 @@ $(OBJCOPY_bootblock) -O binary $< $@
ifneq ($(CONFIG_HAVE_BOOTBLOCK),y) -$(objcbfs)/bootblock.bin: +$(objcbfs)/bootblock.bin: $(objcbfs) dd if=/dev/zero of=$@ bs=64 count=1 endif
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35531 )
Change subject: Makefile: Create the build directory before bootblock.bin ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35531/3/Makefile.inc File Makefile.inc:
https://review.coreboot.org/c/coreboot/+/35531/3/Makefile.inc@507 PS3, Line 507: build-dirs $(objcbfs) $(objgenerated): Uhh... this is a bit odd. The whole point of the target 'build-dirs' is to be a phony target that creates those two directories, but now you created two more targets (those directories themselves) that also do the same thing (making 'build-dirs' kinda pointless). Why didn't you just add 'build-dirs' as a dependency to the bootblock.bin target?
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35531 )
Change subject: Makefile: Create the build directory before bootblock.bin ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35531/3/Makefile.inc File Makefile.inc:
https://review.coreboot.org/c/coreboot/+/35531/3/Makefile.inc@507 PS3, Line 507: build-dirs $(objcbfs) $(objgenerated):
Why didn't you just add 'build-dirs' as a dependency to the bootblock.bin target?
That had failed very spectacularly when I originally tried it (mid August), saying that build/cbfs/fallback wasn't yet present. However, it now seems well behaved. I'll see if I can figure out what the difference is in the morning.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35531 )
Change subject: Makefile: Create the build directory before bootblock.bin ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35531/3/Makefile.inc File Makefile.inc:
https://review.coreboot.org/c/coreboot/+/35531/3/Makefile.inc@507 PS3, Line 507: build-dirs $(objcbfs) $(objgenerated):
Why didn't you just add 'build-dirs' as a dependency to the bootblock.bin target? […]
I mean, that's how make is supposed to work? If it didn't before then maybe there was/is another hidden error somewhere, but then we should find and fix that rather than doing some weird other solution that looks wrong and where nobody can explain why it works better than the obvious solution.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35531 )
Change subject: Makefile: Create the build directory before bootblock.bin ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35531/3/Makefile.inc File Makefile.inc:
https://review.coreboot.org/c/coreboot/+/35531/3/Makefile.inc@507 PS3, Line 507: build-dirs $(objcbfs) $(objgenerated):
I mean, that's how make is supposed to work? If it didn't before then maybe there was/is another hid […]
I added the actual directory variables because it was a pain trying to search to see what random phoney target name is creating the directory I needed. Why have a phoney target when we can have real targets doing what Makefiles are supposed to do?
Maybe I should have gone through and gotten rid of all of the references to the build-dirs target.
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35531 )
Change subject: Makefile: Create the build directory before bootblock.bin ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35531/3/Makefile.inc File Makefile.inc:
https://review.coreboot.org/c/coreboot/+/35531/3/Makefile.inc@507 PS3, Line 507: build-dirs $(objcbfs) $(objgenerated):
I added the actual directory variables because it was a pain trying to search to see what random pho […]
I think the phony target was created originally to decouple the dependencies of files to their directories: if you have foo/bar.o depend on foo/, any change to foo/* will update foo's timestamp. This in turn means that make considers foo/bar.o to be out of date, regenerating it.
Might be worth checking if we're still needing and achieving that decoupling or if we're cargo-culting some stuff along that outlived its usefulness a long time ago.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35531 )
Change subject: Makefile: Create the build directory before bootblock.bin ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35531/3/Makefile.inc File Makefile.inc:
https://review.coreboot.org/c/coreboot/+/35531/3/Makefile.inc@507 PS3, Line 507: build-dirs $(objcbfs) $(objgenerated):
Why have a phoney target when we can have real targets doing what Makefiles are supposed to do?
I don't know, I didn't put it there. I just think we should be consistent rather than having different rules to do the same thing all mixed together.
I think the phony target was created originally to decouple the dependencies of files to their directories: if you have foo/bar.o depend on foo/, any change to foo/* will update foo's timestamp.
AFAIK phony targets are always considered "out of date", so they (and anything depending on them) will always be rebuilt. So I don't think this example works like that (if foo is phony and foo/bar.o depends on foo, it will always be rebuilt). If you want to do that (make sure directory exists but don't imply dependency), you can use an order-only prerequisite instead (and then it doesn't matter whether it's phony or not).
-
Julius Werner (Code Review) -
Marshall Dawson (Code Review)
-
Martin Roth (Code Review)
-
Patrick Georgi (Code Review)