| Message ID | 20260611110523.322273-2-richard.purdie@linuxfoundation.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] lib: Drop pyinotify | expand |
Hi Richard, On 6/11/26 1:05 PM, Richard Purdie via lists.openembedded.org wrote: > Go through the remaining shell=True references in bitbake and clean them up > to use lists where possible. By using existing helpers, chdir and PATH > settings can be dropped. > > In toaster, the current branch can be obtained more easily with modern git > commands. > > Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org> > --- > lib/bb/fetch2/crate.py | 21 ++++--------------- > lib/bb/fetch2/gomod.py | 7 ++----- > lib/bb/ui/ncurses.py | 2 +- > .../bldcontrol/localhostbecontroller.py | 21 +++++++++++-------- > lib/toaster/toastermain/settings.py | 4 ++-- > 5 files changed, 21 insertions(+), 34 deletions(-) > > diff --git a/lib/bb/fetch2/crate.py b/lib/bb/fetch2/crate.py > index 8f928ea6b4d..f94e153ee3b 100644 > --- a/lib/bb/fetch2/crate.py > +++ b/lib/bb/fetch2/crate.py > @@ -17,7 +17,7 @@ import subprocess > import re > from functools import cmp_to_key > import bb > -from bb.fetch2 import logger, subprocess_setup, UnpackError > +from bb.fetch2 import logger, subprocess_setup, UnpackError, runfetchcmd > from bb.fetch2.wget import Wget > > > @@ -110,19 +110,15 @@ class Crate(Wget): > # possible metadata we need to write out > metadata = {} > > - # change to the rootdir to unpack but save the old working dir > - save_cwd = os.getcwd() > - os.chdir(rootdir) > - > bp = d.getVar('BP') > if bp == ud.parm.get('name'): > - cmd = "tar -xz --no-same-owner -f %s" % thefile > + cmd = ['tar', '-xz', '--no-same-owner', '-f', thefile] > ud.unpack_tracer.unpack("crate-extract", rootdir) > else: > cargo_bitbake = self._cargo_bitbake_path(rootdir) > ud.unpack_tracer.unpack("cargo-extract", cargo_bitbake) > > - cmd = "tar -xz --no-same-owner -f %s -C %s" % (thefile, cargo_bitbake) > + cmd = ['tar', '-xz', '--no-same-owner', '-f', thefile, '-C', cargo_bitbake] > > # ensure we've got these paths made > bb.utils.mkdirhier(cargo_bitbake) > @@ -135,17 +131,8 @@ class Crate(Wget): > metadata['files'] = {} > metadata['package'] = tarhash > > - path = d.getVar('PATH') > - if path: > - cmd = "PATH=\"%s\" %s" % (path, cmd) > bb.note("Unpacking %s to %s/" % (thefile, os.getcwd())) > - > - ret = subprocess.call(cmd, preexec_fn=subprocess_setup, shell=True) > - > - os.chdir(save_cwd) > - > - if ret != 0: > - raise UnpackError("Unpack command %s failed with return value %s" % (cmd, ret), ud.url) > + runfetchcmd(cmd, d, workdir=rootdir) > > # if we have metadata to write out.. > if len(metadata) > 0: > diff --git a/lib/bb/fetch2/gomod.py b/lib/bb/fetch2/gomod.py > index 8a64f644dfd..5cdf8f99814 100644 > --- a/lib/bb/fetch2/gomod.py > +++ b/lib/bb/fetch2/gomod.py > @@ -135,13 +135,10 @@ class GoMod(Wget): > unpackdir = os.path.dirname(unpackpath) > bb.utils.mkdirhier(unpackdir) > ud.unpack_tracer.unpack("file-copy", unpackdir) > - cmd = f"cp {ud.localpath} {unpackpath}" > - path = d.getVar('PATH') > - if path: > - cmd = f"PATH={path} {cmd}" > + cmd = ['cp', ud.localpath, unpackpath] > name = os.path.basename(unpackpath) > bb.note(f"Unpacking {name} to {unpackdir}/") > - subprocess.check_call(cmd, shell=True, preexec_fn=subprocess_setup) > + runfetchcmd(cmd, d) > > if name.endswith('.zip'): > # Unpack the go.mod file from the zip file > diff --git a/lib/bb/ui/ncurses.py b/lib/bb/ui/ncurses.py > index 18a706547aa..228fa9a6dc7 100644 > --- a/lib/bb/ui/ncurses.py > +++ b/lib/bb/ui/ncurses.py > @@ -291,7 +291,7 @@ class NCursesUI: > # bb.error("log data follows (%s)" % logfile) > # number_of_lines = data.getVar("BBINCLUDELOGS_LINES", d) > # if number_of_lines: > -# subprocess.check_call('tail -n%s %s' % (number_of_lines, logfile), shell=True) > +# subprocess.check_call(['tail', '-n' + number_of_lines, logfile]) > # else: > # f = open(logfile, "r") > # while True: > diff --git a/lib/toaster/bldcontrol/localhostbecontroller.py b/lib/toaster/bldcontrol/localhostbecontroller.py > index 50069d47a4c..dcbd9a8260d 100644 > --- a/lib/toaster/bldcontrol/localhostbecontroller.py > +++ b/lib/toaster/bldcontrol/localhostbecontroller.py > @@ -51,7 +51,10 @@ class LocalhostBEController(BuildEnvironmentController): > env=os.environ.copy() > > logger.debug("lbc_shellcmd: (%s) %s" % (cwd, command)) > - p = subprocess.Popen(command, cwd = cwd, shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE, env=env) > + if isinstance(command, str): > + p = subprocess.Popen(command, cwd = cwd, shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE, env=env) > + else: > + p = subprocess.Popen(command, cwd = cwd, stdout=subprocess.PIPE, stderr=subprocess.PIPE, env=env) > if nowait: > return > (out,err) = p.communicate() > @@ -136,7 +139,7 @@ class LocalhostBEController(BuildEnvironmentController): > cached_layers = {} > > try: > - for remotes in self._shellcmd("git remote -v", self.be.sourcedir,env=git_env).split("\n"): > + for remotes in self._shellcmd(['git', 'remote', '-v'], self.be.sourcedir, env=git_env).split("\n"): > try: > remote = remotes.split("\t")[1].split(" ")[0] > if remote not in cached_layers: > @@ -172,8 +175,7 @@ class LocalhostBEController(BuildEnvironmentController): > # see if our directory is a git repository > if os.path.exists(localdirname): > try: > - localremotes = self._shellcmd("git remote -v", > - localdirname,env=git_env) > + localremotes = self._shellcmd(['git' ,'remote', '-v'], localdirname, env=git_env) > # NOTE: this nice-to-have check breaks when using git remaping to get past firewall > # Re-enable later with .gitconfig remapping checks > #if not giturl in localremotes and commit != 'HEAD': > @@ -186,12 +188,12 @@ class LocalhostBEController(BuildEnvironmentController): > else: > if giturl in cached_layers: > logger.debug("localhostbecontroller git-copying %s to %s" % (cached_layers[giturl], localdirname)) > - self._shellcmd("git clone \"%s\" \"%s\"" % (cached_layers[giturl], localdirname),env=git_env) > - self._shellcmd("git remote remove origin", localdirname,env=git_env) > - self._shellcmd("git remote add origin \"%s\"" % giturl, localdirname,env=git_env) > + self._shellcmd(['git', 'clone', cached_layers[giturl]], localdirname], env=git_env) Isn't there a spurious square bracket after cached_layers[giturl]? (the one after localdirname is correct I believe). Cheers, Quentin > + self._shellcmd(['git', 'remote', 'remove', 'origin'], localdirname, env=git_env) > + self._shellcmd(['git', 'remote', 'add', 'origin', giturl], localdirname, env=git_env) > else: > logger.debug("localhostbecontroller: cloning %s in %s" % (giturl, localdirname)) > - self._shellcmd('git clone "%s" "%s"' % (giturl, localdirname),env=git_env) > + self._shellcmd(['git', 'clone', giturl, localdirname], env=git_env) > > # branch magic name "HEAD" will inhibit checkout > if commit != "HEAD": > @@ -199,7 +201,8 @@ class LocalhostBEController(BuildEnvironmentController): > ref = commit if re.match('^[a-fA-F0-9]+$', commit) else 'origin/%s' % commit > # DEBUGGING NOTE: this is the 'git fetch" to disable after the initial clone to > # prevent inserted debugging commands from being lost > - self._shellcmd('git fetch && git reset --hard "%s"' % ref, localdirname,env=git_env) > + self._shellcmd(['git', 'fetch'], localdirname, env=git_env) > + self._shellcmd(['git', 'reset', '--hard', ref], localdirname, env=git_env) > > # verify our repositories > for name, dirpath, index in gitrepos[(giturl, commit)]: > diff --git a/lib/toaster/toastermain/settings.py b/lib/toaster/toastermain/settings.py > index d2a449627f8..9d2dbc464ec 100644 > --- a/lib/toaster/toastermain/settings.py > +++ b/lib/toaster/toastermain/settings.py > @@ -229,8 +229,8 @@ from os.path import dirname as DN > SITE_ROOT=DN(DN(os.path.abspath(__file__))) > > import subprocess > -TOASTER_BRANCH = subprocess.Popen('git branch | grep "^* " | tr -d "* "', cwd = SITE_ROOT, shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE).communicate()[0] > -TOASTER_REVISION = subprocess.Popen('git rev-parse HEAD ', cwd = SITE_ROOT, shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE).communicate()[0] > +TOASTER_BRANCH = subprocess.check_output(['git', 'branch', '--show-current'], cwd=SITE_ROOT, text=True).strip() > +TOASTER_REVISION = subprocess.check_output(['git', 'rev-parse', 'HEAD'], cwd=SITE_ROOT, text=True).strip() > > ROOT_URLCONF = 'toastermain.urls' > > > > > -=-=-=-=-=-=-=-=-=-=-=- > Links: You receive all messages sent to this group. > View/Reply Online (#19655): https://lists.openembedded.org/g/bitbake-devel/message/19655 > Mute This Topic: https://lists.openembedded.org/mt/119754620/6293953 > Group Owner: bitbake-devel+owner@lists.openembedded.org > Unsubscribe: https://lists.openembedded.org/g/bitbake-devel/unsub [quentin.schulz@cherry.de] > -=-=-=-=-=-=-=-=-=-=-=- >
On 6/11/26 13:05, Richard Purdie via lists.openembedded.org wrote: > @@ -186,12 +188,12 @@ class LocalhostBEController(BuildEnvironmentController): > else: > if giturl in cached_layers: > logger.debug("localhostbecontroller git-copying %s to %s" % (cached_layers[giturl], localdirname)) > - self._shellcmd("git clone \"%s\" \"%s\"" % (cached_layers[giturl], localdirname),env=git_env) > - self._shellcmd("git remote remove origin", localdirname,env=git_env) > - self._shellcmd("git remote add origin \"%s\"" % giturl, localdirname,env=git_env) > + self._shellcmd(['git', 'clone', cached_layers[giturl]], localdirname], env=git_env) One ] to much. > diff --git a/lib/toaster/toastermain/settings.py b/lib/toaster/toastermain/settings.py > index d2a449627f8..9d2dbc464ec 100644 > --- a/lib/toaster/toastermain/settings.py > +++ b/lib/toaster/toastermain/settings.py > @@ -229,8 +229,8 @@ from os.path import dirname as DN > SITE_ROOT=DN(DN(os.path.abspath(__file__))) > > import subprocess > -TOASTER_BRANCH = subprocess.Popen('git branch | grep "^* " | tr -d "* "', cwd = SITE_ROOT, shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE).communicate()[0] > -TOASTER_REVISION = subprocess.Popen('git rev-parse HEAD ', cwd = SITE_ROOT, shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE).communicate()[0] > +TOASTER_BRANCH = subprocess.check_output(['git', 'branch', '--show-current'], cwd=SITE_ROOT, text=True).strip() If I am reading the correct doc version, the minimum Git version supported is 1.8.3.1 while --show-current was added in 2.22.0. > +TOASTER_REVISION = subprocess.check_output(['git', 'rev-parse', 'HEAD'], cwd=SITE_ROOT, text=True).strip() > > ROOT_URLCONF = 'toastermain.urls' Best regards, Anders Heimer
On Thu, 2026-06-11 at 14:18 +0200, Anders Heimer wrote: > On 6/11/26 13:05, Richard Purdie via lists.openembedded.org wrote: > > @@ -186,12 +188,12 @@ class LocalhostBEController(BuildEnvironmentController): > > else: > > if giturl in cached_layers: > > logger.debug("localhostbecontroller git-copying %s to %s" % (cached_layers[giturl], localdirname)) > > - self._shellcmd("git clone \"%s\" \"%s\"" % (cached_layers[giturl], localdirname),env=git_env) > > - self._shellcmd("git remote remove origin", localdirname,env=git_env) > > - self._shellcmd("git remote add origin \"%s\"" % giturl, localdirname,env=git_env) > > + self._shellcmd(['git', 'clone', cached_layers[giturl]], localdirname], env=git_env) > One ] to much. Thanks (and Quentin!). I'll tweak that. > > diff --git a/lib/toaster/toastermain/settings.py b/lib/toaster/toastermain/settings.py > > index d2a449627f8..9d2dbc464ec 100644 > > --- a/lib/toaster/toastermain/settings.py > > +++ b/lib/toaster/toastermain/settings.py > > @@ -229,8 +229,8 @@ from os.path import dirname as DN > > SITE_ROOT=DN(DN(os.path.abspath(__file__))) > > > > import subprocess > > -TOASTER_BRANCH = subprocess.Popen('git branch | grep "^* " | tr -d "* "', cwd = SITE_ROOT, shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE).communicate()[0] > > -TOASTER_REVISION = subprocess.Popen('git rev-parse HEAD ', cwd = SITE_ROOT, shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE).communicate()[0] > > +TOASTER_BRANCH = subprocess.check_output(['git', 'branch', '--show-current'], cwd=SITE_ROOT, text=True).strip() > If I am reading the correct doc version, the minimum Git version > supported is 1.8.3.1 while --show-current was added in 2.22.0. > 2.22 was released in June 2019 so at this point I think it is fine to rely on that. Before I added that code, I did check which versions our 'supported' distros have and we're fine there. So we can update the minimum version, I don't think it has changed in many years. Cheers, Richard
diff --git a/lib/bb/fetch2/crate.py b/lib/bb/fetch2/crate.py index 8f928ea6b4d..f94e153ee3b 100644 --- a/lib/bb/fetch2/crate.py +++ b/lib/bb/fetch2/crate.py @@ -17,7 +17,7 @@ import subprocess import re from functools import cmp_to_key import bb -from bb.fetch2 import logger, subprocess_setup, UnpackError +from bb.fetch2 import logger, subprocess_setup, UnpackError, runfetchcmd from bb.fetch2.wget import Wget @@ -110,19 +110,15 @@ class Crate(Wget): # possible metadata we need to write out metadata = {} - # change to the rootdir to unpack but save the old working dir - save_cwd = os.getcwd() - os.chdir(rootdir) - bp = d.getVar('BP') if bp == ud.parm.get('name'): - cmd = "tar -xz --no-same-owner -f %s" % thefile + cmd = ['tar', '-xz', '--no-same-owner', '-f', thefile] ud.unpack_tracer.unpack("crate-extract", rootdir) else: cargo_bitbake = self._cargo_bitbake_path(rootdir) ud.unpack_tracer.unpack("cargo-extract", cargo_bitbake) - cmd = "tar -xz --no-same-owner -f %s -C %s" % (thefile, cargo_bitbake) + cmd = ['tar', '-xz', '--no-same-owner', '-f', thefile, '-C', cargo_bitbake] # ensure we've got these paths made bb.utils.mkdirhier(cargo_bitbake) @@ -135,17 +131,8 @@ class Crate(Wget): metadata['files'] = {} metadata['package'] = tarhash - path = d.getVar('PATH') - if path: - cmd = "PATH=\"%s\" %s" % (path, cmd) bb.note("Unpacking %s to %s/" % (thefile, os.getcwd())) - - ret = subprocess.call(cmd, preexec_fn=subprocess_setup, shell=True) - - os.chdir(save_cwd) - - if ret != 0: - raise UnpackError("Unpack command %s failed with return value %s" % (cmd, ret), ud.url) + runfetchcmd(cmd, d, workdir=rootdir) # if we have metadata to write out.. if len(metadata) > 0: diff --git a/lib/bb/fetch2/gomod.py b/lib/bb/fetch2/gomod.py index 8a64f644dfd..5cdf8f99814 100644 --- a/lib/bb/fetch2/gomod.py +++ b/lib/bb/fetch2/gomod.py @@ -135,13 +135,10 @@ class GoMod(Wget): unpackdir = os.path.dirname(unpackpath) bb.utils.mkdirhier(unpackdir) ud.unpack_tracer.unpack("file-copy", unpackdir) - cmd = f"cp {ud.localpath} {unpackpath}" - path = d.getVar('PATH') - if path: - cmd = f"PATH={path} {cmd}" + cmd = ['cp', ud.localpath, unpackpath] name = os.path.basename(unpackpath) bb.note(f"Unpacking {name} to {unpackdir}/") - subprocess.check_call(cmd, shell=True, preexec_fn=subprocess_setup) + runfetchcmd(cmd, d) if name.endswith('.zip'): # Unpack the go.mod file from the zip file diff --git a/lib/bb/ui/ncurses.py b/lib/bb/ui/ncurses.py index 18a706547aa..228fa9a6dc7 100644 --- a/lib/bb/ui/ncurses.py +++ b/lib/bb/ui/ncurses.py @@ -291,7 +291,7 @@ class NCursesUI: # bb.error("log data follows (%s)" % logfile) # number_of_lines = data.getVar("BBINCLUDELOGS_LINES", d) # if number_of_lines: -# subprocess.check_call('tail -n%s %s' % (number_of_lines, logfile), shell=True) +# subprocess.check_call(['tail', '-n' + number_of_lines, logfile]) # else: # f = open(logfile, "r") # while True: diff --git a/lib/toaster/bldcontrol/localhostbecontroller.py b/lib/toaster/bldcontrol/localhostbecontroller.py index 50069d47a4c..dcbd9a8260d 100644 --- a/lib/toaster/bldcontrol/localhostbecontroller.py +++ b/lib/toaster/bldcontrol/localhostbecontroller.py @@ -51,7 +51,10 @@ class LocalhostBEController(BuildEnvironmentController): env=os.environ.copy() logger.debug("lbc_shellcmd: (%s) %s" % (cwd, command)) - p = subprocess.Popen(command, cwd = cwd, shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE, env=env) + if isinstance(command, str): + p = subprocess.Popen(command, cwd = cwd, shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE, env=env) + else: + p = subprocess.Popen(command, cwd = cwd, stdout=subprocess.PIPE, stderr=subprocess.PIPE, env=env) if nowait: return (out,err) = p.communicate() @@ -136,7 +139,7 @@ class LocalhostBEController(BuildEnvironmentController): cached_layers = {} try: - for remotes in self._shellcmd("git remote -v", self.be.sourcedir,env=git_env).split("\n"): + for remotes in self._shellcmd(['git', 'remote', '-v'], self.be.sourcedir, env=git_env).split("\n"): try: remote = remotes.split("\t")[1].split(" ")[0] if remote not in cached_layers: @@ -172,8 +175,7 @@ class LocalhostBEController(BuildEnvironmentController): # see if our directory is a git repository if os.path.exists(localdirname): try: - localremotes = self._shellcmd("git remote -v", - localdirname,env=git_env) + localremotes = self._shellcmd(['git' ,'remote', '-v'], localdirname, env=git_env) # NOTE: this nice-to-have check breaks when using git remaping to get past firewall # Re-enable later with .gitconfig remapping checks #if not giturl in localremotes and commit != 'HEAD': @@ -186,12 +188,12 @@ class LocalhostBEController(BuildEnvironmentController): else: if giturl in cached_layers: logger.debug("localhostbecontroller git-copying %s to %s" % (cached_layers[giturl], localdirname)) - self._shellcmd("git clone \"%s\" \"%s\"" % (cached_layers[giturl], localdirname),env=git_env) - self._shellcmd("git remote remove origin", localdirname,env=git_env) - self._shellcmd("git remote add origin \"%s\"" % giturl, localdirname,env=git_env) + self._shellcmd(['git', 'clone', cached_layers[giturl]], localdirname], env=git_env) + self._shellcmd(['git', 'remote', 'remove', 'origin'], localdirname, env=git_env) + self._shellcmd(['git', 'remote', 'add', 'origin', giturl], localdirname, env=git_env) else: logger.debug("localhostbecontroller: cloning %s in %s" % (giturl, localdirname)) - self._shellcmd('git clone "%s" "%s"' % (giturl, localdirname),env=git_env) + self._shellcmd(['git', 'clone', giturl, localdirname], env=git_env) # branch magic name "HEAD" will inhibit checkout if commit != "HEAD": @@ -199,7 +201,8 @@ class LocalhostBEController(BuildEnvironmentController): ref = commit if re.match('^[a-fA-F0-9]+$', commit) else 'origin/%s' % commit # DEBUGGING NOTE: this is the 'git fetch" to disable after the initial clone to # prevent inserted debugging commands from being lost - self._shellcmd('git fetch && git reset --hard "%s"' % ref, localdirname,env=git_env) + self._shellcmd(['git', 'fetch'], localdirname, env=git_env) + self._shellcmd(['git', 'reset', '--hard', ref], localdirname, env=git_env) # verify our repositories for name, dirpath, index in gitrepos[(giturl, commit)]: diff --git a/lib/toaster/toastermain/settings.py b/lib/toaster/toastermain/settings.py index d2a449627f8..9d2dbc464ec 100644 --- a/lib/toaster/toastermain/settings.py +++ b/lib/toaster/toastermain/settings.py @@ -229,8 +229,8 @@ from os.path import dirname as DN SITE_ROOT=DN(DN(os.path.abspath(__file__))) import subprocess -TOASTER_BRANCH = subprocess.Popen('git branch | grep "^* " | tr -d "* "', cwd = SITE_ROOT, shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE).communicate()[0] -TOASTER_REVISION = subprocess.Popen('git rev-parse HEAD ', cwd = SITE_ROOT, shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE).communicate()[0] +TOASTER_BRANCH = subprocess.check_output(['git', 'branch', '--show-current'], cwd=SITE_ROOT, text=True).strip() +TOASTER_REVISION = subprocess.check_output(['git', 'rev-parse', 'HEAD'], cwd=SITE_ROOT, text=True).strip() ROOT_URLCONF = 'toastermain.urls'
Go through the remaining shell=True references in bitbake and clean them up to use lists where possible. By using existing helpers, chdir and PATH settings can be dropped. In toaster, the current branch can be obtained more easily with modern git commands. Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org> --- lib/bb/fetch2/crate.py | 21 ++++--------------- lib/bb/fetch2/gomod.py | 7 ++----- lib/bb/ui/ncurses.py | 2 +- .../bldcontrol/localhostbecontroller.py | 21 +++++++++++-------- lib/toaster/toastermain/settings.py | 4 ++-- 5 files changed, 21 insertions(+), 34 deletions(-)