[1.50][PATCH 0/9] Review request

[1.50][PATCH 0/9] Review request

[Anuj Mittal]

Please review these changes for 1.50/hardknott.

Thanks,

Anuj

The following changes since commit bcd4285116ea4990f10d53698e0a81ae1e7ce24c:

  test/fetch: Update urls to match upstream branch name changes (2021-10-19 22:27:00 +0100)

are available in the Git repository at:

  git://push.openembedded.org/bitbake-contrib stable/1.50-next

Caner Altinbasak (1):
  npmsw: Avoid race condition with multiple npm fetchers

Richard Purdie (7):
  fetch2/git: Avoid races over mirror tarball creation
  fetch2/git: Use os.rename instead of mv
  tests/runqueue: Ensure hashserv exits before deleting files
  bitbake-worker: Allow shutdown/database flush of pseudo server at task
    exit
  bitbake-worker: Handle pseudo shutdown in Ctrl+C case
  build: Make exception printing clearer
  build: Ensure python stdout/stderr is logged correctly

Ross Burton (1):
  fetch2: clarify the command-no-found error message

 bin/bitbake-worker        | 12 +++++++++++-
 lib/bb/build.py           |  6 +++++-
 lib/bb/fetch2/__init__.py |  2 +-
 lib/bb/fetch2/git.py      | 21 +++++++++++++++++++--
 lib/bb/fetch2/npmsw.py    |  4 ++++
 lib/bb/tests/runqueue.py  |  2 +-
 6 files changed, 41 insertions(+), 6 deletions(-)

-- 
2.31.1

[1.50][PATCH 1/9] fetch2/git: Avoid races over mirror tarball creation

[Anuj Mittal]

From: Richard Purdie <richard.purdie@linuxfoundation.org>

There is a potential race over the mirror tarballs where a partial git repo
could be extracted causing fetcher failures if the tarball is being rewritten
whilst another build accesses it.

Create the mirror tarball atomically to avoid this.

[YOCTO #14441]

Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
(cherry picked from commit 3250bc950c56bd7dd2114df26e5a8e13b04ceac8)
Signed-off-by: Anuj Mittal <anuj.mittal@intel.com>
---
 lib/bb/fetch2/git.py | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/lib/bb/fetch2/git.py b/lib/bb/fetch2/git.py
index cf7424ebf..95cd971d5 100644
--- a/lib/bb/fetch2/git.py
+++ b/lib/bb/fetch2/git.py
@@ -68,6 +68,7 @@ import subprocess
 import tempfile
 import bb
 import bb.progress
+from contextlib import contextmanager
 from   bb.fetch2 import FetchMethod
 from   bb.fetch2 import runfetchcmd
 from   bb.fetch2 import logger
@@ -414,6 +415,20 @@ class Git(FetchMethod):
                 bb.utils.remove(tmpdir, recurse=True)
 
     def build_mirror_data(self, ud, d):
+
+        # Create as a temp file and move atomically into position to avoid races
+        @contextmanager
+        def create_atomic(filename, d):
+            fd, tfile = tempfile.mkstemp(dir=os.path.dirname(filename))
+            try:
+                yield tfile
+                umask = os.umask(0o666)
+                os.umask(umask)
+                os.chmod(tfile, (0o666 & ~umask))
+                runfetchcmd("mv %s %s" % (tfile, filename), d)
+            finally:
+                os.close(fd)
+
         if ud.shallow and ud.write_shallow_tarballs:
             if not os.path.exists(ud.fullshallow):
                 if os.path.islink(ud.fullshallow):
@@ -424,7 +439,8 @@ class Git(FetchMethod):
                     self.clone_shallow_local(ud, shallowclone, d)
 
                     logger.info("Creating tarball of git repository")
-                    runfetchcmd("tar -czf %s ." % ud.fullshallow, d, workdir=shallowclone)
+                    with create_atomic(ud.fullshallow, d) as tfile:
+                        runfetchcmd("tar -czf %s ." % tfile, d, workdir=shallowclone)
                     runfetchcmd("touch %s.done" % ud.fullshallow, d)
                 finally:
                     bb.utils.remove(tempdir, recurse=True)
@@ -433,7 +449,8 @@ class Git(FetchMethod):
                 os.unlink(ud.fullmirror)
 
             logger.info("Creating tarball of git repository")
-            runfetchcmd("tar -czf %s ." % ud.fullmirror, d, workdir=ud.clonedir)
+            with create_atomic(ud.fullmirror, d) as tfile:
+                runfetchcmd("tar -czf %s ." % tfile, d, workdir=ud.clonedir)
             runfetchcmd("touch %s.done" % ud.fullmirror, d)
 
     def clone_shallow_local(self, ud, dest, d):
-- 
2.31.1

[1.50][PATCH 2/9] fetch2/git: Use os.rename instead of mv

[Anuj Mittal]

From: Richard Purdie <richard.purdie@linuxfoundation.org>

os.rename will overwrite the destination file if present so we can use this
instead of the process call overhead.

Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
(cherry picked from commit b3cccaa6a896c41d8c9be5eebc327f726542d16b)
Signed-off-by: Anuj Mittal <anuj.mittal@intel.com>
---
 lib/bb/fetch2/git.py | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/bb/fetch2/git.py b/lib/bb/fetch2/git.py
index 95cd971d5..00105183b 100644
--- a/lib/bb/fetch2/git.py
+++ b/lib/bb/fetch2/git.py
@@ -418,14 +418,14 @@ class Git(FetchMethod):
 
         # Create as a temp file and move atomically into position to avoid races
         @contextmanager
-        def create_atomic(filename, d):
+        def create_atomic(filename):
             fd, tfile = tempfile.mkstemp(dir=os.path.dirname(filename))
             try:
                 yield tfile
                 umask = os.umask(0o666)
                 os.umask(umask)
                 os.chmod(tfile, (0o666 & ~umask))
-                runfetchcmd("mv %s %s" % (tfile, filename), d)
+                os.rename(tfile, filename)
             finally:
                 os.close(fd)
 
@@ -439,7 +439,7 @@ class Git(FetchMethod):
                     self.clone_shallow_local(ud, shallowclone, d)
 
                     logger.info("Creating tarball of git repository")
-                    with create_atomic(ud.fullshallow, d) as tfile:
+                    with create_atomic(ud.fullshallow) as tfile:
                         runfetchcmd("tar -czf %s ." % tfile, d, workdir=shallowclone)
                     runfetchcmd("touch %s.done" % ud.fullshallow, d)
                 finally:
@@ -449,7 +449,7 @@ class Git(FetchMethod):
                 os.unlink(ud.fullmirror)
 
             logger.info("Creating tarball of git repository")
-            with create_atomic(ud.fullmirror, d) as tfile:
+            with create_atomic(ud.fullmirror) as tfile:
                 runfetchcmd("tar -czf %s ." % tfile, d, workdir=ud.clonedir)
             runfetchcmd("touch %s.done" % ud.fullmirror, d)
 
-- 
2.31.1

[1.50][PATCH 3/9] tests/runqueue: Ensure hashserv exits before deleting files

[Anuj Mittal]

From: Richard Purdie <richard.purdie@linuxfoundation.org>

We've seen races where the socket may be gone but the server is still writing
out it's database. Handle that case too to avoid cleanup tracebacks.

[YOCTO #14440]

Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
(cherry picked from commit b9e4fb843cb9d3a4d4404af093a781fab5520465)
Signed-off-by: Anuj Mittal <anuj.mittal@intel.com>
---
 lib/bb/tests/runqueue.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/bb/tests/runqueue.py b/lib/bb/tests/runqueue.py
index 3d51779d6..4f335b8f1 100644
--- a/lib/bb/tests/runqueue.py
+++ b/lib/bb/tests/runqueue.py
@@ -361,7 +361,7 @@ class RunQueueTests(unittest.TestCase):
 
     def shutdown(self, tempdir):
         # Wait for the hashserve socket to disappear else we'll see races with the tempdir cleanup
-        while os.path.exists(tempdir + "/hashserve.sock"):
+        while (os.path.exists(tempdir + "/hashserve.sock") or os.path.exists(tempdir + "cache/hashserv.db-wal")):
             time.sleep(0.5)
 
 
-- 
2.31.1

[1.50][PATCH 4/9] npmsw: Avoid race condition with multiple npm fetchers

[Anuj Mittal]

From: Caner Altinbasak <cal@brightsign.biz>

If multiple npmsw fetchers are trying to download the same npm file, one of them
can try to download the file while other is calling verify. npmsw methods gets
called without holding the lock, which causes race conditions in fetching and
verification etc. Lock the lockfile before calling proxy fetcher methods.

Signed-off-by: Caner Altinbasak <cal@brightsign.biz>
Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
(cherry picked from commit fa39e6689d0f0fff772e1c81682698f4b1587b8a)
Signed-off-by: Anuj Mittal <anuj.mittal@intel.com>
---
 lib/bb/fetch2/npmsw.py | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lib/bb/fetch2/npmsw.py b/lib/bb/fetch2/npmsw.py
index 0c3511d8a..fdecbc6db 100644
--- a/lib/bb/fetch2/npmsw.py
+++ b/lib/bb/fetch2/npmsw.py
@@ -29,6 +29,8 @@ from bb.fetch2.npm import npm_integrity
 from bb.fetch2.npm import npm_localfile
 from bb.fetch2.npm import npm_unpack
 from bb.utils import is_semver
+from bb.utils import lockfile
+from bb.utils import unlockfile
 
 def foreach_dependencies(shrinkwrap, callback=None, dev=False):
     """
@@ -187,7 +189,9 @@ class NpmShrinkWrap(FetchMethod):
             proxy_ud = ud.proxy.ud[proxy_url]
             proxy_d = ud.proxy.d
             proxy_ud.setup_localpath(proxy_d)
+            lf = lockfile(proxy_ud.lockfile)
             returns.append(handle(proxy_ud.method, proxy_ud, proxy_d))
+            unlockfile(lf)
         return returns
 
     def verify_donestamp(self, ud, d):
-- 
2.31.1

[1.50][PATCH 5/9] bitbake-worker: Allow shutdown/database flush of pseudo server at task exit

[Anuj Mittal]

From: Richard Purdie <richard.purdie@linuxfoundation.org>

We have a problem where pseudo server processes exist after bitbake exits
and hold the pseudo database in memory. In a docker container, the processes
will be killed as the container is destroyed with no warning and no opportunity
to write the data to disk. This leads to permissions/inode corruptions and
data loss.

Send a shutdown message to pseudo which in new versions of pseudo will flush
the database, thereby fixing some of the issues people using docker containers
see.

Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
(cherry picked from commit a07a971b40acd3eee12e203d2cfa3e49f56109f6)
Signed-off-by: Anuj Mittal <anuj.mittal@intel.com>
---
 bin/bitbake-worker | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/bin/bitbake-worker b/bin/bitbake-worker
index 3ca8c1853..b3877b15c 100755
--- a/bin/bitbake-worker
+++ b/bin/bitbake-worker
@@ -16,6 +16,8 @@ import signal
 import pickle
 import traceback
 import queue
+import shlex
+import subprocess
 from multiprocessing import Lock
 from threading import Thread
 
@@ -145,6 +147,7 @@ def fork_off_task(cfg, data, databuilder, workerdata, fn, task, taskname, taskha
     # a fork() or exec*() activates PSEUDO...
 
     envbackup = {}
+    fakeroot = False
     fakeenv = {}
     umask = None
 
@@ -164,6 +167,7 @@ def fork_off_task(cfg, data, databuilder, workerdata, fn, task, taskname, taskha
 
     # We can't use the fakeroot environment in a dry run as it possibly hasn't been built
     if 'fakeroot' in taskdep and taskname in taskdep['fakeroot'] and not dry_run:
+        fakeroot = True
         envvars = (workerdata["fakerootenv"][fn] or "").split()
         for key, value in (var.split('=') for var in envvars):
             envbackup[key] = os.environ.get(key)
@@ -282,7 +286,11 @@ def fork_off_task(cfg, data, databuilder, workerdata, fn, task, taskname, taskha
             try:
                 if dry_run:
                     return 0
-                return bb.build.exec_task(fn, taskname, the_data, cfg.profile)
+                ret = bb.build.exec_task(fn, taskname, the_data, cfg.profile)
+                if fakeroot:
+                    fakerootcmd = shlex.split(the_data.getVar("FAKEROOTCMD"))
+                    subprocess.run(fakerootcmd + ['-S'], check=True, stdout=subprocess.PIPE)
+                return ret
             except:
                 os._exit(1)
         if not profiling:
-- 
2.31.1

[1.50][PATCH 6/9] bitbake-worker: Handle pseudo shutdown in Ctrl+C case

[Anuj Mittal]

From: Richard Purdie <richard.purdie@linuxfoundation.org>

If the build is interrupted, handle the shutdown of pseudo even in this
case to avoid data corruption inside docker containers.

[YOCTO #14555]

Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
(cherry picked from commit a2a04c6fe94bc56efcff299c669a151746e35916)
Signed-off-by: Anuj Mittal <anuj.mittal@intel.com>
---
 bin/bitbake-worker | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/bin/bitbake-worker b/bin/bitbake-worker
index b3877b15c..4318ce611 100755
--- a/bin/bitbake-worker
+++ b/bin/bitbake-worker
@@ -286,10 +286,12 @@ def fork_off_task(cfg, data, databuilder, workerdata, fn, task, taskname, taskha
             try:
                 if dry_run:
                     return 0
-                ret = bb.build.exec_task(fn, taskname, the_data, cfg.profile)
-                if fakeroot:
-                    fakerootcmd = shlex.split(the_data.getVar("FAKEROOTCMD"))
-                    subprocess.run(fakerootcmd + ['-S'], check=True, stdout=subprocess.PIPE)
+                try:
+                    ret = bb.build.exec_task(fn, taskname, the_data, cfg.profile)
+                finally:
+                    if fakeroot:
+                        fakerootcmd = shlex.split(the_data.getVar("FAKEROOTCMD"))
+                        subprocess.run(fakerootcmd + ['-S'], check=True, stdout=subprocess.PIPE)
                 return ret
             except:
                 os._exit(1)
-- 
2.31.1

[1.50][PATCH 7/9] fetch2: clarify the command-no-found error message

[Anuj Mittal]

From: Ross Burton <ross@burtonini.com>

If runfetchcmd() fails with bb.process.NotFoundError, the message output
is simply "Fetch command" which doesn't really explain what the problem
is.

Add "not found" to clarify what happened.

Signed-off-by: Ross Burton <ross.burton@arm.com>
Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
(cherry picked from commit 8de9dc02ed6a73b47f2ab10be30d1aed7954bc72)
Signed-off-by: Anuj Mittal <anuj.mittal@intel.com>
---
 lib/bb/fetch2/__init__.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/bb/fetch2/__init__.py b/lib/bb/fetch2/__init__.py
index c8e91262a..dbf8b50e6 100644
--- a/lib/bb/fetch2/__init__.py
+++ b/lib/bb/fetch2/__init__.py
@@ -871,7 +871,7 @@ def runfetchcmd(cmd, d, quiet=False, cleanup=None, log=None, workdir=None):
         (output, errors) = bb.process.run(cmd, log=log, shell=True, stderr=subprocess.PIPE, cwd=workdir)
         success = True
     except bb.process.NotFoundError as e:
-        error_message = "Fetch command %s" % (e.command)
+        error_message = "Fetch command %s not found" % (e.command)
     except bb.process.ExecutionError as e:
         if e.stdout:
             output = "output:\n%s\n%s" % (e.stdout, e.stderr)
-- 
2.31.1

[1.50][PATCH 8/9] build: Make exception printing clearer

[Anuj Mittal]

From: Richard Purdie <richard.purdie@linuxfoundation.org>

Shows:

ERROR: SystemExit(1)

instead of:

ERROR: 1

Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
(cherry picked from commit df89e37c33e4398a5f8ece9a8b973be3fe2ff361)
Signed-off-by: Anuj Mittal <anuj.mittal@intel.com>
---
 lib/bb/build.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/bb/build.py b/lib/bb/build.py
index ad031cc70..85ffd5361 100644
--- a/lib/bb/build.py
+++ b/lib/bb/build.py
@@ -703,7 +703,7 @@ def _exec_task(fn, task, d, quieterr):
                 # logs once already so don't duplicate
                 if verboseStdoutLogging:
                     errprinted = True
-                logger.error(str(exc))
+                logger.error(repr(exc))
                 event.fire(TaskFailed(task, fn, logfn, localdata, errprinted), localdata)
             return 1
     finally:
-- 
2.31.1

[1.50][PATCH 9/9] build: Ensure python stdout/stderr is logged correctly

[Anuj Mittal]

From: Richard Purdie <richard.purdie@linuxfoundation.org>

Currently we see things like:

Log data follows:
| DEBUG: Executing python function do_pythontest_exit
| DEBUG: Python function do_pythontest_exit finished
| ERROR: 1
| This is python stdout

Whilst after the change we see things like:

Log data follows:
| DEBUG: Executing python function do_pythontest_exit
| This is python stdout
| DEBUG: Python function do_pythontest_exit finished
| ERROR: 1

since the output is now correctly mixed with the log messages. In some cases the logging
tests indicate the output is being lost entirely which is bad for debugging and makes
things rather confusing.

Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
(cherry picked from commit 8966b43761500e0505333d8c9a3f0f2c3dbe7559)
Signed-off-by: Anuj Mittal <anuj.mittal@intel.com>
---
 lib/bb/build.py | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lib/bb/build.py b/lib/bb/build.py
index 85ffd5361..163572be8 100644
--- a/lib/bb/build.py
+++ b/lib/bb/build.py
@@ -298,6 +298,10 @@ def exec_func_python(func, d, runfile, cwd=None):
         comp = utils.better_compile(code, func, "exec_python_func() autogenerated")
         utils.better_exec(comp, {"d": d}, code, "exec_python_func() autogenerated")
     finally:
+        # We want any stdout/stderr to be printed before any other log messages to make debugging
+        # more accurate. In some cases we seem to lose stdout/stderr entirely in logging tests without this.
+        sys.stdout.flush()
+        sys.stderr.flush()
         bb.debug(2, "Python function %s finished" % func)
 
         if cwd and olddir:
-- 
2.31.1