[PATCH v5 1/8] binman: add support for skipping file concatenation for mkimage
Simon Glass
sjg at chromium.org
Wed Aug 31 15:46:55 CEST 2022
Hi Quentin,
On Wed, 31 Aug 2022 at 03:25, Quentin Schulz
<quentin.schulz at 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 at 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 at 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 at 0leil.net> wrote:
> >>>>>>
> >>>>>> From: Quentin Schulz <quentin.schulz at 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 at 0leil.net>
> >>>>>> Reviewed-by: Simon Glass <sjg at chromium.org>
> >>>>>> Signed-off-by: Quentin Schulz <quentin.schulz at 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
More information about the U-Boot
mailing list