June 2017 - flashrom-gerrit - mail.coreboot.org

Change in flashrom[staging]: fixup! Kill doit()
by Nico Huber (Code Review) 18 Jun '17

18 Jun '17

Nico Huber has submitted this change and it was merged. ( https://review.coreboot.org/20250 ) Change subject: fixup! Kill doit() ...................................................................... fixup! Kill doit() Fix building with CONFIG_INTERNAL=no because force_boardmismatch doesn't exist when internal is not enabled. Change-Id: Id9e715f09ef934bc36221b3e72c578ae96e0a3af Signed-off-by: Urja Rannikko <urjaman(a)gmail.com> Reviewed-on: https://review.coreboot.org/20250 Reviewed-by: Nico Huber <nico.h(a)gmx.de> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M cli_classic.c 1 file changed, 2 insertions(+), 0 deletions(-) Approvals: Nico Huber: Looks good to me, approved build bot (Jenkins): Verified diff --git a/cli_classic.c b/cli_classic.c index 0a09cfd..4c3db4d 100644 --- a/cli_classic.c +++ b/cli_classic.c @@ -553,7 +553,9 @@ flashrom_layout_set(fill_flash, layout); flashrom_flag_set(fill_flash, FLASHROM_FLAG_FORCE, !!force); +#if CONFIG_INTERNAL == 1 flashrom_flag_set(fill_flash, FLASHROM_FLAG_FORCE_BOARDMISMATCH, !!force_boardmismatch); +#endif flashrom_flag_set(fill_flash, FLASHROM_FLAG_VERIFY_AFTER_WRITE, !dont_verify_it); flashrom_flag_set(fill_flash, FLASHROM_FLAG_VERIFY_WHOLE_CHIP, !dont_verify_all); -- To view, visit https://review.coreboot.org/20250 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: merged Gerrit-Change-Id: Id9e715f09ef934bc36221b3e72c578ae96e0a3af Gerrit-Change-Number: 20250 Gerrit-PatchSet: 3 Gerrit-Owner: Urja Rannikko <urjaman(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Urja Rannikko <urjaman(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>

1 0

Change in flashrom[staging]: fixup! Kill doit()
by build bot (Jenkins) (Code Review) 18 Jun '17

18 Jun '17

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/20250 ) Change subject: fixup! Kill doit() ...................................................................... Patch Set 2: Verified+1 Build Successful https://qa.coreboot.org/job/flashrom-customrules/390/ : SUCCESS -- To view, visit https://review.coreboot.org/20250 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: Id9e715f09ef934bc36221b3e72c578ae96e0a3af Gerrit-Change-Number: 20250 Gerrit-PatchSet: 2 Gerrit-Owner: Urja Rannikko <urjaman(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Urja Rannikko <urjaman(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Sun, 18 Jun 2017 10:07:52 +0000 Gerrit-HasComments: No

1 0

Change in flashrom[staging]: fixup! Kill doit()
by Urja Rannikko (Code Review) 18 Jun '17

18 Jun '17

Hello build bot (Jenkins), Nico Huber, I'd like you to reexamine a change. Please visit https://review.coreboot.org/20250 to look at the new patch set (#2). Change subject: fixup! Kill doit() ...................................................................... fixup! Kill doit() Fix building with CONFIG_INTERNAL=no because force_boardmismatch doesn't exist when internal is not enabled. Change-Id: Id9e715f09ef934bc36221b3e72c578ae96e0a3af Signed-off-by: Urja Rannikko <urjaman(a)gmail.com> --- M cli_classic.c 1 file changed, 2 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/50/20250/2 -- To view, visit https://review.coreboot.org/20250 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: newpatchset Gerrit-Change-Id: Id9e715f09ef934bc36221b3e72c578ae96e0a3af Gerrit-Change-Number: 20250 Gerrit-PatchSet: 2 Gerrit-Owner: Urja Rannikko <urjaman(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Urja Rannikko <urjaman(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>

1 0

Change in flashrom[staging]: Convert flashrom to git
by Nico Huber (Code Review) 17 Jun '17

17 Jun '17

Nico Huber has posted comments on this change. ( https://review.coreboot.org/19206 ) Change subject: Convert flashrom to git ...................................................................... Patch Set 4: (10 comments) Looks pretty good overall, except for the complex getrevision.sh changes. Splitting it out into a follow-up would get the other changes in (with a minor update, see inline comments), I suppose. https://review.coreboot.org/#/c/19206/4/Makefile File Makefile: https://review.coreboot.org/#/c/19206/4/Makefile@a1374 PS4, Line 1374: Why didn't I bikeshed this for being totally unrelated? https://review.coreboot.org/#/c/19206/4/Makefile@1396 PS4, Line 1396: @tar -cz --format=ustar -f $(EXPORTDIR)/flashrom-$(RELEASENAME).tar.gz -C $(EXPORTDIR)/ \ : $(TAROPTIONS) flashrom-$(RELEASENAME)/ : # Delete the exported directory again because it is most likely what's expected by the user. : @rm -rf $(EXPORTDIR)/flashrom-$(RELEASENAME) Missing quotes around everything with $(RELEASENAME). https://review.coreboot.org/#/c/19206/1/util/getrevision.sh File util/getrevision.sh: https://review.coreboot.org/#/c/19206/1/util/getrevision.sh@66 PS1, Line 66: "to the commit history from an upstream repository. If no git remote"\ > Done Looks good, but did you test it? Using echo this way there shouldn't be any line breaks in the output at all. https://review.coreboot.org/#/c/19206/1/util/getrevision.sh@69 PS1, Line 69: > not needed? Why not? The message should draw the user's attention and isn't usual program output (i.e. make progress). https://review.coreboot.org/#/c/19206/1/util/getrevision.sh@224 PS1, Line 224: urn > I'm wondering if this is all too elaborate. I'd rather keep it simple and s Sounds good, I was all for splitting from the beginning ;) We could just update getrevision.sh to use git for a start. Add the upcache + complex version string (we should start a discussion on the ML probably) later. https://review.coreboot.org/#/c/19206/1/util/getrevision.sh@224 PS1, Line 224: return : fi : done : } : : revision() { : local sha=$(git_last_commit "$1" 2>/dev/null) : # > What we'd need here instead is What I meant here is roughly: In the `-n "$up_remote"` case, we only construct $up_refs based on registered remotes and $upstream_branches. If the user just registered the upstream remote without ever fetching it, $up_refs will contain refs unknown to git. What we need to do is kind of force_update_upcache but for the detected already present remotes. We could spare us all that hassle and some of the already written code if we'd always use an upcache. https://review.coreboot.org/#/c/19206/1/util/getrevision.sh@257 PS1, Line 257: update_upcache || { echo "offline" ; return ; } : up_refs=$upcache_refs : fi : : # Find nearest commit contained in this branch that is also in any of the up_refs, i.e. the branch point : # of the current branch. This might be the latest commit if it's in any of the upstream branches. : loca > TBH, I'm not sure what to do about that here :-/ Don't worry. It's only a cosmetic issue. And I don't remember it accurately. https://review.coreboot.org/#/c/19206/1/util/getrevision.sh@269 PS1, Line 269: > Yep, now it is! Then we don't need the `if`. https://review.coreboot.org/#/c/19206/4/util/getrevision.sh File util/getrevision.sh: https://review.coreboot.org/#/c/19206/4/util/getrevision.sh@72 PS4, Line 72: "$upstream_url"\ Maybe indent it a little. https://review.coreboot.org/#/c/19206/1/util/git-hooks/pre-push File util/git-hooks/pre-push: https://review.coreboot.org/#/c/19206/1/util/git-hooks/pre-push@53 PS1, Line 53: "$commit" ]; then > If we're going that route, "Neither Signoff nor Ack were found..." would so Sounds better but is not what I meant: Both have to be present. -- To view, visit https://review.coreboot.org/19206 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: I64eef21982cac0a0a7419bcd2c8a936672ae9cb2 Gerrit-Change-Number: 19206 Gerrit-PatchSet: 4 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Philippe Mathieu-Daudé <philippe.mathieu.daude(a)gmail.com> Gerrit-Reviewer: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Reviewer: david.hendricks(a)gmail.com Gerrit-Comment-Date: Sat, 17 Jun 2017 13:31:43 +0000 Gerrit-HasComments: Yes

1 0

Change in flashrom[staging]: fixup of "Kill doit()"
by Nico Huber (Code Review) 17 Jun '17

17 Jun '17

Nico Huber has posted comments on this change. ( https://review.coreboot.org/20250 ) Change subject: fixup of "Kill doit()" ...................................................................... Patch Set 1: (2 comments) https://review.coreboot.org/#/c/20250/1//COMMIT_MSG Commit Message: https://review.coreboot.org/#/c/20250/1//COMMIT_MSG@7 PS1, Line 7: fixup of "Kill doit()" fixup! Kill doit() https://review.coreboot.org/#/c/20250/1//COMMIT_MSG@10 PS1, Line 10: doesnt doesn't -- To view, visit https://review.coreboot.org/20250 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: Id9e715f09ef934bc36221b3e72c578ae96e0a3af Gerrit-Change-Number: 20250 Gerrit-PatchSet: 1 Gerrit-Owner: Urja Rannikko <urjaman(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Urja Rannikko <urjaman(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Sat, 17 Jun 2017 12:06:37 +0000 Gerrit-HasComments: Yes

1 0

Change in flashrom[staging]: fixup of "Kill doit()"
by Nico Huber (Code Review) 17 Jun '17

17 Jun '17

Nico Huber has posted comments on this change. ( https://review.coreboot.org/20250 ) Change subject: fixup of "Kill doit()" ...................................................................... Patch Set 1: Code-Review+2 > Re: the guards, then you'd need to build internal.o always (since > Makefile doesnt build it if no CONFIG_INTERNAL), and include an > useless piece of .bss in the binary. You dislike guards that much? I wouldn't mind the wasted .bss space when it makes the code more readable. I wrongly assumed we'd always compile internal.c (guess why, it has CONFIG_INTERNAL guards in it, OMG). Since it's not, your patch looks suddenly good :-P > Oh, the handling of that flag seems all kinds of weird to me, in > general. My fault. I needed to expose it through the library interface. Don't know why I didn't realize that the flashctx is the wrong place for the flags back then. Probably because there was no other context... > I could go with not having it in the flashctx, which would make it > an internal programmer parameter that flashrom_image_write happens > to look at (but there's guards there for it anyways already). > (Having it only in flashctx comes with the issue that the flashctx > doesnt exist when initializing the programmer...) The mid-term idea is to either make flashctx always available or add a programmer context (cf. `struct flash_programmer` in libflashrom.h). > Or it could be split into two parameters, but ... ehh, i'm not even > sure if having the flashctx part makes sense without internal, etc. Eh, no. It should just move (together with all other global variables) into a programmer context. > > This is very confusing to me and I'm here to fix the build, not fix > the architechture :P Agreed. -- To view, visit https://review.coreboot.org/20250 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: Id9e715f09ef934bc36221b3e72c578ae96e0a3af Gerrit-Change-Number: 20250 Gerrit-PatchSet: 1 Gerrit-Owner: Urja Rannikko <urjaman(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Urja Rannikko <urjaman(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Sat, 17 Jun 2017 11:57:59 +0000 Gerrit-HasComments: No

1 0

Change in flashrom[staging]: fixup of "Kill doit()"
by Urja Rannikko (Code Review) 17 Jun '17

17 Jun '17

Urja Rannikko has posted comments on this change. ( https://review.coreboot.org/20250 ) Change subject: fixup of "Kill doit()" ...................................................................... Patch Set 1: Oh, the handling of that flag seems all kinds of weird to me, in general. I could go with not having it in the flashctx, which would make it an internal programmer parameter that flashrom_image_write happens to look at (but there's guards there for it anyways already). (Having it only in flashctx comes with the issue that the flashctx doesnt exist when initializing the programmer...) Or it could be split into two parameters, but ... ehh, i'm not even sure if having the flashctx part makes sense without internal, etc. This is very confusing to me and I'm here to fix the build, not fix the architechture :P -- To view, visit https://review.coreboot.org/20250 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: Id9e715f09ef934bc36221b3e72c578ae96e0a3af Gerrit-Change-Number: 20250 Gerrit-PatchSet: 1 Gerrit-Owner: Urja Rannikko <urjaman(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Urja Rannikko <urjaman(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Sat, 17 Jun 2017 11:09:04 +0000 Gerrit-HasComments: No

1 0

Change in flashrom[staging]: fixup of "Kill doit()"
by Urja Rannikko (Code Review) 17 Jun '17

17 Jun '17

Urja Rannikko has posted comments on this change. ( https://review.coreboot.org/20250 ) Change subject: fixup of "Kill doit()" ...................................................................... Patch Set 1: (re: the message, yeah you could but that wasnt the obvious workflow, actually I was very confused...) Re: the guards, then you'd need to build internal.o always (since Makefile doesnt build it if no CONFIG_INTERNAL), and include an useless piece of .bss in the binary. You dislike guards that much? -- To view, visit https://review.coreboot.org/20250 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: Id9e715f09ef934bc36221b3e72c578ae96e0a3af Gerrit-Change-Number: 20250 Gerrit-PatchSet: 1 Gerrit-Owner: Urja Rannikko <urjaman(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Urja Rannikko <urjaman(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Sat, 17 Jun 2017 10:50:53 +0000 Gerrit-HasComments: No

1 0

Change in flashrom[staging]: fixup of "Kill doit()"
by Nico Huber (Code Review) 17 Jun '17

17 Jun '17

Nico Huber has posted comments on this change. ( https://review.coreboot.org/20250 ) Change subject: fixup of "Kill doit()" ...................................................................... Patch Set 1: (1 comment) Well, you can fix up the commit message after you got your Change-Id... Hmmm, I used this once, don't remember any trouble though. Maybe I used a different commit-msg hook, dunno. https://review.coreboot.org/#/c/20250/1/cli_classic.c File cli_classic.c: https://review.coreboot.org/#/c/20250/1/cli_classic.c@556 PS1, Line 556: #if CONFIG_INTERNAL == 1 Ugh, guards... How about moving the `force_boardmismatch` declaration and definition out of their respective guards? -- To view, visit https://review.coreboot.org/20250 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: Id9e715f09ef934bc36221b3e72c578ae96e0a3af Gerrit-Change-Number: 20250 Gerrit-PatchSet: 1 Gerrit-Owner: Urja Rannikko <urjaman(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Urja Rannikko <urjaman(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Sat, 17 Jun 2017 10:31:33 +0000 Gerrit-HasComments: Yes

1 0

Change in flashrom[staging]: Enable continuous SPI reads
by build bot (Jenkins) (Code Review) 17 Jun '17

17 Jun '17

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/20223 ) Change subject: Enable continuous SPI reads ...................................................................... Patch Set 4: Verified+1 Build Successful https://qa.coreboot.org/job/flashrom-customrules/389/ : SUCCESS -- To view, visit https://review.coreboot.org/20223 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: Iadf909c9216578b1c5dacd4c4991bb436e32edc9 Gerrit-Change-Number: 20223 Gerrit-PatchSet: 4 Gerrit-Owner: Urja Rannikko <urjaman(a)gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Reviewer: Urja Rannikko <urjaman(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Sat, 17 Jun 2017 09:07:03 +0000 Gerrit-HasComments: No

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit June 2017