Hi Tom,
On 1/20/26 6:55 PM, Tom Rini wrote:
On Tue, Jan 20, 2026 at 06:36:33PM +0100, Quentin Schulz wrote:
Hi Tom,
On 1/20/26 4:47 PM, Tom Rini wrote:
From: Dmitrii Sharshakov <[email protected]>
Check if elftools package is available before running DecodeElf().
This clarifies the error message and adds the required test for
coverage.
Signed-off-by: Dmitrii Sharshakov <[email protected]>
Signed-off-by: Andrew Soknacki <[email protected]>
Reviewed-by: Tom Rini <[email protected]>
[trini: Add the test provided by Andrew on IRC, to fix coverage]
Signed-off-by: Tom Rini <[email protected]>
---
Changes in v3:
- Add the test, provided by Andrew, to address the coverage failure in
CI
---
tools/binman/elf.py | 2 ++
tools/binman/elf_test.py | 12 ++++++++++++
2 files changed, 14 insertions(+)
diff --git a/tools/binman/elf.py b/tools/binman/elf.py
index 6ac960e04196..899c84ad36d6 100644
--- a/tools/binman/elf.py
+++ b/tools/binman/elf.py
@@ -570,6 +570,8 @@ def is_valid(data):
Returns:
bool: True if a valid Elf file, False if not
"""
+ if not ELF_TOOLS:
+ raise ValueError("Python: No module named 'elftools'")
try:
DecodeElf(data, 0)
return True
diff --git a/tools/binman/elf_test.py b/tools/binman/elf_test.py
index 5b1733928986..3ad0bf4c4b09 100644
--- a/tools/binman/elf_test.py
+++ b/tools/binman/elf_test.py
@@ -373,6 +373,18 @@ class TestElf(unittest.TestCase):
self.assertEqual(True, elf.is_valid(data))
self.assertEqual(False, elf.is_valid(data[4:]))
+ def test_is_valid_fail(self):
+ """Test calling is_valid() without elftools"""
+ old_val = elf.ELF_TOOLS
+ try:
+ elf.ELF_TOOLS = False
+ with self.assertRaises(ValueError) as e:
+ elf.is_valid(b'')
+ self.assertIn("Python: No module named 'elftools'",
+ str(e.exception))
+ finally:
+ elf.ELF_TOOLS = old_val
+
I'm not sure this is a good idea. The issue is that you're modifying the
variable from a python module. The check may be run by other tests at the
same time in different threads and I think the tests will unnecessarily be
skipped or fail?
I think we may want something like:
@unittest.mock.patch('binman.elf.ELF_TOOLS', False)
def test_is_valid_fail(self):
?
It's following the same practice as all of the other tests however. I
Which is probably incorrect. Also not sure about the os.environ
modification... probably should also be a global or temporary mock.
might do a follow-up to try what you suggest, since learning about how
to fix this problem yesterday I realized that I can fix I think my "no
cover" in commit 66be03b7ee19 ("binman: blob_dtb: improve error message
when SPL is not found") correctly now, as I think it's the same kind of
failure.
Yeah, you can likely mock self.FdtContents() to return None, None to
trigger the FileNotFoundError exception with the desired parameters. The
only issue is whether self.FdtContents is called multiple times within
the same function (maybe from functions/methods called from that
function), then it gets harder.
Personally, I find the exercise as an example of testing for tests sake.
Coverage test is difficult. Sometimes it's unnecessary tests so it makes
the coverage tool happy we don't regress. How does one decide which
tests mean something /me shrugs.
We didn't (and can't?) have a test that caught the real problem, which
is a lack of elftools giving a hard to understand error message. The
patch from Dmitrii fixes that real problem (re-use the test+raise logic
other functions do), but python testing requests a test for this new
failure.
I haven't looked much into it, but reading elf.py, I didn't like that
some methods require elftools and some not. I'm wondering if we
shouldn't split them in two and have the one that relies on elftools not
even check if it's there. It imports the module, done. If it cannot,
it'll complain about
NameError: name 'ELFFile' is not defined
NameError: name 'ELFError' is not defined. Did you mean: 'EOFError'?
and it's up to the caller to handle whether it should be a hard failure
(not catch it) or not?
or we could also just raise an Exception to tell the user to install
pyelftools if we really need a hint at what to do when the exception is
raised.
Quentin
