| Message ID | 20231218084403.599015-3-alex@linutronix.de |
|---|---|
| State | Accepted, archived |
| Commit | 8721c52041e910bd4d8a9235b52f274f4f02c8a3 |
| Headers | show |
| Series | [1/7] bitbake/runqueue: rework 'bitbake -S printdiff' logic | expand |
On Mon, 2023-12-18 at 09:43 +0100, Alexander Kanavin wrote: > find_siginfo() returns two different data structures depending > on whether its third argument (list of hashes to find) is empty or > not: > - a dict of timestamps keyed by path > - a dict of paths keyed by hash > > This is not a good API design; it's much better to return > a dict of dicts that include both timestamp and path, keyed by > hash. Then the API consumer can decide how they want to use these > fields, particularly for additional diagnostics or informational > output. > > I also took the opportunity to add a binary field that > tells if the match came from sstate or local stamps dir, which > will help prioritize local stamps when looking up most > recent task signatures. > > Signed-off-by: Alexander Kanavin <alex@linutronix.de> > --- > meta/lib/oe/buildhistory_analysis.py | 2 +- > meta/lib/oe/sstatesig.py | 31 +++++++++------------ > meta/lib/oeqa/selftest/cases/sstatetests.py | 10 +++---- > 3 files changed, 19 insertions(+), 24 deletions(-) I can see why you've done this. I had thought this might have been a compatibility issue but it wasn't, it was just bad design from the start (and I wasn't responsible for it for a change!). It gives me a real headache for merging though since it requires OE- Core and bitbake to change in lockstep. There is a risk people might just update bitbake but not OE-Core and also vice versa, they update OE-Core but not bitbake. This can happen for all kinds of reasons, e.g. mixing a newer bitbake with an older release. The latter is easy to address with a bitbake version check bit the former is not really possible to detect with our current code as bitbake can't know OE-Core doesn't support what it needs. With other changes in the pipeline like inherit_defer and the fact there is a bug related to this on a stable branch, it makes it all the more likely people will try and mix/match different versions. I've a few ideas on how we might be able to detect potential problems, I'll continue to try and work something out but I want to make it clear there are some things it is hard to change :/. Cheers, Richard
On Fri, 5 Jan 2024 at 12:22, Richard Purdie <richard.purdie@linuxfoundation.org> wrote: > I've a few ideas on how we might be able to detect potential problems, > I'll continue to try and work something out but I want to make it clear > there are some things it is hard to change :/. There's the option of adding another function (find_siginfo_v2) and leaving this one as it was, this is how the kernel does things. They don't shy away from adding v3, v4 and so on either. But I thought less code is better than more code. Alex
On Fri, 2024-01-05 at 12:42 +0100, Alexander Kanavin wrote: > On Fri, 5 Jan 2024 at 12:22, Richard Purdie > <richard.purdie@linuxfoundation.org> wrote: > > > I've a few ideas on how we might be able to detect potential problems, > > I'll continue to try and work something out but I want to make it clear > > there are some things it is hard to change :/. > > There's the option of adding another function (find_siginfo_v2) and > leaving this one as it was, this is how the kernel does things. They > don't shy away from adding v3, v4 and so on either. But I thought less > code is better than more code. That is effectively what I just merged, except I did break compatibility by dropping the v1. We need to give the user good messages about how to fix their problem which is why a number is needed rather than just a function name. Cheers, Richard
diff --git a/meta/lib/oe/buildhistory_analysis.py b/meta/lib/oe/buildhistory_analysis.py index b1856846b6a..4edad01580c 100644 --- a/meta/lib/oe/buildhistory_analysis.py +++ b/meta/lib/oe/buildhistory_analysis.py @@ -562,7 +562,7 @@ def compare_siglists(a_blob, b_blob, taskdiff=False): elif not hash2 in hashfiles: out.append("Unable to find matching sigdata for %s with hash %s" % (desc, hash2)) else: - out2 = bb.siggen.compare_sigfiles(hashfiles[hash1], hashfiles[hash2], recursecb, collapsed=True) + out2 = bb.siggen.compare_sigfiles(hashfiles[hash1]['path'], hashfiles[hash2]['path'], recursecb, collapsed=True) for line in out2: m = hashlib.sha256() m.update(line.encode('utf-8')) diff --git a/meta/lib/oe/sstatesig.py b/meta/lib/oe/sstatesig.py index 8a97fb0c04b..0342bcdc87a 100644 --- a/meta/lib/oe/sstatesig.py +++ b/meta/lib/oe/sstatesig.py @@ -349,7 +349,6 @@ def find_siginfo(pn, taskname, taskhashlist, d): pn, taskname = key.split(':', 1) hashfiles = {} - filedates = {} def get_hashval(siginfo): if siginfo.endswith('.siginfo'): @@ -357,6 +356,12 @@ def find_siginfo(pn, taskname, taskhashlist, d): else: return siginfo.rpartition('.')[2] + def get_time(fullpath): + try: + return os.stat(fullpath).st_mtime + except OSError: + return None + # First search in stamps dir localdata = d.createCopy() localdata.setVar('MULTIMACH_TARGET_SYS', '*') @@ -372,24 +377,21 @@ def find_siginfo(pn, taskname, taskhashlist, d): filespec = '%s.%s.sigdata.*' % (stamp, taskname) foundall = False import glob + bb.debug(1, "Calling glob.glob on {}".format(filespec)) for fullpath in glob.glob(filespec): match = False if taskhashlist: for taskhash in taskhashlist: if fullpath.endswith('.%s' % taskhash): - hashfiles[taskhash] = fullpath + hashfiles[taskhash] = {'path':fullpath, 'sstate':False, 'time':get_time(fullpath)} if len(hashfiles) == len(taskhashlist): foundall = True break else: - try: - filedates[fullpath] = os.stat(fullpath).st_mtime - except OSError: - continue hashval = get_hashval(fullpath) - hashfiles[hashval] = fullpath + hashfiles[hashval] = {'path':fullpath, 'sstate':False, 'time':get_time(fullpath)} - if not taskhashlist or (len(filedates) < 2 and not foundall): + if not taskhashlist or (len(hashfiles) < 2 and not foundall): # That didn't work, look in sstate-cache hashes = taskhashlist or ['?' * 64] localdata = bb.data.createCopy(d) @@ -412,22 +414,15 @@ def find_siginfo(pn, taskname, taskhashlist, d): localdata.setVar('SSTATE_EXTRAPATH', "${NATIVELSBSTRING}/") filespec = '%s.siginfo' % localdata.getVar('SSTATE_PKG') + bb.debug(1, "Calling glob.glob on {}".format(filespec)) matchedfiles = glob.glob(filespec) for fullpath in matchedfiles: actual_hashval = get_hashval(fullpath) if actual_hashval in hashfiles: continue - hashfiles[hashval] = fullpath - if not taskhashlist: - try: - filedates[fullpath] = os.stat(fullpath).st_mtime - except: - continue + hashfiles[actual_hashval] = {'path':fullpath, 'sstate':True, 'time':get_time(fullpath)} - if taskhashlist: - return hashfiles - else: - return filedates + return hashfiles bb.siggen.find_siginfo = find_siginfo diff --git a/meta/lib/oeqa/selftest/cases/sstatetests.py b/meta/lib/oeqa/selftest/cases/sstatetests.py index 6af3c8f2178..6da3ef3ca77 100644 --- a/meta/lib/oeqa/selftest/cases/sstatetests.py +++ b/meta/lib/oeqa/selftest/cases/sstatetests.py @@ -765,14 +765,14 @@ addtask tmptask2 before do_tmptask1 hashes = [hash1, hash2] hashfiles = find_siginfo(key, None, hashes) self.assertCountEqual(hashes, hashfiles) - bb.siggen.compare_sigfiles(hashfiles[hash1], hashfiles[hash2], recursecb) + bb.siggen.compare_sigfiles(hashfiles[hash1]['path'], hashfiles[hash2]['path'], recursecb) for pn in pns: recursecb_count = 0 - filedates = find_siginfo(pn, "do_tmptask1") - self.assertGreaterEqual(len(filedates), 2) - latestfiles = sorted(filedates.keys(), key=lambda f: filedates[f])[-2:] - bb.siggen.compare_sigfiles(latestfiles[-2], latestfiles[-1], recursecb) + matches = find_siginfo(pn, "do_tmptask1") + self.assertGreaterEqual(len(matches), 2) + latesthashes = sorted(matches.keys(), key=lambda h: matches[h]['time'])[-2:] + bb.siggen.compare_sigfiles(matches[latesthashes[-2]]['path'], matches[latesthashes[-1]]['path'], recursecb) self.assertEqual(recursecb_count,1) class SStatePrintdiff(SStateBase):
find_siginfo() returns two different data structures depending on whether its third argument (list of hashes to find) is empty or not: - a dict of timestamps keyed by path - a dict of paths keyed by hash This is not a good API design; it's much better to return a dict of dicts that include both timestamp and path, keyed by hash. Then the API consumer can decide how they want to use these fields, particularly for additional diagnostics or informational output. I also took the opportunity to add a binary field that tells if the match came from sstate or local stamps dir, which will help prioritize local stamps when looking up most recent task signatures. Signed-off-by: Alexander Kanavin <alex@linutronix.de> --- meta/lib/oe/buildhistory_analysis.py | 2 +- meta/lib/oe/sstatesig.py | 31 +++++++++------------ meta/lib/oeqa/selftest/cases/sstatetests.py | 10 +++---- 3 files changed, 19 insertions(+), 24 deletions(-)