Alan Green has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/33931
Change subject: flashchips.c: Sort file by vendor and model ......................................................................
flashchips.c: Sort file by vendor and model
For self-consistency, and to allow tools to assist with merging the chromium fork of flashrom, sort the entries of flashchips.c. The file is already largely sorted, though deviations have crept in over time.
This is a non-clever mostly ASCII-order sorting. It is not intended to be permanent. Post-merge we can explore options such as breaking apart flashchips.c into per-vendor files or directories.
Change-Id: I75a99583592526f60ba5264e92391bf8b1213b20 --- M flashchips.c 1 file changed, 5,510 insertions(+), 5,510 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/31/33931/1
Hello Edward O'Callaghan, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/33931
to look at the new patch set (#2).
Change subject: flashchips.c: Sort file by vendor and model ......................................................................
flashchips.c: Sort file by vendor and model
For self-consistency, and to allow tools to assist with merging the chromium fork of flashrom, sort the entries of flashchips.c. The file is already largely sorted, though deviations have crept in over time.
This is a non-clever mostly ASCII-order sorting. It is not intended to be permanent. Post-merge we can explore options such as breaking apart flashchips.c into per-vendor files or directories.
Change-Id: I75a99583592526f60ba5264e92391bf8b1213b20 --- M flashchips.c 1 file changed, 5,510 insertions(+), 5,510 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/31/33931/2
Hello Edward O'Callaghan, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/33931
to look at the new patch set (#3).
Change subject: flashchips.c: Sort file by vendor and model ......................................................................
flashchips.c: Sort file by vendor and model
For self-consistency, and to allow tools to assist with merging the chromium fork of flashrom, sort the entries of flashchips.c. The file is already largely sorted, though deviations have crept in over time.
This is a non-clever mostly ASCII-order sorting. It is not intended to be permanent. Post-merge we can explore options such as breaking apart flashchips.c into per-vendor files or directories.
Change-Id: I75a99583592526f60ba5264e92391bf8b1213b20 --- M flashchips.c 1 file changed, 5,510 insertions(+), 5,510 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/31/33931/3
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33931 )
Change subject: flashchips.c: Sort file by vendor and model ......................................................................
Patch Set 3: Code-Review+2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33931 )
Change subject: flashchips.c: Sort file by vendor and model ......................................................................
Patch Set 3: Code-Review-1
(1 comment)
This doesn't seem to adhere to the currently defined order. I wouldn't mind to change that definition, though. Also, the post-merge plans are better discussed on the mailing list. There are other people that have plans too :) For instance during the OSF Hackathon in June [1], we discussed making it an easier to handle ASCII file, that can (optionally) be compiled into C.
[1] http://docs.google.com/document/d/18qKvEbfPszjsJJGJhwi8kRVDUG3GZkADzQSH6WFsK...
https://review.coreboot.org/#/c/33931/3/flashchips.c File flashchips.c:
https://review.coreboot.org/#/c/33931/3/flashchips.c@29 PS3, Line 29: Within families keep them in order of density. I suppose this is not true anymore after this patch?
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33931 )
Change subject: flashchips.c: Sort file by vendor and model ......................................................................
Patch Set 3:
Should a Signed-off-by line to the commit message?
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33931 )
Change subject: flashchips.c: Sort file by vendor and model ......................................................................
Patch Set 3:
Patch Set 3: Code-Review-1
(1 comment)
This doesn't seem to adhere to the currently defined order. I wouldn't mind to change that definition, though. Also, the post-merge plans are better discussed on the mailing list. There are other people that have plans too :) For instance during the OSF Hackathon in June [1], we discussed making it an easier to handle ASCII file, that can (optionally) be compiled into C.
[1] http://docs.google.com/document/d/18qKvEbfPszjsJJGJhwi8kRVDUG3GZkADzQSH6WFsK...
Hi Nico,
Long time, hope you have been well?
I actually had a similar idea of a mini DSL that describes chips and breaking up the configs into a per-vendor thing. It would be good to discuss that more as I should be back around more again these days!
Alan worked on this in collaboration with me. I am trying to get rid of the Chromium flashrom fork and get all the extra chip (+chipset) support from that into upstream. Unfortunately they have diverged *significantly* [9+ years] so it has been a real battle. Alan was able to contrive a tool that allowed us to reconcile many of the differences in this specific file however there are some conditions to be able to reach the point of convergence, this obviously being one of them.
More precisely, I think if we can come up with a ordering function that produces reproducible results of ordering that would also work well however this was Alan's best first go at getting a ordering that works enough to make progress merging the two flashchips.c implementations.
Once we have all the chip definitions in one place again we can move forward with greater and better plans on a nicer representation. I hadn't seen that doc until now however I don't see a problem with this specific blocking that plan from happening, in fact this could help aid in that since the ordering and cleanups make the file consistent enough now to be parsed up into a data structure and written out to ASCII or whatever representation we decide to choice.
Cheers, Edward.
Hello Edward O'Callaghan, David Hendricks, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/33931
to look at the new patch set (#4).
Change subject: flashchips.c: Sort file by vendor and model ......................................................................
flashchips.c: Sort file by vendor and model
For self-consistency, and to allow tools to assist with merging the chromium fork of flashrom, sort the entries of flashchips.c. The file is already largely sorted, though deviations have crept in over time.
This is a non-clever mostly ASCII-order sorting. It is not intended to be permanent.
Change-Id: I75a99583592526f60ba5264e92391bf8b1213b20 --- M flashchips.c 1 file changed, 5,515 insertions(+), 5,512 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/31/33931/4
Hello Edward O'Callaghan, David Hendricks, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/33931
to look at the new patch set (#5).
Change subject: flashchips.c: Sort file by vendor and model ......................................................................
flashchips.c: Sort file by vendor and model
For self-consistency, and to allow tools to assist with merging the chromium fork of flashrom, sort the entries of flashchips.c. The file is already largely sorted, though deviations have crept in over time.
This is a non-clever mostly ASCII-order sorting. It is not intended to be permanent.
Change-Id: I75a99583592526f60ba5264e92391bf8b1213b20 --- M flashchips.c 1 file changed, 5,515 insertions(+), 5,512 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/31/33931/5
Alan Green has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33931 )
Change subject: flashchips.c: Sort file by vendor and model ......................................................................
Patch Set 5:
(1 comment)
Patch Set 3: Code-Review-1
(1 comment)
This doesn't seem to adhere to the currently defined order. I wouldn't mind to change that definition, though. Also, the post-merge plans are better discussed on the mailing list. There are other people that have plans too :) For instance during the OSF Hackathon in June [1], we discussed making it an easier to handle ASCII file, that can (optionally) be compiled into C.
[1] http://docs.google.com/document/d/18qKvEbfPszjsJJGJhwi8kRVDUG3GZkADzQSH6WFsK...
Thank you for the additional context. It is helpful.
I removed mention of possible future directions from the change description to avoid confusion.
https://review.coreboot.org/#/c/33931/3/flashchips.c File flashchips.c:
https://review.coreboot.org/#/c/33931/3/flashchips.c@29 PS3, Line 29: Within families keep them in order of density.
I suppose this is not true anymore after this patch?
You're right. Thank you for pointing it out. I update the comment to match the new reality.
Alan Green has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33931 )
Change subject: flashchips.c: Sort file by vendor and model ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/33931/3/flashchips.c File flashchips.c:
https://review.coreboot.org/#/c/33931/3/flashchips.c@29 PS3, Line 29: Within families keep them in order of density.
You're right. Thank you for pointing it out. I update the comment to match the new reality.
Done
Hello Edward O'Callaghan, David Hendricks, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/33931
to look at the new patch set (#7).
Change subject: flashchips.c: Sort file by vendor and model ......................................................................
flashchips.c: Sort file by vendor and model
For self-consistency, and to allow tools to assist with merging the chromium fork of flashrom, sort the entries of flashchips.c. The file is already largely sorted, though deviations have crept in over time.
This is a non-clever mostly ASCII-order sorting. It is not intended to be permanent.
Change-Id: I75a99583592526f60ba5264e92391bf8b1213b20 --- M flashchips.c 1 file changed, 5,515 insertions(+), 5,512 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/31/33931/7
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33931 )
Change subject: flashchips.c: Sort file by vendor and model ......................................................................
Patch Set 7: Code-Review+1
Hi Nico, are we good to go on this particular one?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33931 )
Change subject: flashchips.c: Sort file by vendor and model ......................................................................
Patch Set 7:
Patch Set 7: Code-Review+1
Hi Nico, are we good to go on this particular one?
Absolutely not. Did you review the 5k lines patch? I would do some testing on my own, probably add a few lines of C that prints the database to compare before/after the patch.
It's good that you ask btw. Your sudden appearance in the flashrom developers group in Gerrit already caused some friction. I don't know if any other member knows about that? I know that you are up to the task, it just caught me by surprise. And technically, with zero contributions to flashrom proper, you wouldn't even be in a reviewer group if we had one separate from coreboot's ;)
Alan Green has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33931 )
Change subject: flashchips.c: Sort file by vendor and model ......................................................................
Patch Set 7:
Patch Set 7:
Patch Set 7: Code-Review+1
Hi Nico, are we good to go on this particular one?
Absolutely not. Did you review the 5k lines patch? I would do some testing on my own, probably add a few lines of C that prints the database to compare before/after the patch.
Hi Nico,
Here are some notes about how this patch was made, and how I came to be confident that it only moves entries in the flashchips array rather than duplicating or omitting any.
The patch was generated from python script that parses entries out of flashchips.c. The script is currently out for review at https://chromium-review.googlesource.com/c/chromiumos/third_party/flashrom/+.... It may or may not be submitted.
I first tested that the script could read all the entries from flashchips.c and then rewrite the file without differences. I then used "rewrite --sort" to generate a version of flashchips.c with the entries sorted.
I've run a number of checks on the resulting file to ensure there are no differences in the flashchips array apart from the order of the entries.
The most comprehensive check involved a small awk script which constructs one line per entry in the flashchips array. I ran this script against the original flashchips.c and the sorted flashchips.c, to produce two files of 532 lines each:
$ awk '/^.{/ {x=$0; inside = 1} {if (inside) {x=x $0}} /^.}/ {inside = 0; print x}' orig_flashchips.c > orig_entries
$ awk '/^.{/ {x=$0; inside = 1} {if (inside) {x=x $0}} /^.}/ {inside = 0; print x}' sorted_flashchips.c > sorted_entries
$ wc -l {orig,sorted}_entries 532 orig_entries 532 sorted_entries 1064 total
532 is the expected number. flashrom -L reports 519 chips, and there are also 11 entries for chips with known vendors but unknown names, 1 entry for "Unknown" "SFDP-capable chip" and 1 entry for "Programmer" "Opaque flash chip". (flashrom -L's counting takes place in print.c in print_supported_chips().)
By sorting and diffing each of {orig,sorted}_entries files, I was able to verify that for every line in the orig_entries file there was an identical line in the sorted_entries file.
This shows that the entries in the before and after versions of flashchips.c are textually identical, apart from their order.
It's good that you ask btw. Your sudden appearance in the flashrom developers group in Gerrit already caused some friction. I don't know if any other member knows about that? I know that you are up to the task, it just caught me by surprise. And technically, with zero contributions to flashrom proper, you wouldn't even be in a reviewer group if we had one separate from coreboot's ;)
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33931 )
Change subject: flashchips.c: Sort file by vendor and model ......................................................................
Patch Set 7: Code-Review+2
[...] By sorting and diffing each of {orig,sorted}_entries files, I was able to verify that for every line in the orig_entries file there was an identical line in the sorted_entries file.
This shows that the entries in the before and after versions of flashchips.c are textually identical, apart from their order.
Thanks Alan for the elaborate description. However, my inner stickler says that such checking should be left to the reviewer. My second idea was pretty much like what you did with awk, so I had to come up with something else: I used `csplit` to extract the entries and some sed-foo to name each file after the vendor-chip and compared the resulting directories with `diff -r`: no complaints :)
I guess all that together is enough testing.
Nico Huber has submitted this change and it was merged. ( https://review.coreboot.org/c/flashrom/+/33931 )
Change subject: flashchips.c: Sort file by vendor and model ......................................................................
flashchips.c: Sort file by vendor and model
For self-consistency, and to allow tools to assist with merging the chromium fork of flashrom, sort the entries of flashchips.c. The file is already largely sorted, though deviations have crept in over time.
This is a non-clever mostly ASCII-order sorting. It is not intended to be permanent.
Change-Id: I75a99583592526f60ba5264e92391bf8b1213b20 Reviewed-on: https://review.coreboot.org/c/flashrom/+/33931 Reviewed-by: Edward O'Callaghan quasisec@chromium.org Reviewed-by: Nico Huber nico.h@gmx.de Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M flashchips.c 1 file changed, 5,515 insertions(+), 5,512 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Edward O'Callaghan: Looks good to me, but someone else must approve
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33931 )
Change subject: flashchips.c: Sort file by vendor and model ......................................................................
Patch Set 8:
Oops, this introduced a bug: the SFDP entry is no longer at the end of flashchips.c, so probing on a SFDP-capable Winbond chip results in added noise (flashrom says things about an unknown chip, and then has two definitions for the same chip).
It's trivial to fix, but it might affect the codebase merging.
Alan Green has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33931 )
Change subject: flashchips.c: Sort file by vendor and model ......................................................................
Patch Set 8:
Patch Set 8:
Oops, this introduced a bug: the SFDP entry is no longer at the end of flashchips.c, so probing on a SFDP-capable Winbond chip results in added noise (flashrom says things about an unknown chip, and then has two definitions for the same chip).
It's trivial to fix, but it might affect the codebase merging.
Oops indeed. I'll send a patch to put it back at the end.
Alan Green has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33931 )
Change subject: flashchips.c: Sort file by vendor and model ......................................................................
Patch Set 8:
Patch Set 8:
Patch Set 8:
Oops, this introduced a bug: the SFDP entry is no longer at the end of flashchips.c, so probing on a SFDP-capable Winbond chip results in added noise (flashrom says things about an unknown chip, and then has two definitions for the same chip).
It's trivial to fix, but it might affect the codebase merging.
Oops indeed. I'll send a patch to put it back at the end.
I have sent https://review.coreboot.org/c/flashrom/+/35037 for review.
-
Alan Green (Code Review) -
Angel Pons (Code Review)
-
Edward O'Callaghan (Code Review)
-
Nico Huber (Code Review)
-
Paul Menzel (Code Review)