[PATCH 0/9] binman: Show missing blob message when building U-Boot

[PATCH 0/9] binman: Show missing blob message when building U-Boot

[Jonas Karlman]

binman currently support showing a helpful missing blob message, but
only when the --allow-missing flag is used.

This changes so that binman is invoked with the --allow-missing flag
and the helpful message can be shown by default when building U-Boot.

Using the following:

  make rockpro64-rk3399_defconfig
  make CROSS_COMPILE="aarch64-linux-gnu-"

Before this series a build fails with:

  binman: Filename 'atf-bl31' not found in input path (...)

After this series a build fails with:

  Image 'simple-bin' is missing external blobs and is non-functional: atf-bl31

  /binman/simple-bin/fit/images/@atf-SEQ/atf-bl31 (atf-bl31):
     See the documentation for your board. You may need to build ARM Trusted
     Firmware and build with BL31=/path/to/bl31.bin

  Image 'simple-bin' is missing external blobs but is still functional: tee-os

  /binman/simple-bin/fit/images/@tee-SEQ/tee-os (tee-os):
     See the documentation for your board. You may need to build Open Portable
     Trusted Execution Environment (OP-TEE) with TEE=/path/to/tee.bin

  Some images are invalid

Builds will continue to fail when there is missing blobs, and the use of
BINMAN_ALLOW_MISSING=1 now only enables the --ignore-missing flag.

This series also fixes a few minor issues that prevented some missing
and optional blobs to be detected for fit and mkimage entries.

Patch 1-3 contains spelling fixes and code cleanup for related parts.
Patch 4-6 improve missing/optional detection for fit and mkimage entries.
Patch 7-8 improve the missing blob warning message output.
Patch 9 finally update Makefile to always pass the --allow-missing flag.

The series is based on top of [1], and is the follow-up series meant to
address the issue with missing blob message for mkimage entries.

[1] https://patchwork.ozlabs.org/project/uboot/cover/20230219150629.4012377-1-jonas@kwiboo.se/

Jonas Karlman (9):
  binman: Remove redundant SetAllowFakeBlob from blob-ext entry
  binman: Fix spelling of nodes in code comments
  binman: Use correct argument name in docstrings
  binman: Override CheckOptional in fit entry
  binman: Implement missing check functions in mkimage entry
  binman: Mark mkimage entry missing when its subnodes is missing
  binman: Fix blank line usage for invalid images warning text
  binman: Show filename in missing blob help message
  Makefile: Show binman missing blob message

 Makefile                                      |  2 +-
 tools/binman/control.py                       | 24 ++++++---
 tools/binman/entry.py                         |  2 +-
 tools/binman/etype/blob.py                    |  2 +-
 tools/binman/etype/blob_ext.py                |  8 ---
 tools/binman/etype/fit.py                     |  9 +++-
 tools/binman/etype/mkimage.py                 | 54 ++++++++++++++++++-
 tools/binman/etype/section.py                 |  6 +--
 tools/binman/ftest.py                         |  9 ++++
 tools/binman/state.py                         |  2 +-
 .../test/278_mkimage_missing_multiple.dts     | 19 +++++++
 11 files changed, 111 insertions(+), 26 deletions(-)
 create mode 100644 tools/binman/test/278_mkimage_missing_multiple.dts

-- 
2.39.2

[PATCH 1/9] binman: Remove redundant SetAllowFakeBlob from blob-ext entry

[Jonas Karlman]

Entry_blob_ext contains an implementation of SetAllowFakeBlob that is
identical to the one in the base Entry class, remove it.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
 tools/binman/etype/blob_ext.py | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/tools/binman/etype/blob_ext.py b/tools/binman/etype/blob_ext.py
index fba6271de2bb..d6b0ca17c3f3 100644
--- a/tools/binman/etype/blob_ext.py
+++ b/tools/binman/etype/blob_ext.py
@@ -26,11 +26,3 @@ class Entry_blob_ext(Entry_blob):
     def __init__(self, section, etype, node):
         Entry_blob.__init__(self, section, etype, node)
         self.external = True
-
-    def SetAllowFakeBlob(self, allow_fake):
-        """Set whether the entry allows to create a fake blob
-
-        Args:
-            allow_fake_blob: True if allowed, False if not allowed
-        """
-        self.allow_fake = allow_fake
-- 
2.39.2

Re: [PATCH 1/9] binman: Remove redundant SetAllowFakeBlob from blob-ext entry

[Simon Glass]

On Sun, 19 Feb 2023 at 15:02, Jonas Karlman <jonas@kwiboo.se> wrote:
>
> Entry_blob_ext contains an implementation of SetAllowFakeBlob that is
> identical to the one in the base Entry class, remove it.
>
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> ---
>  tools/binman/etype/blob_ext.py | 8 --------
>  1 file changed, 8 deletions(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH 1/9] binman: Remove redundant SetAllowFakeBlob from blob-ext entry

[Simon Glass]

On Sun, 19 Feb 2023 at 15:02, Jonas Karlman <jonas@kwiboo.se> wrote:
>
> Entry_blob_ext contains an implementation of SetAllowFakeBlob that is
> identical to the one in the base Entry class, remove it.
>
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> ---
>  tools/binman/etype/blob_ext.py | 8 --------
>  1 file changed, 8 deletions(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot-dm/next, thanks!

[PATCH 2/9] binman: Fix spelling of nodes in code comments

[Jonas Karlman]

Replace notes with nodes in code comments and docstrings.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
 tools/binman/etype/fit.py     | 2 +-
 tools/binman/etype/section.py | 2 +-
 tools/binman/state.py         | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
index cd2943533ce2..822de7982768 100644
--- a/tools/binman/etype/fit.py
+++ b/tools/binman/etype/fit.py
@@ -823,7 +823,7 @@ class Entry_fit(Entry_section):
         self.mkimage = self.AddBintool(btools, 'mkimage')
 
     def CheckMissing(self, missing_list):
-        # We must use our private entry list for this since generator notes
+        # We must use our private entry list for this since generator nodes
         # which are removed from self._entries will otherwise not show up as
         # missing
         for entry in self._priv_entries.values():
diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py
index 57b91ff726c0..8bf5aa437d1a 100644
--- a/tools/binman/etype/section.py
+++ b/tools/binman/etype/section.py
@@ -172,7 +172,7 @@ class Entry_section(Entry):
     def IsSpecialSubnode(self, node):
         """Check if a node is a special one used by the section itself
 
-        Some notes are used for hashing / signatures and do not add entries to
+        Some nodes are used for hashing / signatures and do not add entries to
         the actual section.
 
         Returns:
diff --git a/tools/binman/state.py b/tools/binman/state.py
index 56e5bf8bc10f..33563199840e 100644
--- a/tools/binman/state.py
+++ b/tools/binman/state.py
@@ -306,7 +306,7 @@ def GetUpdateNodes(node, for_repack=False):
     """Yield all the nodes that need to be updated in all device trees
 
     The property referenced by this node is added to any device trees which
-    have the given node. Due to removable of unwanted notes, SPL and TPL may
+    have the given node. Due to removable of unwanted nodes, SPL and TPL may
     not have this node.
 
     Args:
-- 
2.39.2

Re: [PATCH 2/9] binman: Fix spelling of nodes in code comments

[Simon Glass]

On Sun, 19 Feb 2023 at 15:02, Jonas Karlman <jonas@kwiboo.se> wrote:
>
> Replace notes with nodes in code comments and docstrings.
>
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> ---
>  tools/binman/etype/fit.py     | 2 +-
>  tools/binman/etype/section.py | 2 +-
>  tools/binman/state.py         | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH 2/9] binman: Fix spelling of nodes in code comments

[Simon Glass]

On Sun, 19 Feb 2023 at 15:02, Jonas Karlman <jonas@kwiboo.se> wrote:
>
> Replace notes with nodes in code comments and docstrings.
>
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> ---
>  tools/binman/etype/fit.py     | 2 +-
>  tools/binman/etype/section.py | 2 +-
>  tools/binman/state.py         | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot-dm/next, thanks!

[PATCH 3/9] binman: Use correct argument name in docstrings

[Jonas Karlman]

Use correct argument name in docstrings.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
 tools/binman/entry.py         | 2 +-
 tools/binman/etype/blob.py    | 2 +-
 tools/binman/etype/section.py | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/binman/entry.py b/tools/binman/entry.py
index 5eacc5fa6c42..52b5d335f8ba 100644
--- a/tools/binman/entry.py
+++ b/tools/binman/entry.py
@@ -1104,7 +1104,7 @@ features to produce new behaviours.
         If there are faked blobs, the entries are added to the list
 
         Args:
-            fake_blobs_list: List of Entry objects to be added to
+            faked_blobs_list: List of Entry objects to be added to
         """
         # This is meaningless for anything other than blobs
         pass
diff --git a/tools/binman/etype/blob.py b/tools/binman/etype/blob.py
index c7ddcedffb82..a80741e36338 100644
--- a/tools/binman/etype/blob.py
+++ b/tools/binman/etype/blob.py
@@ -102,7 +102,7 @@ class Entry_blob(Entry):
         If there are faked blobs, the entries are added to the list
 
         Args:
-            fake_blobs_list: List of Entry objects to be added to
+            faked_blobs_list: List of Entry objects to be added to
         """
         if self.faked:
             faked_blobs_list.append(self)
diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py
index 8bf5aa437d1a..d3926f791c72 100644
--- a/tools/binman/etype/section.py
+++ b/tools/binman/etype/section.py
@@ -885,7 +885,7 @@ class Entry_section(Entry):
         """Set whether a section allows to create a fake blob
 
         Args:
-            allow_fake_blob: True if allowed, False if not allowed
+            allow_fake: True if allowed, False if not allowed
         """
         super().SetAllowFakeBlob(allow_fake)
         for entry in self._entries.values():
@@ -909,7 +909,7 @@ class Entry_section(Entry):
         If there are faked blobs, the entries are added to the list
 
         Args:
-            fake_blobs_list: List of Entry objects to be added to
+            faked_blobs_list: List of Entry objects to be added to
         """
         for entry in self._entries.values():
             entry.CheckFakedBlobs(faked_blobs_list)
-- 
2.39.2

Re: [PATCH 3/9] binman: Use correct argument name in docstrings

[Simon Glass]

On Sun, 19 Feb 2023 at 15:02, Jonas Karlman <jonas@kwiboo.se> wrote:
>
> Use correct argument name in docstrings.
>
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> ---
>  tools/binman/entry.py         | 2 +-
>  tools/binman/etype/blob.py    | 2 +-
>  tools/binman/etype/section.py | 4 ++--
>  3 files changed, 4 insertions(+), 4 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH 3/9] binman: Use correct argument name in docstrings

[Simon Glass]

On Sun, 19 Feb 2023 at 15:02, Jonas Karlman <jonas@kwiboo.se> wrote:
>
> Use correct argument name in docstrings.
>
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> ---
>  tools/binman/entry.py         | 2 +-
>  tools/binman/etype/blob.py    | 2 +-
>  tools/binman/etype/section.py | 4 ++--
>  3 files changed, 4 insertions(+), 4 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot-dm/next, thanks!

[PATCH 4/9] binman: Override CheckOptional in fit entry

[Jonas Karlman]

Missing optional blobs was not reported for generated entries, e.g.
tee-os on rockchip targets. Implement a CheckOptional to fix this.

After this the following can be shown:

  Image 'simple-bin' is missing external blobs but is still functional: tee-os

  /binman/simple-bin/fit/images/@tee-SEQ/tee-os (tee-os):
     See the documentation for your board. You may need to build Open Portable
     Trusted Execution Environment (OP-TEE) with TEE=/path/to/tee.bin

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
 tools/binman/etype/fit.py | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
index 822de7982768..314e03b4a964 100644
--- a/tools/binman/etype/fit.py
+++ b/tools/binman/etype/fit.py
@@ -828,3 +828,10 @@ class Entry_fit(Entry_section):
         # missing
         for entry in self._priv_entries.values():
             entry.CheckMissing(missing_list)
+
+    def CheckOptional(self, optional_list):
+        # We must use our private entry list for this since generator nodes
+        # which are removed from self._entries will otherwise not show up as
+        # optional
+        for entry in self._priv_entries.values():
+            entry.CheckOptional(optional_list)
-- 
2.39.2

Re: [PATCH 4/9] binman: Override CheckOptional in fit entry

[Simon Glass]

On Sun, 19 Feb 2023 at 15:02, Jonas Karlman <jonas@kwiboo.se> wrote:
>
> Missing optional blobs was not reported for generated entries, e.g.
> tee-os on rockchip targets. Implement a CheckOptional to fix this.
>
> After this the following can be shown:
>
>   Image 'simple-bin' is missing external blobs but is still functional: tee-os
>
>   /binman/simple-bin/fit/images/@tee-SEQ/tee-os (tee-os):
>      See the documentation for your board. You may need to build Open Portable
>      Trusted Execution Environment (OP-TEE) with TEE=/path/to/tee.bin
>
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> ---
>  tools/binman/etype/fit.py | 7 +++++++
>  1 file changed, 7 insertions(+)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

Do we need a test for this?

[PATCH 5/9] binman: Implement missing check functions in mkimage entry

[Jonas Karlman]

The mkimage entry is working like a section entry but inherits from
Entry not Entry_section. Copy implementations of missing Check-functions
from Entry_section and adopt to Entry_mkimage.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
 tools/binman/etype/mkimage.py | 44 ++++++++++++++++++++++++++++++++++-
 1 file changed, 43 insertions(+), 1 deletion(-)

diff --git a/tools/binman/etype/mkimage.py b/tools/binman/etype/mkimage.py
index cb264c3cad0b..49d3462a154c 100644
--- a/tools/binman/etype/mkimage.py
+++ b/tools/binman/etype/mkimage.py
@@ -206,16 +206,31 @@ class Entry_mkimage(Entry):
             self._imagename.SetAllowMissing(allow_missing)
 
     def SetAllowFakeBlob(self, allow_fake):
-        """Set whether the sub nodes allows to create a fake blob
+        """Set whether a section allows to create a fake blob
 
         Args:
             allow_fake: True if allowed, False if not allowed
         """
+        super().SetAllowFakeBlob(allow_fake)
         for entry in self._mkimage_entries.values():
             entry.SetAllowFakeBlob(allow_fake)
         if self._imagename:
             self._imagename.SetAllowFakeBlob(allow_fake)
 
+    def CheckMissing(self, missing_list):
+        """Check if any entries in this section have missing external blobs
+
+        If there are missing (non-optional) blobs, the entries are added to the
+        list
+
+        Args:
+            missing_list: List of Entry objects to be added to
+        """
+        for entry in self._mkimage_entries.values():
+            entry.CheckMissing(missing_list)
+        if self._imagename:
+            self._imagename.CheckMissing(missing_list)
+
     def CheckFakedBlobs(self, faked_blobs_list):
         """Check if any entries in this section have faked external blobs
 
@@ -229,6 +244,33 @@ class Entry_mkimage(Entry):
         if self._imagename:
             self._imagename.CheckFakedBlobs(faked_blobs_list)
 
+    def CheckOptional(self, optional_list):
+        """Check the section for missing but optional external blobs
+
+        If there are missing (optional) blobs, the entries are added to the list
+
+        Args:
+            optional_list (list): List of Entry objects to be added to
+        """
+        for entry in self._mkimage_entries.values():
+            entry.CheckOptional(optional_list)
+        if self._imagename:
+            self._imagename.CheckOptional(optional_list)
+
+    def check_missing_bintools(self, missing_list):
+        """Check if any entries in this section have missing bintools
+
+        If there are missing bintools, these are added to the list
+
+        Args:
+            missing_list: List of Bintool objects to be added to
+        """
+        super().check_missing_bintools(missing_list)
+        for entry in self._mkimage_entries.values():
+            entry.check_missing_bintools(missing_list)
+        if self._imagename:
+            self._imagename.check_missing_bintools(missing_list)
+
     def AddBintools(self, btools):
         super().AddBintools(btools)
         self.mkimage = self.AddBintool(btools, 'mkimage')
-- 
2.39.2

Re: [PATCH 5/9] binman: Implement missing check functions in mkimage entry

[Simon Glass]

On Sun, 19 Feb 2023 at 15:02, Jonas Karlman <jonas@kwiboo.se> wrote:
>
> The mkimage entry is working like a section entry but inherits from
> Entry not Entry_section. Copy implementations of missing Check-functions
> from Entry_section and adopt to Entry_mkimage.
>
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> ---
>  tools/binman/etype/mkimage.py | 44 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 43 insertions(+), 1 deletion(-)

Again I suspect this needs a test to make sure it works?

[PATCH 6/9] binman: Mark mkimage entry missing when its subnodes is missing

[Jonas Karlman]

Using the mkimage entry with the multiple-data-files prop and having a
missing external blob result in an unexpected ValueError exception using
the --allow-missing flag.

  ValueError: Filename 'missing.bin' not found in input path (...)

Fix this by using _pathname that is resolved by ObtainContents for blob
entries, ObtainContents also handles allow missing for external blobs.

Mark mkimage entry as missing and return without running mkimage when
missing entries is reported by CheckMissing.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
 tools/binman/etype/mkimage.py                 | 10 +++++++++-
 tools/binman/ftest.py                         |  9 +++++++++
 .../test/278_mkimage_missing_multiple.dts     | 19 +++++++++++++++++++
 3 files changed, 37 insertions(+), 1 deletion(-)
 create mode 100644 tools/binman/test/278_mkimage_missing_multiple.dts

diff --git a/tools/binman/etype/mkimage.py b/tools/binman/etype/mkimage.py
index 49d3462a154c..2673a8607f37 100644
--- a/tools/binman/etype/mkimage.py
+++ b/tools/binman/etype/mkimage.py
@@ -156,7 +156,8 @@ class Entry_mkimage(Entry):
             for entry in self._mkimage_entries.values():
                 if not entry.ObtainContents(fake_size=fake_size):
                     return False
-                fnames.append(tools.get_input_filename(entry.GetDefaultFilename()))
+                if entry._pathname:
+                    fnames.append(entry._pathname)
             input_fname = ":".join(fnames)
         else:
             data, input_fname, uniq = self.collect_contents_to_file(
@@ -171,6 +172,13 @@ class Entry_mkimage(Entry):
         outfile = self._filename if self._filename else 'mkimage-out.%s' % uniq
         output_fname = tools.get_output_filename(outfile)
 
+        missing_list = []
+        self.CheckMissing(missing_list)
+        self.missing = bool(missing_list)
+        if self.missing:
+            self.SetContents(b'')
+            return self.allow_missing
+
         args = ['-d', input_fname]
         if self._data_to_imagename:
             args += ['-n', input_fname]
diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index 48ac1540bfd8..803b8c5accf4 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -6393,6 +6393,15 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
         data = self._DoReadFile('277_rockchip_tpl.dts')
         self.assertEqual(ROCKCHIP_TPL_DATA, data[:len(ROCKCHIP_TPL_DATA)])
 
+    def testMkimageMissingBlobMultiple(self):
+        """Test using mkimage to build an image"""
+        with test_util.capture_sys_output() as (stdout, stderr):
+            self._DoTestFile('278_mkimage_missing_multiple.dts', allow_missing=True)
+        err = stderr.getvalue()
+        self.assertRegex(
+            err,
+            "Image '.*' is missing external blobs and is non-functional: .*")
+
 
 if __name__ == "__main__":
     unittest.main()
diff --git a/tools/binman/test/278_mkimage_missing_multiple.dts b/tools/binman/test/278_mkimage_missing_multiple.dts
new file mode 100644
index 000000000000..f84aea49ead9
--- /dev/null
+++ b/tools/binman/test/278_mkimage_missing_multiple.dts
@@ -0,0 +1,19 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+/dts-v1/;
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	binman {
+		mkimage {
+			args = "-n test -T script";
+			multiple-data-files;
+
+			blob-ext {
+				filename = "missing.bin";
+			};
+		};
+	};
+};
-- 
2.39.2

Re: [PATCH 6/9] binman: Mark mkimage entry missing when its subnodes is missing

[Simon Glass]

On Sun, 19 Feb 2023 at 15:02, Jonas Karlman <jonas@kwiboo.se> wrote:
>
> Using the mkimage entry with the multiple-data-files prop and having a
> missing external blob result in an unexpected ValueError exception using
> the --allow-missing flag.
>
>   ValueError: Filename 'missing.bin' not found in input path (...)
>
> Fix this by using _pathname that is resolved by ObtainContents for blob
> entries, ObtainContents also handles allow missing for external blobs.
>
> Mark mkimage entry as missing and return without running mkimage when
> missing entries is reported by CheckMissing.
>
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> ---
>  tools/binman/etype/mkimage.py                 | 10 +++++++++-
>  tools/binman/ftest.py                         |  9 +++++++++
>  .../test/278_mkimage_missing_multiple.dts     | 19 +++++++++++++++++++
>  3 files changed, 37 insertions(+), 1 deletion(-)
>  create mode 100644 tools/binman/test/278_mkimage_missing_multiple.dts
>

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH 8/9] binman: Show filename in missing blob help message

[Jonas Karlman]

Show the filename next to the node path in missing blob help messages,
also show a generic missing blob message when there was no help message
for the help tag.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
 tools/binman/control.py | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/tools/binman/control.py b/tools/binman/control.py
index b5ede60c1cde..a5ff1cb56245 100644
--- a/tools/binman/control.py
+++ b/tools/binman/control.py
@@ -105,8 +105,8 @@ def _ReadMissingBlobHelp():
     _FinishTag(tag, msg, result)
     return result
 
-def _ShowBlobHelp(path, text):
-    tout.warning('%s:' % path)
+def _ShowBlobHelp(path, text, fname):
+    tout.warning('%s (%s):' % (path, fname))
     for line in text.splitlines():
         tout.warning('   %s' % line)
     tout.warning('')
@@ -126,10 +126,17 @@ def _ShowHelpForMissingBlobs(missing_list):
         tags = entry.GetHelpTags()
 
         # Show the first match help message
+        shown_help = False
         for tag in tags:
             if tag in missing_blob_help:
-                _ShowBlobHelp(entry._node.path, missing_blob_help[tag])
+                _ShowBlobHelp(entry._node.path, missing_blob_help[tag],
+                              entry.GetDefaultFilename())
+                shown_help = True
                 break
+        # Or a generic help message
+        if not shown_help:
+            _ShowBlobHelp(entry._node.path, "Missing blob",
+                          entry.GetDefaultFilename())
 
 def GetEntryModules(include_testing=True):
     """Get a set of entry class implementations
-- 
2.39.2

Re: [PATCH 8/9] binman: Show filename in missing blob help message

[Simon Glass]

On Sun, 19 Feb 2023 at 15:02, Jonas Karlman <jonas@kwiboo.se> wrote:
>
> Show the filename next to the node path in missing blob help messages,
> also show a generic missing blob message when there was no help message
> for the help tag.
>
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> ---
>  tools/binman/control.py | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

Test?

[PATCH 7/9] binman: Fix blank line usage for invalid images warning text

[Jonas Karlman]

There is no blank line between last missing blob help message and the
header line for optional blob help messages.

  Image 'simple-bin' is missing external blobs and is non-functional: atf-bl31

  /binman/simple-bin/fit/images/@atf-SEQ/atf-bl31:
     See the documentation for your board. You may need to build ARM Trusted
     Firmware and build with BL31=/path/to/bl31.bin
  Image 'simple-bin' is missing external blobs but is still functional: tee-os

  /binman/simple-bin/fit/images/@tee-SEQ/tee-os:
     See the documentation for your board. You may need to build Open Portable
     Trusted Execution Environment (OP-TEE) with TEE=/path/to/tee.bin

  Some images are invalid

With this a blank line is inserted to make the text more readable.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
 tools/binman/control.py | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/tools/binman/control.py b/tools/binman/control.py
index e64740094f60..b5ede60c1cde 100644
--- a/tools/binman/control.py
+++ b/tools/binman/control.py
@@ -106,9 +106,10 @@ def _ReadMissingBlobHelp():
     return result
 
 def _ShowBlobHelp(path, text):
-    tout.warning('\n%s:' % path)
+    tout.warning('%s:' % path)
     for line in text.splitlines():
         tout.warning('   %s' % line)
+    tout.warning('')
 
 def _ShowHelpForMissingBlobs(missing_list):
     """Show help for each missing blob to help the user take action
@@ -598,7 +599,7 @@ def ProcessImage(image, update_fdt, write_map, get_contents=True,
     missing_list = []
     image.CheckMissing(missing_list)
     if missing_list:
-        tout.warning("Image '%s' is missing external blobs and is non-functional: %s" %
+        tout.warning("Image '%s' is missing external blobs and is non-functional: %s\n" %
                      (image.name, ' '.join([e.name for e in missing_list])))
         _ShowHelpForMissingBlobs(missing_list)
 
@@ -606,7 +607,7 @@ def ProcessImage(image, update_fdt, write_map, get_contents=True,
     image.CheckFakedBlobs(faked_list)
     if faked_list:
         tout.warning(
-            "Image '%s' has faked external blobs and is non-functional: %s" %
+            "Image '%s' has faked external blobs and is non-functional: %s\n" %
             (image.name, ' '.join([os.path.basename(e.GetDefaultFilename())
                                    for e in faked_list])))
 
@@ -614,7 +615,7 @@ def ProcessImage(image, update_fdt, write_map, get_contents=True,
     image.CheckOptional(optional_list)
     if optional_list:
         tout.warning(
-            "Image '%s' is missing external blobs but is still functional: %s" %
+            "Image '%s' is missing external blobs but is still functional: %s\n" %
             (image.name, ' '.join([e.name for e in optional_list])))
         _ShowHelpForMissingBlobs(optional_list)
 
@@ -622,7 +623,7 @@ def ProcessImage(image, update_fdt, write_map, get_contents=True,
     image.check_missing_bintools(missing_bintool_list)
     if missing_bintool_list:
         tout.warning(
-            "Image '%s' has missing bintools and is non-functional: %s" %
+            "Image '%s' has missing bintools and is non-functional: %s\n" %
             (image.name, ' '.join([os.path.basename(bintool.name)
                                    for bintool in missing_bintool_list])))
     return any([missing_list, faked_list, missing_bintool_list])
@@ -756,7 +757,7 @@ def Binman(args):
             # This can only be True if -M is provided, since otherwise binman
             # would have raised an error already
             if invalid:
-                msg = '\nSome images are invalid'
+                msg = 'Some images are invalid'
                 if args.ignore_missing:
                     tout.warning(msg)
                 else:
-- 
2.39.2

Re: [PATCH 7/9] binman: Fix blank line usage for invalid images warning text

[Simon Glass]

On Sun, 19 Feb 2023 at 15:02, Jonas Karlman <jonas@kwiboo.se> wrote:
>
> There is no blank line between last missing blob help message and the
> header line for optional blob help messages.
>
>   Image 'simple-bin' is missing external blobs and is non-functional: atf-bl31
>
>   /binman/simple-bin/fit/images/@atf-SEQ/atf-bl31:
>      See the documentation for your board. You may need to build ARM Trusted
>      Firmware and build with BL31=/path/to/bl31.bin
>   Image 'simple-bin' is missing external blobs but is still functional: tee-os
>
>   /binman/simple-bin/fit/images/@tee-SEQ/tee-os:
>      See the documentation for your board. You may need to build Open Portable
>      Trusted Execution Environment (OP-TEE) with TEE=/path/to/tee.bin
>
>   Some images are invalid
>
> With this a blank line is inserted to make the text more readable.
>
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> ---
>  tools/binman/control.py | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH 9/9] Makefile: Show binman missing blob message

[Jonas Karlman]

When binman is invoked during a build of U-Boot and an external blob is
missing, the user is usually presented with a generic file not found in
input path message.

Invoke binman with --allow-missing so that binman can show relevant
missing blob help messages. Build continue to fail with missing blobs
unless BINMAN_ALLOW_MISSING=1 is used.

This changes the following error message:

  binman: Filename 'atf-bl31' not found in input path (...)

to the following:

  Image 'itb' is missing external blobs and is non-functional: atf-blob

  /binman/itb/fit/images/atf/atf-blob (bl31.bin):
     See the documentation for your board. You may need to build ARM Trusted
     Firmware and build with BL31=/path/to/bl31.bin

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 58f8c7a35335..c2860824f6f2 100644
--- a/Makefile
+++ b/Makefile
@@ -1326,7 +1326,7 @@ cmd_binman = $(srctree)/tools/binman/binman $(if $(BINMAN_DEBUG),-D) \
                 --toolpath $(objtree)/tools \
 		$(if $(BINMAN_VERBOSE),-v$(BINMAN_VERBOSE)) \
 		build -u -d u-boot.dtb -O . -m \
-		$(if $(BINMAN_ALLOW_MISSING),--allow-missing --ignore-missing) \
+		--allow-missing $(if $(BINMAN_ALLOW_MISSING),--ignore-missing) \
 		-I . -I $(srctree) -I $(srctree)/board/$(BOARDDIR) \
 		-I arch/$(ARCH)/dts -a of-list=$(CONFIG_OF_LIST) \
 		$(foreach f,$(BINMAN_INDIRS),-I $(f)) \
-- 
2.39.2

Re: [PATCH 9/9] Makefile: Show binman missing blob message

[Simon Glass]

Hi Jonas

+Tom Rini

On Sun, 19 Feb 2023 at 15:02, Jonas Karlman <jonas@kwiboo.se> wrote:
>
> When binman is invoked during a build of U-Boot and an external blob is
> missing, the user is usually presented with a generic file not found in
> input path message.
>
> Invoke binman with --allow-missing so that binman can show relevant
> missing blob help messages. Build continue to fail with missing blobs
> unless BINMAN_ALLOW_MISSING=1 is used.
>
> This changes the following error message:
>
>   binman: Filename 'atf-bl31' not found in input path (...)
>
> to the following:
>
>   Image 'itb' is missing external blobs and is non-functional: atf-blob
>
>   /binman/itb/fit/images/atf/atf-blob (bl31.bin):
>      See the documentation for your board. You may need to build ARM Trusted
>      Firmware and build with BL31=/path/to/bl31.bin
>
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> ---
>  Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Makefile b/Makefile
> index 58f8c7a35335..c2860824f6f2 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1326,7 +1326,7 @@ cmd_binman = $(srctree)/tools/binman/binman $(if $(BINMAN_DEBUG),-D) \
>                  --toolpath $(objtree)/tools \
>                 $(if $(BINMAN_VERBOSE),-v$(BINMAN_VERBOSE)) \
>                 build -u -d u-boot.dtb -O . -m \
> -               $(if $(BINMAN_ALLOW_MISSING),--allow-missing --ignore-missing) \
> +               --allow-missing $(if $(BINMAN_ALLOW_MISSING),--ignore-missing) \
>                 -I . -I $(srctree) -I $(srctree)/board/$(BOARDDIR) \
>                 -I arch/$(ARCH)/dts -a of-list=$(CONFIG_OF_LIST) \
>                 $(foreach f,$(BINMAN_INDIRS),-I $(f)) \
> --
> 2.39.2
>

I agree this is better, but we should see what Tom thinks.

Reviewed-by: Simon Glass <sjg@chromium.org>

Regards,
Simon

Re: [PATCH 9/9] Makefile: Show binman missing blob message

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2169 bytes --]

On Tue, Feb 21, 2023 at 12:41:52PM -0700, Simon Glass wrote:
> Hi Jonas
> 
> +Tom Rini
> 
> On Sun, 19 Feb 2023 at 15:02, Jonas Karlman <jonas@kwiboo.se> wrote:
> >
> > When binman is invoked during a build of U-Boot and an external blob is
> > missing, the user is usually presented with a generic file not found in
> > input path message.
> >
> > Invoke binman with --allow-missing so that binman can show relevant
> > missing blob help messages. Build continue to fail with missing blobs
> > unless BINMAN_ALLOW_MISSING=1 is used.
> >
> > This changes the following error message:
> >
> >   binman: Filename 'atf-bl31' not found in input path (...)
> >
> > to the following:
> >
> >   Image 'itb' is missing external blobs and is non-functional: atf-blob
> >
> >   /binman/itb/fit/images/atf/atf-blob (bl31.bin):
> >      See the documentation for your board. You may need to build ARM Trusted
> >      Firmware and build with BL31=/path/to/bl31.bin
> >
> > Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> > ---
> >  Makefile | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/Makefile b/Makefile
> > index 58f8c7a35335..c2860824f6f2 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -1326,7 +1326,7 @@ cmd_binman = $(srctree)/tools/binman/binman $(if $(BINMAN_DEBUG),-D) \
> >                  --toolpath $(objtree)/tools \
> >                 $(if $(BINMAN_VERBOSE),-v$(BINMAN_VERBOSE)) \
> >                 build -u -d u-boot.dtb -O . -m \
> > -               $(if $(BINMAN_ALLOW_MISSING),--allow-missing --ignore-missing) \
> > +               --allow-missing $(if $(BINMAN_ALLOW_MISSING),--ignore-missing) \
> >                 -I . -I $(srctree) -I $(srctree)/board/$(BOARDDIR) \
> >                 -I arch/$(ARCH)/dts -a of-list=$(CONFIG_OF_LIST) \
> >                 $(foreach f,$(BINMAN_INDIRS),-I $(f)) \
> > --
> > 2.39.2
> >
> 
> I agree this is better, but we should see what Tom thinks.
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>

This sounds like a binman bug. We shouldn't need to say --allow-missing
to then make use of the missing-msg node.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 9/9] Makefile: Show binman missing blob message

[Simon Glass]

Hi Tom,

On Tue, 21 Feb 2023 at 16:09, Tom Rini <trini@konsulko.com> wrote:
>
> On Tue, Feb 21, 2023 at 12:41:52PM -0700, Simon Glass wrote:
> > Hi Jonas
> >
> > +Tom Rini
> >
> > On Sun, 19 Feb 2023 at 15:02, Jonas Karlman <jonas@kwiboo.se> wrote:
> > >
> > > When binman is invoked during a build of U-Boot and an external blob is
> > > missing, the user is usually presented with a generic file not found in
> > > input path message.
> > >
> > > Invoke binman with --allow-missing so that binman can show relevant
> > > missing blob help messages. Build continue to fail with missing blobs
> > > unless BINMAN_ALLOW_MISSING=1 is used.
> > >
> > > This changes the following error message:
> > >
> > >   binman: Filename 'atf-bl31' not found in input path (...)
> > >
> > > to the following:
> > >
> > >   Image 'itb' is missing external blobs and is non-functional: atf-blob
> > >
> > >   /binman/itb/fit/images/atf/atf-blob (bl31.bin):
> > >      See the documentation for your board. You may need to build ARM Trusted
> > >      Firmware and build with BL31=/path/to/bl31.bin
> > >
> > > Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> > > ---
> > >  Makefile | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/Makefile b/Makefile
> > > index 58f8c7a35335..c2860824f6f2 100644
> > > --- a/Makefile
> > > +++ b/Makefile
> > > @@ -1326,7 +1326,7 @@ cmd_binman = $(srctree)/tools/binman/binman $(if $(BINMAN_DEBUG),-D) \
> > >                  --toolpath $(objtree)/tools \
> > >                 $(if $(BINMAN_VERBOSE),-v$(BINMAN_VERBOSE)) \
> > >                 build -u -d u-boot.dtb -O . -m \
> > > -               $(if $(BINMAN_ALLOW_MISSING),--allow-missing --ignore-missing) \
> > > +               --allow-missing $(if $(BINMAN_ALLOW_MISSING),--ignore-missing) \
> > >                 -I . -I $(srctree) -I $(srctree)/board/$(BOARDDIR) \
> > >                 -I arch/$(ARCH)/dts -a of-list=$(CONFIG_OF_LIST) \
> > >                 $(foreach f,$(BINMAN_INDIRS),-I $(f)) \
> > > --
> > > 2.39.2
> > >
> >
> > I agree this is better, but we should see what Tom thinks.
> >
> > Reviewed-by: Simon Glass <sjg@chromium.org>
>
> This sounds like a binman bug. We shouldn't need to say --allow-missing
> to then make use of the missing-msg node.

Without --allow-missing binman just dies when it cannot find
something. This is useful because it allows debugging of problems.

I'm not sure how easy it would be to change this. The error is
reported by the tools module which has no knowledge of binman. We
could catch the exception, but again that makes things non-debuggable.

At the moment we have two cases:
- normal: missing files cause an immediate abort
- --allow-missing: missing files are collected with a report at the end

Worse, we can have missing tools, not just missing blobs. The
complexity of this gets completely out of hand if we try to meld the
normal and --allow-missing cases together.

I actually don't see much of a downside to passing allow-missing
always. But perhaps the BINMAN_ALLOW_MISSING should be renamed to
BINMAN_IGNORE_MISSING (with a similar change in buildman). We could
actually keep BINMAN_ALLOW_MISSING to mean to pass --allow-missing,
pointing out in the docs that this is a bad idea as you won't see any
blob help?

Regards,
Simon

Re: [PATCH 9/9] Makefile: Show binman missing blob message

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 4068 bytes --]

On Wed, Feb 22, 2023 at 02:20:08PM -0700, Simon Glass wrote:
> Hi Tom,
> 
> On Tue, 21 Feb 2023 at 16:09, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Tue, Feb 21, 2023 at 12:41:52PM -0700, Simon Glass wrote:
> > > Hi Jonas
> > >
> > > +Tom Rini
> > >
> > > On Sun, 19 Feb 2023 at 15:02, Jonas Karlman <jonas@kwiboo.se> wrote:
> > > >
> > > > When binman is invoked during a build of U-Boot and an external blob is
> > > > missing, the user is usually presented with a generic file not found in
> > > > input path message.
> > > >
> > > > Invoke binman with --allow-missing so that binman can show relevant
> > > > missing blob help messages. Build continue to fail with missing blobs
> > > > unless BINMAN_ALLOW_MISSING=1 is used.
> > > >
> > > > This changes the following error message:
> > > >
> > > >   binman: Filename 'atf-bl31' not found in input path (...)
> > > >
> > > > to the following:
> > > >
> > > >   Image 'itb' is missing external blobs and is non-functional: atf-blob
> > > >
> > > >   /binman/itb/fit/images/atf/atf-blob (bl31.bin):
> > > >      See the documentation for your board. You may need to build ARM Trusted
> > > >      Firmware and build with BL31=/path/to/bl31.bin
> > > >
> > > > Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> > > > ---
> > > >  Makefile | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/Makefile b/Makefile
> > > > index 58f8c7a35335..c2860824f6f2 100644
> > > > --- a/Makefile
> > > > +++ b/Makefile
> > > > @@ -1326,7 +1326,7 @@ cmd_binman = $(srctree)/tools/binman/binman $(if $(BINMAN_DEBUG),-D) \
> > > >                  --toolpath $(objtree)/tools \
> > > >                 $(if $(BINMAN_VERBOSE),-v$(BINMAN_VERBOSE)) \
> > > >                 build -u -d u-boot.dtb -O . -m \
> > > > -               $(if $(BINMAN_ALLOW_MISSING),--allow-missing --ignore-missing) \
> > > > +               --allow-missing $(if $(BINMAN_ALLOW_MISSING),--ignore-missing) \
> > > >                 -I . -I $(srctree) -I $(srctree)/board/$(BOARDDIR) \
> > > >                 -I arch/$(ARCH)/dts -a of-list=$(CONFIG_OF_LIST) \
> > > >                 $(foreach f,$(BINMAN_INDIRS),-I $(f)) \
> > > > --
> > > > 2.39.2
> > > >
> > >
> > > I agree this is better, but we should see what Tom thinks.
> > >
> > > Reviewed-by: Simon Glass <sjg@chromium.org>
> >
> > This sounds like a binman bug. We shouldn't need to say --allow-missing
> > to then make use of the missing-msg node.
> 
> Without --allow-missing binman just dies when it cannot find
> something. This is useful because it allows debugging of problems.
> 
> I'm not sure how easy it would be to change this. The error is
> reported by the tools module which has no knowledge of binman. We
> could catch the exception, but again that makes things non-debuggable.

Why wouldn't it be debuggable? Wouldn't it be more debuggable now that
we're saying "You're missing foo.blob, this is typically provided by the
foo project", which we can just say because the message has been filled
out already in the etype.

> At the moment we have two cases:
> - normal: missing files cause an immediate abort
> - --allow-missing: missing files are collected with a report at the end
> 
> Worse, we can have missing tools, not just missing blobs. The
> complexity of this gets completely out of hand if we try to meld the
> normal and --allow-missing cases together.
> 
> I actually don't see much of a downside to passing allow-missing
> always. But perhaps the BINMAN_ALLOW_MISSING should be renamed to
> BINMAN_IGNORE_MISSING (with a similar change in buildman). We could
> actually keep BINMAN_ALLOW_MISSING to mean to pass --allow-missing,
> pointing out in the docs that this is a bad idea as you won't see any
> blob help?

Maybe the problem is the flag is badly named? I still think this speaks
to bugs in the tooling if we can't make use of the helpful error message
that we have available without some special flags to be set.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 9/9] Makefile: Show binman missing blob message

[Simon Glass]

Hi Tom,

On Wed, 22 Feb 2023 at 14:45, Tom Rini <trini@konsulko.com> wrote:
>
> On Wed, Feb 22, 2023 at 02:20:08PM -0700, Simon Glass wrote:
> > Hi Tom,
> >
> > On Tue, 21 Feb 2023 at 16:09, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Tue, Feb 21, 2023 at 12:41:52PM -0700, Simon Glass wrote:
> > > > Hi Jonas
> > > >
> > > > +Tom Rini
> > > >
> > > > On Sun, 19 Feb 2023 at 15:02, Jonas Karlman <jonas@kwiboo.se> wrote:
> > > > >
> > > > > When binman is invoked during a build of U-Boot and an external blob is
> > > > > missing, the user is usually presented with a generic file not found in
> > > > > input path message.
> > > > >
> > > > > Invoke binman with --allow-missing so that binman can show relevant
> > > > > missing blob help messages. Build continue to fail with missing blobs
> > > > > unless BINMAN_ALLOW_MISSING=1 is used.
> > > > >
> > > > > This changes the following error message:
> > > > >
> > > > >   binman: Filename 'atf-bl31' not found in input path (...)
> > > > >
> > > > > to the following:
> > > > >
> > > > >   Image 'itb' is missing external blobs and is non-functional: atf-blob
> > > > >
> > > > >   /binman/itb/fit/images/atf/atf-blob (bl31.bin):
> > > > >      See the documentation for your board. You may need to build ARM Trusted
> > > > >      Firmware and build with BL31=/path/to/bl31.bin
> > > > >
> > > > > Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> > > > > ---
> > > > >  Makefile | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/Makefile b/Makefile
> > > > > index 58f8c7a35335..c2860824f6f2 100644
> > > > > --- a/Makefile
> > > > > +++ b/Makefile
> > > > > @@ -1326,7 +1326,7 @@ cmd_binman = $(srctree)/tools/binman/binman $(if $(BINMAN_DEBUG),-D) \
> > > > >                  --toolpath $(objtree)/tools \
> > > > >                 $(if $(BINMAN_VERBOSE),-v$(BINMAN_VERBOSE)) \
> > > > >                 build -u -d u-boot.dtb -O . -m \
> > > > > -               $(if $(BINMAN_ALLOW_MISSING),--allow-missing --ignore-missing) \
> > > > > +               --allow-missing $(if $(BINMAN_ALLOW_MISSING),--ignore-missing) \
> > > > >                 -I . -I $(srctree) -I $(srctree)/board/$(BOARDDIR) \
> > > > >                 -I arch/$(ARCH)/dts -a of-list=$(CONFIG_OF_LIST) \
> > > > >                 $(foreach f,$(BINMAN_INDIRS),-I $(f)) \
> > > > > --
> > > > > 2.39.2
> > > > >
> > > >
> > > > I agree this is better, but we should see what Tom thinks.
> > > >
> > > > Reviewed-by: Simon Glass <sjg@chromium.org>
> > >
> > > This sounds like a binman bug. We shouldn't need to say --allow-missing
> > > to then make use of the missing-msg node.
> >
> > Without --allow-missing binman just dies when it cannot find
> > something. This is useful because it allows debugging of problems.
> >
> > I'm not sure how easy it would be to change this. The error is
> > reported by the tools module which has no knowledge of binman. We
> > could catch the exception, but again that makes things non-debuggable.
>
> Why wouldn't it be debuggable? Wouldn't it be more debuggable now that
> we're saying "You're missing foo.blob, this is typically provided by the
> foo project", which we can just say because the message has been filled
> out already in the etype.

Perhaps just take my word for it? The feature you are asking for here
is exactly what the -M flag does, but you are trying to make it so
that a 'file not found' error is not reported immediately, but appears
later. That makes it hard to debug, since the 'exception' is supressed
and only later do you see there is a problem...so hard to debug.

>
> > At the moment we have two cases:
> > - normal: missing files cause an immediate abort
> > - --allow-missing: missing files are collected with a report at the end
> >
> > Worse, we can have missing tools, not just missing blobs. The
> > complexity of this gets completely out of hand if we try to meld the
> > normal and --allow-missing cases together.
> >
> > I actually don't see much of a downside to passing allow-missing
> > always. But perhaps the BINMAN_ALLOW_MISSING should be renamed to
> > BINMAN_IGNORE_MISSING (with a similar change in buildman). We could
> > actually keep BINMAN_ALLOW_MISSING to mean to pass --allow-missing,
> > pointing out in the docs that this is a bad idea as you won't see any
> > blob help?
>
> Maybe the problem is the flag is badly named? I still think this speaks
> to bugs in the tooling if we can't make use of the helpful error message
> that we have available without some special flags to be set.

Without -M binman is just opening the files you tell it, failing if it
cannot, exactly as things used to be before binman.

Yes --allow-missing might not be the best name, since we don't really
allow it, just collect the information and report it. Binman still
fails the build in that case. Perhaps we need:

(nothing) - die as soon as any missing tool or file is
--collect-missing   - collect all the missing blobs and tools, rather
than dying right away, returning 103 if there are any problems
--allow-missing   - report success (with warnings output) even if
there are missing blobs / tools

Regards,
Simon

Re: [PATCH 0/9] binman: Show missing blob message when building U-Boot

[Simon Glass]

Hi Jonas,

On Sun, 19 Feb 2023 at 14:02, Jonas Karlman <jonas@kwiboo.se> wrote:
>
> binman currently support showing a helpful missing blob message, but
> only when the --allow-missing flag is used.
>
> This changes so that binman is invoked with the --allow-missing flag
> and the helpful message can be shown by default when building U-Boot.
>
> Using the following:
>
>   make rockpro64-rk3399_defconfig
>   make CROSS_COMPILE="aarch64-linux-gnu-"
>
> Before this series a build fails with:
>
>   binman: Filename 'atf-bl31' not found in input path (...)
>
> After this series a build fails with:
>
>   Image 'simple-bin' is missing external blobs and is non-functional: atf-bl31
>
>   /binman/simple-bin/fit/images/@atf-SEQ/atf-bl31 (atf-bl31):
>      See the documentation for your board. You may need to build ARM Trusted
>      Firmware and build with BL31=/path/to/bl31.bin
>
>   Image 'simple-bin' is missing external blobs but is still functional: tee-os
>
>   /binman/simple-bin/fit/images/@tee-SEQ/tee-os (tee-os):
>      See the documentation for your board. You may need to build Open Portable
>      Trusted Execution Environment (OP-TEE) with TEE=/path/to/tee.bin
>
>   Some images are invalid
>
> Builds will continue to fail when there is missing blobs, and the use of
> BINMAN_ALLOW_MISSING=1 now only enables the --ignore-missing flag.
>
> This series also fixes a few minor issues that prevented some missing
> and optional blobs to be detected for fit and mkimage entries.
>
> Patch 1-3 contains spelling fixes and code cleanup for related parts.
> Patch 4-6 improve missing/optional detection for fit and mkimage entries.
> Patch 7-8 improve the missing blob warning message output.
> Patch 9 finally update Makefile to always pass the --allow-missing flag.
>
> The series is based on top of [1], and is the follow-up series meant to
> address the issue with missing blob message for mkimage entries.
>
> [1] https://patchwork.ozlabs.org/project/uboot/cover/20230219150629.4012377-1-jonas@kwiboo.se/
>
> Jonas Karlman (9):
>   binman: Remove redundant SetAllowFakeBlob from blob-ext entry
>   binman: Fix spelling of nodes in code comments
>   binman: Use correct argument name in docstrings
>   binman: Override CheckOptional in fit entry
>   binman: Implement missing check functions in mkimage entry
>   binman: Mark mkimage entry missing when its subnodes is missing
>   binman: Fix blank line usage for invalid images warning text
>   binman: Show filename in missing blob help message
>   Makefile: Show binman missing blob message
>
>  Makefile                                      |  2 +-
>  tools/binman/control.py                       | 24 ++++++---
>  tools/binman/entry.py                         |  2 +-
>  tools/binman/etype/blob.py                    |  2 +-
>  tools/binman/etype/blob_ext.py                |  8 ---
>  tools/binman/etype/fit.py                     |  9 +++-
>  tools/binman/etype/mkimage.py                 | 54 ++++++++++++++++++-
>  tools/binman/etype/section.py                 |  6 +--
>  tools/binman/ftest.py                         |  9 ++++
>  tools/binman/state.py                         |  2 +-
>  .../test/278_mkimage_missing_multiple.dts     | 19 +++++++
>  11 files changed, 111 insertions(+), 26 deletions(-)
>  create mode 100644 tools/binman/test/278_mkimage_missing_multiple.dts
>
> --
> 2.39.2
>

I applied what I could of this to -next

Could you please take another look and see if we can get the rest in?

Regards,
Simon

Re: [PATCH 0/9] binman: Show missing blob message when building U-Boot

[Jonas Karlman]

Hi Simon,
On 2023-03-10 21:49, Simon Glass wrote:
> Hi Jonas,
> 
> On Sun, 19 Feb 2023 at 14:02, Jonas Karlman <jonas@kwiboo.se> wrote:
>>
>> binman currently support showing a helpful missing blob message, but
>> only when the --allow-missing flag is used.
>>
>> This changes so that binman is invoked with the --allow-missing flag
>> and the helpful message can be shown by default when building U-Boot.
>>
>> Using the following:
>>
>>   make rockpro64-rk3399_defconfig
>>   make CROSS_COMPILE="aarch64-linux-gnu-"
>>
>> Before this series a build fails with:
>>
>>   binman: Filename 'atf-bl31' not found in input path (...)
>>
>> After this series a build fails with:
>>
>>   Image 'simple-bin' is missing external blobs and is non-functional: atf-bl31
>>
>>   /binman/simple-bin/fit/images/@atf-SEQ/atf-bl31 (atf-bl31):
>>      See the documentation for your board. You may need to build ARM Trusted
>>      Firmware and build with BL31=/path/to/bl31.bin
>>
>>   Image 'simple-bin' is missing external blobs but is still functional: tee-os
>>
>>   /binman/simple-bin/fit/images/@tee-SEQ/tee-os (tee-os):
>>      See the documentation for your board. You may need to build Open Portable
>>      Trusted Execution Environment (OP-TEE) with TEE=/path/to/tee.bin
>>
>>   Some images are invalid
>>
>> Builds will continue to fail when there is missing blobs, and the use of
>> BINMAN_ALLOW_MISSING=1 now only enables the --ignore-missing flag.
>>
>> This series also fixes a few minor issues that prevented some missing
>> and optional blobs to be detected for fit and mkimage entries.
>>
>> Patch 1-3 contains spelling fixes and code cleanup for related parts.
>> Patch 4-6 improve missing/optional detection for fit and mkimage entries.
>> Patch 7-8 improve the missing blob warning message output.
>> Patch 9 finally update Makefile to always pass the --allow-missing flag.
>>
>> The series is based on top of [1], and is the follow-up series meant to
>> address the issue with missing blob message for mkimage entries.
>>
>> [1] https://patchwork.ozlabs.org/project/uboot/cover/20230219150629.4012377-1-jonas@kwiboo.se/
>>
>> Jonas Karlman (9):
>>   binman: Remove redundant SetAllowFakeBlob from blob-ext entry
>>   binman: Fix spelling of nodes in code comments
>>   binman: Use correct argument name in docstrings
>>   binman: Override CheckOptional in fit entry
>>   binman: Implement missing check functions in mkimage entry
>>   binman: Mark mkimage entry missing when its subnodes is missing
>>   binman: Fix blank line usage for invalid images warning text
>>   binman: Show filename in missing blob help message
>>   Makefile: Show binman missing blob message
>>
>>  Makefile                                      |  2 +-
>>  tools/binman/control.py                       | 24 ++++++---
>>  tools/binman/entry.py                         |  2 +-
>>  tools/binman/etype/blob.py                    |  2 +-
>>  tools/binman/etype/blob_ext.py                |  8 ---
>>  tools/binman/etype/fit.py                     |  9 +++-
>>  tools/binman/etype/mkimage.py                 | 54 ++++++++++++++++++-
>>  tools/binman/etype/section.py                 |  6 +--
>>  tools/binman/ftest.py                         |  9 ++++
>>  tools/binman/state.py                         |  2 +-
>>  .../test/278_mkimage_missing_multiple.dts     | 19 +++++++
>>  11 files changed, 111 insertions(+), 26 deletions(-)
>>  create mode 100644 tools/binman/test/278_mkimage_missing_multiple.dts
>>
>> --
>> 2.39.2
>>
> 
> I applied what I could of this to -next
> 
> Could you please take another look and see if we can get the rest in?

Sure, I will take a look at a v2 (with more tests) in next few days.

Still unclear what to do about "Makefile: Show binman missing blob
message" at [2]. Should I re-send it as-is, change it or just drop it?

[2] https://patchwork.ozlabs.org/project/uboot/patch/20230219220158.4160763-10-jonas@kwiboo.se/#3063614

Regards,
Jonas

> 
> Regards,
> Simon

Re: [PATCH 0/9] binman: Show missing blob message when building U-Boot

[Simon Glass]

Hi Jonas,

On Thu, 16 Mar 2023 at 01:45, Jonas Karlman <jonas@kwiboo.se> wrote:
>
> Hi Simon,
> On 2023-03-10 21:49, Simon Glass wrote:
> > Hi Jonas,
> >
> > On Sun, 19 Feb 2023 at 14:02, Jonas Karlman <jonas@kwiboo.se> wrote:
> >>
> >> binman currently support showing a helpful missing blob message, but
> >> only when the --allow-missing flag is used.
> >>
> >> This changes so that binman is invoked with the --allow-missing flag
> >> and the helpful message can be shown by default when building U-Boot.
> >>
> >> Using the following:
> >>
> >>   make rockpro64-rk3399_defconfig
> >>   make CROSS_COMPILE="aarch64-linux-gnu-"
> >>
> >> Before this series a build fails with:
> >>
> >>   binman: Filename 'atf-bl31' not found in input path (...)
> >>
> >> After this series a build fails with:
> >>
> >>   Image 'simple-bin' is missing external blobs and is non-functional: atf-bl31
> >>
> >>   /binman/simple-bin/fit/images/@atf-SEQ/atf-bl31 (atf-bl31):
> >>      See the documentation for your board. You may need to build ARM Trusted
> >>      Firmware and build with BL31=/path/to/bl31.bin
> >>
> >>   Image 'simple-bin' is missing external blobs but is still functional: tee-os
> >>
> >>   /binman/simple-bin/fit/images/@tee-SEQ/tee-os (tee-os):
> >>      See the documentation for your board. You may need to build Open Portable
> >>      Trusted Execution Environment (OP-TEE) with TEE=/path/to/tee.bin
> >>
> >>   Some images are invalid
> >>
> >> Builds will continue to fail when there is missing blobs, and the use of
> >> BINMAN_ALLOW_MISSING=1 now only enables the --ignore-missing flag.
> >>
> >> This series also fixes a few minor issues that prevented some missing
> >> and optional blobs to be detected for fit and mkimage entries.
> >>
> >> Patch 1-3 contains spelling fixes and code cleanup for related parts.
> >> Patch 4-6 improve missing/optional detection for fit and mkimage entries.
> >> Patch 7-8 improve the missing blob warning message output.
> >> Patch 9 finally update Makefile to always pass the --allow-missing flag.
> >>
> >> The series is based on top of [1], and is the follow-up series meant to
> >> address the issue with missing blob message for mkimage entries.
> >>
> >> [1] https://patchwork.ozlabs.org/project/uboot/cover/20230219150629.4012377-1-jonas@kwiboo.se/
> >>
> >> Jonas Karlman (9):
> >>   binman: Remove redundant SetAllowFakeBlob from blob-ext entry
> >>   binman: Fix spelling of nodes in code comments
> >>   binman: Use correct argument name in docstrings
> >>   binman: Override CheckOptional in fit entry
> >>   binman: Implement missing check functions in mkimage entry
> >>   binman: Mark mkimage entry missing when its subnodes is missing
> >>   binman: Fix blank line usage for invalid images warning text
> >>   binman: Show filename in missing blob help message
> >>   Makefile: Show binman missing blob message
> >>
> >>  Makefile                                      |  2 +-
> >>  tools/binman/control.py                       | 24 ++++++---
> >>  tools/binman/entry.py                         |  2 +-
> >>  tools/binman/etype/blob.py                    |  2 +-
> >>  tools/binman/etype/blob_ext.py                |  8 ---
> >>  tools/binman/etype/fit.py                     |  9 +++-
> >>  tools/binman/etype/mkimage.py                 | 54 ++++++++++++++++++-
> >>  tools/binman/etype/section.py                 |  6 +--
> >>  tools/binman/ftest.py                         |  9 ++++
> >>  tools/binman/state.py                         |  2 +-
> >>  .../test/278_mkimage_missing_multiple.dts     | 19 +++++++
> >>  11 files changed, 111 insertions(+), 26 deletions(-)
> >>  create mode 100644 tools/binman/test/278_mkimage_missing_multiple.dts
> >>
> >> --
> >> 2.39.2
> >>
> >
> > I applied what I could of this to -next
> >
> > Could you please take another look and see if we can get the rest in?
>
> Sure, I will take a look at a v2 (with more tests) in next few days.
>
> Still unclear what to do about "Makefile: Show binman missing blob
> message" at [2]. Should I re-send it as-is, change it or just drop it?
>
> [2] https://patchwork.ozlabs.org/project/uboot/patch/20230219220158.4160763-10-jonas@kwiboo.se/#3063614

I'd say resend since it seems right to me. I'll probably need to talk
to Tom about it.

Regards,
SImon