Hi Quentin, On Wed, 31 Aug 2022 at 03:25, Quentin Schulz <quentin.sch...@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.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://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
