zopefoundation / AccessControl

Security framework for Zope.
Other
12 stars 16 forks source link

Use `PURE_PYTHON` to switch default implementation to python. #119

Closed sallner closed 2 years ago

sallner commented 2 years ago

This is a first step to solve #117 . The integration of .meta.toml can be done afterwards.

d-maurer commented 2 years ago

Steffen Allner wrote at 2021-11-26 06:34 -0800: @.*** commented on this pull request.

@@ -25,6 +25,13 @@

""" from future import absolute_import +import os + +# We cannot use PURE_PYTHON as this would be propagated to ExtensionClass +# and Acquisition. Due to restrictions of python implementation concerning +# proxies it is not possible to run all with pure python and we influence only +# the AccessControl implementation here. +CAPI = not int(os.environ.get('AC_PURE_PYTHON', '0'))

As written in the comment above, it is not possible to have all packages AccessControl, Acquisition and ExtensionClass. The CAPI is also only valid in this package, so I do not see a benefit to include the switch of the other packages.

Several packages support the PURE_PYTHON envvar to use the fallback Python implementation rather than the C optimization. If AccessControl is used together with those packages in a PURE_PYTHON context, it, too, should use the Python and not the "C" implementation. Ideally, it would use the same envvar (i.e. PURE_PYTHON) and not its own one (AC_PURE_PYTHON).

icemac commented 2 years ago

@d-maurer wrote:

If AccessControl is used together with those packages in a PURE_PYTHON context, it, too, should use the Python and not the "C" implementation. Ideally, it would use the same envvar (i.e. PURE_PYTHON) and not its own one (AC_PURE_PYTHON).

The problem is that the tests of AccessControl do not run successfully when using PURE_PYTHON=1, see https://github.com/zopefoundation/AccessControl/pull/120 where I am running the tests using PURE_PYTHON=1 and AC_PURE_PYTHON=1

d-maurer commented 2 years ago

Michael Howitz wrote at 2021-11-28 23:28 -0800: @.*** wrote:

If AccessControl is used together with those packages in a PURE_PYTHON context, it, too, should use the Python and not the "C" implementation. Ideally, it would use the same envvar (i.e. PURE_PYTHON) and not its own one (AC_PURE_PYTHON).

The problem is that the tests of AccessControl do not run successfully when using PURE_PYTHON=1, see https://github.com/zopefoundation/AccessControl/pull/120 where I am running the tests using PURE_PYTHON=1 and AC_PURE_PYTHON=1

In my view, we have 2 different use cases:

  1. control the use of the Python implementation of AccessControl via an envvar (rather than an API call) without using Python implementations for other packages

  2. control the use Python versions for all packages via envvar PURE_PYTHON.

This PR targets the first use case; my change request is motivated by the wish that the second use case is also supported.

icemac commented 2 years ago

@d-maurer Do you think this is okay now?

d-maurer commented 2 years ago

Michael Howitz wrote at 2022-2-15 23:50 -0800: @.*** Do you think this is okay now?

Likely, I will find time at the beginning of the next week.

icemac commented 2 years ago

@d-maurer I change the implementation of _what_not_even_god_should_do to use the version from the C code if PURE_PYTHON is not set. Do you think this is better now?

d-maurer commented 2 years ago

Michael Howitz wrote at 2022-2-21 23:24 -0800: @.*** I change the implementation of _what_not_even_god_should_do to use the version from the C code if PURE_PYTHON is not set. Do you think this is better now?

Better.

When I remember right, other packages use different logic to determine whether the C implementation should be used

We might (or might not) want to harmonize this logic.

This might be particularly important for AccessControl as its C implementation has a dependency on the C implementation of ExtensionClass. Therefore, these two packages (at least) should use the same logic to determine whether the C or Python implementations should be used.

icemac commented 2 years ago

@d-maurer AccessControl does not support PyPy according to its setup.py.

zope.interface has logic to use the Python implementation if the C extension cannot be built for whatever reason. But this is too complicated to replicate it here.

Let's keep it as it is for now.