[2.0][PATCH 0/8] Patch review
Please review this set of patches for kirkstone/2.0 and have comments back by end of day Wednesday.
Passded a-full on autobuilder:
https://autobuilder.yoctoproject.org/typhoon/#/builders/83/builds/4294
The following changes since commit ac576d6fad6bba0cfea931883f25264ea83747ca:
bitbake-user-manual: npm fetcher: improve description of SRC_URI format (2022-09-16 17:52:26 +0100)
are available in the Git repository at:
https://git.openembedded.org/bitbake-contrib stable/2.0-nut
http://cgit.openembedded.org/bitbake-contrib/log/?h=stable/2.0-nut
Joshua Watt (1):
siggen: Fix insufficent entropy in sigtask file names
Neil Horman (1):
Fix npm to use https rather than http
Pavel Zhukov (1):
gitsm: Error out if submodule refers to parent repo
Richard Purdie (5):
runqueue: Ensure deferred tasks are sorted by multiconfig
runqueue: Improve deadlock warning messages
runqueue: Drop deadlock breaking force fail
bitbake: Add copyright headers where missing asyncrpc/client: Fix unix domain socket chdir race issues
bin/bitbake-prserv | 2 ++ bin/bitbake-worker | 2 ++ bin/git-make-shallow | 2 ++ lib/bb/COW.py | 2 ++ lib/bb/asyncrpc/__init__.py | 2 ++ lib/bb/asyncrpc/client.py | 24 +++++++++++++++--------- lib/bb/asyncrpc/serv.py | 2 ++ lib/bb/codeparser.py | 2 ++ lib/bb/compress/_pipecompress.py | 2 ++ lib/bb/compress/lz4.py | 2 ++ lib/bb/compress/zstd.py | 2 ++ lib/bb/daemonize.py | 2 ++ lib/bb/exceptions.py | 2 ++ lib/bb/fetch2/gitsm.py | 3 +++ lib/bb/fetch2/npm.py | 2 +- lib/bb/fetch2/osc.py | 2 ++ lib/bb/process.py | 2 ++ lib/bb/runqueue.py | 20 +++++++++++--------- lib/bb/siggen.py | 4 +++- lib/bb/tests/compression.py | 2 ++ lib/bb/tests/cooker.py | 2 ++ lib/bb/utils.py | 21 +++++++++++++++++++++ lib/bblayers/__init__.py | 2 ++ lib/bblayers/action.py | 2 ++ lib/bblayers/common.py | 2 ++ lib/bblayers/layerindex.py | 2 ++ lib/bblayers/query.py | 2 ++ lib/prserv/__init__.py | 2 ++ lib/prserv/client.py | 2 ++ lib/prserv/db.py | 2 ++ lib/prserv/serv.py | 2 ++ lib/toaster/manage.py | 2 ++ 32 files changed, 106 insertions(+), 20 deletions(-)
Signature · 1 line
2.25.1
[bitbake][kirkstone][2.0][PATCH 1/8] runqueue: Ensure deferred tasks are sorted by multiconfig
From: Richard Purdie <[email protected]>
We have to prefer one multiconfig over another when deferring tasks, else we'll have cross-linked build trees and nothing will be able to build.
In the original population code, we sort like this but we don't after rehashing. Ensure we have the same sorting after rehashing toa void deadlocks.
--- lib/bb/runqueue.py | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)
lib/bb/runqueue.py +8 -5
@@ -2523,11 +2523,14 @@ class RunQueueExecute: if update_tasks: self.sqdone = False - for tid in [t[0] for t in update_tasks]: - h = pending_hash_index(tid, self.rqdata) - if h in self.sqdata.hashes and tid != self.sqdata.hashes[h]: - self.sq_deferred[tid] = self.sqdata.hashes[h] - bb.note("Deferring %s after %s" % (tid, self.sqdata.hashes[h])) + for mc in sorted(self.sqdata.multiconfigs): + for tid in sorted([t[0] for t in update_tasks]): + if mc_from_tid(tid) != mc: + continue + h = pending_hash_index(tid, self.rqdata) + if h in self.sqdata.hashes and tid != self.sqdata.hashes[h]: + self.sq_deferred[tid] = self.sqdata.hashes[h] + bb.note("Deferring %s after %s" % (tid, self.sqdata.hashes[h])) update_scenequeue_data([t[0] for t in update_tasks], self.sqdata, self.rqdata, self.rq, self.cooker, self.stampcache, self, summary=False) for (tid, harddepfail, origvalid) in update_tasks:
Signature · 1 line
2.25.1
[bitbake][kirkstone][2.0][PATCH 4/8] siggen: Fix insufficent entropy in sigtask file names
In reply to Steve SakomanFrom: Joshua Watt <[email protected]>
Signature generation uses mkstemp() to get a file descriptor to a unique file and then write the signature into it. However, the unique file name generation in glibc is based on the system timestamp, which means that with highly parallel builds it is more likely than one might expect expected that a conflict will occur between two different builder nodes. When operating over NFS (such as a shared sstate cache), this can cause race conditions and rare failures (particularly with NFS servers that may not correctly implement O_EXCL).
The signature generation code is particularly susceptible to races since a single "sigtask." prefix used for all signatures from all tasks, which makes collision even more likely.
To work around this, add an internal implementation of mkstemp() that adds additional truly random entropy to the file name to eliminate conflicts.
--- lib/bb/siggen.py | 2 +- lib/bb/utils.py | 21 +++++++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-)
lib/bb/siggen.py +1 -1
@@ -419,7 +419,7 @@ class SignatureGeneratorBasic(SignatureGenerator): bb.error("Taskhash mismatch %s versus %s for %s" % (computed_taskhash, self.taskhash[tid], tid)) sigfile = sigfile.replace(self.taskhash[tid], computed_taskhash) - fd, tmpfile = tempfile.mkstemp(dir=os.path.dirname(sigfile), prefix="sigtask.") + fd, tmpfile = bb.utils.mkstemp(dir=os.path.dirname(sigfile), prefix="sigtask.") try: with bb.compress.zstd.open(fd, "wt", encoding="utf-8", num_threads=1) as f: json.dump(data, f, sort_keys=True, separators=(",", ":"), cls=SetEncoder)
lib/bb/utils.py +21 -0
@@ -28,6 +28,8 @@ import signal import collections import copy import ctypes +import random +import tempfile from subprocess import getstatusoutput from contextlib import contextmanager from ctypes import cdll
@@ -1756,3 +1758,22 @@ def is_local_uid(uid=''): if str(uid) == line_split[2]: return True return False + +def mkstemp(suffix=None, prefix=None, dir=None, text=False): + """ + Generates a unique filename, independent of time. + + mkstemp() in glibc (at least) generates unique file names based on the + current system time. When combined with highly parallel builds, and + operating over NFS (e.g. shared sstate/downloads) this can result in + conflicts and race conditions. + + This function adds additional entropy to the file name so that a collision + is independent of time and thus extremely unlikely. + """ + entropy = "".join(random.choices("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ1234567890", k=20)) + if prefix: + prefix = prefix + entropy + else: + prefix = tempfile.gettempprefix() + entropy + return tempfile.mkstemp(suffix=suffix, prefix=prefix, dir=dir, text=text)
Signature · 1 line
2.25.1
[bitbake][kirkstone][2.0][PATCH 6/8] Fix npm to use https rather than http
In reply to Steve SakomanFrom: Neil Horman <[email protected]>
Hit this error while building nlf-native recently:
{
"error": {
"summary": "URI malformed",
"detail": ""
}
}
Some poking about led me to discover that:
1) The npm.py tool replaces npm:// with http://, not https://
2) Some versions of the npm tool don't handle 301 redirects properly, choosing to display the above error instead when using the default nodejs registry
It would be good to go fix npm to handle the redirect properly, but it seems like it would also be good to assume secure http when contacting a registry, hence, this patch
--- lib/bb/fetch2/npm.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
lib/bb/fetch2/npm.py +1 -1
@@ -156,7 +156,7 @@ class Npm(FetchMethod): raise ParameterError("Invalid 'version' parameter", ud.url) # Extract the 'registry' part of the url - ud.registry = re.sub(r"^npm://", "http://", ud.url.split(";")[0]) + ud.registry = re.sub(r"^npm://", "https://", ud.url.split(";")[0]) # Using the 'downloadfilename' parameter as local filename # or the npm package name.
Signature · 1 line
2.25.1
[bitbake][kirkstone][2.0][PATCH 2/8] runqueue: Improve deadlock warning messages
In reply to Steve SakomanFrom: Richard Purdie <[email protected]>
Tweak the deadlock breaking messages to be explict about which task is blocked on which other task. The messages currently imply it is "freeing" the blocking task which is confusing.
--- lib/bb/runqueue.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
lib/bb/runqueue.py +6 -4
@@ -2254,10 +2254,12 @@ class RunQueueExecute: # No more tasks can be run. If we have deferred setscene tasks we should run them. if self.sq_deferred: - tid = self.sq_deferred.pop(list(self.sq_deferred.keys())[0]) - logger.warning("Runqeueue deadlocked on deferred tasks, forcing task %s" % tid) - if tid not in self.runq_complete: - self.sq_task_failoutright(tid) + deferred_tid = list(self.sq_deferred.keys())[0] + blocking_tid = self.sq_deferred.pop(deferred_tid) + logger.warning("Runqeueue deadlocked on deferred tasks, forcing task %s blocked by %s" % (deferred_tid, blocking_tid)) + if blocking_tid not in self.runq_complete: + logger.warning("Failing blocking task %s" % (blocking_tid)) + self.sq_task_failoutright(blocking_tid) return True if self.failed_tids:
Signature · 1 line
2.25.1
[bitbake][kirkstone][2.0][PATCH 5/8] bitbake: Add copyright headers where missing
In reply to Steve SakomanFrom: Richard Purdie <[email protected]>
Where copyright headers were not present, add them to make things clear.
--- bin/bitbake-prserv | 2 ++ bin/bitbake-worker | 2 ++ bin/git-make-shallow | 2 ++ lib/bb/COW.py | 2 ++ lib/bb/asyncrpc/__init__.py | 2 ++ lib/bb/asyncrpc/client.py | 2 ++ lib/bb/asyncrpc/serv.py | 2 ++ lib/bb/codeparser.py | 2 ++ lib/bb/compress/_pipecompress.py | 2 ++ lib/bb/compress/lz4.py | 2 ++ lib/bb/compress/zstd.py | 2 ++ lib/bb/daemonize.py | 2 ++ lib/bb/exceptions.py | 2 ++ lib/bb/fetch2/osc.py | 2 ++ lib/bb/process.py | 2 ++ lib/bb/siggen.py | 2 ++ lib/bb/tests/compression.py | 2 ++ lib/bb/tests/cooker.py | 2 ++ lib/bblayers/__init__.py | 2 ++ lib/bblayers/action.py | 2 ++ lib/bblayers/common.py | 2 ++ lib/bblayers/layerindex.py | 2 ++ lib/bblayers/query.py | 2 ++ lib/prserv/__init__.py | 2 ++ lib/prserv/client.py | 2 ++ lib/prserv/db.py | 2 ++ lib/prserv/serv.py | 2 ++ lib/toaster/manage.py | 2 ++ 28 files changed, 56 insertions(+)
bin/bitbake-prserv +2 -0
@@ -1,5 +1,7 @@ #!/usr/bin/env python3 # +# Copyright BitBake Contributors +# # SPDX-License-Identifier: GPL-2.0-only #
bin/bitbake-worker +2 -0
@@ -1,5 +1,7 @@ #!/usr/bin/env python3 # +# Copyright BitBake Contributors +# # SPDX-License-Identifier: GPL-2.0-only #
bin/git-make-shallow +2 -0
@@ -1,5 +1,7 @@ #!/usr/bin/env python3 # +# Copyright BitBake Contributors +# # SPDX-License-Identifier: GPL-2.0-only #
lib/bb/COW.py +2 -0
@@ -3,6 +3,8 @@ # # Copyright (C) 2006 Tim Ansell # +# SPDX-License-Identifier: GPL-2.0-only +# # Please Note: # Be careful when using mutable types (ie Dict and Lists) - operations involving these are SLOW. # Assign a file to __warn__ to get warnings about slow operations.
lib/bb/asyncrpc/__init__.py +2 -0
@@ -1,4 +1,6 @@ # +# Copyright BitBake Contributors +# # SPDX-License-Identifier: GPL-2.0-only #
lib/bb/asyncrpc/client.py +2 -0
@@ -1,4 +1,6 @@ # +# Copyright BitBake Contributors +# # SPDX-License-Identifier: GPL-2.0-only #
lib/bb/asyncrpc/serv.py +2 -0
@@ -1,4 +1,6 @@ # +# Copyright BitBake Contributors +# # SPDX-License-Identifier: GPL-2.0-only #
lib/bb/codeparser.py +2 -0
@@ -1,4 +1,6 @@ # +# Copyright BitBake Contributors +# # SPDX-License-Identifier: GPL-2.0-only #
lib/bb/compress/_pipecompress.py +2 -0
@@ -1,4 +1,6 @@ # +# Copyright BitBake Contributors +# # SPDX-License-Identifier: GPL-2.0-only # # Helper library to implement streaming compression and decompression using an
lib/bb/compress/lz4.py +2 -0
@@ -1,4 +1,6 @@ # +# Copyright BitBake Contributors +# # SPDX-License-Identifier: GPL-2.0-only #
lib/bb/compress/zstd.py +2 -0
@@ -1,4 +1,6 @@ # +# Copyright BitBake Contributors +# # SPDX-License-Identifier: GPL-2.0-only #
lib/bb/daemonize.py +2 -0
@@ -1,4 +1,6 @@ # +# Copyright BitBake Contributors +# # SPDX-License-Identifier: GPL-2.0-only #
lib/bb/exceptions.py +2 -0
@@ -1,4 +1,6 @@ # +# Copyright BitBake Contributors +# # SPDX-License-Identifier: GPL-2.0-only #
lib/bb/fetch2/osc.py +2 -0
@@ -1,4 +1,6 @@ # +# Copyright BitBake Contributors +# # SPDX-License-Identifier: GPL-2.0-only # """
lib/bb/process.py +2 -0
@@ -1,4 +1,6 @@ # +# Copyright BitBake Contributors +# # SPDX-License-Identifier: GPL-2.0-only #
lib/bb/siggen.py +2 -0
@@ -1,4 +1,6 @@ # +# Copyright BitBake Contributors +# # SPDX-License-Identifier: GPL-2.0-only #
lib/bb/tests/compression.py +2 -0
@@ -1,4 +1,6 @@ # +# Copyright BitBake Contributors +# # SPDX-License-Identifier: GPL-2.0-only #
lib/bb/tests/cooker.py +2 -0
@@ -1,6 +1,8 @@ # # BitBake Tests for cooker.py # +# Copyright BitBake Contributors +# # SPDX-License-Identifier: GPL-2.0-only #
lib/bblayers/__init__.py +2 -0
@@ -1,4 +1,6 @@ # +# Copyright BitBake Contributors +# # SPDX-License-Identifier: GPL-2.0-only #
lib/bblayers/action.py +2 -0
@@ -1,4 +1,6 @@ # +# Copyright BitBake Contributors +# # SPDX-License-Identifier: GPL-2.0-only #
lib/bblayers/common.py +2 -0
@@ -1,4 +1,6 @@ # +# Copyright BitBake Contributors +# # SPDX-License-Identifier: GPL-2.0-only #
lib/bblayers/layerindex.py +2 -0
@@ -1,4 +1,6 @@ # +# Copyright BitBake Contributors +# # SPDX-License-Identifier: GPL-2.0-only #
lib/bblayers/query.py +2 -0
@@ -1,4 +1,6 @@ # +# Copyright BitBake Contributors +# # SPDX-License-Identifier: GPL-2.0-only #
lib/prserv/__init__.py +2 -0
@@ -1,4 +1,6 @@ # +# Copyright BitBake Contributors +# # SPDX-License-Identifier: GPL-2.0-only #
lib/prserv/client.py +2 -0
@@ -1,4 +1,6 @@ # +# Copyright BitBake Contributors +# # SPDX-License-Identifier: GPL-2.0-only #
lib/prserv/db.py +2 -0
@@ -1,4 +1,6 @@ # +# Copyright BitBake Contributors +# # SPDX-License-Identifier: GPL-2.0-only #
lib/prserv/serv.py +2 -0
@@ -1,4 +1,6 @@ # +# Copyright BitBake Contributors +# # SPDX-License-Identifier: GPL-2.0-only #
lib/toaster/manage.py +2 -0
@@ -1,5 +1,7 @@ #!/usr/bin/env python3 # +# Copyright BitBake Contributors +# # SPDX-License-Identifier: GPL-2.0-only #
Signature · 1 line
2.25.1
[bitbake][kirkstone][2.0][PATCH 3/8] runqueue: Drop deadlock breaking force fail
In reply to Steve SakomanFrom: Richard Purdie <[email protected]>
I'm 99% certain this failing of a scenequeue task corrupts runqueue and causes all kinds of breakage. I'd rather runqueue deadlocked than corrupted and did weird things so drop this code.
We've seen builds where the deadlock triggers and it then tries to run tasks where the SQ task already ran with very confusing failures. It is likely it is this code causing it.
--- lib/bb/runqueue.py | 3 --- 1 file changed, 3 deletions(-)
lib/bb/runqueue.py +0 -3
@@ -2257,9 +2257,6 @@ class RunQueueExecute: deferred_tid = list(self.sq_deferred.keys())[0] blocking_tid = self.sq_deferred.pop(deferred_tid) logger.warning("Runqeueue deadlocked on deferred tasks, forcing task %s blocked by %s" % (deferred_tid, blocking_tid)) - if blocking_tid not in self.runq_complete: - logger.warning("Failing blocking task %s" % (blocking_tid)) - self.sq_task_failoutright(blocking_tid) return True if self.failed_tids:
Signature · 1 line
2.25.1
[bitbake][kirkstone][2.0][PATCH 8/8] asyncrpc/client: Fix unix domain socket chdir race issues
In reply to Steve SakomanFrom: Richard Purdie <[email protected]>
The connect_unix() call had a bug where if a relative path to a socket was passed (which the non-async client always does), and the current working directory was changed after the initial call, it would fail to reconnect if it became disconnected, since the socket couldn't be found relative to the new current working directory.
To work around this, change the socket connection for UNIX domain sockets to be synchronous and change current working before connecting. This isn't ideal since the connection could block the entire event loop, but in practice this shouldn't happen since the socket are local files anyway.
Help debugging and resolving from Joshua Watt.
--- lib/bb/asyncrpc/client.py | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-)
lib/bb/asyncrpc/client.py +13 -9
@@ -31,7 +31,17 @@ class AsyncClient(object): async def connect_unix(self, path): async def connect_sock(): - return await asyncio.open_unix_connection(path) + # AF_UNIX has path length issues so chdir here to workaround + cwd = os.getcwd() + try: + os.chdir(os.path.dirname(path)) + # The socket must be opened synchronously so that CWD doesn't get + # changed out from underneath us so we pass as a sock into asyncio + sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM, 0) + sock.connect(os.path.basename(path)) + finally: + os.chdir(cwd) + return await asyncio.open_unix_connection(sock=sock) self._connect_sock = connect_sock
@@ -150,14 +160,8 @@ class Client(object): setattr(self, m, self._get_downcall_wrapper(downcall)) def connect_unix(self, path): - # AF_UNIX has path length issues so chdir here to workaround - cwd = os.getcwd() - try: - os.chdir(os.path.dirname(path)) - self.loop.run_until_complete(self.client.connect_unix(os.path.basename(path))) - self.loop.run_until_complete(self.client.connect()) - finally: - os.chdir(cwd) + self.loop.run_until_complete(self.client.connect_unix(path)) + self.loop.run_until_complete(self.client.connect()) @property def max_chunk(self):
Signature · 1 line
2.25.1
[bitbake][kirkstone][2.0][PATCH 7/8] gitsm: Error out if submodule refers to parent repo
In reply to Steve SakomanFrom: Pavel Zhukov <[email protected]>
If submodule refers to specific revision of the parent repository it causes deadlock in bitbake locking mechanism (lock is acquired to fetch the parent and cannot be released before all submodules are fetched). raise FetchError in such situation to prevent deadlocking.
[Yocto 14045]
--- lib/bb/fetch2/gitsm.py | 3 +++ 1 file changed, 3 insertions(+)
lib/bb/fetch2/gitsm.py +3 -0
@@ -115,6 +115,9 @@ class GitSM(Git): # This has to be a file reference proto = "file" url = "gitsm://" + uris[module] + if "{}{}".format(ud.host, ud.path) in url: + raise bb.fetch2.FetchError("Submodule refers to the parent repository. This will cause deadlock situation in current version of Bitbake." \ + "Consider using git fetcher instead.") url += ';protocol=%s' % proto url += ";name=%s" % module
Signature · 1 line
2.25.1