<p><a href="https://review.coreboot.org/23071">View Change</a></p><p>28 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/Makefile">File payloads/external/LinuxBoot/Makefile:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/Makefile@26">Patch Set #11, Line 26:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">why not fail when mkdir fails?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/Makefile@28">Patch Set #11, Line 28:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">why not fail when tar fails?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/Makefile@31">Patch Set #11, Line 31:</a> <code style="font-family:monospace,monospace">config: $(kernel_dir)/.config</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">depends on $(kernel_dir)</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/Makefile@33">Patch Set #11, Line 33:</a> <code style="font-family:monospace,monospace">ifneq ($(CONFIG_LINUXBOOT_KERNEL_CON</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">ifneq ($(CONFIG_LINUXBOOT_KERNEL_CONFIGFILE),"")</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/Makefile@35">Patch Set #11, Line 35:</a> <code style="font-family:monospace,monospace">endif</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">else. otherwise the default config always override it.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/Makefile@43">Patch Set #11, Line 43:</a> <code style="font-family:monospace,monospace">$(project_dir)/kernel-image: config</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">depends on $(kernel_dir)/. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/Makefile@46">Patch Set #11, Line 46:</a> <code style="font-family:monospace,monospace">CPUS)</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">don't we have CPUS variable for this?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/Makefile@51">Patch Set #11, Line 51:</a> <code style="font-family:monospace,monospace">x86</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">ok, seems confusing though</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/Makefile@54">Patch Set #11, Line 54:</a> <code style="font-family:monospace,monospace">$(project_dir)/kernel-image</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">not a direct dependency</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/Makefile@56">Patch Set #11, Line 56:</a> <code style="font-family:monospace,monospace">$(MAKE)</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I mean, does it make sense to maybe translate `CPUS` be to `--jobs` here? Will it accelerate anythin […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/Makefile@60">Patch Set #11, Line 60:</a> <code style="font-family:monospace,monospace">test -e $(project_dir)/kernel-image &</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">isn't needed, its result isn't even used</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/Makefile@61">Patch Set #11, Line 61:</a> <code style="font-family:monospace,monospace">test -e $(project_dir)/initramfs.cpio.xz &</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">isn't needed, its result isn't even used</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/Makefile@62">Patch Set #11, Line 62:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">not needed</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/Makefile@65">Patch Set #11, Line 65:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">If deleting fails, `make distclean` should fail too, shouldn't it? If so, no `exit 0` should be here […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/Makefile@65">Patch Set #11, Line 65:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">globbing will spare hidden files and directories, is this intentional?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/Makefile@70">Patch Set #11, Line 70:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">contains non-existent targets […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/targets/u-root.mk">File payloads/external/LinuxBoot/targets/u-root.mk:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/targets/u-root.mk@18">Patch Set #11, Line 18:</a> <code style="font-family:monospace,monospace">ir=$(shell pwd)/linuxboot</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">command -v go 1>/dev/null 2>&1 && echo go […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/targets/u-root.mk@29">Patch Set #11, Line 29:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Was the intent here that we not fail if the directory exists? mkdir -p won't fail in that case: […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/targets/u-root.mk@51">Patch Set #11, Line 51:</a> <code style="font-family:monospace,monospace">exit 1</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">did you test whether a `git am` failure will make `make` stop here? Cause it's in a subshell...</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/targets/u-root.mk@55">Patch Set #11, Line 55:</a> <code style="font-family:monospace,monospace">$(project_dir)/initramfs.cpio.xz: checkout</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">depends on checkout</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/targets/u-root.mk@60">Patch Set #11, Line 60:</a> <code style="font-family:monospace,monospace">  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)}</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">a too long line</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/targets/u-root.mk@60">Patch Set #11, Line 60:</a> <code style="font-family:monospace,monospace">$(project_dir)</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">where is it going, I don't know</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/targets/u-root.mk@62">Patch Set #11, Line 62:</a> <code style="font-family:monospace,monospace">{$(CONFIG_LINUXBOOT_UROOT_COMMANDS)}</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">my `dash` doesn't support the '{}' notation</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/targets/u-root.mk@62">Patch Set #11, Line 62:</a> <code style="font-family:monospace,monospace">        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)}</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">a too long line</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/targets/u-root.mk@66">Patch Set #11, Line 66:</a> <code style="font-family:monospace,monospace">   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</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">a too long line</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/targets/u-root.mk@68">Patch Set #11, Line 68:</a> <code style="font-family:monospace,monospace">        cd $(uroot_dir); GOARCH=$(CONFIG_LINUXBOOT_ARCH) GOPATH=$(go_path_dir) ./u-root -build=bb -o $(project_dir)/initramfs.cpio</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">a too long line</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/targets/u-root.mk@73">Patch Set #11, Line 73:</a> <code style="font-family:monospace,monospace">$(projec</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">not a direct dependency, this is likely to break parallel build</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/23071/11/payloads/external/LinuxBoot/targets/u-root.mk@75">Patch Set #11, Line 75:</a> <code style="font-family:monospace,monospace">.PHONY: build checkout fetch all check</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">isn't complete (e.g. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li></ul><p>To view, visit <a href="https://review.coreboot.org/23071">change 23071</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://review.coreboot.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://review.coreboot.org/23071"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: coreboot </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: I3a25ff6812e046acc688cbbb203cf262ad751659 </div>
<div style="display:none"> Gerrit-Change-Number: 23071 </div>
<div style="display:none"> Gerrit-PatchSet: 17 </div>
<div style="display:none"> Gerrit-Owner: Philipp Deppenwiese <zaolin.daisuki@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: Alex Thiessen <alex.thiessen.de+coreboot@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> </div>
<div style="display:none"> Gerrit-Reviewer: Chris K <c@chrisko.ch> </div>
<div style="display:none"> Gerrit-Reviewer: Martin Roth <martinroth@google.com> </div>
<div style="display:none"> Gerrit-Reviewer: Nico Huber <nico.h@gmx.de> </div>
<div style="display:none"> Gerrit-Reviewer: Paul Menzel <paulepanter@users.sourceforge.net> </div>
<div style="display:none"> Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: Ronald G. Minnich <rminnich@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> </div>
<div style="display:none"> Gerrit-Reviewer: ron minnich </div>
<div style="display:none"> Gerrit-Comment-Date: Fri, 16 Feb 2018 20:28:05 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: No </div>