[PATCH 0/7] De-globalization of 'devtool'

[PATCH 0/7] De-globalization of 'devtool'

[chris.laplante]

From: Chris Laplante <chris.laplante@agilent.com>

This patchset removes global variables from the 'devtool' script. It
also changes `Context` to be a dataclass.

All devtool self-tests pass, tested via: oe-selftest -r devtool

Chris Laplante (7):
  devtool: un-globalize the 'basepath' variable
  devtool: un-globalize 'workspace' variable
  devtool: un-globalize 'context' variable and convert it to a dataclass
  devtool: un-globalize 'config' variable
  devtool: un-globalize 'plugins' variable
  devtool: misc cleanups
  devtool: remove unused 'config' param from '_create_workspace' method

 scripts/devtool | 88 +++++++++++++++++++++++++------------------------
 1 file changed, 45 insertions(+), 43 deletions(-)

--
2.43.0

[PATCH 1/7] devtool: un-globalize the 'basepath' variable

[chris.laplante]

From: Chris Laplante <chris.laplante@agilent.com>

Signed-off-by: Chris Laplante <chris.laplante@agilent.com>
---
 scripts/devtool | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/scripts/devtool b/scripts/devtool
index 60ea3e8298..acc4e0e982 100755
--- a/scripts/devtool
+++ b/scripts/devtool
@@ -13,10 +13,8 @@ import argparse
 import glob
 import re
 import configparser
-import subprocess
 import logging
 
-basepath = ''
 workspace = {}
 config = None
 context = None
@@ -33,13 +31,15 @@ logger = scriptutils.logger_create('devtool')
 plugins = []
 
 
-class ConfigHandler(object):
+class ConfigHandler:
+    basepath = None
     config_file = ''
     config_obj = None
     init_path = ''
     workspace_path = ''
 
-    def __init__(self, filename):
+    def __init__(self, basepath, filename):
+        self.basepath = basepath
         self.config_file = filename
         self.config_obj = configparser.ConfigParser()
 
@@ -59,14 +59,14 @@ class ConfigHandler(object):
 
             if self.config_obj.has_option('General', 'init_path'):
                 pth = self.get('General', 'init_path')
-                self.init_path = os.path.join(basepath, pth)
+                self.init_path = os.path.join(self.basepath, pth)
                 if not os.path.exists(self.init_path):
                     logger.error('init_path %s specified in config file cannot be found' % pth)
                     return False
         else:
             self.config_obj.add_section('General')
 
-        self.workspace_path = self.get('General', 'workspace_path', os.path.join(basepath, 'workspace'))
+        self.workspace_path = self.get('General', 'workspace_path', os.path.join(self.basepath, 'workspace'))
         return True
 
 
@@ -86,7 +86,7 @@ class Context:
         self.__dict__.update(kwargs)
 
 
-def read_workspace():
+def read_workspace(basepath):
     global workspace
     workspace = {}
     if not os.path.exists(os.path.join(config.workspace_path, 'conf', 'layer.conf')):
@@ -209,7 +209,6 @@ def _enable_workspace_layer(workspacedir, config, basepath):
 
 
 def main():
-    global basepath
     global config
     global context
 
@@ -264,7 +263,7 @@ def main():
 
     logger.debug('Using basepath %s' % basepath)
 
-    config = ConfigHandler(os.path.join(basepath, 'conf', 'devtool.conf'))
+    config = ConfigHandler(basepath, os.path.join(basepath, 'conf', 'devtool.conf'))
     if not config.read():
         return -1
     context.config = config
@@ -332,7 +331,7 @@ def main():
 
     try:
         if not getattr(args, 'no_workspace', False):
-            read_workspace()
+            read_workspace(basepath)
 
         ret = args.func(args, config, basepath, workspace)
     except DevtoolError as err:
-- 
2.43.0

[PATCH 2/7] devtool: un-globalize 'workspace' variable

[chris.laplante]

From: Chris Laplante <chris.laplante@agilent.com>

Signed-off-by: Chris Laplante <chris.laplante@agilent.com>
---
 scripts/devtool | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/scripts/devtool b/scripts/devtool
index acc4e0e982..d7a5903c9f 100755
--- a/scripts/devtool
+++ b/scripts/devtool
@@ -15,7 +15,6 @@ import re
 import configparser
 import logging
 
-workspace = {}
 config = None
 context = None
 
@@ -87,7 +86,6 @@ class Context:
 
 
 def read_workspace(basepath):
-    global workspace
     workspace = {}
     if not os.path.exists(os.path.join(config.workspace_path, 'conf', 'layer.conf')):
         if context.fixed_setup:
@@ -132,6 +130,8 @@ def read_workspace(basepath):
                 logger.debug('Found recipe %s' % pnvalues)
                 workspace[pn] = pnvalues
 
+    return workspace
+
 def create_workspace(args, config, basepath, workspace):
     if args.layerpath:
         workspacedir = os.path.abspath(args.layerpath)
@@ -330,9 +330,9 @@ def main():
     args = parser.parse_args(unparsed_args, namespace=global_args)
 
     try:
+        workspace = {}
         if not getattr(args, 'no_workspace', False):
-            read_workspace(basepath)
-
+            workspace = read_workspace(basepath)
         ret = args.func(args, config, basepath, workspace)
     except DevtoolError as err:
         if str(err):
-- 
2.43.0

[PATCH 3/7] devtool: un-globalize 'context' variable and convert it to a dataclass

[chris.laplante]

From: Chris Laplante <chris.laplante@agilent.com>

Please excuse the usage of 'typing' slipping in here - it's just how
dataclasses work :/.

Signed-off-by: Chris Laplante <chris.laplante@agilent.com>
---
 scripts/devtool | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/scripts/devtool b/scripts/devtool
index d7a5903c9f..1ace6fb035 100755
--- a/scripts/devtool
+++ b/scripts/devtool
@@ -7,6 +7,7 @@
 # SPDX-License-Identifier: GPL-2.0-only
 #
 
+import dataclasses
 import sys
 import os
 import argparse
@@ -15,8 +16,10 @@ import re
 import configparser
 import logging
 
+# This can be removed once our minimum is Python 3.9: https://docs.python.org/3/whatsnew/3.9.html#type-hinting-generics-in-standard-collections
+from typing import List
+
 config = None
-context = None
 
 
 scripts_path = os.path.dirname(os.path.realpath(__file__))
@@ -80,12 +83,15 @@ class ConfigHandler:
             self.config_obj.add_section(section)
         self.config_obj.set(section, option, value)
 
+
+@dataclasses.dataclass
 class Context:
-    def __init__(self, **kwargs):
-        self.__dict__.update(kwargs)
+    fixed_setup: bool
+    config: ConfigHandler
+    pluginpaths: List[str]
 
 
-def read_workspace(basepath):
+def read_workspace(basepath, context):
     workspace = {}
     if not os.path.exists(os.path.join(config.workspace_path, 'conf', 'layer.conf')):
         if context.fixed_setup:
@@ -210,13 +216,10 @@ def _enable_workspace_layer(workspacedir, config, basepath):
 
 def main():
     global config
-    global context
 
     if sys.getfilesystemencoding() != "utf-8":
         sys.exit("Please use a locale setting which supports utf-8.\nPython can't change the filesystem locale after loading so we need a utf-8 when python starts or things won't work.")
 
-    context = Context(fixed_setup=False)
-
     # Default basepath
     basepath = os.path.dirname(os.path.abspath(__file__))
 
@@ -241,21 +244,23 @@ def main():
     elif global_args.quiet:
         logger.setLevel(logging.ERROR)
 
+    is_fixed_setup = False
+
     if global_args.basepath:
         # Override
         basepath = global_args.basepath
         if os.path.exists(os.path.join(basepath, '.devtoolbase')):
-            context.fixed_setup = True
+            is_fixed_setup = True
     else:
         pth = basepath
         while pth != '' and pth != os.sep:
             if os.path.exists(os.path.join(pth, '.devtoolbase')):
-                context.fixed_setup = True
+                is_fixed_setup = True
                 basepath = pth
                 break
             pth = os.path.dirname(pth)
 
-        if not context.fixed_setup:
+        if not is_fixed_setup:
             basepath = os.environ.get('BUILDDIR')
             if not basepath:
                 logger.error("This script can only be run after initialising the build environment (e.g. by using oe-init-build-env)")
@@ -266,7 +271,6 @@ def main():
     config = ConfigHandler(basepath, os.path.join(basepath, 'conf', 'devtool.conf'))
     if not config.read():
         return -1
-    context.config = config
 
     bitbake_subdir = config.get('General', 'bitbake_subdir', '')
     if bitbake_subdir:
@@ -299,7 +303,9 @@ def main():
 
     # Search BBPATH first to allow layers to override plugins in scripts_path
     pluginpaths = [os.path.join(path, 'lib', 'devtool') for path in global_args.bbpath.split(':') + [scripts_path]]
-    context.pluginpaths = pluginpaths
+
+    context = Context(fixed_setup=False, config=config, pluginpaths=pluginpaths)
+
     for pluginpath in pluginpaths:
         scriptutils.load_plugins(logger, plugins, pluginpath)
 
@@ -332,7 +338,7 @@ def main():
     try:
         workspace = {}
         if not getattr(args, 'no_workspace', False):
-            workspace = read_workspace(basepath)
+            workspace = read_workspace(basepath, context)
         ret = args.func(args, config, basepath, workspace)
     except DevtoolError as err:
         if str(err):
-- 
2.43.0

[PATCH 4/7] devtool: un-globalize 'config' variable

[chris.laplante]

From: Chris Laplante <chris.laplante@agilent.com>

'read_workspace' can now access it via the 'context' that's passed in

Signed-off-by: Chris Laplante <chris.laplante@agilent.com>
---
 scripts/devtool | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/scripts/devtool b/scripts/devtool
index 1ace6fb035..d96d25a51a 100755
--- a/scripts/devtool
+++ b/scripts/devtool
@@ -19,8 +19,6 @@ import logging
 # This can be removed once our minimum is Python 3.9: https://docs.python.org/3/whatsnew/3.9.html#type-hinting-generics-in-standard-collections
 from typing import List
 
-config = None
-
 
 scripts_path = os.path.dirname(os.path.realpath(__file__))
 lib_path = scripts_path + '/lib'
@@ -93,19 +91,19 @@ class Context:
 
 def read_workspace(basepath, context):
     workspace = {}
-    if not os.path.exists(os.path.join(config.workspace_path, 'conf', 'layer.conf')):
+    if not os.path.exists(os.path.join(context.config.workspace_path, 'conf', 'layer.conf')):
         if context.fixed_setup:
             logger.error("workspace layer not set up")
             sys.exit(1)
         else:
-            logger.info('Creating workspace layer in %s' % config.workspace_path)
-            _create_workspace(config.workspace_path, config, basepath)
+            logger.info('Creating workspace layer in %s' % context.config.workspace_path)
+            _create_workspace(context.config.workspace_path, context.config, basepath)
     if not context.fixed_setup:
-        _enable_workspace_layer(config.workspace_path, config, basepath)
+        _enable_workspace_layer(context.config.workspace_path, context.config, basepath)
 
-    logger.debug('Reading workspace in %s' % config.workspace_path)
+    logger.debug('Reading workspace in %s' % context.config.workspace_path)
     externalsrc_re = re.compile(r'^EXTERNALSRC(:pn-([^ =]+))? *= *"([^"]*)"$')
-    for fn in glob.glob(os.path.join(config.workspace_path, 'appends', '*.bbappend')):
+    for fn in glob.glob(os.path.join(context.config.workspace_path, 'appends', '*.bbappend')):
         with open(fn, 'r') as f:
             pnvalues = {}
             pn = None
@@ -116,7 +114,7 @@ def read_workspace(basepath, context):
                     pn = res.group(2) or recipepn
                     # Find the recipe file within the workspace, if any
                     bbfile = os.path.basename(fn).replace('.bbappend', '.bb').replace('%', '*')
-                    recipefile = glob.glob(os.path.join(config.workspace_path,
+                    recipefile = glob.glob(os.path.join(context.config.workspace_path,
                                                         'recipes',
                                                         recipepn,
                                                         bbfile))
@@ -130,7 +128,7 @@ def read_workspace(basepath, context):
             if pnvalues:
                 if not pn:
                     raise DevtoolError("Found *.bbappend in %s, but could not determine EXTERNALSRC:pn-*. "
-                            "Maybe still using old syntax?" % config.workspace_path)
+                            "Maybe still using old syntax?" % context.config.workspace_path)
                 if not pnvalues.get('srctreebase', None):
                     pnvalues['srctreebase'] = pnvalues['srctree']
                 logger.debug('Found recipe %s' % pnvalues)
@@ -215,8 +213,6 @@ def _enable_workspace_layer(workspacedir, config, basepath):
 
 
 def main():
-    global config
-
     if sys.getfilesystemencoding() != "utf-8":
         sys.exit("Please use a locale setting which supports utf-8.\nPython can't change the filesystem locale after loading so we need a utf-8 when python starts or things won't work.")
 
-- 
2.43.0

[PATCH 5/7] devtool: un-globalize 'plugins' variable

[chris.laplante]

From: Chris Laplante <chris.laplante@agilent.com>

It never had to be a global anyway

Signed-off-by: Chris Laplante <chris.laplante@agilent.com>
---
 scripts/devtool | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/scripts/devtool b/scripts/devtool
index d96d25a51a..b43a958497 100755
--- a/scripts/devtool
+++ b/scripts/devtool
@@ -28,8 +28,6 @@ import scriptutils
 import argparse_oe
 logger = scriptutils.logger_create('devtool')
 
-plugins = []
-
 
 class ConfigHandler:
     basepath = None
@@ -302,6 +300,7 @@ def main():
 
     context = Context(fixed_setup=False, config=config, pluginpaths=pluginpaths)
 
+    plugins = []
     for pluginpath in pluginpaths:
         scriptutils.load_plugins(logger, plugins, pluginpath)
 
-- 
2.43.0

[PATCH 6/7] devtool: misc cleanups

[chris.laplante]

From: Chris Laplante <chris.laplante@agilent.com>

1. Bad None comparison
2. Reliance on transitive includes in bb
3. Unbound 'ret' variable

Signed-off-by: Chris Laplante <chris.laplante@agilent.com>
---
 scripts/devtool | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/scripts/devtool b/scripts/devtool
index b43a958497..1994d4b507 100755
--- a/scripts/devtool
+++ b/scripts/devtool
@@ -45,7 +45,7 @@ class ConfigHandler:
         try:
             ret = self.config_obj.get(section, option)
         except (configparser.NoOptionError, configparser.NoSectionError):
-            if default != None:
+            if default is not None:
                 ret = default
             else:
                 raise
@@ -147,7 +147,7 @@ def create_workspace(args, config, basepath, workspace):
         _enable_workspace_layer(workspacedir, config, basepath)
 
 def _create_workspace(workspacedir, config, basepath, layerseries=None):
-    import bb
+    import bb.utils
 
     confdir = os.path.join(workspacedir, 'conf')
     if os.path.exists(os.path.join(confdir, 'layer.conf')):
@@ -192,7 +192,7 @@ def _create_workspace(workspacedir, config, basepath, layerseries=None):
 
 def _enable_workspace_layer(workspacedir, config, basepath):
     """Ensure the workspace layer is in bblayers.conf"""
-    import bb
+    import bb.utils
     bblayers_conf = os.path.join(basepath, 'conf', 'bblayers.conf')
     if not os.path.exists(bblayers_conf):
         logger.error('Unable to find bblayers.conf')
@@ -286,6 +286,7 @@ def main():
     scriptutils.logger_setup_color(logger, global_args.color)
 
     if global_args.bbpath is None:
+        import bb
         try:
             tinfoil = setup_tinfoil(config_only=True, basepath=basepath)
             try:
@@ -341,6 +342,7 @@ def main():
         ret = err.exitcode
     except argparse_oe.ArgumentUsageError as ae:
         parser.error_subcommand(ae.message, ae.subcommand)
+        ret = 2
 
     return ret
 
-- 
2.43.0

[PATCH 7/7] devtool: remove unused 'config' param from '_create_workspace' method

[chris.laplante]

From: Chris Laplante <chris.laplante@agilent.com>

Signed-off-by: Chris Laplante <chris.laplante@agilent.com>
---
 scripts/devtool | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/scripts/devtool b/scripts/devtool
index 1994d4b507..ebd3e4c11b 100755
--- a/scripts/devtool
+++ b/scripts/devtool
@@ -95,7 +95,7 @@ def read_workspace(basepath, context):
             sys.exit(1)
         else:
             logger.info('Creating workspace layer in %s' % context.config.workspace_path)
-            _create_workspace(context.config.workspace_path, context.config, basepath)
+            _create_workspace(context.config.workspace_path, basepath)
     if not context.fixed_setup:
         _enable_workspace_layer(context.config.workspace_path, context.config, basepath)
 
@@ -134,7 +134,7 @@ def read_workspace(basepath, context):
 
     return workspace
 
-def create_workspace(args, config, basepath, workspace):
+def create_workspace(args, config, basepath, _workspace):
     if args.layerpath:
         workspacedir = os.path.abspath(args.layerpath)
     else:
@@ -142,11 +142,11 @@ def create_workspace(args, config, basepath, workspace):
     layerseries = None
     if args.layerseries:
         layerseries = args.layerseries
-    _create_workspace(workspacedir, config, basepath, layerseries)
+    _create_workspace(workspacedir, basepath, layerseries)
     if not args.create_only:
         _enable_workspace_layer(workspacedir, config, basepath)
 
-def _create_workspace(workspacedir, config, basepath, layerseries=None):
+def _create_workspace(workspacedir, basepath, layerseries=None):
     import bb.utils
 
     confdir = os.path.join(workspacedir, 'conf')
-- 
2.43.0

Re: [OE-core] [PATCH 0/7] De-globalization of 'devtool'

[Alexander Kanavin]

On Sun, 12 Jan 2025 at 15:55, Chris Laplante via
lists.openembedded.org
<chris.laplante=agilent.com@lists.openembedded.org> wrote:
> This patchset removes global variables from the 'devtool' script. It
> also changes `Context` to be a dataclass.

Thanks, these changes look really nice.

Alex

RE: [OE-core] [PATCH 0/7] De-globalization of 'devtool'

[chris.laplante]

> 
> On Sun, 12 Jan 2025 at 15:55, Chris Laplante via lists.openembedded.org
> <chris.laplante=agilent.com@lists.openembedded.org> wrote:
> > This patchset removes global variables from the 'devtool' script. It
> > also changes `Context` to be a dataclass.
> 
> Thanks, these changes look really nice.


My pleasure :)

Thanks,
Chris

Re: [OE-core] [PATCH 0/7] De-globalization of 'devtool'

[Mathieu Dubois-Briand]

On Sun Jan 12, 2025 at 3:53 PM CET, Chris Laplante via lists.openembedded.org wrote:
> From: Chris Laplante <chris.laplante@agilent.com>
>
> This patchset removes global variables from the 'devtool' script. It
> also changes `Context` to be a dataclass.
>
> All devtool self-tests pass, tested via: oe-selftest -r devtool
>
> Chris Laplante (7):
>   devtool: un-globalize the 'basepath' variable
>   devtool: un-globalize 'workspace' variable
>   devtool: un-globalize 'context' variable and convert it to a dataclass
>   devtool: un-globalize 'config' variable
>   devtool: un-globalize 'plugins' variable
>   devtool: misc cleanups
>   devtool: remove unused 'config' param from '_create_workspace' method
>
>  scripts/devtool | 88 +++++++++++++++++++++++++------------------------
>  1 file changed, 45 insertions(+), 43 deletions(-)
>
> --
> 2.43.0

Hi Chris,

I believe this series is breaking some builds on the autobuilder. We
got the following error:

devtool: error: argument <subcommand>: invalid choice: 'sdk-install' (choose from create-workspace, add, modify, extract, sync, rename, update-recipe, status, reset, finish, edit-recipe, find-recipe, configure-help, import, deploy-target, undeploy-target, build-image, ide-sdk, search, menuconfig, upgrade, latest-version, check-upgrade-status, build, export)

https://valkyrie.yoctoproject.org/#/builders/80/builds/725/steps/13/logs/stdio

Can you have a look at this issue please ?

-- 
Mathieu Dubois-Briand, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

RE: [OE-core] [PATCH 0/7] De-globalization of 'devtool'

[chris.laplante]

Hi Mathieu,

> Hi Chris,
> 
> I believe this series is breaking some builds on the autobuilder. We got the
> following error:
> 
> devtool: error: argument <subcommand>: invalid choice: 'sdk-install' (choose
> from create-workspace, add, modify, extract, sync, rename, update-recipe,
> status, reset, finish, edit-recipe, find-recipe, configure-help, import, deploy-
> target, undeploy-target, build-image, ide-sdk, search, menuconfig, upgrade,
> latest-version, check-upgrade-status, build, export)
> 
> https://nam12.safelinks.protection.outlook.com/?url=https%3A%2F%2Fvalk
> yrie.yoctoproject.org%2F%23%2Fbuilders%2F80%2Fbuilds%2F725%2Fsteps
> %2F13%2Flogs%2Fstdio&data=05%7C02%7Cchris.laplante%40agilent.com%
> 7C90775d234a164700487a08dd33df95e8%7Ca9c0bc098b46420693512b
> a12fb4a5c0%7C0%7C0%7C638723757571791775%7CUnknown%7CTWFpb
> GZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4z
> MiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=WumZ5
> CzE%2BlW6df%2B0leHeHV9lFyknKFTpDPTWKDueRCU%3D&reserved=0
> 
> Can you have a look at this issue please ?


Absolutely, will do.

Chris