Message ID | 20220826153634.3086393-2-foss+uboot@0leil.net |
---|---|
State | Superseded |
Delegated to: | Kever Yang |
Headers | show |
Series | migrate u-boot-rockchip.bin to binman and generate an image for SPI | expand |
Hi Quentin, On Fri, 26 Aug 2022 at 09:37, Quentin Schulz <foss+uboot@0leil.net> wrote: > > From: Quentin Schulz <quentin.schulz@theobroma-systems.com> > > Some image types handled by mkimage require the datafiles to be passed > independently (-d data1:data2) for specific handling of each. A > concatenation of datafiles prior to passing them to mkimage wouldn't > work. > > That is the case for rkspi for example which requires page alignment > and only writing 2KB every 4KB. > > This adds the ability to tell binman to pass the datafiles without > prior concatenation to mkimage, by adding the multiple-data-files > boolean property to the mkimage node. > > Cc: Quentin Schulz <foss+uboot@0leil.net> > Reviewed-by: Simon Glass <sjg@chromium.org> > Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com> > --- > > v5: > - changed to use full path from input dir with tools.get_input_filename > to make it possible to run the unit tests, > - added unit test, > > > tools/binman/entries.rst | 22 ++++++++++ > tools/binman/etype/mkimage.py | 41 +++++++++++++++++-- > tools/binman/ftest.py | 16 ++++++++ Please put the new test at the end. > .../test/241_mkimage_multiple_data_files.dts | 21 ++++++++++ > 4 files changed, 96 insertions(+), 4 deletions(-) > create mode 100644 tools/binman/test/241_mkimage_multiple_data_files.dts This is pretty close but it still missing a line of test coverage. Please try 'binman test -T' to see it. I'd also prefer a shorter filename for the 241 file. I've pushed a tree containing a suggested fix (updating this patch). I can update it when applying if you like, otherwise please send a new version. Also note that the files have been renumbered, so the latest update is at u-boot-dm/testing Regards, Simon
Hi Simon, On 8/27/22 02:21, Simon Glass wrote: > Hi Quentin, > > On Fri, 26 Aug 2022 at 09:37, Quentin Schulz <foss+uboot@0leil.net> wrote: >> >> From: Quentin Schulz <quentin.schulz@theobroma-systems.com> >> >> Some image types handled by mkimage require the datafiles to be passed >> independently (-d data1:data2) for specific handling of each. A >> concatenation of datafiles prior to passing them to mkimage wouldn't >> work. >> >> That is the case for rkspi for example which requires page alignment >> and only writing 2KB every 4KB. >> >> This adds the ability to tell binman to pass the datafiles without >> prior concatenation to mkimage, by adding the multiple-data-files >> boolean property to the mkimage node. >> >> Cc: Quentin Schulz <foss+uboot@0leil.net> >> Reviewed-by: Simon Glass <sjg@chromium.org> >> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com> >> --- >> >> v5: >> - changed to use full path from input dir with tools.get_input_filename >> to make it possible to run the unit tests, >> - added unit test, >> >> >> tools/binman/entries.rst | 22 ++++++++++ >> tools/binman/etype/mkimage.py | 41 +++++++++++++++++-- >> tools/binman/ftest.py | 16 ++++++++ > > Please put the new test at the end. > >> .../test/241_mkimage_multiple_data_files.dts | 21 ++++++++++ >> 4 files changed, 96 insertions(+), 4 deletions(-) >> create mode 100644 tools/binman/test/241_mkimage_multiple_data_files.dts > > This is pretty close but it still missing a line of test coverage. > Please try 'binman test -T' to see it. I'd also prefer a shorter This does not work on Fedora. 1) there's no python3-coverage binary available, 2) After replacing python3-coverage with just coverage, the tests are stuck and never finish, (I have seen the patches to use COVERAGE environment variable so I guess the required changes might be tackled soon in master), Any tip on how to identify which test is stuck except going through them one by one? python3-coverage is also not available in the container image built from tools/docker/Dockerfile. > filename for the 241 file. > > I've pushed a tree containing a suggested fix (updating this patch). I > can update it when applying if you like, otherwise please send a new > version. > Where did you push the tree? Cheers, Quentin
Hi Quentin, On Tue, 30 Aug 2022 at 03:57, Quentin Schulz <quentin.schulz@theobroma-systems.com> wrote: > > Hi Simon, > > On 8/27/22 02:21, Simon Glass wrote: > > Hi Quentin, > > > > On Fri, 26 Aug 2022 at 09:37, Quentin Schulz <foss+uboot@0leil.net> wrote: > >> > >> From: Quentin Schulz <quentin.schulz@theobroma-systems.com> > >> > >> Some image types handled by mkimage require the datafiles to be passed > >> independently (-d data1:data2) for specific handling of each. A > >> concatenation of datafiles prior to passing them to mkimage wouldn't > >> work. > >> > >> That is the case for rkspi for example which requires page alignment > >> and only writing 2KB every 4KB. > >> > >> This adds the ability to tell binman to pass the datafiles without > >> prior concatenation to mkimage, by adding the multiple-data-files > >> boolean property to the mkimage node. > >> > >> Cc: Quentin Schulz <foss+uboot@0leil.net> > >> Reviewed-by: Simon Glass <sjg@chromium.org> > >> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com> > >> --- > >> > >> v5: > >> - changed to use full path from input dir with tools.get_input_filename > >> to make it possible to run the unit tests, > >> - added unit test, > >> > >> > >> tools/binman/entries.rst | 22 ++++++++++ > >> tools/binman/etype/mkimage.py | 41 +++++++++++++++++-- > >> tools/binman/ftest.py | 16 ++++++++ > > > > Please put the new test at the end. > > > >> .../test/241_mkimage_multiple_data_files.dts | 21 ++++++++++ > >> 4 files changed, 96 insertions(+), 4 deletions(-) > >> create mode 100644 tools/binman/test/241_mkimage_multiple_data_files.dts > > > > This is pretty close but it still missing a line of test coverage. > > Please try 'binman test -T' to see it. I'd also prefer a shorter > > This does not work on Fedora. > 1) there's no python3-coverage binary available, > 2) After replacing python3-coverage with just coverage, the tests are > stuck and never finish, (I have seen the patches to use COVERAGE > environment variable so I guess the required changes might be tackled > soon in master), > > Any tip on how to identify which test is stuck except going through them > one by one? One way is to add comment blocks '''...''' across the ftest.py file, using a binary chop to identify the problem. Or, since tests are run in series, you could hack test_util to pass verbose parameters when it runs the tests - see 'cmd =' in run_test_coverage(). > > python3-coverage is also not available in the container image built from > tools/docker/Dockerfile. does 'python3 -m coverage' work? or this: https://coverage.readthedocs.io/en/6.3.2/install.html > > > filename for the 241 file. > > > > I've pushed a tree containing a suggested fix (updating this patch). I > > can update it when applying if you like, otherwise please send a new > > version. > > > > Where did you push the tree? Sorry I forgot to mention that: https://github.com/sjg20/u-boot/tree/try-rk4 Regards, SImon
Hi Simon, On 8/30/22 17:56, Simon Glass wrote: > Hi Quentin, > > On Tue, 30 Aug 2022 at 03:57, Quentin Schulz > <quentin.schulz@theobroma-systems.com> wrote: >> >> Hi Simon, >> >> On 8/27/22 02:21, Simon Glass wrote: >>> Hi Quentin, >>> >>> On Fri, 26 Aug 2022 at 09:37, Quentin Schulz <foss+uboot@0leil.net> wrote: >>>> >>>> From: Quentin Schulz <quentin.schulz@theobroma-systems.com> >>>> >>>> Some image types handled by mkimage require the datafiles to be passed >>>> independently (-d data1:data2) for specific handling of each. A >>>> concatenation of datafiles prior to passing them to mkimage wouldn't >>>> work. >>>> >>>> That is the case for rkspi for example which requires page alignment >>>> and only writing 2KB every 4KB. >>>> >>>> This adds the ability to tell binman to pass the datafiles without >>>> prior concatenation to mkimage, by adding the multiple-data-files >>>> boolean property to the mkimage node. >>>> >>>> Cc: Quentin Schulz <foss+uboot@0leil.net> >>>> Reviewed-by: Simon Glass <sjg@chromium.org> >>>> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com> >>>> --- >>>> >>>> v5: >>>> - changed to use full path from input dir with tools.get_input_filename >>>> to make it possible to run the unit tests, >>>> - added unit test, >>>> >>>> >>>> tools/binman/entries.rst | 22 ++++++++++ >>>> tools/binman/etype/mkimage.py | 41 +++++++++++++++++-- >>>> tools/binman/ftest.py | 16 ++++++++ >>> >>> Please put the new test at the end. >>> >>>> .../test/241_mkimage_multiple_data_files.dts | 21 ++++++++++ >>>> 4 files changed, 96 insertions(+), 4 deletions(-) >>>> create mode 100644 tools/binman/test/241_mkimage_multiple_data_files.dts >>> >>> This is pretty close but it still missing a line of test coverage. >>> Please try 'binman test -T' to see it. I'd also prefer a shorter >> >> This does not work on Fedora. >> 1) there's no python3-coverage binary available, >> 2) After replacing python3-coverage with just coverage, the tests are >> stuck and never finish, (I have seen the patches to use COVERAGE >> environment variable so I guess the required changes might be tackled >> soon in master), >> >> Any tip on how to identify which test is stuck except going through them >> one by one? > > One way is to add comment blocks '''...''' across the ftest.py file, > using a binary chop to identify the problem. > > Or, since tests are run in series, you could hack test_util to pass > verbose parameters when it runs the tests - see 'cmd =' in > run_test_coverage(). > I just commented out tests and found the following two are failing on my system: testCompUtilVersions and testListBintools. After digging a bit it seems that it is stuck here: https://source.denx.de/u-boot/u-boot/-/blob/master/tools/patman/command.py#L105 for bzip2. Furthermore: bzip2 -V > /dev/null bzip2 -V > /dev/null 2>&1 both get stuck which I assume is where the issue lies :) bzip2 --help is just fine BTW. I tested on a colleague's PC running Ubuntu 22.04.1, it works as intended. I guess I'll have to check if Fedora or Ubuntu has patches on top of bzip2 source code that triggers/patches this behavior. >> >> python3-coverage is also not available in the container image built from >> tools/docker/Dockerfile. > > does 'python3 -m coverage' work? > diff --git a/tools/patman/test_util.py b/tools/patman/test_util.py index 0f6d1aa902..eaa769a564 100644 --- a/tools/patman/test_util.py +++ b/tools/patman/test_util.py @@ -58,11 +58,11 @@ def run_test_coverage(prog, filter_fname, exclude_list, build_dir, required=None prefix = '' if build_dir: prefix = 'PYTHONPATH=$PYTHONPATH:%s/sandbox_spl/tools ' % build_dir - cmd = ('%spython3-coverage run ' + cmd = ('%spython3 -m coverage run ' '--omit "%s" %s %s %s -P1' % (prefix, ','.join(glob_list), prog, extra_args or '', test_cmd)) os.system(cmd) - stdout = command.output('python3-coverage', 'report') + stdout = command.output('python3', '-m', 'coverage', 'report') lines = stdout.splitlines() if required: # Convert '/path/to/name.py' just the module name 'name' @@ -81,7 +81,7 @@ def run_test_coverage(prog, filter_fname, exclude_list, build_dir, required=None print(coverage) if coverage != '100%': print(stdout) - print("To get a report in 'htmlcov/index.html', type: python3-coverage html") + print("To get a report in 'htmlcov/index.html', type: python3 -m coverage html") print('Coverage error: %s, but should be 100%%' % coverage) ok = False if not ok: works just fine for me. Michal Suchánek seems to disagree with me on this one, see https://lore.kernel.org/u-boot/20220830101149.GM28810@kitsune.suse.cz/ > or this: > > https://urldefense.proofpoint.com/v2/url?u=https-3A__coverage.readthedocs.io_en_6.3.2_install.html&d=DwIBaQ&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&m=AH6ijvL2fp8TDrFjgeja0AVykFkzBjRPDOAfg8m_eKMHJW7RzTVA1wMpGn7qBwOe&s=BLW968ZKOcdPWg0s4-4AlA_rqiJCCCKPjP-Y-Fux6oI&e= > >> >>> filename for the 241 file. >>> >>> I've pushed a tree containing a suggested fix (updating this patch). I >>> can update it when applying if you like, otherwise please send a new >>> version. >>> >> >> Where did you push the tree? > > Sorry I forgot to mention that: > > https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_sjg20_u-2Dboot_tree_try-2Drk4&d=DwIBaQ&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&m=AH6ijvL2fp8TDrFjgeja0AVykFkzBjRPDOAfg8m_eKMHJW7RzTVA1wMpGn7qBwOe&s=7LOQoSkcQA52SvFgC_aUR4l2MtMWjdVM-t_bCKUetEs&e= > I do not understand how you found out coverage was not happy about my patchset. I have the same percentage reported from your branch or my local one. What am I missing? Regarding the content of the changed commits: testMkimageMultipleNoContent is not testing what is says it does? It's using multiple-data-files DT property which only impacts -d parameter of mkimage and the comment for the test is """Test using mkimage with -n and no data""". What exactly are you trying to test? Cheers, Quentin
Hi Quentin, On Tue, 30 Aug 2022 at 11:54, Quentin Schulz <quentin.schulz@theobroma-systems.com> wrote: > > Hi Simon, > > On 8/30/22 17:56, Simon Glass wrote: > > Hi Quentin, > > > > On Tue, 30 Aug 2022 at 03:57, Quentin Schulz > > <quentin.schulz@theobroma-systems.com> wrote: > >> > >> Hi Simon, > >> > >> On 8/27/22 02:21, Simon Glass wrote: > >>> Hi Quentin, > >>> > >>> On Fri, 26 Aug 2022 at 09:37, Quentin Schulz <foss+uboot@0leil.net> wrote: > >>>> > >>>> From: Quentin Schulz <quentin.schulz@theobroma-systems.com> > >>>> > >>>> Some image types handled by mkimage require the datafiles to be passed > >>>> independently (-d data1:data2) for specific handling of each. A > >>>> concatenation of datafiles prior to passing them to mkimage wouldn't > >>>> work. > >>>> > >>>> That is the case for rkspi for example which requires page alignment > >>>> and only writing 2KB every 4KB. > >>>> > >>>> This adds the ability to tell binman to pass the datafiles without > >>>> prior concatenation to mkimage, by adding the multiple-data-files > >>>> boolean property to the mkimage node. > >>>> > >>>> Cc: Quentin Schulz <foss+uboot@0leil.net> > >>>> Reviewed-by: Simon Glass <sjg@chromium.org> > >>>> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com> > >>>> --- > >>>> > >>>> v5: > >>>> - changed to use full path from input dir with tools.get_input_filename > >>>> to make it possible to run the unit tests, > >>>> - added unit test, > >>>> > >>>> > >>>> tools/binman/entries.rst | 22 ++++++++++ > >>>> tools/binman/etype/mkimage.py | 41 +++++++++++++++++-- > >>>> tools/binman/ftest.py | 16 ++++++++ > >>> > >>> Please put the new test at the end. > >>> > >>>> .../test/241_mkimage_multiple_data_files.dts | 21 ++++++++++ > >>>> 4 files changed, 96 insertions(+), 4 deletions(-) > >>>> create mode 100644 tools/binman/test/241_mkimage_multiple_data_files.dts > >>> > >>> This is pretty close but it still missing a line of test coverage. > >>> Please try 'binman test -T' to see it. I'd also prefer a shorter > >> > >> This does not work on Fedora. > >> 1) there's no python3-coverage binary available, > >> 2) After replacing python3-coverage with just coverage, the tests are > >> stuck and never finish, (I have seen the patches to use COVERAGE > >> environment variable so I guess the required changes might be tackled > >> soon in master), > >> > >> Any tip on how to identify which test is stuck except going through them > >> one by one? > > > > One way is to add comment blocks '''...''' across the ftest.py file, > > using a binary chop to identify the problem. > > > > Or, since tests are run in series, you could hack test_util to pass > > verbose parameters when it runs the tests - see 'cmd =' in > > run_test_coverage(). > > > > I just commented out tests and found the following two are failing on my > system: > testCompUtilVersions and testListBintools. > > After digging a bit it seems that it is stuck here: > https://source.denx.de/u-boot/u-boot/-/blob/master/tools/patman/command.py#L105 > for bzip2. > > Furthermore: > bzip2 -V > /dev/null > bzip2 -V > /dev/null 2>&1 I wonder why that would hang. Can you try 'bzip2 -V' on the cmdline? > both get stuck which I assume is where the issue lies :) > > bzip2 --help is just fine BTW. > > I tested on a colleague's PC running Ubuntu 22.04.1, it works as > intended. I guess I'll have to check if Fedora or Ubuntu has patches on > top of bzip2 source code that triggers/patches this behavior. Very strange! > > >> > >> python3-coverage is also not available in the container image built from > >> tools/docker/Dockerfile. > > > > does 'python3 -m coverage' work? > > > > diff --git a/tools/patman/test_util.py b/tools/patman/test_util.py > index 0f6d1aa902..eaa769a564 100644 > --- a/tools/patman/test_util.py > +++ b/tools/patman/test_util.py > @@ -58,11 +58,11 @@ def run_test_coverage(prog, filter_fname, > exclude_list, build_dir, required=None > prefix = '' > if build_dir: > prefix = 'PYTHONPATH=$PYTHONPATH:%s/sandbox_spl/tools ' % > build_dir > - cmd = ('%spython3-coverage run ' > + cmd = ('%spython3 -m coverage run ' > '--omit "%s" %s %s %s -P1' % (prefix, ','.join(glob_list), > prog, extra_args or '', > test_cmd)) > os.system(cmd) > - stdout = command.output('python3-coverage', 'report') > + stdout = command.output('python3', '-m', 'coverage', 'report') > lines = stdout.splitlines() > if required: > # Convert '/path/to/name.py' just the module name 'name' > @@ -81,7 +81,7 @@ def run_test_coverage(prog, filter_fname, > exclude_list, build_dir, required=None > print(coverage) > if coverage != '100%': > print(stdout) > - print("To get a report in 'htmlcov/index.html', type: > python3-coverage html") > + print("To get a report in 'htmlcov/index.html', type: python3 > -m coverage html") > print('Coverage error: %s, but should be 100%%' % coverage) > ok = False > if not ok: > > works just fine for me. > > Michal Suchánek seems to disagree with me on this one, see > https://lore.kernel.org/u-boot/20220830101149.GM28810@kitsune.suse.cz/ I don't fully understand that point. I think it is fine to specify the tool as an env var. But if -m coverage works in general, let's use it. If not, we'll have the env var. > > > or this: > > > > https://urldefense.proofpoint.com/v2/url?u=https-3A__coverage.readthedocs.io_en_6.3.2_install.html&d=DwIBaQ&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&m=AH6ijvL2fp8TDrFjgeja0AVykFkzBjRPDOAfg8m_eKMHJW7RzTVA1wMpGn7qBwOe&s=BLW968ZKOcdPWg0s4-4AlA_rqiJCCCKPjP-Y-Fux6oI&e= > > > >> > >>> filename for the 241 file. > >>> > >>> I've pushed a tree containing a suggested fix (updating this patch). I > >>> can update it when applying if you like, otherwise please send a new > >>> version. > >>> > >> > >> Where did you push the tree? > > > > Sorry I forgot to mention that: > > > > https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_sjg20_u-2Dboot_tree_try-2Drk4&d=DwIBaQ&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&m=AH6ijvL2fp8TDrFjgeja0AVykFkzBjRPDOAfg8m_eKMHJW7RzTVA1wMpGn7qBwOe&s=7LOQoSkcQA52SvFgC_aUR4l2MtMWjdVM-t_bCKUetEs&e= > > > > > I do not understand how you found out coverage was not happy about my > patchset. I have the same percentage reported from your branch or my > local one. What am I missing? > > Regarding the content of the changed commits: > testMkimageMultipleNoContent is not testing what is says it does? > It's using multiple-data-files DT property which only impacts -d > parameter of mkimage and the comment for the test is """Test using > mkimage with -n and no data""". > > What exactly are you trying to test? 'binman test -T' I pushed your original patches to the try-rk4-orig branch. My changes are in try-rk4. With yours I see this: ======================== Running binman tests ======================== ........................................................................................................................................................................................................................................................................................................................................................................................................................................................................ ---------------------------------------------------------------------- Ran 456 tests in 19.669s OK 99% Name Stmts Miss Cover --------------------------------------------------------------------------- tools/binman/__init__.py 0 0 100% tools/binman/bintool.py 254 0 100% tools/binman/btool/btool_gzip.py 5 0 100% tools/binman/btool/bzip2.py 5 0 100% tools/binman/btool/cbfstool.py 24 0 100% tools/binman/btool/fiptool.py 22 0 100% tools/binman/btool/futility.py 24 0 100% tools/binman/btool/ifwitool.py 22 0 100% tools/binman/btool/lz4.py 28 0 100% tools/binman/btool/lzma_alone.py 34 0 100% tools/binman/btool/lzop.py 5 0 100% tools/binman/btool/mkimage.py 29 0 100% tools/binman/btool/xz.py 5 0 100% tools/binman/btool/zstd.py 5 0 100% tools/binman/cbfs_util.py 366 0 100% tools/binman/cmdline.py 73 0 100% tools/binman/control.py 342 0 100% tools/binman/elf.py 195 0 100% tools/binman/entry.py 483 0 100% tools/binman/etype/atf_bl31.py 5 0 100% tools/binman/etype/atf_fip.py 67 0 100% tools/binman/etype/blob.py 39 0 100% tools/binman/etype/blob_dtb.py 46 0 100% tools/binman/etype/blob_ext.py 11 0 100% tools/binman/etype/blob_ext_list.py 32 0 100% tools/binman/etype/blob_named_by_arg.py 9 0 100% tools/binman/etype/blob_phase.py 16 0 100% tools/binman/etype/cbfs.py 101 0 100% tools/binman/etype/collection.py 30 0 100% tools/binman/etype/cros_ec_rw.py 5 0 100% tools/binman/etype/fdtmap.py 62 0 100% tools/binman/etype/files.py 35 0 100% tools/binman/etype/fill.py 13 0 100% tools/binman/etype/fit.py 214 0 100% tools/binman/etype/fmap.py 34 0 100% tools/binman/etype/gbb.py 37 0 100% tools/binman/etype/image_header.py 53 0 100% tools/binman/etype/intel_cmc.py 4 0 100% tools/binman/etype/intel_descriptor.py 39 0 100% tools/binman/etype/intel_fit.py 12 0 100% tools/binman/etype/intel_fit_ptr.py 17 0 100% tools/binman/etype/intel_fsp.py 4 0 100% tools/binman/etype/intel_fsp_m.py 4 0 100% tools/binman/etype/intel_fsp_s.py 4 0 100% tools/binman/etype/intel_fsp_t.py 4 0 100% tools/binman/etype/intel_ifwi.py 67 0 100% tools/binman/etype/intel_me.py 4 0 100% tools/binman/etype/intel_mrc.py 6 0 100% tools/binman/etype/intel_refcode.py 6 0 100% tools/binman/etype/intel_vbt.py 4 0 100% tools/binman/etype/intel_vga.py 4 0 100% tools/binman/etype/mkimage.py 80 1 99% tools/binman/etype/opensbi.py 5 0 100% tools/binman/etype/powerpc_mpc85xx_bootpg_resetvec.py 6 0 100% tools/binman/etype/pre_load.py 77 0 100% tools/binman/etype/scp.py 5 0 100% tools/binman/etype/section.py 376 0 100% tools/binman/etype/tee_os.py 5 0 100% tools/binman/etype/text.py 21 0 100% tools/binman/etype/u_boot.py 7 0 100% tools/binman/etype/u_boot_dtb.py 9 0 100% tools/binman/etype/u_boot_dtb_with_ucode.py 51 0 100% tools/binman/etype/u_boot_elf.py 19 0 100% tools/binman/etype/u_boot_env.py 27 0 100% tools/binman/etype/u_boot_expanded.py 4 0 100% tools/binman/etype/u_boot_img.py 7 0 100% tools/binman/etype/u_boot_nodtb.py 7 0 100% tools/binman/etype/u_boot_spl.py 11 0 100% tools/binman/etype/u_boot_spl_bss_pad.py 14 0 100% tools/binman/etype/u_boot_spl_dtb.py 9 0 100% tools/binman/etype/u_boot_spl_elf.py 7 0 100% tools/binman/etype/u_boot_spl_expanded.py 12 0 100% tools/binman/etype/u_boot_spl_nodtb.py 11 0 100% tools/binman/etype/u_boot_spl_with_ucode_ptr.py 8 0 100% tools/binman/etype/u_boot_tpl.py 11 0 100% tools/binman/etype/u_boot_tpl_bss_pad.py 14 0 100% tools/binman/etype/u_boot_tpl_dtb.py 9 0 100% tools/binman/etype/u_boot_tpl_dtb_with_ucode.py 8 0 100% tools/binman/etype/u_boot_tpl_elf.py 7 0 100% tools/binman/etype/u_boot_tpl_expanded.py 12 0 100% tools/binman/etype/u_boot_tpl_nodtb.py 11 0 100% tools/binman/etype/u_boot_tpl_with_ucode_ptr.py 12 0 100% tools/binman/etype/u_boot_ucode.py 33 0 100% tools/binman/etype/u_boot_with_ucode_ptr.py 42 0 100% tools/binman/etype/vblock.py 38 0 100% tools/binman/etype/x86_reset16.py 7 0 100% tools/binman/etype/x86_reset16_spl.py 7 0 100% tools/binman/etype/x86_reset16_tpl.py 7 0 100% tools/binman/etype/x86_start16.py 7 0 100% tools/binman/etype/x86_start16_spl.py 7 0 100% tools/binman/etype/x86_start16_tpl.py 7 0 100% tools/binman/fip_util.py 202 0 100% tools/binman/fmap_util.py 48 0 100% tools/binman/image.py 164 0 100% tools/binman/state.py 201 0 100% --------------------------------------------------------------------------- TOTAL 4541 1 99% To get a report in 'htmlcov/index.html', type: python3-coverage html Coverage error: 99%, but should be 100% ValueError: Test coverage failure It is only a tiny difference! Basically we need to support the contents of an entry being unavailable, temporarily or permanently, so I added a test for that. Regards, Simon
Hi Simon, On 8/31/22 05:15, Simon Glass wrote: > Hi Quentin, > > On Tue, 30 Aug 2022 at 11:54, Quentin Schulz > <quentin.schulz@theobroma-systems.com> wrote: >> >> Hi Simon, >> >> On 8/30/22 17:56, Simon Glass wrote: >>> Hi Quentin, >>> >>> On Tue, 30 Aug 2022 at 03:57, Quentin Schulz >>> <quentin.schulz@theobroma-systems.com> wrote: >>>> >>>> Hi Simon, >>>> >>>> On 8/27/22 02:21, Simon Glass wrote: >>>>> Hi Quentin, >>>>> >>>>> On Fri, 26 Aug 2022 at 09:37, Quentin Schulz <foss+uboot@0leil.net> wrote: >>>>>> >>>>>> From: Quentin Schulz <quentin.schulz@theobroma-systems.com> >>>>>> >>>>>> Some image types handled by mkimage require the datafiles to be passed >>>>>> independently (-d data1:data2) for specific handling of each. A >>>>>> concatenation of datafiles prior to passing them to mkimage wouldn't >>>>>> work. >>>>>> >>>>>> That is the case for rkspi for example which requires page alignment >>>>>> and only writing 2KB every 4KB. >>>>>> >>>>>> This adds the ability to tell binman to pass the datafiles without >>>>>> prior concatenation to mkimage, by adding the multiple-data-files >>>>>> boolean property to the mkimage node. >>>>>> >>>>>> Cc: Quentin Schulz <foss+uboot@0leil.net> >>>>>> Reviewed-by: Simon Glass <sjg@chromium.org> >>>>>> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com> >>>>>> --- >>>>>> >>>>>> v5: >>>>>> - changed to use full path from input dir with tools.get_input_filename >>>>>> to make it possible to run the unit tests, >>>>>> - added unit test, >>>>>> >>>>>> >>>>>> tools/binman/entries.rst | 22 ++++++++++ >>>>>> tools/binman/etype/mkimage.py | 41 +++++++++++++++++-- >>>>>> tools/binman/ftest.py | 16 ++++++++ >>>>> >>>>> Please put the new test at the end. >>>>> >>>>>> .../test/241_mkimage_multiple_data_files.dts | 21 ++++++++++ >>>>>> 4 files changed, 96 insertions(+), 4 deletions(-) >>>>>> create mode 100644 tools/binman/test/241_mkimage_multiple_data_files.dts >>>>> >>>>> This is pretty close but it still missing a line of test coverage. >>>>> Please try 'binman test -T' to see it. I'd also prefer a shorter >>>> >>>> This does not work on Fedora. >>>> 1) there's no python3-coverage binary available, >>>> 2) After replacing python3-coverage with just coverage, the tests are >>>> stuck and never finish, (I have seen the patches to use COVERAGE >>>> environment variable so I guess the required changes might be tackled >>>> soon in master), >>>> >>>> Any tip on how to identify which test is stuck except going through them >>>> one by one? >>> >>> One way is to add comment blocks '''...''' across the ftest.py file, >>> using a binary chop to identify the problem. >>> >>> Or, since tests are run in series, you could hack test_util to pass >>> verbose parameters when it runs the tests - see 'cmd =' in >>> run_test_coverage(). >>> >> >> I just commented out tests and found the following two are failing on my >> system: >> testCompUtilVersions and testListBintools. >> >> After digging a bit it seems that it is stuck here: >> https://urldefense.proofpoint.com/v2/url?u=https-3A__source.denx.de_u-2Dboot_u-2Dboot_-2D_blob_master_tools_patman_command.py-23L105&d=DwIFaQ&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&m=MbdarH_QzLsQMF7mlCoDjHMwaiirTOGXbHZkHpb79AsDp1RElUfGn9NaYl4FQIJw&s=goCAUmylx43E8E8Yf9t6iJdmCGODogQVuiVjJ7qebwc&e= >> for bzip2. >> >> Furthermore: >> bzip2 -V > /dev/null >> bzip2 -V > /dev/null 2>&1 > > I wonder why that would hang. Can you try 'bzip2 -V' on the cmdline? > >> both get stuck which I assume is where the issue lies :) >> >> bzip2 --help is just fine BTW. >> >> I tested on a colleague's PC running Ubuntu 22.04.1, it works as >> intended. I guess I'll have to check if Fedora or Ubuntu has patches on >> top of bzip2 source code that triggers/patches this behavior. > > Very strange! > OK. Upstream "bug", see: https://sourceware.org/git/?p=bzip2.git;a=blob;f=bzip2.c;h=1538faf73a8b311f53f0fe608347de761196de90;hb=HEAD#l1902 When you pass the -V or -L option, the program does not exit (unlike --help). I guess it tries to compress something from stdin and endlessly waits. I cannot explain why: bzip2 -V returns -1 directly but bzip2 -V > /dev/null is stuck. $ strace bzip2 -V > /dev/null [...] write(2, "bzip2, a block-sorting file comp"..., 540bzip2, a block-sorting file compressor. Version 1.0.8, 13-Jul-2019. Copyright (C) 1996-2019 by Julian Seward. This program is free software; you can redistribute it and/or modify it under the terms set out in the LICENSE file, which is included in the bzip2 source distribution. This program is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the LICENSE file for more details. ) = 540 ioctl(1, TCGETS, 0x7ffdddef4390) = -1 ENOTTY (Inappropriate ioctl for device) mmap(NULL, 3600384, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7ff9a1091000 mmap(NULL, 3600384, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7ff9a0d22000 mmap(NULL, 266240, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7ff9a163f000 newfstatat(0, "", {st_mode=S_IFCHR|0620, st_rdev=makedev(0x88, 0x4), ...}, AT_EMPTY_PATH) = 0 read(0, This is "fixed" in Ubuntu with: https://git.launchpad.net/ubuntu/+source/bzip2/tree/debian/patches/20-legacy.patch?h=ubuntu/jammy I suggest that we use bzip2 --help instead in binman. It does print the version name there so it should be just fine. I'll send a patch for binman and open a bug or something on bzip2 ML to find out what exactly they are trying to do (if it's on purpose for example). >> >>>> >>>> python3-coverage is also not available in the container image built from >>>> tools/docker/Dockerfile. >>> >>> does 'python3 -m coverage' work? >>> >> >> diff --git a/tools/patman/test_util.py b/tools/patman/test_util.py >> index 0f6d1aa902..eaa769a564 100644 >> --- a/tools/patman/test_util.py >> +++ b/tools/patman/test_util.py >> @@ -58,11 +58,11 @@ def run_test_coverage(prog, filter_fname, >> exclude_list, build_dir, required=None >> prefix = '' >> if build_dir: >> prefix = 'PYTHONPATH=$PYTHONPATH:%s/sandbox_spl/tools ' % >> build_dir >> - cmd = ('%spython3-coverage run ' >> + cmd = ('%spython3 -m coverage run ' >> '--omit "%s" %s %s %s -P1' % (prefix, ','.join(glob_list), >> prog, extra_args or '', >> test_cmd)) >> os.system(cmd) >> - stdout = command.output('python3-coverage', 'report') >> + stdout = command.output('python3', '-m', 'coverage', 'report') >> lines = stdout.splitlines() >> if required: >> # Convert '/path/to/name.py' just the module name 'name' >> @@ -81,7 +81,7 @@ def run_test_coverage(prog, filter_fname, >> exclude_list, build_dir, required=None >> print(coverage) >> if coverage != '100%': >> print(stdout) >> - print("To get a report in 'htmlcov/index.html', type: >> python3-coverage html") >> + print("To get a report in 'htmlcov/index.html', type: python3 >> -m coverage html") >> print('Coverage error: %s, but should be 100%%' % coverage) >> ok = False >> if not ok: >> >> works just fine for me. >> >> Michal Suchánek seems to disagree with me on this one, see >> https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_u-2Dboot_20220830101149.GM28810-40kitsune.suse.cz_&d=DwIFaQ&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&m=MbdarH_QzLsQMF7mlCoDjHMwaiirTOGXbHZkHpb79AsDp1RElUfGn9NaYl4FQIJw&s=PIpNEgfpEtiIeShj3dhklIwaomQemLRGI3wo8nKxsr8&e= > > I don't fully understand that point. > > I think it is fine to specify the tool as an env var. > > But if -m coverage works in general, let's use it. If not, we'll have > the env var. > It works for me and I believe it is better. Installing coverage from pip will install a "coverage" binary. In essence, having a COVERAGE environment variable is fine, but having it set to python3-coverage by default means it works by default only on Debian/Ubuntu-based distros: see https://pkgs.org/search/?q=python3-coverage&on=files Also, installing with pip, one can run python3 -m coverage without adding ~/.local/bin to PATH (unlike using python3-coverage or coverage). I suggest we move this discussion to the patch from Michal :) > >> >>> or this: >>> >>> https://urldefense.proofpoint.com/v2/url?u=https-3A__coverage.readthedocs.io_en_6.3.2_install.html&d=DwIBaQ&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&m=AH6ijvL2fp8TDrFjgeja0AVykFkzBjRPDOAfg8m_eKMHJW7RzTVA1wMpGn7qBwOe&s=BLW968ZKOcdPWg0s4-4AlA_rqiJCCCKPjP-Y-Fux6oI&e= >>> >>>> >>>>> filename for the 241 file. >>>>> >>>>> I've pushed a tree containing a suggested fix (updating this patch). I >>>>> can update it when applying if you like, otherwise please send a new >>>>> version. >>>>> >>>> >>>> Where did you push the tree? >>> >>> Sorry I forgot to mention that: >>> >>> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_sjg20_u-2Dboot_tree_try-2Drk4&d=DwIBaQ&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&m=AH6ijvL2fp8TDrFjgeja0AVykFkzBjRPDOAfg8m_eKMHJW7RzTVA1wMpGn7qBwOe&s=7LOQoSkcQA52SvFgC_aUR4l2MtMWjdVM-t_bCKUetEs&e= >>> >> >> >> I do not understand how you found out coverage was not happy about my >> patchset. I have the same percentage reported from your branch or my >> local one. What am I missing? >> >> Regarding the content of the changed commits: >> testMkimageMultipleNoContent is not testing what is says it does? >> It's using multiple-data-files DT property which only impacts -d >> parameter of mkimage and the comment for the test is """Test using >> mkimage with -n and no data""". >> >> What exactly are you trying to test? > > 'binman test -T' > > I pushed your original patches to the try-rk4-orig branch. My changes > are in try-rk4. > > With yours I see this: > > ======================== Running binman tests ======================== > ........................................................................................................................................................................................................................................................................................................................................................................................................................................................................ > ---------------------------------------------------------------------- > Ran 456 tests in 19.669s > > OK > > 99% > Name Stmts Miss Cover > --------------------------------------------------------------------------- > tools/binman/__init__.py 0 0 100% > tools/binman/bintool.py 254 0 100% > tools/binman/btool/btool_gzip.py 5 0 100% > tools/binman/btool/bzip2.py 5 0 100% > tools/binman/btool/cbfstool.py 24 0 100% > tools/binman/btool/fiptool.py 22 0 100% > tools/binman/btool/futility.py 24 0 100% > tools/binman/btool/ifwitool.py 22 0 100% > tools/binman/btool/lz4.py 28 0 100% > tools/binman/btool/lzma_alone.py 34 0 100% > tools/binman/btool/lzop.py 5 0 100% > tools/binman/btool/mkimage.py 29 0 100% > tools/binman/btool/xz.py 5 0 100% > tools/binman/btool/zstd.py 5 0 100% > tools/binman/cbfs_util.py 366 0 100% > tools/binman/cmdline.py 73 0 100% > tools/binman/control.py 342 0 100% > tools/binman/elf.py 195 0 100% > tools/binman/entry.py 483 0 100% > tools/binman/etype/atf_bl31.py 5 0 100% > tools/binman/etype/atf_fip.py 67 0 100% > tools/binman/etype/blob.py 39 0 100% > tools/binman/etype/blob_dtb.py 46 0 100% > tools/binman/etype/blob_ext.py 11 0 100% > tools/binman/etype/blob_ext_list.py 32 0 100% > tools/binman/etype/blob_named_by_arg.py 9 0 100% > tools/binman/etype/blob_phase.py 16 0 100% > tools/binman/etype/cbfs.py 101 0 100% > tools/binman/etype/collection.py 30 0 100% > tools/binman/etype/cros_ec_rw.py 5 0 100% > tools/binman/etype/fdtmap.py 62 0 100% > tools/binman/etype/files.py 35 0 100% > tools/binman/etype/fill.py 13 0 100% > tools/binman/etype/fit.py 214 0 100% > tools/binman/etype/fmap.py 34 0 100% > tools/binman/etype/gbb.py 37 0 100% > tools/binman/etype/image_header.py 53 0 100% > tools/binman/etype/intel_cmc.py 4 0 100% > tools/binman/etype/intel_descriptor.py 39 0 100% > tools/binman/etype/intel_fit.py 12 0 100% > tools/binman/etype/intel_fit_ptr.py 17 0 100% > tools/binman/etype/intel_fsp.py 4 0 100% > tools/binman/etype/intel_fsp_m.py 4 0 100% > tools/binman/etype/intel_fsp_s.py 4 0 100% > tools/binman/etype/intel_fsp_t.py 4 0 100% > tools/binman/etype/intel_ifwi.py 67 0 100% > tools/binman/etype/intel_me.py 4 0 100% > tools/binman/etype/intel_mrc.py 6 0 100% > tools/binman/etype/intel_refcode.py 6 0 100% > tools/binman/etype/intel_vbt.py 4 0 100% > tools/binman/etype/intel_vga.py 4 0 100% > tools/binman/etype/mkimage.py 80 1 99% > tools/binman/etype/opensbi.py 5 0 100% > tools/binman/etype/powerpc_mpc85xx_bootpg_resetvec.py 6 0 100% > tools/binman/etype/pre_load.py 77 0 100% > tools/binman/etype/scp.py 5 0 100% > tools/binman/etype/section.py 376 0 100% > tools/binman/etype/tee_os.py 5 0 100% > tools/binman/etype/text.py 21 0 100% > tools/binman/etype/u_boot.py 7 0 100% > tools/binman/etype/u_boot_dtb.py 9 0 100% > tools/binman/etype/u_boot_dtb_with_ucode.py 51 0 100% > tools/binman/etype/u_boot_elf.py 19 0 100% > tools/binman/etype/u_boot_env.py 27 0 100% > tools/binman/etype/u_boot_expanded.py 4 0 100% > tools/binman/etype/u_boot_img.py 7 0 100% > tools/binman/etype/u_boot_nodtb.py 7 0 100% > tools/binman/etype/u_boot_spl.py 11 0 100% > tools/binman/etype/u_boot_spl_bss_pad.py 14 0 100% > tools/binman/etype/u_boot_spl_dtb.py 9 0 100% > tools/binman/etype/u_boot_spl_elf.py 7 0 100% > tools/binman/etype/u_boot_spl_expanded.py 12 0 100% > tools/binman/etype/u_boot_spl_nodtb.py 11 0 100% > tools/binman/etype/u_boot_spl_with_ucode_ptr.py 8 0 100% > tools/binman/etype/u_boot_tpl.py 11 0 100% > tools/binman/etype/u_boot_tpl_bss_pad.py 14 0 100% > tools/binman/etype/u_boot_tpl_dtb.py 9 0 100% > tools/binman/etype/u_boot_tpl_dtb_with_ucode.py 8 0 100% > tools/binman/etype/u_boot_tpl_elf.py 7 0 100% > tools/binman/etype/u_boot_tpl_expanded.py 12 0 100% > tools/binman/etype/u_boot_tpl_nodtb.py 11 0 100% > tools/binman/etype/u_boot_tpl_with_ucode_ptr.py 12 0 100% > tools/binman/etype/u_boot_ucode.py 33 0 100% > tools/binman/etype/u_boot_with_ucode_ptr.py 42 0 100% > tools/binman/etype/vblock.py 38 0 100% > tools/binman/etype/x86_reset16.py 7 0 100% > tools/binman/etype/x86_reset16_spl.py 7 0 100% > tools/binman/etype/x86_reset16_tpl.py 7 0 100% > tools/binman/etype/x86_start16.py 7 0 100% > tools/binman/etype/x86_start16_spl.py 7 0 100% > tools/binman/etype/x86_start16_tpl.py 7 0 100% > tools/binman/fip_util.py 202 0 100% > tools/binman/fmap_util.py 48 0 100% > tools/binman/image.py 164 0 100% > tools/binman/state.py 201 0 100% > --------------------------------------------------------------------------- > TOTAL 4541 1 99% > > To get a report in 'htmlcov/index.html', type: python3-coverage html > Coverage error: 99%, but should be 100% > ValueError: Test coverage failure > I get 52% coverage only with that exact same branch, something's definitely wrong here in my setup. And I **definitely** do not have tools/binman/etype/mkimage.py listed in there.... Mmmmmm. > > It is only a tiny difference! Basically we need to support the > contents of an entry being unavailable, temporarily or permanently, so > I added a test for that. > I'll play with binman until I manage to get a coverage percentage equal to yours. Cheers, Quentin
Hi Quentin, On Wed, 31 Aug 2022 at 03:25, Quentin Schulz <quentin.schulz@theobroma-systems.com> wrote: > > Hi Simon, > > On 8/31/22 05:15, Simon Glass wrote: > > Hi Quentin, > > > > On Tue, 30 Aug 2022 at 11:54, Quentin Schulz > > <quentin.schulz@theobroma-systems.com> wrote: > >> > >> Hi Simon, > >> > >> On 8/30/22 17:56, Simon Glass wrote: > >>> Hi Quentin, > >>> > >>> On Tue, 30 Aug 2022 at 03:57, Quentin Schulz > >>> <quentin.schulz@theobroma-systems.com> wrote: > >>>> > >>>> Hi Simon, > >>>> > >>>> On 8/27/22 02:21, Simon Glass wrote: > >>>>> Hi Quentin, > >>>>> > >>>>> On Fri, 26 Aug 2022 at 09:37, Quentin Schulz <foss+uboot@0leil.net> wrote: > >>>>>> > >>>>>> From: Quentin Schulz <quentin.schulz@theobroma-systems.com> > >>>>>> > >>>>>> Some image types handled by mkimage require the datafiles to be passed > >>>>>> independently (-d data1:data2) for specific handling of each. A > >>>>>> concatenation of datafiles prior to passing them to mkimage wouldn't > >>>>>> work. > >>>>>> > >>>>>> That is the case for rkspi for example which requires page alignment > >>>>>> and only writing 2KB every 4KB. > >>>>>> > >>>>>> This adds the ability to tell binman to pass the datafiles without > >>>>>> prior concatenation to mkimage, by adding the multiple-data-files > >>>>>> boolean property to the mkimage node. > >>>>>> > >>>>>> Cc: Quentin Schulz <foss+uboot@0leil.net> > >>>>>> Reviewed-by: Simon Glass <sjg@chromium.org> > >>>>>> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com> > >>>>>> --- > >>>>>> > >>>>>> v5: > >>>>>> - changed to use full path from input dir with tools.get_input_filename > >>>>>> to make it possible to run the unit tests, > >>>>>> - added unit test, > >>>>>> > >>>>>> > >>>>>> tools/binman/entries.rst | 22 ++++++++++ > >>>>>> tools/binman/etype/mkimage.py | 41 +++++++++++++++++-- > >>>>>> tools/binman/ftest.py | 16 ++++++++ > >>>>> > >>>>> Please put the new test at the end. > >>>>> > >>>>>> .../test/241_mkimage_multiple_data_files.dts | 21 ++++++++++ > >>>>>> 4 files changed, 96 insertions(+), 4 deletions(-) > >>>>>> create mode 100644 tools/binman/test/241_mkimage_multiple_data_files.dts > >>>>> > >>>>> This is pretty close but it still missing a line of test coverage. > >>>>> Please try 'binman test -T' to see it. I'd also prefer a shorter > >>>> > >>>> This does not work on Fedora. > >>>> 1) there's no python3-coverage binary available, > >>>> 2) After replacing python3-coverage with just coverage, the tests are > >>>> stuck and never finish, (I have seen the patches to use COVERAGE > >>>> environment variable so I guess the required changes might be tackled > >>>> soon in master), > >>>> > >>>> Any tip on how to identify which test is stuck except going through them > >>>> one by one? > >>> > >>> One way is to add comment blocks '''...''' across the ftest.py file, > >>> using a binary chop to identify the problem. > >>> > >>> Or, since tests are run in series, you could hack test_util to pass > >>> verbose parameters when it runs the tests - see 'cmd =' in > >>> run_test_coverage(). > >>> > >> > >> I just commented out tests and found the following two are failing on my > >> system: > >> testCompUtilVersions and testListBintools. > >> > >> After digging a bit it seems that it is stuck here: > >> https://urldefense.proofpoint.com/v2/url?u=https-3A__source.denx.de_u-2Dboot_u-2Dboot_-2D_blob_master_tools_patman_command.py-23L105&d=DwIFaQ&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&m=MbdarH_QzLsQMF7mlCoDjHMwaiirTOGXbHZkHpb79AsDp1RElUfGn9NaYl4FQIJw&s=goCAUmylx43E8E8Yf9t6iJdmCGODogQVuiVjJ7qebwc&e= > >> for bzip2. > >> > >> Furthermore: > >> bzip2 -V > /dev/null > >> bzip2 -V > /dev/null 2>&1 > > > > I wonder why that would hang. Can you try 'bzip2 -V' on the cmdline? > > > >> both get stuck which I assume is where the issue lies :) > >> > >> bzip2 --help is just fine BTW. > >> > >> I tested on a colleague's PC running Ubuntu 22.04.1, it works as > >> intended. I guess I'll have to check if Fedora or Ubuntu has patches on > >> top of bzip2 source code that triggers/patches this behavior. > > > > Very strange! > > > > OK. Upstream "bug", see: > https://sourceware.org/git/?p=bzip2.git;a=blob;f=bzip2.c;h=1538faf73a8b311f53f0fe608347de761196de90;hb=HEAD#l1902 > > When you pass the -V or -L option, the program does not exit (unlike > --help). I guess it tries to compress something from stdin and endlessly > waits. I cannot explain why: > bzip2 -V > returns -1 directly > but > bzip2 -V > /dev/null > is stuck. > > $ strace bzip2 -V > /dev/null > [...] > write(2, "bzip2, a block-sorting file comp"..., 540bzip2, a > block-sorting file compressor. Version 1.0.8, 13-Jul-2019. > > Copyright (C) 1996-2019 by Julian Seward. > > This program is free software; you can redistribute it and/or modify > it under the terms set out in the LICENSE file, which is included > in the bzip2 source distribution. > > This program is distributed in the hope that it will be useful, > but WITHOUT ANY WARRANTY; without even the implied warranty of > MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > LICENSE file for more details. > > ) = 540 > ioctl(1, TCGETS, 0x7ffdddef4390) = -1 ENOTTY (Inappropriate ioctl > for device) > mmap(NULL, 3600384, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, > 0) = 0x7ff9a1091000 > mmap(NULL, 3600384, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, > 0) = 0x7ff9a0d22000 > mmap(NULL, 266240, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, > 0) = 0x7ff9a163f000 > newfstatat(0, "", {st_mode=S_IFCHR|0620, st_rdev=makedev(0x88, 0x4), > ...}, AT_EMPTY_PATH) = 0 > read(0, > > This is "fixed" in Ubuntu with: > https://git.launchpad.net/ubuntu/+source/bzip2/tree/debian/patches/20-legacy.patch?h=ubuntu/jammy > > I suggest that we use bzip2 --help instead in binman. It does print the > version name there so it should be just fine. I'll send a patch for > binman and open a bug or something on bzip2 ML to find out what exactly > they are trying to do (if it's on purpose for example). > > >> > >>>> > >>>> python3-coverage is also not available in the container image built from > >>>> tools/docker/Dockerfile. > >>> > >>> does 'python3 -m coverage' work? > >>> > >> > >> diff --git a/tools/patman/test_util.py b/tools/patman/test_util.py > >> index 0f6d1aa902..eaa769a564 100644 > >> --- a/tools/patman/test_util.py > >> +++ b/tools/patman/test_util.py > >> @@ -58,11 +58,11 @@ def run_test_coverage(prog, filter_fname, > >> exclude_list, build_dir, required=None > >> prefix = '' > >> if build_dir: > >> prefix = 'PYTHONPATH=$PYTHONPATH:%s/sandbox_spl/tools ' % > >> build_dir > >> - cmd = ('%spython3-coverage run ' > >> + cmd = ('%spython3 -m coverage run ' > >> '--omit "%s" %s %s %s -P1' % (prefix, ','.join(glob_list), > >> prog, extra_args or '', > >> test_cmd)) > >> os.system(cmd) > >> - stdout = command.output('python3-coverage', 'report') > >> + stdout = command.output('python3', '-m', 'coverage', 'report') > >> lines = stdout.splitlines() > >> if required: > >> # Convert '/path/to/name.py' just the module name 'name' > >> @@ -81,7 +81,7 @@ def run_test_coverage(prog, filter_fname, > >> exclude_list, build_dir, required=None > >> print(coverage) > >> if coverage != '100%': > >> print(stdout) > >> - print("To get a report in 'htmlcov/index.html', type: > >> python3-coverage html") > >> + print("To get a report in 'htmlcov/index.html', type: python3 > >> -m coverage html") > >> print('Coverage error: %s, but should be 100%%' % coverage) > >> ok = False > >> if not ok: > >> > >> works just fine for me. > >> > >> Michal Suchánek seems to disagree with me on this one, see > >> https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_u-2Dboot_20220830101149.GM28810-40kitsune.suse.cz_&d=DwIFaQ&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&m=MbdarH_QzLsQMF7mlCoDjHMwaiirTOGXbHZkHpb79AsDp1RElUfGn9NaYl4FQIJw&s=PIpNEgfpEtiIeShj3dhklIwaomQemLRGI3wo8nKxsr8&e= > > > > I don't fully understand that point. > > > > I think it is fine to specify the tool as an env var. > > > > But if -m coverage works in general, let's use it. If not, we'll have > > the env var. > > > > It works for me and I believe it is better. Installing coverage from pip > will install a "coverage" binary. In essence, having a COVERAGE > environment variable is fine, but having it set to python3-coverage by > default means it works by default only on Debian/Ubuntu-based distros: > see https://pkgs.org/search/?q=python3-coverage&on=files > > Also, installing with pip, one can run python3 -m coverage without > adding ~/.local/bin to PATH (unlike using python3-coverage or coverage). > > I suggest we move this discussion to the patch from Michal :) > > > > >> > >>> or this: > >>> > >>> https://urldefense.proofpoint.com/v2/url?u=https-3A__coverage.readthedocs.io_en_6.3.2_install.html&d=DwIBaQ&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&m=AH6ijvL2fp8TDrFjgeja0AVykFkzBjRPDOAfg8m_eKMHJW7RzTVA1wMpGn7qBwOe&s=BLW968ZKOcdPWg0s4-4AlA_rqiJCCCKPjP-Y-Fux6oI&e= > >>> > >>>> > >>>>> filename for the 241 file. > >>>>> > >>>>> I've pushed a tree containing a suggested fix (updating this patch). I > >>>>> can update it when applying if you like, otherwise please send a new > >>>>> version. > >>>>> > >>>> > >>>> Where did you push the tree? > >>> > >>> Sorry I forgot to mention that: > >>> > >>> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_sjg20_u-2Dboot_tree_try-2Drk4&d=DwIBaQ&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&m=AH6ijvL2fp8TDrFjgeja0AVykFkzBjRPDOAfg8m_eKMHJW7RzTVA1wMpGn7qBwOe&s=7LOQoSkcQA52SvFgC_aUR4l2MtMWjdVM-t_bCKUetEs&e= > >>> > >> > >> > >> I do not understand how you found out coverage was not happy about my > >> patchset. I have the same percentage reported from your branch or my > >> local one. What am I missing? > >> > >> Regarding the content of the changed commits: > >> testMkimageMultipleNoContent is not testing what is says it does? > >> It's using multiple-data-files DT property which only impacts -d > >> parameter of mkimage and the comment for the test is """Test using > >> mkimage with -n and no data""". > >> > >> What exactly are you trying to test? > > > > 'binman test -T' > > > > I pushed your original patches to the try-rk4-orig branch. My changes > > are in try-rk4. > > > > With yours I see this: > > > > ======================== Running binman tests ======================== > > ........................................................................................................................................................................................................................................................................................................................................................................................................................................................................ > > ---------------------------------------------------------------------- > > Ran 456 tests in 19.669s > > > > OK > > > > 99% > > Name Stmts Miss Cover > > --------------------------------------------------------------------------- > > tools/binman/__init__.py 0 0 100% > > tools/binman/bintool.py 254 0 100% > > tools/binman/btool/btool_gzip.py 5 0 100% > > tools/binman/btool/bzip2.py 5 0 100% > > tools/binman/btool/cbfstool.py 24 0 100% > > tools/binman/btool/fiptool.py 22 0 100% > > tools/binman/btool/futility.py 24 0 100% > > tools/binman/btool/ifwitool.py 22 0 100% > > tools/binman/btool/lz4.py 28 0 100% > > tools/binman/btool/lzma_alone.py 34 0 100% > > tools/binman/btool/lzop.py 5 0 100% > > tools/binman/btool/mkimage.py 29 0 100% > > tools/binman/btool/xz.py 5 0 100% > > tools/binman/btool/zstd.py 5 0 100% > > tools/binman/cbfs_util.py 366 0 100% > > tools/binman/cmdline.py 73 0 100% > > tools/binman/control.py 342 0 100% > > tools/binman/elf.py 195 0 100% > > tools/binman/entry.py 483 0 100% > > tools/binman/etype/atf_bl31.py 5 0 100% > > tools/binman/etype/atf_fip.py 67 0 100% > > tools/binman/etype/blob.py 39 0 100% > > tools/binman/etype/blob_dtb.py 46 0 100% > > tools/binman/etype/blob_ext.py 11 0 100% > > tools/binman/etype/blob_ext_list.py 32 0 100% > > tools/binman/etype/blob_named_by_arg.py 9 0 100% > > tools/binman/etype/blob_phase.py 16 0 100% > > tools/binman/etype/cbfs.py 101 0 100% > > tools/binman/etype/collection.py 30 0 100% > > tools/binman/etype/cros_ec_rw.py 5 0 100% > > tools/binman/etype/fdtmap.py 62 0 100% > > tools/binman/etype/files.py 35 0 100% > > tools/binman/etype/fill.py 13 0 100% > > tools/binman/etype/fit.py 214 0 100% > > tools/binman/etype/fmap.py 34 0 100% > > tools/binman/etype/gbb.py 37 0 100% > > tools/binman/etype/image_header.py 53 0 100% > > tools/binman/etype/intel_cmc.py 4 0 100% > > tools/binman/etype/intel_descriptor.py 39 0 100% > > tools/binman/etype/intel_fit.py 12 0 100% > > tools/binman/etype/intel_fit_ptr.py 17 0 100% > > tools/binman/etype/intel_fsp.py 4 0 100% > > tools/binman/etype/intel_fsp_m.py 4 0 100% > > tools/binman/etype/intel_fsp_s.py 4 0 100% > > tools/binman/etype/intel_fsp_t.py 4 0 100% > > tools/binman/etype/intel_ifwi.py 67 0 100% > > tools/binman/etype/intel_me.py 4 0 100% > > tools/binman/etype/intel_mrc.py 6 0 100% > > tools/binman/etype/intel_refcode.py 6 0 100% > > tools/binman/etype/intel_vbt.py 4 0 100% > > tools/binman/etype/intel_vga.py 4 0 100% > > tools/binman/etype/mkimage.py 80 1 99% > > tools/binman/etype/opensbi.py 5 0 100% > > tools/binman/etype/powerpc_mpc85xx_bootpg_resetvec.py 6 0 100% > > tools/binman/etype/pre_load.py 77 0 100% > > tools/binman/etype/scp.py 5 0 100% > > tools/binman/etype/section.py 376 0 100% > > tools/binman/etype/tee_os.py 5 0 100% > > tools/binman/etype/text.py 21 0 100% > > tools/binman/etype/u_boot.py 7 0 100% > > tools/binman/etype/u_boot_dtb.py 9 0 100% > > tools/binman/etype/u_boot_dtb_with_ucode.py 51 0 100% > > tools/binman/etype/u_boot_elf.py 19 0 100% > > tools/binman/etype/u_boot_env.py 27 0 100% > > tools/binman/etype/u_boot_expanded.py 4 0 100% > > tools/binman/etype/u_boot_img.py 7 0 100% > > tools/binman/etype/u_boot_nodtb.py 7 0 100% > > tools/binman/etype/u_boot_spl.py 11 0 100% > > tools/binman/etype/u_boot_spl_bss_pad.py 14 0 100% > > tools/binman/etype/u_boot_spl_dtb.py 9 0 100% > > tools/binman/etype/u_boot_spl_elf.py 7 0 100% > > tools/binman/etype/u_boot_spl_expanded.py 12 0 100% > > tools/binman/etype/u_boot_spl_nodtb.py 11 0 100% > > tools/binman/etype/u_boot_spl_with_ucode_ptr.py 8 0 100% > > tools/binman/etype/u_boot_tpl.py 11 0 100% > > tools/binman/etype/u_boot_tpl_bss_pad.py 14 0 100% > > tools/binman/etype/u_boot_tpl_dtb.py 9 0 100% > > tools/binman/etype/u_boot_tpl_dtb_with_ucode.py 8 0 100% > > tools/binman/etype/u_boot_tpl_elf.py 7 0 100% > > tools/binman/etype/u_boot_tpl_expanded.py 12 0 100% > > tools/binman/etype/u_boot_tpl_nodtb.py 11 0 100% > > tools/binman/etype/u_boot_tpl_with_ucode_ptr.py 12 0 100% > > tools/binman/etype/u_boot_ucode.py 33 0 100% > > tools/binman/etype/u_boot_with_ucode_ptr.py 42 0 100% > > tools/binman/etype/vblock.py 38 0 100% > > tools/binman/etype/x86_reset16.py 7 0 100% > > tools/binman/etype/x86_reset16_spl.py 7 0 100% > > tools/binman/etype/x86_reset16_tpl.py 7 0 100% > > tools/binman/etype/x86_start16.py 7 0 100% > > tools/binman/etype/x86_start16_spl.py 7 0 100% > > tools/binman/etype/x86_start16_tpl.py 7 0 100% > > tools/binman/fip_util.py 202 0 100% > > tools/binman/fmap_util.py 48 0 100% > > tools/binman/image.py 164 0 100% > > tools/binman/state.py 201 0 100% > > --------------------------------------------------------------------------- > > TOTAL 4541 1 99% > > > > To get a report in 'htmlcov/index.html', type: python3-coverage html > > Coverage error: 99%, but should be 100% > > ValueError: Test coverage failure > > > > I get 52% coverage only with that exact same branch, something's > definitely wrong here in my setup. And I **definitely** do not have > tools/binman/etype/mkimage.py listed in there.... Mmmmmm. You may need to get some bintools with 'binman tool -f missing'. But in any case, you only need to worry about coverage in mkimage.py which is what you changed. > > > > > It is only a tiny difference! Basically we need to support the > > contents of an entry being unavailable, temporarily or permanently, so > > I added a test for that. > > > > I'll play with binman until I manage to get a coverage percentage equal > to yours. OK, I'd appreciate a docs patch if you can produce one from your efforts, or any feedback on how to make this automatic / easy. Regards, Simon
Hi Simon, On 8/31/22 15:46, Simon Glass wrote: > Hi Quentin, > > On Wed, 31 Aug 2022 at 03:25, Quentin Schulz > <quentin.schulz@theobroma-systems.com> wrote: >> >> Hi Simon, >> >> On 8/31/22 05:15, Simon Glass wrote: >>> Hi Quentin, >>> >>> On Tue, 30 Aug 2022 at 11:54, Quentin Schulz >>> <quentin.schulz@theobroma-systems.com> wrote: >>>> >>>> Hi Simon, >>>> >>>> On 8/30/22 17:56, Simon Glass wrote: >>>>> Hi Quentin, >>>>> >>>>> On Tue, 30 Aug 2022 at 03:57, Quentin Schulz >>>>> <quentin.schulz@theobroma-systems.com> wrote: [...] >>>> I do not understand how you found out coverage was not happy about my >>>> patchset. I have the same percentage reported from your branch or my >>>> local one. What am I missing? >>>> >>>> Regarding the content of the changed commits: >>>> testMkimageMultipleNoContent is not testing what is says it does? >>>> It's using multiple-data-files DT property which only impacts -d >>>> parameter of mkimage and the comment for the test is """Test using >>>> mkimage with -n and no data""". >>>> >>>> What exactly are you trying to test? >>> >>> 'binman test -T' >>> >>> I pushed your original patches to the try-rk4-orig branch. My changes >>> are in try-rk4. >>> I would change https://github.com/sjg20/u-boot/blob/try-rk4/tools/binman/ftest.py#L5917 from """Test using mkimage with -n and no data""" to """Test passing multiple data files to mkimage with one data file having no content""" or something like that. Do you want me to send a v6 for this? Otherwise, looks fine to me :) With the bzip2 patch I just sent, I could see the line missing from unit tests. However, unit tests aren't happy on my PC. $ tools/binman/binman tool --list Name Version Description Path --------------- ----------- ------------------------- ------------------------------ gzip 1.11 gzip compression /usr/bin/gzip bzip2 1.0.8 bzip2 compression /usr/bin/bzip2 cbfstool unknown Manipulate CBFS files /home/qschulz/work/upstream/coreboot/util/cbfstool/cbfstool fiptool Unknown vers Manipulate ATF FIP files /home/qschulz/work/upstream/trusted-firmware-a/tools/fiptool/fiptool futility v0.0.3050-18 Chromium OS firmware utili /home/qschulz/work/upstream/coreboot/util/futility/futility ifwitool unknown Manipulate Intel IFWI file /home/qschulz/work/upstream/coreboot/util/cbfstool/ifwitool lz4 v1.9.3 lz4 compression /usr/bin/lz4 lzma_alone - lzma_alone compression (not found) lzop v1.04 lzo compression /usr/bin/lzop mkimage 2022.10-rc3- Generate image for U-Boot /usr/bin/mkimage xz 5.2.5 xz compression /usr/bin/xz zstd v1.5.2 zstd compression /usr/bin/zstd $ tools/binman/binman test -T ======================== Running binman tests ======================== .........................................................sss.............................................F....Fs..............................................................................................F.................................................................................................................................................EEEEFF.................................FF.......FF............F................F.FFFF................. ====================================================================== ERROR: testSymbols (binman.ftest.TestFunctional) Test binman can assign symbols embedded in U-Boot ---------------------------------------------------------------------- KeyError: '_binman_sym_magic' ====================================================================== ERROR: testSymbolsExpanded (binman.ftest.TestFunctional) Test binman can assign symbols in expanded entries ---------------------------------------------------------------------- KeyError: '_binman_sym_magic' ====================================================================== ERROR: testSymbolsNoDtb (binman.ftest.TestFunctional) Test binman can assign symbols embedded in U-Boot SPL ---------------------------------------------------------------------- KeyError: '_binman_sym_magic' ====================================================================== ERROR: testSymbolsSubsection (binman.ftest.TestFunctional) Test binman can assign symbols from a subsection ---------------------------------------------------------------------- KeyError: '_binman_sym_magic' ====================================================================== FAIL: testExtractAllEntries (binman.ftest.TestFunctional) Test extracting all entries ---------------------------------------------------------------------- AssertionError: 969 != 0 ====================================================================== FAIL: testExtractCbfsCompressed (binman.ftest.TestFunctional) Test extracting CBFS compressed data ---------------------------------------------------------------------- AssertionError: 969 != 0 ====================================================================== FAIL: testListCmd (binman.ftest.TestFunctional) Test listing the files in an image using an Fdtmap ---------------------------------------------------------------------- AssertionError: Lists differ: ['Nam[463 chars]80 105 u-boot-dtb 80 3c9', [184 chars]bf8'] != ['Nam[463 chars]80 0 u-boot-dtb 80 3c9', [184 chars]bf8'] First differing element 7: ' u-boot-dtb 180 105 u-boot-dtb 80 3c9' ' u-boot-dtb 180 0 u-boot-dtb 80 3c9' Diff is 888 characters long. Set self.maxDiff to None to see it. ====================================================================== FAIL: testSymbolsTplSection (binman.ftest.TestFunctional) Test binman can assign symbols embedded in U-Boot TPL in a section ---------------------------------------------------------------------- AssertionError: b'\xff\xff\xff\xffBSYM\x04\x00\x00\x00 \x00\x00\x00\x00\x00[36 chars]0klm' != b'\xff\xff\xff\xff56780123456789abcdefghijklm' ====================================================================== FAIL: testSymbolsTplSectionX86 (binman.ftest.TestFunctional) Test binman can assign symbols in a section with end-at-4gb ---------------------------------------------------------------------- AssertionError: b'\xff\xff\xff\xffBSYM\x04\xff\xff\xff \xff\xff\xff\x00\x00[36 chars]0klm' != b'\xff\xff\xff\xff56780123456789abcdefghijklm' ====================================================================== FAIL: testBadSymbolSize (binman.elf_test.TestElf) Test that an attempt to use an 8-bit symbol are detected ---------------------------------------------------------------------- AssertionError: ValueError not raised ====================================================================== FAIL: testDebug (binman.elf_test.TestElf) Check that enabling debug in the elf module produced debug output ---------------------------------------------------------------------- AssertionError: False is not true ====================================================================== FAIL: testNoValue (binman.elf_test.TestElf) Test the case where we have no value for the symbol ---------------------------------------------------------------------- AssertionError: b'BSYM\xff\xff\xff\xff\xff\xff\xff\xff\xff\[43 chars]aaaa' != b'aaaaaaaaaaaaaaaaaaaaaaaaaaaa' ====================================================================== FAIL: testOutsideFile (binman.elf_test.TestElf) Test a symbol which extends outside the entry area is detected ---------------------------------------------------------------------- AssertionError: ValueError not raised ====================================================================== FAIL: test_cbfs_arch (binman.cbfs_util_test.TestCbfs) Test on non-x86 architecture ---------------------------------------------------------------------- AssertionError: cbfstool produced a different result Stdout: diff -y <(xxd -g1 /tmp/expect) <(xxd -g1 /tmp/actual) | colordiff ====================================================================== FAIL: test_cbfs_offset (binman.cbfs_util_test.TestCbfs) Test a CBFS with files at particular offsets ---------------------------------------------------------------------- AssertionError: cbfstool produced a different result Stdout: diff -y <(xxd -g1 /tmp/expect) <(xxd -g1 /tmp/actual) | colordiff ====================================================================== FAIL: test_cbfs_raw (binman.cbfs_util_test.TestCbfs) Test base handling of a Coreboot Filesystem (CBFS) ---------------------------------------------------------------------- AssertionError: cbfstool produced a different result Stdout: diff -y <(xxd -g1 /tmp/expect) <(xxd -g1 /tmp/actual) | colordiff ====================================================================== FAIL: test_cbfs_raw_compress (binman.cbfs_util_test.TestCbfs) Test base handling of compressing raw files ---------------------------------------------------------------------- AssertionError: b'' != b'compress xxxxxxxxxxxxxxxxxxxxxx data' ====================================================================== FAIL: test_cbfs_raw_space (binman.cbfs_util_test.TestCbfs) Test files with unused space in the CBFS ---------------------------------------------------------------------- AssertionError: cbfstool produced a different result Stdout: diff -y <(xxd -g1 /tmp/expect) <(xxd -g1 /tmp/actual) | colordiff ====================================================================== FAIL: test_cbfs_stage (binman.cbfs_util_test.TestCbfs) Tests handling of a Coreboot Filesystem (CBFS) ---------------------------------------------------------------------- AssertionError: cbfstool produced a different result Stdout: diff -y <(xxd -g1 /tmp/expect) <(xxd -g1 /tmp/actual) | colordiff ====================================================================== SKIP: testCompUtilCompressions (binman.ftest.TestFunctional) Test compression algorithms ---------------------------------------------------------------------- lzma_alone not available ====================================================================== SKIP: testCompUtilPadding (binman.ftest.TestFunctional) Test padding of compression algorithms ---------------------------------------------------------------------- lzma_alone not available ====================================================================== SKIP: testCompUtilVersions (binman.ftest.TestFunctional) Test tool version of compression algorithms ---------------------------------------------------------------------- lzma_alone not available ====================================================================== SKIP: testExtractCbfsRaw (binman.ftest.TestFunctional) Test extracting CBFS compressed data without decompressing it ---------------------------------------------------------------------- lzma_alone not available ---------------------------------------------------------------------- Ran 454 tests in 11.738s FAILED (failures=15, errors=4, skipped=4) 99% Name Stmts Miss Cover --------------------------------------------------------------------------- tools/binman/__init__.py 0 0 100% tools/binman/bintool.py 254 0 100% tools/binman/btool/btool_gzip.py 5 0 100% tools/binman/btool/bzip2.py 15 0 100% tools/binman/btool/cbfstool.py 24 0 100% tools/binman/btool/fiptool.py 22 0 100% tools/binman/btool/futility.py 24 0 100% tools/binman/btool/ifwitool.py 22 0 100% tools/binman/btool/lz4.py 28 0 100% tools/binman/btool/lzma_alone.py 34 3 91% tools/binman/btool/lzop.py 5 0 100% tools/binman/btool/mkimage.py 29 0 100% tools/binman/btool/xz.py 5 0 100% tools/binman/btool/zstd.py 5 0 100% tools/binman/cbfs_util.py 366 0 100% tools/binman/cmdline.py 73 0 100% tools/binman/control.py 342 0 100% tools/binman/elf.py 195 18 91% tools/binman/entry.py 483 0 100% tools/binman/etype/atf_bl31.py 5 0 100% tools/binman/etype/atf_fip.py 67 0 100% tools/binman/etype/blob.py 39 0 100% tools/binman/etype/blob_dtb.py 46 0 100% tools/binman/etype/blob_ext.py 11 0 100% tools/binman/etype/blob_ext_list.py 32 0 100% tools/binman/etype/blob_named_by_arg.py 9 0 100% tools/binman/etype/blob_phase.py 16 0 100% tools/binman/etype/cbfs.py 101 0 100% tools/binman/etype/collection.py 30 0 100% tools/binman/etype/cros_ec_rw.py 5 0 100% tools/binman/etype/fdtmap.py 62 0 100% tools/binman/etype/files.py 35 0 100% tools/binman/etype/fill.py 13 0 100% tools/binman/etype/fit.py 214 0 100% tools/binman/etype/fmap.py 34 0 100% tools/binman/etype/gbb.py 37 0 100% tools/binman/etype/image_header.py 53 0 100% tools/binman/etype/intel_cmc.py 4 0 100% tools/binman/etype/intel_descriptor.py 39 0 100% tools/binman/etype/intel_fit.py 12 0 100% tools/binman/etype/intel_fit_ptr.py 17 0 100% tools/binman/etype/intel_fsp.py 4 0 100% tools/binman/etype/intel_fsp_m.py 4 0 100% tools/binman/etype/intel_fsp_s.py 4 0 100% tools/binman/etype/intel_fsp_t.py 4 0 100% tools/binman/etype/intel_ifwi.py 67 0 100% tools/binman/etype/intel_me.py 4 0 100% tools/binman/etype/intel_mrc.py 6 0 100% tools/binman/etype/intel_refcode.py 6 0 100% tools/binman/etype/intel_vbt.py 4 0 100% tools/binman/etype/intel_vga.py 4 0 100% tools/binman/etype/mkimage.py 68 0 100% tools/binman/etype/opensbi.py 5 0 100% tools/binman/etype/powerpc_mpc85xx_bootpg_resetvec.py 6 0 100% tools/binman/etype/pre_load.py 77 0 100% tools/binman/etype/scp.py 5 0 100% tools/binman/etype/section.py 376 12 97% tools/binman/etype/tee_os.py 5 0 100% tools/binman/etype/text.py 21 0 100% tools/binman/etype/u_boot.py 7 0 100% tools/binman/etype/u_boot_dtb.py 9 0 100% tools/binman/etype/u_boot_dtb_with_ucode.py 51 0 100% tools/binman/etype/u_boot_elf.py 19 0 100% tools/binman/etype/u_boot_env.py 27 0 100% tools/binman/etype/u_boot_expanded.py 4 0 100% tools/binman/etype/u_boot_img.py 7 0 100% tools/binman/etype/u_boot_nodtb.py 7 0 100% tools/binman/etype/u_boot_spl.py 11 0 100% tools/binman/etype/u_boot_spl_bss_pad.py 14 0 100% tools/binman/etype/u_boot_spl_dtb.py 9 0 100% tools/binman/etype/u_boot_spl_elf.py 7 0 100% tools/binman/etype/u_boot_spl_expanded.py 12 0 100% tools/binman/etype/u_boot_spl_nodtb.py 11 0 100% tools/binman/etype/u_boot_spl_with_ucode_ptr.py 8 0 100% tools/binman/etype/u_boot_tpl.py 11 0 100% tools/binman/etype/u_boot_tpl_bss_pad.py 14 0 100% tools/binman/etype/u_boot_tpl_dtb.py 9 0 100% tools/binman/etype/u_boot_tpl_dtb_with_ucode.py 8 0 100% tools/binman/etype/u_boot_tpl_elf.py 7 0 100% tools/binman/etype/u_boot_tpl_expanded.py 12 0 100% tools/binman/etype/u_boot_tpl_nodtb.py 11 0 100% tools/binman/etype/u_boot_tpl_with_ucode_ptr.py 12 0 100% tools/binman/etype/u_boot_ucode.py 33 0 100% tools/binman/etype/u_boot_with_ucode_ptr.py 42 0 100% tools/binman/etype/vblock.py 38 0 100% tools/binman/etype/x86_reset16.py 7 0 100% tools/binman/etype/x86_reset16_spl.py 7 0 100% tools/binman/etype/x86_reset16_tpl.py 7 0 100% tools/binman/etype/x86_start16.py 7 0 100% tools/binman/etype/x86_start16_spl.py 7 0 100% tools/binman/etype/x86_start16_tpl.py 7 0 100% tools/binman/fip_util.py 202 0 100% tools/binman/fmap_util.py 48 0 100% tools/binman/image.py 164 4 98% tools/binman/state.py 201 0 100% --------------------------------------------------------------------------- TOTAL 4539 37 99% To get a report in 'htmlcov/index.html', type: python3 -m coverage html Coverage error: 99%, but should be 100% ValueError: Test coverage failure I guess I just should send a new mail so the community can have a look at it? I sent two patches for low hanging fruits but I'm afraid I won't have time to look at those issues any time soon :/ I'm running Fedora Server 36 x86_64 fully up-to-date if that helps. [...] >> I get 52% coverage only with that exact same branch, something's >> definitely wrong here in my setup. And I **definitely** do not have >> tools/binman/etype/mkimage.py listed in there.... Mmmmmm. > > You may need to get some bintools with 'binman tool -f missing'. But It expects apt package manager, I only have dnf :) Also, tries to download binaries from a Google Drive, no thank you :) > in any case, you only need to worry about coverage in mkimage.py which > is what you changed. > That's fair. Thanks for fixing my patches, lemme know if you want a v6 instead. [...] >> >> I'll play with binman until I manage to get a coverage percentage equal >> to yours. > > OK, I'd appreciate a docs patch if you can produce one from your > efforts, or any feedback on how to make this automatic / easy. > I copied the errors I'm having right now a few paragraphs above but the biggest issue was bzip2 getting stuck and messing up the whole test suite. With it patched, I was able to see the line that was kept untested in my patches. Otherwise I believe your advice of running binman test -T was enough if that had worked, just bad luck :) Thanks! Quentin
Hi Quentin, On Wed, 31 Aug 2022 at 10:39, Quentin Schulz <quentin.schulz@theobroma-systems.com> wrote: > > Hi Simon, > > On 8/31/22 15:46, Simon Glass wrote: > > Hi Quentin, > > > > On Wed, 31 Aug 2022 at 03:25, Quentin Schulz > > <quentin.schulz@theobroma-systems.com> wrote: > >> > >> Hi Simon, > >> > >> On 8/31/22 05:15, Simon Glass wrote: > >>> Hi Quentin, > >>> > >>> On Tue, 30 Aug 2022 at 11:54, Quentin Schulz > >>> <quentin.schulz@theobroma-systems.com> wrote: > >>>> > >>>> Hi Simon, > >>>> > >>>> On 8/30/22 17:56, Simon Glass wrote: > >>>>> Hi Quentin, > >>>>> > >>>>> On Tue, 30 Aug 2022 at 03:57, Quentin Schulz > >>>>> <quentin.schulz@theobroma-systems.com> wrote: > [...] > >>>> I do not understand how you found out coverage was not happy about my > >>>> patchset. I have the same percentage reported from your branch or my > >>>> local one. What am I missing? > >>>> > >>>> Regarding the content of the changed commits: > >>>> testMkimageMultipleNoContent is not testing what is says it does? > >>>> It's using multiple-data-files DT property which only impacts -d > >>>> parameter of mkimage and the comment for the test is """Test using > >>>> mkimage with -n and no data""". > >>>> > >>>> What exactly are you trying to test? > >>> > >>> 'binman test -T' > >>> > >>> I pushed your original patches to the try-rk4-orig branch. My changes > >>> are in try-rk4. > >>> > > I would change > https://github.com/sjg20/u-boot/blob/try-rk4/tools/binman/ftest.py#L5917 > from > """Test using mkimage with -n and no data""" > to > """Test passing multiple data files to mkimage with one data file having > no content""" > or something like that. Do you want me to send a v6 for this? > > Otherwise, looks fine to me :) > > With the bzip2 patch I just sent, I could see the line missing from unit > tests. > > However, unit tests aren't happy on my PC. > > $ tools/binman/binman tool --list > Name Version Description Path > --------------- ----------- ------------------------- > ------------------------------ > gzip 1.11 gzip compression /usr/bin/gzip > bzip2 1.0.8 bzip2 compression /usr/bin/bzip2 > cbfstool unknown Manipulate CBFS files > /home/qschulz/work/upstream/coreboot/util/cbfstool/cbfstool > fiptool Unknown vers Manipulate ATF FIP files > /home/qschulz/work/upstream/trusted-firmware-a/tools/fiptool/fiptool > futility v0.0.3050-18 Chromium OS firmware utili > /home/qschulz/work/upstream/coreboot/util/futility/futility > ifwitool unknown Manipulate Intel IFWI file > /home/qschulz/work/upstream/coreboot/util/cbfstool/ifwitool > lz4 v1.9.3 lz4 compression /usr/bin/lz4 > lzma_alone - lzma_alone compression (not found) > lzop v1.04 lzo compression /usr/bin/lzop > mkimage 2022.10-rc3- Generate image for U-Boot /usr/bin/mkimage > xz 5.2.5 xz compression /usr/bin/xz > zstd v1.5.2 zstd compression /usr/bin/zstd > $ tools/binman/binman test -T > ======================== Running binman tests ======================== > .........................................................sss.............................................F....Fs..............................................................................................F.................................................................................................................................................EEEEFF.................................FF.......FF............F................F.FFFF................. > ====================================================================== > ERROR: testSymbols (binman.ftest.TestFunctional) > Test binman can assign symbols embedded in U-Boot > ---------------------------------------------------------------------- > KeyError: '_binman_sym_magic' This could be a naming difference with the bintools on Fedora. > > ====================================================================== > ERROR: testSymbolsExpanded (binman.ftest.TestFunctional) > Test binman can assign symbols in expanded entries > ---------------------------------------------------------------------- > KeyError: '_binman_sym_magic' > > ====================================================================== > ERROR: testSymbolsNoDtb (binman.ftest.TestFunctional) > Test binman can assign symbols embedded in U-Boot SPL > ---------------------------------------------------------------------- > KeyError: '_binman_sym_magic' > > ====================================================================== > ERROR: testSymbolsSubsection (binman.ftest.TestFunctional) > Test binman can assign symbols from a subsection > ---------------------------------------------------------------------- > KeyError: '_binman_sym_magic' > > ====================================================================== > FAIL: testExtractAllEntries (binman.ftest.TestFunctional) > Test extracting all entries > ---------------------------------------------------------------------- > AssertionError: 969 != 0 > > ====================================================================== > FAIL: testExtractCbfsCompressed (binman.ftest.TestFunctional) > Test extracting CBFS compressed data > ---------------------------------------------------------------------- > AssertionError: 969 != 0 That is a bit of a mystery. > > ====================================================================== > FAIL: testListCmd (binman.ftest.TestFunctional) > Test listing the files in an image using an Fdtmap > ---------------------------------------------------------------------- > AssertionError: Lists differ: ['Nam[463 chars]80 105 u-boot-dtb > 80 3c9', [184 chars]bf8'] != ['Nam[463 chars]80 0 > u-boot-dtb 80 3c9', [184 chars]bf8'] > > First differing element 7: > ' u-boot-dtb 180 105 u-boot-dtb 80 3c9' > ' u-boot-dtb 180 0 u-boot-dtb 80 3c9' > > Diff is 888 characters long. Set self.maxDiff to None to see it. > > ====================================================================== > FAIL: testSymbolsTplSection (binman.ftest.TestFunctional) > Test binman can assign symbols embedded in U-Boot TPL in a section > ---------------------------------------------------------------------- > AssertionError: b'\xff\xff\xff\xffBSYM\x04\x00\x00\x00 > \x00\x00\x00\x00\x00[36 chars]0klm' != > b'\xff\xff\xff\xff56780123456789abcdefghijklm' > > ====================================================================== > FAIL: testSymbolsTplSectionX86 (binman.ftest.TestFunctional) > Test binman can assign symbols in a section with end-at-4gb > ---------------------------------------------------------------------- > AssertionError: b'\xff\xff\xff\xffBSYM\x04\xff\xff\xff > \xff\xff\xff\x00\x00[36 chars]0klm' != > b'\xff\xff\xff\xff56780123456789abcdefghijklm' > > ====================================================================== > FAIL: testBadSymbolSize (binman.elf_test.TestElf) > Test that an attempt to use an 8-bit symbol are detected > ---------------------------------------------------------------------- > AssertionError: ValueError not raised > > ====================================================================== > FAIL: testDebug (binman.elf_test.TestElf) > Check that enabling debug in the elf module produced debug output > ---------------------------------------------------------------------- > AssertionError: False is not true > > ====================================================================== > FAIL: testNoValue (binman.elf_test.TestElf) > Test the case where we have no value for the symbol > ---------------------------------------------------------------------- > AssertionError: b'BSYM\xff\xff\xff\xff\xff\xff\xff\xff\xff\[43 > chars]aaaa' != b'aaaaaaaaaaaaaaaaaaaaaaaaaaaa' Probably the symbol table is not being read correctly in elf.py > > ====================================================================== > FAIL: testOutsideFile (binman.elf_test.TestElf) > Test a symbol which extends outside the entry area is detected > ---------------------------------------------------------------------- > AssertionError: ValueError not raised > > ====================================================================== > FAIL: test_cbfs_arch (binman.cbfs_util_test.TestCbfs) > Test on non-x86 architecture > ---------------------------------------------------------------------- > AssertionError: cbfstool produced a different result There is no particular spec for what cbfstool does, so perhaps you have a different version. Above it shows you have none at all, actually. > > Stdout: > diff -y <(xxd -g1 /tmp/expect) <(xxd -g1 /tmp/actual) | colordiff > > ====================================================================== > FAIL: test_cbfs_offset (binman.cbfs_util_test.TestCbfs) > Test a CBFS with files at particular offsets > ---------------------------------------------------------------------- > AssertionError: cbfstool produced a different result > > Stdout: > diff -y <(xxd -g1 /tmp/expect) <(xxd -g1 /tmp/actual) | colordiff > > ====================================================================== > FAIL: test_cbfs_raw (binman.cbfs_util_test.TestCbfs) > Test base handling of a Coreboot Filesystem (CBFS) > ---------------------------------------------------------------------- > AssertionError: cbfstool produced a different result > > Stdout: > diff -y <(xxd -g1 /tmp/expect) <(xxd -g1 /tmp/actual) | colordiff > > ====================================================================== > FAIL: test_cbfs_raw_compress (binman.cbfs_util_test.TestCbfs) > Test base handling of compressing raw files > ---------------------------------------------------------------------- > AssertionError: b'' != b'compress xxxxxxxxxxxxxxxxxxxxxx data' > > ====================================================================== > FAIL: test_cbfs_raw_space (binman.cbfs_util_test.TestCbfs) > Test files with unused space in the CBFS > ---------------------------------------------------------------------- > AssertionError: cbfstool produced a different result > > Stdout: > diff -y <(xxd -g1 /tmp/expect) <(xxd -g1 /tmp/actual) | colordiff > > ====================================================================== > FAIL: test_cbfs_stage (binman.cbfs_util_test.TestCbfs) > Tests handling of a Coreboot Filesystem (CBFS) > ---------------------------------------------------------------------- > AssertionError: cbfstool produced a different result > > Stdout: > diff -y <(xxd -g1 /tmp/expect) <(xxd -g1 /tmp/actual) | colordiff > > ====================================================================== > SKIP: testCompUtilCompressions (binman.ftest.TestFunctional) > Test compression algorithms > ---------------------------------------------------------------------- > lzma_alone not available > > ====================================================================== > SKIP: testCompUtilPadding (binman.ftest.TestFunctional) > Test padding of compression algorithms > ---------------------------------------------------------------------- > lzma_alone not available > > ====================================================================== > SKIP: testCompUtilVersions (binman.ftest.TestFunctional) > Test tool version of compression algorithms > ---------------------------------------------------------------------- > lzma_alone not available > > ====================================================================== > SKIP: testExtractCbfsRaw (binman.ftest.TestFunctional) > Test extracting CBFS compressed data without decompressing it > ---------------------------------------------------------------------- > lzma_alone not available These are pretty obvious > > ---------------------------------------------------------------------- > Ran 454 tests in 11.738s > > FAILED (failures=15, errors=4, skipped=4) > > 99% > Name Stmts Miss Cover > --------------------------------------------------------------------------- > tools/binman/__init__.py 0 0 100% > tools/binman/bintool.py 254 0 100% > tools/binman/btool/btool_gzip.py 5 0 100% > tools/binman/btool/bzip2.py 15 0 100% > tools/binman/btool/cbfstool.py 24 0 100% > tools/binman/btool/fiptool.py 22 0 100% > tools/binman/btool/futility.py 24 0 100% > tools/binman/btool/ifwitool.py 22 0 100% > tools/binman/btool/lz4.py 28 0 100% > tools/binman/btool/lzma_alone.py 34 3 91% > tools/binman/btool/lzop.py 5 0 100% > tools/binman/btool/mkimage.py 29 0 100% > tools/binman/btool/xz.py 5 0 100% > tools/binman/btool/zstd.py 5 0 100% > tools/binman/cbfs_util.py 366 0 100% > tools/binman/cmdline.py 73 0 100% > tools/binman/control.py 342 0 100% > tools/binman/elf.py 195 18 91% > tools/binman/entry.py 483 0 100% > tools/binman/etype/atf_bl31.py 5 0 100% > tools/binman/etype/atf_fip.py 67 0 100% > tools/binman/etype/blob.py 39 0 100% > tools/binman/etype/blob_dtb.py 46 0 100% > tools/binman/etype/blob_ext.py 11 0 100% > tools/binman/etype/blob_ext_list.py 32 0 100% > tools/binman/etype/blob_named_by_arg.py 9 0 100% > tools/binman/etype/blob_phase.py 16 0 100% > tools/binman/etype/cbfs.py 101 0 100% > tools/binman/etype/collection.py 30 0 100% > tools/binman/etype/cros_ec_rw.py 5 0 100% > tools/binman/etype/fdtmap.py 62 0 100% > tools/binman/etype/files.py 35 0 100% > tools/binman/etype/fill.py 13 0 100% > tools/binman/etype/fit.py 214 0 100% > tools/binman/etype/fmap.py 34 0 100% > tools/binman/etype/gbb.py 37 0 100% > tools/binman/etype/image_header.py 53 0 100% > tools/binman/etype/intel_cmc.py 4 0 100% > tools/binman/etype/intel_descriptor.py 39 0 100% > tools/binman/etype/intel_fit.py 12 0 100% > tools/binman/etype/intel_fit_ptr.py 17 0 100% > tools/binman/etype/intel_fsp.py 4 0 100% > tools/binman/etype/intel_fsp_m.py 4 0 100% > tools/binman/etype/intel_fsp_s.py 4 0 100% > tools/binman/etype/intel_fsp_t.py 4 0 100% > tools/binman/etype/intel_ifwi.py 67 0 100% > tools/binman/etype/intel_me.py 4 0 100% > tools/binman/etype/intel_mrc.py 6 0 100% > tools/binman/etype/intel_refcode.py 6 0 100% > tools/binman/etype/intel_vbt.py 4 0 100% > tools/binman/etype/intel_vga.py 4 0 100% > tools/binman/etype/mkimage.py 68 0 100% > tools/binman/etype/opensbi.py 5 0 100% > tools/binman/etype/powerpc_mpc85xx_bootpg_resetvec.py 6 0 100% > tools/binman/etype/pre_load.py 77 0 100% > tools/binman/etype/scp.py 5 0 100% > tools/binman/etype/section.py 376 12 97% > tools/binman/etype/tee_os.py 5 0 100% > tools/binman/etype/text.py 21 0 100% > tools/binman/etype/u_boot.py 7 0 100% > tools/binman/etype/u_boot_dtb.py 9 0 100% > tools/binman/etype/u_boot_dtb_with_ucode.py 51 0 100% > tools/binman/etype/u_boot_elf.py 19 0 100% > tools/binman/etype/u_boot_env.py 27 0 100% > tools/binman/etype/u_boot_expanded.py 4 0 100% > tools/binman/etype/u_boot_img.py 7 0 100% > tools/binman/etype/u_boot_nodtb.py 7 0 100% > tools/binman/etype/u_boot_spl.py 11 0 100% > tools/binman/etype/u_boot_spl_bss_pad.py 14 0 100% > tools/binman/etype/u_boot_spl_dtb.py 9 0 100% > tools/binman/etype/u_boot_spl_elf.py 7 0 100% > tools/binman/etype/u_boot_spl_expanded.py 12 0 100% > tools/binman/etype/u_boot_spl_nodtb.py 11 0 100% > tools/binman/etype/u_boot_spl_with_ucode_ptr.py 8 0 100% > tools/binman/etype/u_boot_tpl.py 11 0 100% > tools/binman/etype/u_boot_tpl_bss_pad.py 14 0 100% > tools/binman/etype/u_boot_tpl_dtb.py 9 0 100% > tools/binman/etype/u_boot_tpl_dtb_with_ucode.py 8 0 100% > tools/binman/etype/u_boot_tpl_elf.py 7 0 100% > tools/binman/etype/u_boot_tpl_expanded.py 12 0 100% > tools/binman/etype/u_boot_tpl_nodtb.py 11 0 100% > tools/binman/etype/u_boot_tpl_with_ucode_ptr.py 12 0 100% > tools/binman/etype/u_boot_ucode.py 33 0 100% > tools/binman/etype/u_boot_with_ucode_ptr.py 42 0 100% > tools/binman/etype/vblock.py 38 0 100% > tools/binman/etype/x86_reset16.py 7 0 100% > tools/binman/etype/x86_reset16_spl.py 7 0 100% > tools/binman/etype/x86_reset16_tpl.py 7 0 100% > tools/binman/etype/x86_start16.py 7 0 100% > tools/binman/etype/x86_start16_spl.py 7 0 100% > tools/binman/etype/x86_start16_tpl.py 7 0 100% > tools/binman/fip_util.py 202 0 100% > tools/binman/fmap_util.py 48 0 100% > tools/binman/image.py 164 4 98% > tools/binman/state.py 201 0 100% > --------------------------------------------------------------------------- > TOTAL 4539 37 99% > > To get a report in 'htmlcov/index.html', type: python3 -m coverage html > Coverage error: 99%, but should be 100% > ValueError: Test coverage failure > > I guess I just should send a new mail so the community can have a look > at it? I sent two patches for low hanging fruits but I'm afraid I won't > have time to look at those issues any time soon :/ Yes please resend your series, just with the extra test code I added. > > I'm running Fedora Server 36 x86_64 fully up-to-date if that helps. > > [...] > >> I get 52% coverage only with that exact same branch, something's > >> definitely wrong here in my setup. And I **definitely** do not have > >> tools/binman/etype/mkimage.py listed in there.... Mmmmmm. > > > > You may need to get some bintools with 'binman tool -f missing'. But > > It expects apt package manager, I only have dnf :) Patches welcome! > > Also, tries to download binaries from a Google Drive, no thank you :) I don't like it either. Perhaps we could use a web site? I would prefer to build from source, anyway. > > > in any case, you only need to worry about coverage in mkimage.py which > > is what you changed. > > > > That's fair. Thanks for fixing my patches, lemme know if you want a v6 > instead. Yes please, my things were just an example to help. > > [...] > >> > >> I'll play with binman until I manage to get a coverage percentage equal > >> to yours. > > > > OK, I'd appreciate a docs patch if you can produce one from your > > efforts, or any feedback on how to make this automatic / easy. > > > > I copied the errors I'm having right now a few paragraphs above but the > biggest issue was bzip2 getting stuck and messing up the whole test > suite. With it patched, I was able to see the line that was kept > untested in my patches. Otherwise I believe your advice of running > binman test -T was enough if that had worked, just bad luck :) OK good. Regards, Simon
diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst index b3613d7cbd..18bd328c5c 100644 --- a/tools/binman/entries.rst +++ b/tools/binman/entries.rst @@ -1175,6 +1175,9 @@ Properties / Entry arguments: - args: Arguments to pass - data-to-imagename: Indicates that the -d data should be passed in as the image name also (-n) + - multiple-data-files: boolean to tell binman to pass all files as + datafiles to mkimage instead of creating a temporary file the result + of datafiles concatenation The data passed to mkimage via the -d flag is collected from subnodes of the mkimage node, e.g.:: @@ -1205,6 +1208,25 @@ a section, or just multiple subnodes like this:: }; }; +To pass all datafiles untouched to mkimage:: + + mkimage { + args = "-n rk3399 -T rkspi"; + multiple-data-files; + + u-boot-tpl { + }; + + u-boot-spl { + }; + }; + +This calls mkimage to create a Rockchip RK3399-specific first stage +bootloader, made of TPL+SPL. Since this first stage bootloader requires to +align the TPL and SPL but also some weird hacks that is handled by mkimage +directly, binman is told to not perform the concatenation of datafiles prior +to passing the data to mkimage. + To use CONFIG options in the arguments, use a string list instead, as in this example which also produces four arguments:: diff --git a/tools/binman/etype/mkimage.py b/tools/binman/etype/mkimage.py index ddbd9cec65..5f4bc6fa3c 100644 --- a/tools/binman/etype/mkimage.py +++ b/tools/binman/etype/mkimage.py @@ -18,6 +18,9 @@ class Entry_mkimage(Entry): - args: Arguments to pass - data-to-imagename: Indicates that the -d data should be passed in as the image name also (-n) + - multiple-data-files: boolean to tell binman to pass all files as + datafiles to mkimage instead of creating a temporary file the result + of datafiles concatenation The data passed to mkimage via the -d flag is collected from subnodes of the mkimage node, e.g.:: @@ -51,6 +54,25 @@ class Entry_mkimage(Entry): Note that binman places the contents (here SPL and TPL) into a single file and passes that to mkimage using the -d option. + To pass all datafiles untouched to mkimage:: + + mkimage { + args = "-n rk3399 -T rkspi"; + multiple-data-files; + + u-boot-tpl { + }; + + u-boot-spl { + }; + }; + + This calls mkimage to create a Rockchip RK3399-specific first stage + bootloader, made of TPL+SPL. Since this first stage bootloader requires to + align the TPL and SPL but also some weird hacks that is handled by mkimage + directly, binman is told to not perform the concatenation of datafiles prior + to passing the data to mkimage. + To use CONFIG options in the arguments, use a string list instead, as in this example which also produces four arguments:: @@ -96,6 +118,7 @@ class Entry_mkimage(Entry): """ def __init__(self, section, etype, node): super().__init__(section, etype, node) + self._multiple_data_files = fdt_util.GetBool(self._node, 'multiple-data-files') self._mkimage_entries = OrderedDict() self._imagename = None self.align_default = None @@ -122,10 +145,20 @@ class Entry_mkimage(Entry): def ObtainContents(self): # Use a non-zero size for any fake files to keep mkimage happy # Note that testMkimageImagename() relies on this 'mkimage' parameter - data, input_fname, uniq = self.collect_contents_to_file( - self._mkimage_entries.values(), 'mkimage', 1024) - if data is None: - return False + fake_size = 1024 + if self._multiple_data_files: + fnames = [] + uniq = self.GetUniqueName() + for entry in self._mkimage_entries.values(): + if not entry.ObtainContents(fake_size=fake_size): + return False + fnames.append(tools.get_input_filename(entry.GetDefaultFilename())) + input_fname = ":".join(fnames) + else: + data, input_fname, uniq = self.collect_contents_to_file( + self._mkimage_entries.values(), 'mkimage', fake_size) + if data is None: + return False if self._imagename: image_data, imagename_fname, _ = self.collect_contents_to_file( [self._imagename], 'mkimage-n', 1024) diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 0b1774046f..091692ef93 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -5818,6 +5818,22 @@ fdt fdtmap Extract the devicetree blob from the fdtmap expect = U_BOOT_SPL_DATA + U_BOOT_DATA self.assertEqual(expect, data[:len(expect)]) + def testMkimageMultipleDataFiles(self): + """Test passing multiple files to mkimage in a mkimage entry""" + data = self._DoReadFile('241_mkimage_multiple_data_files.dts') + # Size of files are packed in their 4B big-endian format + expect = struct.pack('>I', len(U_BOOT_TPL_DATA)) + expect += struct.pack('>I', len(U_BOOT_SPL_DATA)) + # Size info is always followed by a 4B zero value. + expect += tools.get_bytes(0, 4) + expect += U_BOOT_TPL_DATA + # All but last files are 4B-aligned + align_pad = len(U_BOOT_TPL_DATA) % 4 + if align_pad: + expect += tools.get_bytes(0, align_pad) + expect += U_BOOT_SPL_DATA + self.assertEqual(expect, data[-len(expect):]) + def testCompressDtbPrependInvalid(self): """Test that invalid header is detected""" with self.assertRaises(ValueError) as e: diff --git a/tools/binman/test/241_mkimage_multiple_data_files.dts b/tools/binman/test/241_mkimage_multiple_data_files.dts new file mode 100644 index 0000000000..a092bc39bf --- /dev/null +++ b/tools/binman/test/241_mkimage_multiple_data_files.dts @@ -0,0 +1,21 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + mkimage { + args = "-T script"; + multiple-data-files; + + u-boot-tpl { + }; + + u-boot-spl { + }; + }; + }; +};