Skip to content

Use multi-phase init#175

Open
AA-Turner wants to merge 1 commit into
python-cffi:mainfrom
AA-Turner:multi-phase
Open

Use multi-phase init#175
AA-Turner wants to merge 1 commit into
python-cffi:mainfrom
AA-Turner:multi-phase

Conversation

@AA-Turner
Copy link
Copy Markdown

#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
Copy Markdown
Contributor

arigo commented May 28, 2025

@mattip
Copy link
Copy Markdown
Contributor

mattip commented Jun 25, 2025

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

@mattip
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

Comment thread src/c/_cffi_backend.c
.m_base = PyModuleDef_HEAD_INIT,
.m_name = "_cffi_backend",
.m_size = 0,
.m_methds = FFIBackendMethods,
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

@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

4 participants