[PATCH v2] scripts/spdxcheck: Limit the scope of git.Repo

Ricardo Ribalda posted 1 patch 10 months ago
scripts/spdxcheck.py | 30 +++++++++++++++---------------
1 file changed, 15 insertions(+), 15 deletions(-)
[PATCH v2] scripts/spdxcheck: Limit the scope of git.Repo
Posted by Ricardo Ribalda 10 months ago
If the git.Repo object's scope extends to the Python interpreter's
shutdown phase, its destructor may fail due to the interpreter's state.

Exception ignored in: <function Git.AutoInterrupt.__del__ at 0x7f1941dd5620>
Traceback (most recent call last):
  File "/usr/lib/python3/dist-packages/git/cmd.py", line 565, in __del__
  File "/usr/lib/python3/dist-packages/git/cmd.py", line 546, in _terminate
  File "/usr/lib/python3.13/subprocess.py", line 2227, in terminate
ImportError: sys.meta_path is None, Python is likely shutting down

Make repo a variable of the function read_spdxdata() and scan_git_tree()
to limit the scope of git.Repo and ensure proper resource management.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
Changes in v2:
- Make repo a local variable
- Link to v1: https://lore.kernel.org/r/20250225-spx-v1-1-e935b27eb80d@chromium.org
---
 scripts/spdxcheck.py | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/scripts/spdxcheck.py b/scripts/spdxcheck.py
index 8d608f61bf371647e7ca0129f583e94e535b6193..d5c4c37f1b068486af28110261f74c67301618a9 100755
--- a/scripts/spdxcheck.py
+++ b/scripts/spdxcheck.py
@@ -45,7 +45,9 @@ class dirinfo(object):
                 self.files.append(fname)
 
 # Read the spdx data from the LICENSES directory
-def read_spdxdata(repo):
+def read_spdxdata():
+    repo = git.Repo(os.getcwd())
+    assert not repo.bare
 
     # The subdirectories of LICENSES in the kernel source
     # Note: exceptions needs to be parsed as last directory.
@@ -295,7 +297,15 @@ def exclude_file(fpath):
             return True
     return False
 
-def scan_git_tree(tree, basedir, dirdepth):
+def scan_git_tree(basedir, dirdepth):
+    repo = git.Repo(os.getcwd())
+    tree = repo.head.commit.tree
+
+    basedir = basedir.strip('/')
+    if basedir != '.':
+        for p in basedir.split('/'):
+            tree = tree[p]
+
     parser.set_dirinfo(basedir, dirdepth)
     for el in tree.traverse():
         if not os.path.isfile(el.path):
@@ -306,11 +316,6 @@ def scan_git_tree(tree, basedir, dirdepth):
         with open(el.path, 'rb') as fd:
             parser.parse_lines(fd, args.maxlines, el.path)
 
-def scan_git_subtree(tree, path, dirdepth):
-    for p in path.strip('/').split('/'):
-        tree = tree[p]
-    scan_git_tree(tree, path.strip('/'), dirdepth)
-
 def read_exclude_file(fname):
     rules = []
     if not fname:
@@ -348,12 +353,8 @@ if __name__ == '__main__':
         sys.exit(1)
 
     try:
-        # Use git to get the valid license expressions
-        repo = git.Repo(os.getcwd())
-        assert not repo.bare
-
         # Initialize SPDX data
-        spdx = read_spdxdata(repo)
+        spdx = read_spdxdata()
 
         # Initialize the parser
         parser = id_parser(spdx)
@@ -389,14 +390,13 @@ if __name__ == '__main__':
                     if os.path.isfile(p):
                         parser.parse_lines(open(p, 'rb'), args.maxlines, p)
                     elif os.path.isdir(p):
-                        scan_git_subtree(repo.head.reference.commit.tree, p,
-                                         args.depth)
+                        scan_git_tree(p, args.depth)
                     else:
                         sys.stderr.write('path %s does not exist\n' %p)
                         sys.exit(1)
             else:
                 # Full git tree scan
-                scan_git_tree(repo.head.commit.tree, '.', args.depth)
+                scan_git_tree('.', args.depth)
 
             ndirs = len(parser.spdx_dirs)
             dirsok = 0

---
base-commit: d082ecbc71e9e0bf49883ee4afd435a77a5101b6
change-id: 20250225-spx-382cf543370e

Best regards,
-- 
Ricardo Ribalda <ribalda@chromium.org>
Re: [PATCH v2] scripts/spdxcheck: Limit the scope of git.Repo
Posted by Gon Solo 10 months ago
Am Wed, Apr 09, 2025 at 08:04:19PM +0000 schrieb Ricardo Ribalda:
> Changes in v2:
> - Make repo a local variable
> - Link to v1: https://lore.kernel.org/r/20250225-spx-v1-1-e935b27eb80d@chromium.org

This is not necessary as it was Python's fault and is fixed by 3.13.3
which came out yesterday. I just checked.
Re: [PATCH v2] scripts/spdxcheck: Limit the scope of git.Repo
Posted by Ricardo Ribalda 10 months ago
On Wed, 9 Apr 2025 at 22:40, Gon Solo <gonsolo@gmail.com> wrote:
>
> Am Wed, Apr 09, 2025 at 08:04:19PM +0000 schrieb Ricardo Ribalda:
> > Changes in v2:
> > - Make repo a local variable
> > - Link to v1: https://lore.kernel.org/r/20250225-spx-v1-1-e935b27eb80d@chromium.org
>
> This is not necessary as it was Python's fault and is fixed by 3.13.3
> which came out yesterday. I just checked.

It will take some time before this reaches all distributions. This
patch is relatively simple.

I might be biased, but I think the benefits outweigh the cons.

>


-- 
Ricardo Ribalda
Re: [PATCH v2] scripts/spdxcheck: Limit the scope of git.Repo
Posted by Gon Solo 10 months ago
> > This is not necessary as it was Python's fault and is fixed by 3.13.3
> > which came out yesterday. I just checked.
> 
> It will take some time before this reaches all distributions. This
> patch is relatively simple.
> 
> I might be biased, but I think the benefits outweigh the cons.

3.13.3 is in Arch core-testing and Debian unstable, Debian stable's 3.11
is not affected by this bug. But I agree, it's annoying to be hit by
it. :/

g
Re: [PATCH v2] scripts/spdxcheck: Limit the scope of git.Repo
Posted by Greg Kroah-Hartman 10 months ago
On Thu, Apr 10, 2025 at 08:21:09AM +0200, Gon Solo wrote:
> > > This is not necessary as it was Python's fault and is fixed by 3.13.3
> > > which came out yesterday. I just checked.
> > 
> > It will take some time before this reaches all distributions. This
> > patch is relatively simple.
> > 
> > I might be biased, but I think the benefits outweigh the cons.
> 
> 3.13.3 is in Arch core-testing and Debian unstable, Debian stable's 3.11
> is not affected by this bug. But I agree, it's annoying to be hit by
> it. :/

As this is a bug in python, let's not work around it in our scripts but
rather rely on the proper fix in python instead.

thanks,

greg k-h