Attention is currently required from: Felix Singer, Thomas Heijligen, Anastasia Klimchuk.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68433 )
Change subject: tests: Add prefix to io_mock functions not to clash with macros
......................................................................
Patch Set 3: Code-Review+1
--
To view, visit https://review.coreboot.org/c/flashrom/+/68433
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I7998a8fb1b9e65621e12adbfab5460a245d5606b
Gerrit-Change-Number: 68433
Gerrit-PatchSet: 3
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Wed, 02 Nov 2022 00:51:37 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Felix Singer, Thomas Heijligen, Anastasia Klimchuk.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68432 )
Change subject: tests: Undefine _FORTIFY_SOURCE for unit tests to avoid _chk variants
......................................................................
Patch Set 3: Code-Review+1
--
To view, visit https://review.coreboot.org/c/flashrom/+/68432
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I70cb1cd90d1f377ff4606acad3c1b514120ae4f7
Gerrit-Change-Number: 68432
Gerrit-PatchSet: 3
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Wed, 02 Nov 2022 00:51:27 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Thomas Heijligen, Angel Pons, Anastasia Klimchuk.
Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68162 )
Change subject: meson: Move programmer test sources into programmer definition
......................................................................
Patch Set 16:
(1 comment)
File tests/meson.build:
https://review.coreboot.org/c/flashrom/+/68162/comment/065b0907_f41e1a68
PS10, Line 32: p_data += {
: 'testsrc' : p_data.get('testsrc', []),
: }
: srcs += p_data.get('testsrc')
> > See also line 440 in the top-level meson file. It's done the same way there. […]
I just noticed I made a typo and that's why it didn't work. oopsie 😄
Pushed another patch set and moved the lines to the top-level meson.build
--
To view, visit https://review.coreboot.org/c/flashrom/+/68162
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I307faaf8a9f7ae3c54bd96e7d871a3abb8aadea3
Gerrit-Change-Number: 68162
Gerrit-PatchSet: 16
Gerrit-Owner: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Wed, 02 Nov 2022 00:44:52 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Thomas Heijligen, Angel Pons, Anastasia Klimchuk.
Hello build bot (Jenkins), Thomas Heijligen, Angel Pons, Anastasia Klimchuk,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/68162
to look at the new patch set (#16).
Change subject: meson: Move programmer test sources into programmer definition
......................................................................
meson: Move programmer test sources into programmer definition
Move the definition of the programmer test source files into the
dictionary defining the programmers itself. This way there is a better
overview about which of the available programmers have tests and which
don't.
Also, to keep the tests working, iterate over all programmers and add
their test source files to the list of sources that should be built.
Signed-off-by: Felix Singer <felixsinger(a)posteo.net>
Change-Id: I307faaf8a9f7ae3c54bd96e7d871a3abb8aadea3
---
M meson.build
M tests/meson.build
2 files changed, 32 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/62/68162/16
--
To view, visit https://review.coreboot.org/c/flashrom/+/68162
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I307faaf8a9f7ae3c54bd96e7d871a3abb8aadea3
Gerrit-Change-Number: 68162
Gerrit-PatchSet: 16
Gerrit-Owner: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: newpatchset
Attention is currently required from: Thomas Heijligen, Angel Pons, Anastasia Klimchuk.
Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68162 )
Change subject: meson: Move programmer test sources into programmer definition
......................................................................
Patch Set 15:
(1 comment)
File tests/meson.build:
https://review.coreboot.org/c/flashrom/+/68162/comment/39ffb27a_d3f461d8
PS10, Line 32: p_data += {
: 'testsrc' : p_data.get('testsrc', []),
: }
: srcs += p_data.get('testsrc')
> See also line 440 in the top-level meson file. It's done the same way there. We could add the corresponding lines from here too so that the list of fields is "complete". So this meson file would only iterate over the programmers and add the source files to the srcs list.
> Just rethinking.. Actually, I'm unsure if this adds testsrc to the actual programmers definition or if it's just temporary within this scope. I need to test it.
Just tested it and it didn't work. It seems the modification is just temporary within that scope. Also, modifying the programmer dict is not possible since there is no setter function exposed, just a getter [1]. So we need to do it this way.
Setting the thread to resolved.
[1] https://mesonbuild.com/Reference-manual_elementary_dict.html
--
To view, visit https://review.coreboot.org/c/flashrom/+/68162
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I307faaf8a9f7ae3c54bd96e7d871a3abb8aadea3
Gerrit-Change-Number: 68162
Gerrit-PatchSet: 15
Gerrit-Owner: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Wed, 02 Nov 2022 00:33:27 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Felix Singer, Thomas Heijligen, Angel Pons, Anastasia Klimchuk.
Hello build bot (Jenkins), Thomas Heijligen, Angel Pons, Anastasia Klimchuk,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/68162
to look at the new patch set (#15).
Change subject: meson: Move programmer test sources into programmer definition
......................................................................
meson: Move programmer test sources into programmer definition
Move the definition of the programmer test source files into the
dictionary defining the programmers itself. This way there is a better
overview about which of the available programmers have tests and which
don't.
Also, to keep the tests working, iterate over all programmers and add
their test source files to the list of sources that should be built.
Signed-off-by: Felix Singer <felixsinger(a)posteo.net>
Change-Id: I307faaf8a9f7ae3c54bd96e7d871a3abb8aadea3
---
M meson.build
M tests/meson.build
2 files changed, 34 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/62/68162/15
--
To view, visit https://review.coreboot.org/c/flashrom/+/68162
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I307faaf8a9f7ae3c54bd96e7d871a3abb8aadea3
Gerrit-Change-Number: 68162
Gerrit-PatchSet: 15
Gerrit-Owner: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: newpatchset
Attention is currently required from: Felix Singer, Edward O'Callaghan, Anastasia Klimchuk.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67240 )
Change subject: tree/: Convert flashchip erase_block func ptr to enumerate
......................................................................
Patch Set 15: Code-Review+1
(3 comments)
Patchset:
PS15:
Besides the at45db changes, looks OK.
[Food for thought]
We could use reproducible builds and some temporary macros to easily verify that the bulk of changes (e.g. flashchips.c) doesn't change any behavior. One way to do this is to make a reproducible commit (i.e. one that doesn't change the resulting flashrom binary) preceding this one that replaces the function references with macros that have the same names as the enum values. For example:
#define SPI_ERASE_AT45DB_SECTOR &spi_erase_at45db_sector
The point is that (un)wrapping stuff in macros doesn't change the resulting binaries (if done correctly). In this case, the resulting source files after macro expanding (i.e. preprocessing) should already be equivalent.
Yes, these macros are *cursed*, but they're *temporary*. Think of them as support structures in 3D printing or some other kind of "scaffolding": adding and removing them is work, but it makes the job substantially simpler and less error-prone.
The big question is... Does flashrom's build system (or the cacophony that having two build systems is) support reproducible builds?
File at45db.c:
https://review.coreboot.org/c/flashrom/+/67240/comment/3b10412f_ca55de40
PS15, Line 82: for (i = 0; i < NUM_ERASEFUNCTIONS; i++) {
: const struct block_eraser *const eraser = &flash->chip->block_erasers[i];
: erasefunc_t *erase_func = lookup_erase_func_ptr_(eraser);
: if (erase_func == &spi_erase_at45db_sector) {
: for (j = 0; j < NUM_ERASEREGIONS; j++) {
: cnt += eraser->eraseblocks[j].count;
: }
: }
: }
Is it necessary to refactor this *now*, i.e. in this commit? It doesn't seem to be the case, and it could cause regressions.
Could we go with the simple approach here, and consider refactor in a follow-up change?
```
for (i = 0; i < NUM_ERASEFUNCTIONS; i++) {
if (flash->chip->block_erasers[i].block_erase == SPI_ERASE_AT45DB_SECTOR) {
for (j = 0; j < NUM_ERASEREGIONS; j++) {
cnt += flash->chip->block_erasers[i].eraseblocks[j].count;
}
}
}
```
That is, just replace `&spi_erase_at45db_sector` with `SPI_ERASE_AT45DB_SECTOR`.
File include/flash.h:
https://review.coreboot.org/c/flashrom/+/67240/comment/6f99a0a2_b36cf2b9
PS15, Line 307: TEST_ERASE_INJECTOR, /* special case must come last. */
What does this mean? Looks like it's for testing the code, maybe add a "Used in tests" note to the comment?
--
To view, visit https://review.coreboot.org/c/flashrom/+/67240
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I58415a4f2589812b26d284c96876e88f8d9acf52
Gerrit-Change-Number: 67240
Gerrit-PatchSet: 15
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Wed, 02 Nov 2022 00:26:33 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Thomas Heijligen, Angel Pons, Anastasia Klimchuk.
Hello build bot (Jenkins), Thomas Heijligen, Angel Pons, Anastasia Klimchuk,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/68162
to look at the new patch set (#14).
Change subject: meson: Move programmer test sources into programmer definition
......................................................................
meson: Move programmer test sources into programmer definition
Move the definition of the programmer test source files into the
dictionary defining the programmers itself. This way there is a better
overview about which of the available programmers have tests and which
don't.
Also, to keep the tests working, iterate over all programmers and add
their test source files to the list of sources that should be built.
Signed-off-by: Felix Singer <felixsinger(a)posteo.net>
Change-Id: I307faaf8a9f7ae3c54bd96e7d871a3abb8aadea3
---
M meson.build
M tests/meson.build
2 files changed, 34 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/62/68162/14
--
To view, visit https://review.coreboot.org/c/flashrom/+/68162
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I307faaf8a9f7ae3c54bd96e7d871a3abb8aadea3
Gerrit-Change-Number: 68162
Gerrit-PatchSet: 14
Gerrit-Owner: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: newpatchset
Felix Singer has submitted this change. ( https://review.coreboot.org/c/flashrom/+/68228 )
Change subject: tests/meson.build: Rename list of source files to `test_srcs`
......................................................................
tests/meson.build: Rename list of source files to `test_srcs`
Rename the list of source files to `test_srcs` so that there is less
confusion with the variable `srcs` from the top-level meson.build file
containing the flashrom source files.
Signed-off-by: Felix Singer <felixsinger(a)posteo.net>
Change-Id: Ica0fc3923070bff63323204bd58edb5276dc9493
Reviewed-on: https://review.coreboot.org/c/flashrom/+/68228
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
Reviewed-by: Angel Pons <th3fanbus(a)gmail.com>
---
M tests/meson.build
1 file changed, 20 insertions(+), 3 deletions(-)
Approvals:
build bot (Jenkins): Verified
Angel Pons: Looks good to me, approved
diff --git a/tests/meson.build b/tests/meson.build
index c62cc1b..b0375a1 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -11,7 +11,7 @@
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
# GNU General Public License for more details.
-srcs = files(
+test_srcs = files(
'io_mock.c',
'tests.c',
'libusb_wraps.c',
@@ -34,7 +34,7 @@
)
if not programmer.get('dummy').get('active')
- srcs += programmer.get('dummy').get('srcs')
+ test_srcs += programmer.get('dummy').get('srcs')
endif
mocks = [
@@ -110,7 +110,7 @@
]
flashrom_tests = executable('flashrom_unit_tests',
- srcs,
+ test_srcs,
c_args : [
cargs,
'-ffunction-sections',
--
To view, visit https://review.coreboot.org/c/flashrom/+/68228
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ica0fc3923070bff63323204bd58edb5276dc9493
Gerrit-Change-Number: 68228
Gerrit-PatchSet: 6
Gerrit-Owner: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-MessageType: merged