[coreboot-gerrit] Change in coreboot[master]: payloads: Add LinuxBoot payload in u-root mode

Fri Feb 16 21:28:05 CET 2018

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/Makefile
File payloads/external/LinuxBoot/Makefile:

https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/Makefile@26
PS11, Line 26: 
> why not fail when mkdir fails?
Done

https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/Makefile@28
PS11, Line 28: 
> why not fail when tar fails?
Done

https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/Makefile@31
PS11, Line 31: config: $(kernel_dir)/.config
> depends on $(kernel_dir)
Done

https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/Makefile@33
PS11, Line 33: ifneq ($(CONFIG_LINUXBOOT_KERNEL_CON
> ifneq ($(CONFIG_LINUXBOOT_KERNEL_CONFIGFILE),"")
Done

https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/Makefile@35
PS11, Line 35: endif
> else. otherwise the default config always override it.
Done

https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/Makefile@43
PS11, Line 43: $(project_dir)/kernel-image: config
> depends on $(kernel_dir)/. […]
Done

https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/Makefile@46
PS11, Line 46: CPUS)
> don't we have CPUS variable for this?
Done

https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/Makefile@51
PS11, Line 51: x86
> ok, seems confusing though
Done

https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/Makefile@54
PS11, Line 54: $(project_dir)/kernel-image
> not a direct dependency
Done

https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/Makefile@56
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/Makefile@60
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/Makefile@61
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/Makefile@62
PS11, Line 62: 
> not needed
Done

https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/Makefile@65
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/Makefile@65
PS11, Line 65: 
> globbing will spare hidden files and directories, is this intentional?
Done

https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/Makefile@70
PS11, Line 70: 
> contains non-existent targets […]
Done

https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/targets/u-root.mk
File payloads/external/LinuxBoot/targets/u-root.mk:

https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/targets/u-root.mk@18
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/u-root.mk@29
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/u-root.mk@51
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/u-root.mk@55
PS11, Line 55: $(project_dir)/initramfs.cpio.xz: checkout
> depends on checkout
Done

https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/targets/u-root.mk@60
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/u-root.mk@60
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/u-root.mk@62
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/u-root.mk@62
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/u-root.mk@66
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/u-root.mk@68
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/u-root.mk@73
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/u-root.mk@75
PS11, Line 75: .PHONY: build checkout fetch all check
> isn't complete (e.g. […]
Done

-- 
To view, visit https://review.coreboot.org/23071
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: I3a25ff6812e046acc688cbbb203cf262ad751659
Gerrit-Change-Number: 23071
Gerrit-PatchSet: 17
Gerrit-Owner: Philipp Deppenwiese <zaolin.daisuki at gmail.com>
Gerrit-Reviewer: Alex Thiessen <alex.thiessen.de+coreboot at gmail.com>
Gerrit-Reviewer: Arthur Heymans <arthur at aheymans.xyz>
Gerrit-Reviewer: Chris K <c at chrisko.ch>
Gerrit-Reviewer: Martin Roth <martinroth at google.com>
Gerrit-Reviewer: Nico Huber <nico.h at gmx.de>
Gerrit-Reviewer: Paul Menzel <paulepanter at users.sourceforge.net>
Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki at gmail.com>
Gerrit-Reviewer: Ronald G. Minnich <rminnich at gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply at coreboot.org>
Gerrit-Reviewer: ron minnich
Gerrit-Comment-Date: Fri, 16 Feb 2018 20:28:05 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.coreboot.org/pipermail/coreboot-gerrit/attachments/20180216/580461e3/attachment.html>