Pillow icon indicating copy to clipboard operation
Pillow copied to clipboard

Parse _version contents instead of using exec()

Open radarhere opened this issue 1 year ago • 2 comments

Our Python 3.13 jobs have started failing in GitHub Actions - https://github.com/python-pillow/Pillow/actions/runs/9032048835/job/24819485997#step:9:46

    File "/private/var/folders/3m/p59k4qdj0f17st0gn2cmj3640000gn/T/pip-build-env-v125k6ei/overlay/lib/python3.13/site-packages/setuptools/build_meta.py", line 311, in run_setup
      exec(code, locals())
      ~~~~^^^^^^^^^^^^^^^^
    File "<string>", line 33, in <module>
    File "<string>", line 27, in get_version
  KeyError: '__version__'

This is referring to https://github.com/python-pillow/Pillow/blob/0cad346fc960bc07cde9d1d9135421978e2f28e8/setup.py#L23-L27

Python 3.13 is failing to populate the local variables from exec() - https://github.com/python/cpython/issues/118888

We have various options at this point.

  1. Presuming the bug report is accepted and fixed, the next beta will be out in about three weeks. We could just deal with these failing jobs until then.
  2. We could disable these jobs until then.
  3. We could stop using exec() for this (originally added in #2517), and instead just simply parse the version string out of _version.py after reading the file contents.

This PR suggests Option 3. I don't mind if this is declined, it merely seems like a straightforward solution with no obvious downsides - if _version.py ever changes in the future so that this parsing code doesn't work, get_version() can be changed again.

radarhere avatar May 10 '24 12:05 radarhere

Option 1 would be a bit annoying, I'm fine with either of 2 or 3.

It'd be nice to ditch the exec.

Option 3 feels a bit brittle, but I doubt we'll change _version.py much and if we do it'd probably break the build in an obvious way, especially as we have the version check in the Python and C sides.

hugovk avatar May 10 '24 14:05 hugovk

We can also explicitly pass in a locals to be populated:

diff --git a/setup.py b/setup.py
index 7d8e1c1ee..5ec2f4c01 100644
--- a/setup.py
+++ b/setup.py
@@ -23,8 +23,10 @@ from setuptools.command.build_ext import build_ext
 def get_version():
     version_file = "src/PIL/_version.py"
     with open(version_file, encoding="utf-8") as f:
-        exec(compile(f.read(), version_file, "exec"))
-    return locals()["__version__"]
+        lcl = {}
+        exec(compile(f.read(), version_file, "exec"), locals=lcl)
+    print(locals())
+    return lcl["__version__"]


 configuration = {}

(this patch wont directly work as it uses the 3.13 feature of passing locals as keyword 🤦🏻 )

tacaswell avatar May 10 '24 18:05 tacaswell

Discussion in the CPython issue is saying that this is "an expected and intentional behavior change", so the solution would be either this PR, or https://github.com/python-pillow/Pillow/pull/8057

radarhere avatar May 20 '24 02:05 radarhere

Which PR do you prefer?

hugovk avatar May 21 '24 11:05 hugovk

My preference would be this one, to remove the exec()

radarhere avatar May 21 '24 11:05 radarhere

Thanks!

hugovk avatar May 21 '24 11:05 hugovk

FWIW, and not that anyone asked, but I switched to using a JSON file for managing such variables that need to be acessed both from outside and within the package. This is very easy avoids the responsibility (exec()! eval()!) of parsing. I'm sure other solutions are equally valid.

Themanwithoutaplan avatar May 23 '24 17:05 Themanwithoutaplan