[PATCH v5 1/8] binman: add support for skipping file concatenation for mkimage

[Quentin Schulz]

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://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