[PATCH v5 1/8] binman: add support for skipping file concatenation for mkimage
Quentin Schulz
quentin.schulz at theobroma-systems.com
Wed Aug 31 11:25:19 CEST 2022
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.
>
> 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
More information about the U-Boot
mailing list