bpo-42212: smelly.py also checks the dynamic library

The smelly.py script now also checks the Python dynamic library and extension modules, not only the Python static library. Make also the script more verbose: explain what it does. The GitHub Action job now builds Python with the libpython dynamic library.
vstinner · Nov 24, 2020 · Nov 20, 2020 · Nov 24, 2020 · Nov 24, 2020 · Nov 20, 2020
commit 32165ccb22c98b5d1506021c3a567e65e520cdf5
diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
@@ -60,7 +60,8 @@ jobs:
         run: sudo ./.github/workflows/posix-deps-apt.sh
       - name: Build CPython
         run: |
-          ./configure --with-pydebug
+          # Build Python with the libpython dynamic library
+          ./configure --with-pydebug --enable-shared
           make -j4 regen-all
       - name: Check for changes
         run: |

diff --git a/Misc/NEWS.d/next/Tools-Demos/2020-11-20-15-11-05.bpo-42212.sjzgOf.rst b/Misc/NEWS.d/next/Tools-Demos/2020-11-20-15-11-05.bpo-42212.sjzgOf.rst
@@ -0,0 +1,3 @@
+The smelly.py script now also checks the Python dynamic library and extension
+modules, not only the Python static library. Make also the script more verbose:
+explain what it does.
diff --git a/Tools/scripts/smelly.py b/Tools/scripts/smelly.py
@@ -1,17 +1,45 @@
 #!/usr/bin/env python
 # Script checking that all symbols exported by libpython start with Py or _Py
 
+import os.path
 import subprocess
 import sys
 import sysconfig
 
 
-def get_exported_symbols():
-    LIBRARY = sysconfig.get_config_var('LIBRARY')
-    if not LIBRARY:
-        raise Exception("failed to get LIBRARY")
+ALLOWED_PREFIXES = ('Py', '_Py')
+if sys.platform == 'darwin':
+    ALLOWED_PREFIXES += ('__Py',)
+
+IGNORE_EXTENSION = "_ctypes_test"
+
+
+def is_local_symbol_type(symtype):
+    # Ignore local symbols.
+
+    # If lowercase, the symbol is usually local; if uppercase, the symbol
+    # is global (external).  There are however a few lowercase symbols that
+    # are shown for special global symbols ("u", "v" and "w").
+    if symtype.islower() and symtype not in "uvw":
+        return True
+
+    # Ignore the initialized data section (d and D) and the BSS data
+    # section. For example, ignore "__bss_start (type: B)"
+    # and "_edata (type: D)".
+    if symtype in "bBdD":
+        return True
+
+    return False
+
 
-    args = ('nm', '-p', LIBRARY)
+def get_exported_symbols(library, dynamic=False):
+    print(f"Check that {library} only exports symbols starting with Py or _Py")
+
+    # Only look at dynamic symbols
+    args = ['nm', '--no-sort']
+    if dynamic:
+        args.append('--dynamic')
+    args.append(library)
     print("+ %s" % ' '.join(args))
     proc = subprocess.run(args, stdout=subprocess.PIPE, universal_newlines=True)
     if proc.returncode:
@@ -25,12 +53,9 @@ def get_exported_symbols():
 
 
 def get_smelly_symbols(stdout):
-    symbols = []
-    ignored_symtypes = set()
-
-    allowed_prefixes = ('Py', '_Py')
-    if sys.platform == 'darwin':
-        allowed_prefixes += ('__Py',)
+    smelly_symbols = []
+    python_symbols = []
+    local_symbols = []
 
     for line in stdout.splitlines():
         # Split line '0000000000001b80 D PyTextIOWrapper_Type'
@@ -42,41 +67,96 @@ def get_smelly_symbols(stdout):
             continue
 
         symtype = parts[1].strip()
-        # Ignore private symbols.
-        #
-        # If lowercase, the symbol is usually local; if uppercase, the symbol
-        # is global (external).  There are however a few lowercase symbols that
-        # are shown for special global symbols ("u", "v" and "w").
-        if symtype.islower() and symtype not in "uvw":
-            ignored_symtypes.add(symtype)
+        symbol = parts[-1]
+        symbol = '%s (type: %s)' % (symbol, symtype)
+
+        if symbol.startswith(ALLOWED_PREFIXES):
+            python_symbols.append(symbol)
             continue
 
-        symbol = parts[-1]
-        if symbol.startswith(allowed_prefixes):
+        if is_local_symbol_type(symtype):
+            local_symbols.append(symbol)
+        else:
+            smelly_symbols.append(symbol)
+
+    if local_symbols:
+        print(f"Ignore {len(local_symbols)} local symbols")
+    return smelly_symbols, python_symbols
+
+
+def check_library(library, dynamic=False):
+    nm_output = get_exported_symbols(library, dynamic)
+    smelly_symbols, python_symbols = get_smelly_symbols(nm_output)
+
+    if not smelly_symbols:
+        print(f"OK: no smelly symbol found ({len(python_symbols)} Python symbols)")
+        return 0
+
+    print()
+    smelly_symbols.sort()
+    for symbol in smelly_symbols:
+        print("Smelly symbol: %s" % symbol)
+
+    print()
+    print("ERROR: Found %s smelly symbols!" % len(smelly_symbols))
+    return len(smelly_symbols)
+
+
+def check_extensions():
+    print(__file__)
+    srcdir = os.path.dirname(os.path.dirname(os.path.dirname(__file__)))
+    filename = os.path.join(srcdir, "pybuilddir.txt")
+    try:
+        with open(filename, encoding="utf-8") as fp:
+            pybuilddir = fp.readline()
+    except FileNotFoundError:
+        print(f"Cannot check extensions because {filename} does not exist")
+        return True
+
+    print(f"Check extension modules from {pybuilddir} directory")
+    builddir = os.path.join(srcdir, pybuilddir)
+    nsymbol = 0
+    for name in os.listdir(builddir):
+        if not name.endswith(".so"):
+            continue
+        if IGNORE_EXTENSION in name:
+            print()
+            print(f"Ignore extension: {name}")
             continue
-        symbol = '%s (type: %s)' % (symbol, symtype)
-        symbols.append(symbol)
 
-    if ignored_symtypes:
-        print("Ignored symbol types: %s" % ', '.join(sorted(ignored_symtypes)))
         print()
-    return symbols
+        filename = os.path.join(builddir, name)
+        nsymbol += check_library(filename, dynamic=True)
+
+    return nsymbol
 
 
 def main():
-    nm_output = get_exported_symbols()
-    symbols = get_smelly_symbols(nm_output)
+    # static library
+    LIBRARY = sysconfig.get_config_var('LIBRARY')
+    if not LIBRARY:
+        raise Exception("failed to get LIBRARY variable from sysconfig")
+    nsymbol = check_library(LIBRARY)
+
+    # dynamic library
+    LDLIBRARY = sysconfig.get_config_var('LDLIBRARY')
+    if not LDLIBRARY:
+        raise Exception("failed to get LDLIBRARY variable from sysconfig")
+    if LDLIBRARY != LIBRARY:
+        print()
+        nsymbol += check_library(LDLIBRARY, dynamic=True)
 
-    if not symbols:
-        print("OK: no smelly symbol found")
-        sys.exit(0)
+    # Check extension modules like _ssl.cpython-310d-x86_64-linux-gnu.so
+    nsymbol += check_extensions()
+
+    if nsymbol:
+        print()
+        print(f"ERROR: Found {nsymbol} smelly symbols in total!")
+        sys.exit(1)
 
-    symbols.sort()
-    for symbol in symbols:
-        print("Smelly symbol: %s" % symbol)
     print()
-    print("ERROR: Found %s smelly symbols!" % len(symbols))
-    sys.exit(1)
+    print(f"OK: all exported symbols of all libraries "
+          f"are prefixed with {' or '.join(map(repr, ALLOWED_PREFIXES))}")
 
 
 if __name__ == "__main__":