Hi Quentin,
On Tue, 30 Aug 2022 at 11:54, Quentin Schulz
<quentin.sch...@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.sch...@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+ub...@0leil.net> wrote:
> >>>>
> >>>> From: Quentin Schulz <quentin.sch...@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+ub...@0leil.net>
> >>>> Reviewed-by: Simon Glass <s...@chromium.org>
> >>>> Signed-off-by: Quentin Schulz <quentin.sch...@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.gm28...@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
