| Message ID | 1740508733-4989-1-git-send-email-mark.hatle@kernel.crashing.org |
|---|---|
| State | Accepted, archived |
| Commit | ff523497270f37b484b44a4445c2194791bcb6ff |
| Headers | show |
| Series | [v2] bitbake: runqueue: Verify mcdepends are valid | expand |
On Tue, 2025-02-25 at 12:38 -0600, Mark Hatle via lists.openembedded.org wrote: > From: Mark Hatle <mark.hatle@amd.com> > > In order to avoid a potentially confusing backtrace, check that the mcdepend > is valid when we add it. > > Add a test case to ensure invalid configurations are caught and trigger an > error. > > Signed-off-by: Mark Hatle <mark.hatle@amd.com> > Signed-off-by: Mark Hatle <mark.hatle@kernel.crashing.org> > --- > lib/bb/runqueue.py | 2 ++ > lib/bb/tests/runqueue-tests/recipes/g1.bb | 2 ++ > lib/bb/tests/runqueue-tests/recipes/h1.bb | 0 > lib/bb/tests/runqueue.py | 18 ++++++++++++++++++ > 4 files changed, 22 insertions(+) > create mode 100644 lib/bb/tests/runqueue-tests/recipes/g1.bb > create mode 100644 lib/bb/tests/runqueue-tests/recipes/h1.bb > > diff --git a/lib/bb/runqueue.py b/lib/bb/runqueue.py > index ffb2d2849..539a6065f 100644 > --- a/lib/bb/runqueue.py > +++ b/lib/bb/runqueue.py > @@ -730,6 +730,8 @@ class RunQueueData: > if mc == frommc: > fn = taskData[mcdep].build_targets[pn][0] > newdep = '%s:%s' % (fn,deptask) > + if newdep not in taskData[mcdep].taskentries: > + bb.fatal("Task mcdepends on non-existent task %s" % (newdep)) > taskData[mc].taskentries[tid].tdepends.append(newdep) > > for mc in taskData: > diff --git a/lib/bb/tests/runqueue-tests/recipes/g1.bb b/lib/bb/tests/runqueue-tests/recipes/g1.bb > new file mode 100644 > index 000000000..3c7dca025 > --- /dev/null > +++ b/lib/bb/tests/runqueue-tests/recipes/g1.bb > @@ -0,0 +1,2 @@ > +do_build[mcdepends] = "mc::mc-1:h1:do_invalid" > + > diff --git a/lib/bb/tests/runqueue-tests/recipes/h1.bb b/lib/bb/tests/runqueue-tests/recipes/h1.bb > new file mode 100644 > index 000000000..e69de29bb > diff --git a/lib/bb/tests/runqueue.py b/lib/bb/tests/runqueue.py > index cc87e8d6a..e15fdc330 100644 > --- a/lib/bb/tests/runqueue.py > +++ b/lib/bb/tests/runqueue.py > @@ -314,6 +314,24 @@ class RunQueueTests(unittest.TestCase): > ["mc_2:a1:%s" % t for t in rerun_tasks] > self.assertEqual(set(tasks), set(expected)) > > + # AssertionError is raised by self.run_bitbakecmd in the event of a failure > + # in thise case a failure is required, but we need to do further processing > + # to tell if it's the RIGHT kind of failure. > + with self.assertRaises(AssertionError): > + try: > + self.run_bitbakecmd(["bitbake", "g1"], tempdir, "", extraenv=extraenv, cleanup=True) > + except AssertionError as e: > + # If the word 'Traceback' or 'KeyError' is in the exception text, > + # we've regressed. So verify it's NOT present, and pass the > + # exception to indicate a 'pass'. > + if not ('Traceback' in str(e) or 'KeyError' in str(e)): > + # Raising 'AssertionError' is a test pass > + raise e > + else: > + # Dump the output for triage, but don't raise an > + # exception, this indicates a test failure. > + print("%s: %s" % (type(e), e)) > + > self.shutdown(tempdir) > > def test_hashserv_single(self): > Sorry to ask this but during review Ross pointed out that assertRaisesRegex can take a regex and then you can really simplify this further. Would you be able to tweak this further to use that? As I've mentioned elsewhere, having well written tests helps foster further well written tests in the future. Given how long we end up maintaining these for, it is worth getting them right! Cheers, Richard
On 2/27/25 6:29 AM, Richard Purdie via lists.openembedded.org wrote: > On Tue, 2025-02-25 at 12:38 -0600, Mark Hatle via lists.openembedded.org wrote: >> From: Mark Hatle <mark.hatle@amd.com> >> >> In order to avoid a potentially confusing backtrace, check that the mcdepend >> is valid when we add it. >> >> Add a test case to ensure invalid configurations are caught and trigger an >> error. >> >> Signed-off-by: Mark Hatle <mark.hatle@amd.com> >> Signed-off-by: Mark Hatle <mark.hatle@kernel.crashing.org> >> --- >> lib/bb/runqueue.py | 2 ++ >> lib/bb/tests/runqueue-tests/recipes/g1.bb | 2 ++ >> lib/bb/tests/runqueue-tests/recipes/h1.bb | 0 >> lib/bb/tests/runqueue.py | 18 ++++++++++++++++++ >> 4 files changed, 22 insertions(+) >> create mode 100644 lib/bb/tests/runqueue-tests/recipes/g1.bb >> create mode 100644 lib/bb/tests/runqueue-tests/recipes/h1.bb >> >> diff --git a/lib/bb/runqueue.py b/lib/bb/runqueue.py >> index ffb2d2849..539a6065f 100644 >> --- a/lib/bb/runqueue.py >> +++ b/lib/bb/runqueue.py >> @@ -730,6 +730,8 @@ class RunQueueData: >> if mc == frommc: >> fn = taskData[mcdep].build_targets[pn][0] >> newdep = '%s:%s' % (fn,deptask) >> + if newdep not in taskData[mcdep].taskentries: >> + bb.fatal("Task mcdepends on non-existent task %s" % (newdep)) >> taskData[mc].taskentries[tid].tdepends.append(newdep) >> >> for mc in taskData: >> diff --git a/lib/bb/tests/runqueue-tests/recipes/g1.bb b/lib/bb/tests/runqueue-tests/recipes/g1.bb >> new file mode 100644 >> index 000000000..3c7dca025 >> --- /dev/null >> +++ b/lib/bb/tests/runqueue-tests/recipes/g1.bb >> @@ -0,0 +1,2 @@ >> +do_build[mcdepends] = "mc::mc-1:h1:do_invalid" >> + >> diff --git a/lib/bb/tests/runqueue-tests/recipes/h1.bb b/lib/bb/tests/runqueue-tests/recipes/h1.bb >> new file mode 100644 >> index 000000000..e69de29bb >> diff --git a/lib/bb/tests/runqueue.py b/lib/bb/tests/runqueue.py >> index cc87e8d6a..e15fdc330 100644 >> --- a/lib/bb/tests/runqueue.py >> +++ b/lib/bb/tests/runqueue.py >> @@ -314,6 +314,24 @@ class RunQueueTests(unittest.TestCase): >> ["mc_2:a1:%s" % t for t in rerun_tasks] >> self.assertEqual(set(tasks), set(expected)) >> >> + # AssertionError is raised by self.run_bitbakecmd in the event of a failure >> + # in thise case a failure is required, but we need to do further processing >> + # to tell if it's the RIGHT kind of failure. >> + with self.assertRaises(AssertionError): >> + try: >> + self.run_bitbakecmd(["bitbake", "g1"], tempdir, "", extraenv=extraenv, cleanup=True) >> + except AssertionError as e: >> + # If the word 'Traceback' or 'KeyError' is in the exception text, >> + # we've regressed. So verify it's NOT present, and pass the >> + # exception to indicate a 'pass'. >> + if not ('Traceback' in str(e) or 'KeyError' in str(e)): >> + # Raising 'AssertionError' is a test pass >> + raise e >> + else: >> + # Dump the output for triage, but don't raise an >> + # exception, this indicates a test failure. >> + print("%s: %s" % (type(e), e)) >> + >> self.shutdown(tempdir) >> >> def test_hashserv_single(self): >> > > Sorry to ask this but during review Ross pointed out that > assertRaisesRegex can take a regex and then you can really simplify > this further. Would you be able to tweak this further to use that? I've not been able to get this to work with assertRaisesRegex. What I tried was: with self.assertRaisesRegex(AssertionError, "(?!(Traceback|KeyError))'): self.run_bitbakecmd(["bitbake", "g1"], tempdir, "", extraenv=extraenv, cleanup=True) With the first hunk of the change disabled, it doesn't fail, even though the exception text DOES include both 'Traceback' and 'KeyError'. I'm wondering if this could be a 'multiline' issues, as the traceback looks like Command ['bitbake', 'g1'] failed with NOTE: Reconnecting to bitbake server... Note:Retrying server connection (#1)... (09:54:45.246200) Loading cache...done. Loaded 28 entries from dependency cache. NOTE: Resolving any missing task queue dependencies NOTE: Resolving any missing task queue dependencies NOTE: Resolving any missing task queue dependencies NOTE: Resolving any missing task queue dependencies NOTE: Resolving any missing task queue dependencies NOTE: Resolving any missing task queue dependencies NOTE: Resolving any missing task queue dependencies NOTE: Resolving any missing task queue dependencies NOTE: Resolving any missing task queue dependencies Initialising tasks...ERROR: An uncaught exception occurred in runqueue Traceback (most recent call last): File "<path>/bitbake/lib/bb/runqueue.py", line 1659, in execute_runqueue return self._execute_runqueue() File "<path>/bitbake/lib/bb/runqueue.py", line 1570, in _execute_runqueue if self.rqdata.prepare() == 0: File "<path>/bitbake/lib/bb/runqueue.py", line 858, in prepare revdeps[dep].add(tid) KeyError: 'mc:mc-1:<path>/bitbake/lib/bb/tests/runqueue-tests/recipes/h1.bb:do_invalid' (and some more text below)... Any suggestions on a regex from anyone that can work with the above would be useful, otherwise we may have hit a limitation if the re.search (which I assume) that assertRaisesRegex is using. --Mark > As I've mentioned elsewhere, having well written tests helps foster > further well written tests in the future. Given how long we end up > maintaining these for, it is worth getting them right! > > Cheers, > > Richard > > > > -=-=-=-=-=-=-=-=-=-=-=- > Links: You receive all messages sent to this group. > View/Reply Online (#17345): https://lists.openembedded.org/g/bitbake-devel/message/17345 > Mute This Topic: https://lists.openembedded.org/mt/111382531/3616948 > Group Owner: bitbake-devel+owner@lists.openembedded.org > Unsubscribe: https://lists.openembedded.org/g/bitbake-devel/unsub [mark.hatle@kernel.crashing.org] > -=-=-=-=-=-=-=-=-=-=-=- >
diff --git a/lib/bb/runqueue.py b/lib/bb/runqueue.py index ffb2d2849..539a6065f 100644 --- a/lib/bb/runqueue.py +++ b/lib/bb/runqueue.py @@ -730,6 +730,8 @@ class RunQueueData: if mc == frommc: fn = taskData[mcdep].build_targets[pn][0] newdep = '%s:%s' % (fn,deptask) + if newdep not in taskData[mcdep].taskentries: + bb.fatal("Task mcdepends on non-existent task %s" % (newdep)) taskData[mc].taskentries[tid].tdepends.append(newdep) for mc in taskData: diff --git a/lib/bb/tests/runqueue-tests/recipes/g1.bb b/lib/bb/tests/runqueue-tests/recipes/g1.bb new file mode 100644 index 000000000..3c7dca025 --- /dev/null +++ b/lib/bb/tests/runqueue-tests/recipes/g1.bb @@ -0,0 +1,2 @@ +do_build[mcdepends] = "mc::mc-1:h1:do_invalid" + diff --git a/lib/bb/tests/runqueue-tests/recipes/h1.bb b/lib/bb/tests/runqueue-tests/recipes/h1.bb new file mode 100644 index 000000000..e69de29bb diff --git a/lib/bb/tests/runqueue.py b/lib/bb/tests/runqueue.py index cc87e8d6a..e15fdc330 100644 --- a/lib/bb/tests/runqueue.py +++ b/lib/bb/tests/runqueue.py @@ -314,6 +314,24 @@ class RunQueueTests(unittest.TestCase): ["mc_2:a1:%s" % t for t in rerun_tasks] self.assertEqual(set(tasks), set(expected)) + # AssertionError is raised by self.run_bitbakecmd in the event of a failure + # in thise case a failure is required, but we need to do further processing + # to tell if it's the RIGHT kind of failure. + with self.assertRaises(AssertionError): + try: + self.run_bitbakecmd(["bitbake", "g1"], tempdir, "", extraenv=extraenv, cleanup=True) + except AssertionError as e: + # If the word 'Traceback' or 'KeyError' is in the exception text, + # we've regressed. So verify it's NOT present, and pass the + # exception to indicate a 'pass'. + if not ('Traceback' in str(e) or 'KeyError' in str(e)): + # Raising 'AssertionError' is a test pass + raise e + else: + # Dump the output for triage, but don't raise an + # exception, this indicates a test failure. + print("%s: %s" % (type(e), e)) + self.shutdown(tempdir) def test_hashserv_single(self):