Hello Martin Roth,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/36460
to review the following change.
Change subject: payloads/external/Makefile.inc: Clean up targets ......................................................................
payloads/external/Makefile.inc: Clean up targets
- Combine the short target name with the filename. - All targets depend on top level dotconfig. - Payload targets depend on their payload .config. - Add .PHONY target for the short target names. - Add additional short target names grub, uboot, and memtest. - Remove reversed dependencies of config files on the output. - Set SeaBIOS vgabios target at same level as main SeaBIOS target to prevent rebuilds because of the vbios being included.
This fixes an issue with SeaBIOS and Filo rebuilding every time.
Change-Id: I917c4cce46461ed15212a86dc01b703f818aba23 Signed-off-by: Martin Roth martinroth@google.com Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M payloads/external/Makefile.inc 1 file changed, 18 insertions(+), 20 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/36460/1
diff --git a/payloads/external/Makefile.inc b/payloads/external/Makefile.inc index f6417fd..cdc5552 100644 --- a/payloads/external/Makefile.inc +++ b/payloads/external/Makefile.inc @@ -17,14 +17,20 @@ ##
###################################################################### + +SEABIOS_DIR=payloads/external/SeaBIOS/seabios +SEABIOS_OUT=$(SEABIOS_DIR)/out +FILO_DIR=payloads/external/FILO/filo +FILO_OUT=$(FILO_DIR)/build + # set up payload config and version files for later inclusion ifeq ($(CONFIG_PAYLOAD_SEABIOS),y) -PAYLOAD_CONFIG=payloads/external/SeaBIOS/seabios/.config -PAYLOAD_VERSION=payloads/external/SeaBIOS/seabios/out/autoversion.h +PAYLOAD_CONFIG=$(SEABIOS_DIR)/.config +PAYLOAD_VERSION=$(SEABIOS_OUT)/autoversion.h endif ifeq ($(CONFIG_PAYLOAD_FILO),y) -PAYLOAD_CONFIG=payloads/external/FILO/filo/.config -PAYLOAD_VERSION=payloads/external/FILO/filo/build/version.h +PAYLOAD_CONFIG=$(FILO_DIR)/.config +PAYLOAD_VERSION=$(FILO_OUT)/version.h endif ifeq ($(CONFIG_PAYLOAD_DEPTHCHARGE),y) PAYLOAD_CONFIG=payloads/external/depthcharge/depthcharge/.config @@ -75,10 +81,12 @@ etc/grub.cfg-type := raw etc/grub.cfg-required := the GRUB runtime configuration file ($(CONFIG_GRUB2_RUNTIME_CONFIG_FILE))
+.PHONY: seabios depthcharge grub2 grub tint memtest u-boot uboot ipxe + # SeaBIOS
SEABIOS_CC_OFFSET=$(if $(filter %ccache,$(HOSTCC)),2,1) -payloads/external/SeaBIOS/seabios/out/bios.bin.elf seabios: $(DOTCONFIG) +$(SEABIOS_OUT)/vgabios.bin $(SEABIOS_OUT)/bios.bin.elf seabios: $(DOTCONFIG) $(PAYLOAD_CONFIG) $(MAKE) -C payloads/external/SeaBIOS \ HOSTCC="$(HOSTCC)" \ CC=$(word $(SEABIOS_CC_OFFSET),$(CC_x86_32)) \ @@ -104,10 +112,6 @@ CONFIG_ENABLE_HSUART=$(CONFIG_ENABLE_HSUART) \ CONFIG_CONSOLE_UART_BASE_ADDRESS=$(CONFIG_CONSOLE_UART_BASE_ADDRESS)
-payloads/external/SeaBIOS/seabios/out/vgabios.bin: seabios -payloads/external/SeaBIOS/seabios/.config: payloads/external/SeaBIOS/seabios/out/bios.bin.elf -payloads/external/SeaBIOS/seabios/out/autoversion.h: payloads/external/SeaBIOS/seabios/out/bios.bin.elf - # add a SeaBIOS bootorder file ifneq ($(strip $(CONFIG_SEABIOS_BOOTORDER_FILE)),) cbfs-files-y += bootorder @@ -117,7 +121,7 @@
# Depthcharge
-payloads/external/depthcharge/depthcharge/build/depthcharge.elf depthcharge: $(DOTCONFIG) $(CBFSTOOL) +payloads/external/depthcharge/depthcharge/build/depthcharge.elf depthcharge: $(DOTCONFIG) $(CBFSTOOL) $(PAYLOAD_CONFIG) $(MAKE) -C payloads/external/depthcharge \ BOARD=$(BOARD) \ MFLAGS= MAKEFLAGS= \ @@ -153,7 +157,7 @@
# FILO
-filo: +$(FILO_OUT)/filo.elf filo: $(DOTCONFIG) $(PAYLOAD_CONFIG) $(MAKE) -C payloads/external/FILO \ HOSTCC="$(HOSTCC)" \ CC="$(CC_x86_32)" LD="$(LD_x86_32)" OBJDUMP="$(OBJDUMP_x86_32)" \ @@ -161,13 +165,9 @@ CONFIG_FILO_MASTER=$(CONFIG_FILO_MASTER) \ CONFIG_FILO_STABLE=$(CONFIG_FILO_STABLE)
-payloads/external/FILO/filo/build/filo.elf: filo -payloads/external/FILO/filo/.config: filo -payloads/external/FILO/filo/build/version.h: filo - # Grub
-grub2: $(obj)/config.h +payloads/external/GRUB2/grub2/build/default_payload.elf grub2 grub: $(DOTCONFIG) $(MAKE) -C payloads/external/GRUB2 \ HOSTCC="$(HOSTCC)" \ CC="$(CC_x86_32)" LD="$(LD_x86_32)" \ @@ -179,11 +179,9 @@ CONFIG_GRUB2_REVISION_ID=$(CONFIG_GRUB2_REVISION_ID) \ CONFIG_GRUB2_EXTRA_MODULES=$(CONFIG_GRUB2_EXTRA_MODULES)
-payloads/external/GRUB2/grub2/build/default_payload.elf: grub2 - # U-Boot
-payloads/external/U-Boot/u-boot/u-boot-dtb.bin u-boot: $(DOTCONFIG) +payloads/external/U-Boot/u-boot/u-boot-dtb.bin u-boot uboot: $(DOTCONFIG) $(MAKE) -C payloads/external/U-Boot \ CONFIG_UBOOT_MASTER=$(CONFIG_UBOOT_MASTER) \ CONFIG_UBOOT_STABLE=$(CONFIG_UBOOT_STABLE) @@ -211,7 +209,7 @@ SERIAL_BAUD_RATE=$(CONFIG_TTYS0_BAUD) endif
-payloads/external/Memtest86Plus/memtest86plus/memtest: $(DOTCONFIG) +payloads/external/Memtest86Plus/memtest86plus/memtest memtest: $(DOTCONFIG) $(MAKE) -C payloads/external/Memtest86Plus all \ CC="$(CC_x86_32)" \ LD="$(LD_x86_32)" \
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36460 )
Change subject: payloads/external/Makefile.inc: Clean up targets ......................................................................
Patch Set 1: Code-Review-1
(1 comment)
https://review.coreboot.org/c/coreboot/+/36460/1/payloads/external/Makefile.... File payloads/external/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/36460/1/payloads/external/Makefile.... PS1, Line 170: $(DOTCONFIG) also needs $(obj)/config.h
Hello build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36460
to look at the new patch set (#2).
Change subject: payloads/external/Makefile.inc: Clean up targets ......................................................................
payloads/external/Makefile.inc: Clean up targets
- Combine the short target name with the filename. - All targets depend on top level dotconfig. - Payload targets depend on their payload .config. - Add .PHONY target for the short target names. - Add additional short target names grub, uboot, and memtest. - Remove reversed dependencies of config files on the output. - Set SeaBIOS vgabios target at same level as main SeaBIOS target to prevent rebuilds because of the vbios being included.
This fixes an issue with SeaBIOS rebuilding every time if SeaVGABIOS is build.
Change-Id: I917c4cce46461ed15212a86dc01b703f818aba23 Signed-off-by: Martin Roth martinroth@google.com Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M payloads/external/Makefile.inc 1 file changed, 18 insertions(+), 20 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/36460/2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36460 )
Change subject: payloads/external/Makefile.inc: Clean up targets ......................................................................
Patch Set 2: Code-Review+1
Thank you for the clean-up. Smaller commit would be nice to have.
Attention is currently required from: Arthur Heymans. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36460 )
Change subject: payloads/external/Makefile.inc: Clean up targets ......................................................................
Patch Set 2:
(1 comment)
File payloads/external/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/36460/comment/d77e4951_53090a18 PS1, Line 170: $(DOTCONFIG)
also needs $(obj)/config. […]
Done
Attention is currently required from: Arthur Heymans. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36460 )
Change subject: payloads/external/Makefile.inc: Clean up targets ......................................................................
Patch Set 2:
(2 comments)
Patchset:
PS2: Some of the minor changes look reasonable. The bigger issue (SeaBIOS rebuilding) seems to be fixed already differently upstream, though. Also, removing the intermediate, phony targets would prevent us from calling the sub-makes so these couldn't make their own decisions anymore. Not sure if that's wanted.
File payloads/external/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/36460/comment/d7e449ce_e4911555 PS2, Line 89: $(SEABIOS_OUT)/vgabios.bin $(SEABIOS_OUT)/bios.bin.elf These two sharing the recipe means the recipe will run twice if building both is requested.
Attention is currently required from: Nico Huber, Arthur Heymans. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36460 )
Change subject: payloads/external/Makefile.inc: Clean up targets ......................................................................
Patch Set 2:
(1 comment)
File payloads/external/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/36460/comment/bd3961d7_46a288b7 PS2, Line 89: $(SEABIOS_OUT)/vgabios.bin $(SEABIOS_OUT)/bios.bin.elf
These two sharing the recipe means the recipe will run twice […]
Yes, this is a problem. See CB:39188
Martin L Roth has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/36460?usp=email )
Change subject: payloads/external/Makefile.inc: Clean up targets ......................................................................
Abandoned
Patch is outdated.
-
Angel Pons (Code Review) -
Arthur Heymans (Code Review)
-
Martin L Roth (Code Review)
-
Nico Huber (Code Review)
-
Paul Menzel (Code Review)