Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/23071 )
Change subject: payloads: Add LinuxBoot payload in u-root mode ......................................................................
Patch Set 17:
(28 comments)
https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/Makefil... File payloads/external/LinuxBoot/Makefile:
https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/Makefil... PS11, Line 26:
why not fail when mkdir fails?
Done
https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/Makefil... PS11, Line 28:
why not fail when tar fails?
Done
https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/Makefil... PS11, Line 31: config: $(kernel_dir)/.config
depends on $(kernel_dir)
Done
https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/Makefil... PS11, Line 33: ifneq ($(CONFIG_LINUXBOOT_KERNEL_CON
ifneq ($(CONFIG_LINUXBOOT_KERNEL_CONFIGFILE),"")
Done
https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/Makefil... PS11, Line 35: endif
else. otherwise the default config always override it.
Done
https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/Makefil... PS11, Line 43: $(project_dir)/kernel-image: config
depends on $(kernel_dir)/. […]
Done
https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/Makefil... PS11, Line 46: CPUS)
don't we have CPUS variable for this?
Done
https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/Makefil... PS11, Line 51: x86
ok, seems confusing though
Done
https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/Makefil... PS11, Line 54: $(project_dir)/kernel-image
not a direct dependency
Done
https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/Makefil... PS11, Line 56: $(MAKE)
I mean, does it make sense to maybe translate `CPUS` be to `--jobs` here? Will it accelerate anythin […]
Done
https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/Makefil... PS11, Line 60: test -e $(project_dir)/kernel-image &
isn't needed, its result isn't even used
Done
https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/Makefil... PS11, Line 61: test -e $(project_dir)/initramfs.cpio.xz &
isn't needed, its result isn't even used
Done
https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/Makefil... PS11, Line 62:
not needed
Done
https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/Makefil... PS11, Line 65:
If deleting fails, `make distclean` should fail too, shouldn't it? If so, no `exit 0` should be here […]
Done
https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/Makefil... PS11, Line 65:
globbing will spare hidden files and directories, is this intentional?
Done
https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/Makefil... PS11, Line 70:
contains non-existent targets […]
Done
https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/targets... File payloads/external/LinuxBoot/targets/u-root.mk:
https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/targets... PS11, Line 18: ir=$(shell pwd)/linuxboot
command -v go 1>/dev/null 2>&1 && echo go […]
Done
https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/targets... PS11, Line 29:
Was the intent here that we not fail if the directory exists? mkdir -p won't fail in that case: […]
Done
https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/targets... PS11, Line 51: exit 1
did you test whether a `git am` failure will make `make` stop here? Cause it's in a subshell...
Done
https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/targets... PS11, Line 55: $(project_dir)/initramfs.cpio.xz: checkout
depends on checkout
Done
https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/targets... PS11, Line 60: cd $(uroot_dir); GOARCH=$(CONFIG_LINUXBOOT_ARCH) GOPATH=$(go_path_dir) ./u-root -build=bb -files $(CONFIG_LINUXBOOT_UROOT_FILES) -o $(project_dir)/initramfs.cpio ./cmds/{$(CONFIG_LINUXBOOT_UROOT_COMMANDS)}
a too long line
Done
https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/targets... PS11, Line 60: $(project_dir)
where is it going, I don't know
Done
https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/targets... PS11, Line 62: {$(CONFIG_LINUXBOOT_UROOT_COMMANDS)}
my `dash` doesn't support the '{}' notation
Done
https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/targets... PS11, Line 62: cd $(uroot_dir); GOARCH=$(CONFIG_LINUXBOOT_ARCH) GOPATH=$(go_path_dir) ./u-root -build=bb -o $(project_dir)/initramfs.cpio ./cmds/{$(CONFIG_LINUXBOOT_UROOT_COMMANDS)}
a too long line
Done
https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/targets... PS11, Line 66: cd $(uroot_dir); GOARCH=$(CONFIG_LINUXBOOT_ARCH) GOPATH=$(go_path_dir) ./u-root -build=bb -files $(CONFIG_LINUXBOOT_UROOT_FILES) -o $(project_dir)/initramfs.cpio
a too long line
Done
https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/targets... PS11, Line 68: cd $(uroot_dir); GOARCH=$(CONFIG_LINUXBOOT_ARCH) GOPATH=$(go_path_dir) ./u-root -build=bb -o $(project_dir)/initramfs.cpio
a too long line
Done
https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/targets... PS11, Line 73: $(projec
not a direct dependency, this is likely to break parallel build
Done
https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/targets... PS11, Line 75: .PHONY: build checkout fetch all check
isn't complete (e.g. […]
Done