On 06/09/2020 19:39, Simon Glass wrote:
> When an external blob is missing it can be quite confusing for the user.
> Add a way to provide a help message that is shown.
>
> Signed-off-by: Simon Glass <s...@chromium.org>
> ---
>
> (no changes since v3)
>
> Changes in v3:
> - Add a way to show help messages for missing blobs
>
> tools/binman/README | 6 ++
> tools/binman/control.py | 69 +++++++++++++++++++++-
> tools/binman/entry.py | 9 +++
> tools/binman/ftest.py | 18 +++++-
> tools/binman/missing-blob-help | 11 ++++
> tools/binman/test/168_fit_missing_blob.dts | 9 ++-
> 6 files changed, 119 insertions(+), 3 deletions(-)
> create mode 100644 tools/binman/missing-blob-help
>
> diff --git a/tools/binman/README b/tools/binman/README
> index 37ee3fc2d3b..f7bf285a915 100644
> --- a/tools/binman/README
> +++ b/tools/binman/README
> @@ -343,6 +343,12 @@ compress:
> Sets the compression algortihm to use (for blobs only). See the entry
> documentation for details.
>
> +missing-msg:
> + Sets the tag of the message to show if this entry is missing. This is
> + used for external blobs. When they are missing it is helpful to show
> + information about what needs to be fixed. See missing-blob-help for the
> + message for each tag.
> +
> The attributes supported for images and sections are described below. Several
> are similar to those for entries.
>
> diff --git a/tools/binman/control.py b/tools/binman/control.py
> index 15bfac582a7..ee5771e7292 100644
> --- a/tools/binman/control.py
> +++ b/tools/binman/control.py
> @@ -9,6 +9,7 @@ from collections import OrderedDict
> import glob
> import os
> import pkg_resources
> +import re
>
> import sys
> from patman import tools
> @@ -22,6 +23,11 @@ from patman import tout
> # Make this global so that it can be referenced from tests
> images = OrderedDict()
>
> +# Help text for each type of missing blob, dict:
> +# key: Value of the entry's 'missing-msg' or entry name
> +# value: Text for the help
> +missing_blob_help = {}
> +
> def _ReadImageDesc(binman_node):
> """Read the image descriptions from the /binman node
>
> @@ -54,6 +60,66 @@ def _FindBinmanNode(dtb):
> return node
> return None
>
> +def _ReadMissingBlobHelp():
> + """Read the missing-blob-help file
> +
> + This file containins help messages explaining what to do when external
> blobs
> + are missing.
> +
> + Returns:
> + Dict:
> + key: Message tag (str)
> + value: Message text (str)
> + """
> +
> + def _FinishTag(tag, msg, result):
> + if tag:
> + result[tag] = msg.rstrip()
> + tag = None
> + msg = ''
> + return tag, msg
> +
> + my_data = pkg_resources.resource_string(__name__, 'missing-blob-help')
> + re_tag = re.compile('^([-a-z0-9]+):$')
> + result = {}
> + tag = None
> + msg = ''
> + for line in my_data.decode('utf-8').splitlines():
> + if not line.startswith('#'):
> + m_tag = re_tag.match(line)
> + if m_tag:
> + _, msg = _FinishTag(tag, msg, result)
> + tag = m_tag.group(1)
> + elif tag:
> + msg += line + '\n'
> + _FinishTag(tag, msg, result)
> + return result
This looks a bit complex, I think something like this would be clearer:
result = {}
tag = None
for line in my_data.decode('utf-8').splitlines():
m_tag = re_tag.match(line)
if line.startswith('#'):
continue
elif m_tag:
tag = m_tag.group(1)
result[tag] = []
elif tag:
result[tag].append(line)
for tag, lines in result.items():
result[tag] = "".join(lines).rstrip()
return result
> +
> +def _ShowBlobHelp(path, text):
> + tout.Warning('\n%s:' % path)
> + for line in text.splitlines():
> + tout.Warning(' %s' % line)
> +
> +def _ShowHelpForMissingBlobs(missing_list):
> + """Show help for each missing blob to help the user take action
> +
> + Args:
> + missing_list: List of Entry objects to show help for
> + """
> + global missing_blob_help
> +
> + if not missing_blob_help:
> + missing_blob_help = _ReadMissingBlobHelp()
> +
> + for entry in missing_list:
> + tags = entry.GetHelpTags()
> +
> + # Show the first match help message
> + for tag in tags:
> + if tag in missing_blob_help:
> + _ShowBlobHelp(entry._node.path, missing_blob_help[tag])
> + break
> +
> def GetEntryModules(include_testing=True):
> """Get a set of entry class implementations
>
> @@ -478,6 +544,7 @@ def ProcessImage(image, update_fdt, write_map,
> get_contents=True,
> if missing_list:
> tout.Warning("Image '%s' is missing external blobs and is
> non-functional: %s" %
> (image.name, ' '.join([e.name for e in missing_list])))
> + _ShowHelpForMissingBlobs(missing_list)
> return bool(missing_list)
>
>
> @@ -563,7 +630,7 @@ def Binman(args):
> tools.WriteFile(dtb_item._fname, dtb_item.GetContents())
>
> if missing:
> - tout.Warning("Some images are invalid")
> + tout.Warning("\nSome images are invalid")
> finally:
> tools.FinaliseOutputDir()
> finally:
> diff --git a/tools/binman/entry.py b/tools/binman/entry.py
> index 0f128c4cf5e..f7adc3b1abb 100644
> --- a/tools/binman/entry.py
> +++ b/tools/binman/entry.py
> @@ -178,6 +178,7 @@ class Entry(object):
> self.align_end = fdt_util.GetInt(self._node, 'align-end')
> self.offset_unset = fdt_util.GetBool(self._node, 'offset-unset')
> self.expand_size = fdt_util.GetBool(self._node, 'expand-size')
> + self.missing_msg = fdt_util.GetString(self._node, 'missing-msg')
>
> def GetDefaultFilename(self):
> return None
> @@ -827,3 +828,11 @@ features to produce new behaviours.
> True if allowed, False if not allowed
> """
> return self.allow_missing
> +
> + def GetHelpTags(self):
> + """Get the tags use for missing-blob help
> +
> + Returns:
> + list of possible tags, most desirable first
> + """
> + return list(filter(None, [self.missing_msg, self.name, self.etype]))
> diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
> index a269a7497cb..95b17d0b749 100644
> --- a/tools/binman/ftest.py
> +++ b/tools/binman/ftest.py
> @@ -3561,7 +3561,7 @@ class TestFunctional(unittest.TestCase):
> self._DoTestFile('168_fit_missing_blob.dts',
> allow_missing=True)
> err = stderr.getvalue()
> - self.assertRegex(err, "Image 'main-section'.*missing.*: blob-ext")
> + self.assertRegex(err, "Image 'main-section'.*missing.*: atf-bl31")
>
> def testBlobNamedByArgMissing(self):
> """Test handling of a missing entry arg"""
> @@ -3692,5 +3692,21 @@ class TestFunctional(unittest.TestCase):
> self.assertIn("default-dt entry argument 'test-fdt3' not found in
> fdt list: test-fdt1, test-fdt2",
> str(e.exception))
>
> + def testFitExtblobMissingHelp(self):
> + """Test display of help messages when an external blob is missing"""
> + control.missing_blob_help = control._ReadMissingBlobHelp()
> + control.missing_blob_help['wibble'] = 'Wibble test'
> + control.missing_blob_help['another'] = 'Another test'
> + with test_util.capture_sys_output() as (stdout, stderr):
> + self._DoTestFile('168_fit_missing_blob.dts',
> + allow_missing=True)
> + err = stderr.getvalue()
> +
> + # We can get the tag from the name, the type or the missing-msg
> + # property. Check all three.
> + self.assertIn('You may need to build ARM Trusted', err)
> + self.assertIn('Wibble test', err)
> + self.assertIn('Another test', err)
> +
> if __name__ == "__main__":
> unittest.main()
> diff --git a/tools/binman/missing-blob-help b/tools/binman/missing-blob-help
> new file mode 100644
> index 00000000000..3de195c23c8
> --- /dev/null
> +++ b/tools/binman/missing-blob-help
> @@ -0,0 +1,11 @@
> +# This file contains help messages for missing external blobs. Each message
> has
> +# a tag (MUST be just lower-case text, digits and hyphens) starting in
> column 1,
> +# followed by a colon (:) to indicate its start. The message can include any
> +# number of lines, including blank lines.
> +#
> +# When looking for a tag, Binman uses the value of 'missing-msg' for the
> entry,
> +# the entry name or the entry type, in that order
> +
> +atf-bl31:
> +See the documentation for your board. You may need to build ARM Trusted
> +Firmware and build with BL31=/path/to/bl31.bin
> diff --git a/tools/binman/test/168_fit_missing_blob.dts
> b/tools/binman/test/168_fit_missing_blob.dts
> index e007aa41d8a..15f6cc07e5d 100644
> --- a/tools/binman/test/168_fit_missing_blob.dts
> +++ b/tools/binman/test/168_fit_missing_blob.dts
> @@ -29,9 +29,16 @@
> hash-2 {
> algo = "sha1";
> };
> - blob-ext {
> + atf-bl31 {
> filename = "missing";
> };
> + cros-ec-rw {
> + type = "atf-bl31";
> + missing-msg = "wibble";
> + };
> + another {
> + type = "atf-bl31";
> + };
> };
> };
> };
>
