Re: [PATCH 2/5] binman: Show an error when a file is missing

2020-09-07 Thread Simon Glass 
Hi Andy,

On Mon, 7 Sep 2020 at 03:08, Andy Shevchenko
 wrote:
>
> On Sun, Sep 06, 2020 at 07:43:39PM -0600, Simon Glass wrote:
> > On Fri, 4 Sep 2020 at 03:33, Andy Shevchenko
> >  wrote:
> > > On Thu, Sep 03, 2020 at 07:28:53PM -0600, Simon Glass wrote:
>
> > > > -self._pathname = tools.GetInputFilename(self._filename,
> > > > -
> > > > self.section.GetAllowMissing())
> > > > +self._pathname = tools.GetInputFilename(
> > > > +self._filename, self.external and 
> > > > self.section.GetAllowMissing())
> > >
> > > I hope you know that 'and' has a bit different semantics in Python than 
> > > in C for example.
> >
> > I think I understand it, in the sense that 'x and y' returns y if x is
> > true. Is that what you mean?
>
> There is no boolean object involved!
>
> "Note that neither and nor or restrict the value and type they return to False
> and True, but rather return the last evaluated argument."
>
> If x is last evaluated argument (false), it will be returned. Means that there
> are possibilities to get None, False, empty container, etc as returned value 
> of
> 'and'.

Yes I think that matches with my understanding. It's more like
'if...else' in this case. Anyway, both x and y are booleans in the
code above.

Regards,
Simon

Re: [PATCH 2/5] binman: Show an error when a file is missing

2020-09-07 Thread Andy Shevchenko 
On Sun, Sep 06, 2020 at 07:43:39PM -0600, Simon Glass wrote:
> On Fri, 4 Sep 2020 at 03:33, Andy Shevchenko
>  wrote:
> > On Thu, Sep 03, 2020 at 07:28:53PM -0600, Simon Glass wrote:

> > > -self._pathname = tools.GetInputFilename(self._filename,
> > > -
> > > self.section.GetAllowMissing())
> > > +self._pathname = tools.GetInputFilename(
> > > +self._filename, self.external and 
> > > self.section.GetAllowMissing())
> >
> > I hope you know that 'and' has a bit different semantics in Python than in 
> > C for example.
> 
> I think I understand it, in the sense that 'x and y' returns y if x is
> true. Is that what you mean?

There is no boolean object involved!

"Note that neither and nor or restrict the value and type they return to False
and True, but rather return the last evaluated argument."

If x is last evaluated argument (false), it will be returned. Means that there
are possibilities to get None, False, empty container, etc as returned value of
'and'.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/5] binman: Show an error when a file is missing

2020-09-06 Thread Simon Glass 
Hi Andy,

On Fri, 4 Sep 2020 at 03:33, Andy Shevchenko
 wrote:
>
> On Thu, Sep 03, 2020 at 07:28:53PM -0600, Simon Glass wrote:
> > The recent support for missing external binaries does not show an error
> > message when a file is genuinely missing (i.e. it is missing but not
> > marked as 'external'). This means that when -m is passed to binman, it
> > will never report a missing file.
> >
> > Fix this and add a test.
>
> Acked-by: Andy Shevchenko 
>
> One note below.
>
> >
> > Signed-off-by: Simon Glass 
> > ---
> >
> >  tools/binman/etype/blob.py |  7 ---
> >  tools/binman/ftest.py  |  7 +++
> >  tools/binman/test/173_missing_blob.dts | 14 ++
> >  3 files changed, 25 insertions(+), 3 deletions(-)
> >  create mode 100644 tools/binman/test/173_missing_blob.dts
> >
> > diff --git a/tools/binman/etype/blob.py b/tools/binman/etype/blob.py
> > index c5f97c85a38..66cf243976e 100644
> > --- a/tools/binman/etype/blob.py
> > +++ b/tools/binman/etype/blob.py
> > @@ -37,13 +37,14 @@ class Entry_blob(Entry):
> >
> >  def ObtainContents(self):
> >  self._filename = self.GetDefaultFilename()
> > -self._pathname = tools.GetInputFilename(self._filename,
> > -
> > self.section.GetAllowMissing())
> > +self._pathname = tools.GetInputFilename(
> > +self._filename, self.external and 
> > self.section.GetAllowMissing())
>
> I hope you know that 'and' has a bit different semantics in Python than in C 
> for example.

I think I understand it, in the sense that 'x and y' returns y if x is
true. Is that what you mean?

> And why to move first parameter to new line?

I think the style is to either line up the params agained the '(' or
indented 4 from the start of the line. I'll flip it back as pylint3
doesn't seem to mind either way.

Regards,
Simon

Re: [PATCH 2/5] binman: Show an error when a file is missing

2020-09-04 Thread Andy Shevchenko 
On Thu, Sep 03, 2020 at 07:28:53PM -0600, Simon Glass wrote:
> The recent support for missing external binaries does not show an error
> message when a file is genuinely missing (i.e. it is missing but not
> marked as 'external'). This means that when -m is passed to binman, it
> will never report a missing file.
> 
> Fix this and add a test.

Acked-by: Andy Shevchenko 

One note below.

> 
> Signed-off-by: Simon Glass 
> ---
> 
>  tools/binman/etype/blob.py |  7 ---
>  tools/binman/ftest.py  |  7 +++
>  tools/binman/test/173_missing_blob.dts | 14 ++
>  3 files changed, 25 insertions(+), 3 deletions(-)
>  create mode 100644 tools/binman/test/173_missing_blob.dts
> 
> diff --git a/tools/binman/etype/blob.py b/tools/binman/etype/blob.py
> index c5f97c85a38..66cf243976e 100644
> --- a/tools/binman/etype/blob.py
> +++ b/tools/binman/etype/blob.py
> @@ -37,13 +37,14 @@ class Entry_blob(Entry):
>  
>  def ObtainContents(self):
>  self._filename = self.GetDefaultFilename()
> -self._pathname = tools.GetInputFilename(self._filename,
> -
> self.section.GetAllowMissing())
> +self._pathname = tools.GetInputFilename(
> +self._filename, self.external and self.section.GetAllowMissing())

I hope you know that 'and' has a bit different semantics in Python than in C 
for example.
And why to move first parameter to new line?

>  # Allow the file to be missing
> -if self.external and not self._pathname:
> +if not self._pathname:
>  self.SetContents(b'')
>  self.missing = True
>  return True
> +
>  self.ReadBlobContents()
>  return True
>  
> diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
> index 95b17d0b749..91225459162 100644
> --- a/tools/binman/ftest.py
> +++ b/tools/binman/ftest.py
> @@ -3708,5 +3708,12 @@ class TestFunctional(unittest.TestCase):
>  self.assertIn('Wibble test', err)
>  self.assertIn('Another test', err)
>  
> +def testMissingBlob(self):
> +"""Test handling of a blob containing a missing file"""
> +with self.assertRaises(ValueError) as e:
> +self._DoTestFile('173_missing_blob.dts', allow_missing=True)
> +self.assertIn("Filename 'missing' not found in input path",
> +  str(e.exception))
> +
>  if __name__ == "__main__":
>  unittest.main()
> diff --git a/tools/binman/test/173_missing_blob.dts 
> b/tools/binman/test/173_missing_blob.dts
> new file mode 100644
> index 000..ffb655a1cb4
> --- /dev/null
> +++ b/tools/binman/test/173_missing_blob.dts
> @@ -0,0 +1,14 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +/dts-v1/;
> +
> +/ {
> + #address-cells = <1>;
> + #size-cells = <1>;
> +
> + binman {
> + blob {
> + filename = "missing";
> + };
> + };
> +};
> -- 
> 2.28.0.526.ge36021eeef-goog
> 

-- 
With Best Regards,
Andy Shevchenko

[PATCH 2/5] binman: Show an error when a file is missing

2020-09-03 Thread Simon Glass 
The recent support for missing external binaries does not show an error
message when a file is genuinely missing (i.e. it is missing but not
marked as 'external'). This means that when -m is passed to binman, it
will never report a missing file.

Fix this and add a test.

Signed-off-by: Simon Glass 
---

 tools/binman/etype/blob.py |  7 ---
 tools/binman/ftest.py  |  7 +++
 tools/binman/test/173_missing_blob.dts | 14 ++
 3 files changed, 25 insertions(+), 3 deletions(-)
 create mode 100644 tools/binman/test/173_missing_blob.dts

diff --git a/tools/binman/etype/blob.py b/tools/binman/etype/blob.py
index c5f97c85a38..66cf243976e 100644
--- a/tools/binman/etype/blob.py
+++ b/tools/binman/etype/blob.py
@@ -37,13 +37,14 @@ class Entry_blob(Entry):
 
 def ObtainContents(self):
 self._filename = self.GetDefaultFilename()
-self._pathname = tools.GetInputFilename(self._filename,
-self.section.GetAllowMissing())
+self._pathname = tools.GetInputFilename(
+self._filename, self.external and self.section.GetAllowMissing())
 # Allow the file to be missing
-if self.external and not self._pathname:
+if not self._pathname:
 self.SetContents(b'')
 self.missing = True
 return True
+
 self.ReadBlobContents()
 return True
 
diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index 95b17d0b749..91225459162 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -3708,5 +3708,12 @@ class TestFunctional(unittest.TestCase):
 self.assertIn('Wibble test', err)
 self.assertIn('Another test', err)
 
+def testMissingBlob(self):
+"""Test handling of a blob containing a missing file"""
+with self.assertRaises(ValueError) as e:
+self._DoTestFile('173_missing_blob.dts', allow_missing=True)
+self.assertIn("Filename 'missing' not found in input path",
+  str(e.exception))
+
 if __name__ == "__main__":
 unittest.main()
diff --git a/tools/binman/test/173_missing_blob.dts 
b/tools/binman/test/173_missing_blob.dts
new file mode 100644
index 000..ffb655a1cb4
--- /dev/null
+++ b/tools/binman/test/173_missing_blob.dts
@@ -0,0 +1,14 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+/dts-v1/;
+
+/ {
+   #address-cells = <1>;
+   #size-cells = <1>;
+
+   binman {
+   blob {
+   filename = "missing";
+   };
+   };
+};
-- 
2.28.0.526.ge36021eeef-goog