Skip to content

Use multi-phase init #175

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

AA-Turner
Copy link

#174.

cc @arigo -- this is a minimal implementation for _cffi_backend.c. Questions:

  • Can we remove the PY_VERSION checks? It's hard to test the Python-2 case, and as far as I can tell you only support Python 3.8+.
  • Linked to the above, ideally we would consider module isolation, but this requires Python 3-only API that would mean more #if branching, so I've avoided for now.
  • There's a use of single-phase init (PyModule_Create) in vengine_cpy.py, but the file is marked as deprecated. Is the file still used / should I update this?
  • _my_Py_InitModule() and b_init_cffi_1_0_external_module() will be tricky to migrate. Would it be possible to use the dynamic creation functions here instead?

A

@arigo
Copy link
Contributor

arigo commented May 28, 2025

cc @nitzmahone

@mattip
Copy link
Contributor

mattip commented Jun 25, 2025

@ngoldbaum, is this part of #178 or are these PRs orthagonal?

@mattip
Copy link
Contributor

mattip commented Jun 25, 2025

Linked to the above, ideally we would consider module isolation,

I think that is fine. Python changed the documentation: it used to state that mult-phase init implied module isolation, now it states that module isolation is recommended but not required.

@mattip
Copy link
Contributor

mattip commented Jun 25, 2025

There's a use of single-phase init (PyModule_Create) in vengine_cpy.py, but the file is marked as deprecated. Is the file still used / should I update this?

I see the engine choice logic is here. It seems vengine_cpy.VCPythonEngine is used if

  • We are not on PyPy
  • ffi._backend is not _cffi_backend. As far as I can tell this is only used in tests to try out the ctypes backend, and should not happen in "real life".

@arigo is that a correct assesment? If true, then I would not worry about vengine_cpy too much.

@mattip
Copy link
Contributor

mattip commented Jun 25, 2025

Can we remove the PY_VERSION checks? It's hard to test the Python-2 case, and as far as I can tell you only support Python 3.8+.

I think that is fine to drop Python-2 in the c-based backend code. We keep some python2-compatible code in other places to attempt to still work with PyPy-2, but certainly the c-based backend here is only used on CPython. It might be nice to do this in a separate PR.

In any case, there should be a short blurb about these changes in doc/source/whatsnew.rst since they are user-facing.

.m_base = PyModuleDef_HEAD_INIT,
.m_name = "_cffi_backend",
.m_size = 0,
.m_methds = FFIBackendMethods,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo

Suggested change
.m_methds = FFIBackendMethods,
.m_methods = FFIBackendMethods,

@arigo
Copy link
Contributor

arigo commented Jun 25, 2025

* We are not on PyPy

* `ffi._backend` is not `_cffi_backend`. As far as I can tell this is only used in tests to try out the ctypes backend, and should not happen in "real life".

@arigo is that a correct assesment? If true, then I would not worry about vengine_cpy too much.

No, vengine_cpy is used if we are not on PyPy, and if ffi._backend is _cffi_backend (which is usually true).

@ngoldbaum
Copy link

@ngoldbaum, is this part of #178 or are these PRs orthagonal?

They're orthogonal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants