classification
Title: PEP 587 implementation is not ABI forward compatible
Type: Stage: resolved
Components: Interpreter Core Versions: Python 3.8
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: lukasz.langa, miss-islington, ncoghlan, ned.deily, vstinner
Priority: Keywords: patch

Created on 2019-09-28 00:32 by vstinner, last changed 2019-10-01 10:27 by vstinner. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 16451 merged vstinner, 2019-09-28 02:08
PR 16453 merged vstinner, 2019-09-28 02:30
PR 16487 merged vstinner, 2019-09-30 10:31
PR 16488 merged vstinner, 2019-09-30 10:34
PR 16500 merged vstinner, 2019-09-30 20:26
PR 16508 merged vstinner, 2019-10-01 08:30
PR 16509 merged vstinner, 2019-10-01 09:11
PR 16510 merged miss-islington, 2019-10-01 10:06
Messages (18)
msg353430 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-09-28 00:32
The current implementation of the PEP 587 is not ready for future evolutions of the PyPreConfig and PyConfig structure. Any change will break the ABI compatibility. Their _config_version field is useless.

The first versions of the PEP 587 used a macro to statically initialize the structure. It was decided to get ride of macros to use functions instead.

Example:

   PyConfig config;
   PyStatus status = PyConfig_InitPythonConfig(&config);

PyConfig_InitPythonConfig() gets unitialized memory and doesn't know the size of the config variable.

I propose to require to store the size of the structure in the structure directly:

* Add PyPreConfig.struct_size and PyConfig.struct_size
* Require to initialize these fields to sizeof(PyPreConfig) and sizeof(PyConfig) respectively

Example:
     
   PyConfig config;
   config.struct_size = sizeof(PyConfig);
   PyStatus status = PyConfig_InitPythonConfig(&config);

Thanks to the struct_size field, PyConfig_InitPythonConfig() can support old PyConfig structures without breaking the ABI.

If an old PyConfig is passed to PyConfig_Read(): newer fields will be ignored.
msg353432 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-09-28 02:18
I updated the PEP 587:
https://github.com/python/peps/commit/afa38c0bef7738b8fcc3173daee8488e0420833d
msg353435 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-09-28 02:28
New changeset 441b10cf2855955c86565f8d59e72c2efc0f0a57 by Victor Stinner in branch 'master':
bpo-38304: Add PyConfig.struct_size (GH-16451)
https://github.com/python/cpython/commit/441b10cf2855955c86565f8d59e72c2efc0f0a57
msg353436 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-09-28 02:50
New changeset 6e128382b3e7f83d91d5e73d83a051ff5fb6469c by Victor Stinner in branch '3.8':
bpo-38304: Add PyConfig.struct_size (GH-16451) (GH-16453)
https://github.com/python/cpython/commit/6e128382b3e7f83d91d5e73d83a051ff5fb6469c
msg353437 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-09-28 02:54
Ok, the API should now be ABI future proof thanks to this change.

I experimented a change locally: it's possible to support two sizes of the PyConfig structure (need to modify a few functions in initconfig.c).

I sent an email to python-dev:
https://mail.python.org/archives/list/python-dev@python.org/thread/C7Z2NA2DTM3DLOZCFQAK5A2WFYO3PHHX/
msg353487 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2019-09-29 06:50
The hidden _config_version field wasn't in the accepted PEP.

Both it and struct_size should be removed, as we don't support updating an embedded Python runtime to a new X.Y.0 release without rebuilding the embedding application.
msg353490 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2019-09-29 07:03
Nick, do you consider this a 3.8.0 release blocker?  If so, you should reopen it and mark it as such, since the 3.8.0rc1 cutoff is imminent.
msg353509 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-09-29 21:19
> The hidden _config_version field wasn't in the accepted PEP.

It was in the accepted PEP and I replaced it with struct_size:
https://github.com/python/peps/commit/afa38c0bef7738b8fcc3173daee8488e0420833d

I suggest to discuss the issue on the thread that I started on python-dev.
msg353510 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-09-29 21:21
> Both it and struct_size should be removed, as we don't support updating an embedded Python runtime to a new X.Y.0 release without rebuilding the embedding application.

See my reply on python-dev:
https://mail.python.org/archives/list/python-dev@python.org/message/ZE5K7MEKN32YGHNA5457AROWUNI7JOWC/
msg353563 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-09-30 10:52
New changeset 89f8177dcfdbcf17c85bb6998c946c9f42bf6e27 by Victor Stinner in branch 'master':
bpo-38304: Fix PyConfig usage in python_uwp.cpp (GH-16487)
https://github.com/python/cpython/commit/89f8177dcfdbcf17c85bb6998c946c9f42bf6e27
msg353564 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-09-30 10:53
New changeset 81f6b031c46721478372d77fe2e55aa1f8300ae1 by Victor Stinner in branch '3.8':
bpo-38304: Fix PyConfig usage in python_uwp.cpp (GH-16488)
https://github.com/python/cpython/commit/81f6b031c46721478372d77fe2e55aa1f8300ae1
msg353621 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-09-30 20:33
I reopen the issue since there seems to be disagreement about the whole feature (provide a stable ABI for embedded Python): see bpo-38326.
msg353644 - (view) Author: Ɓukasz Langa (lukasz.langa) * (Python committer) Date: 2019-10-01 07:52
Will you forward port your change to master, Victor?
msg353651 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-10-01 08:31
> Will you forward port your change to master, Victor?

Sure, it was my plan: PR 16508. I chose to fix 3.8 first to unblock the 3.8.0rc1 release.
msg353652 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-10-01 08:56
New changeset 3c30a76f3d3c0dcc1fb4de097fa4a3a4c92c0b0b by Victor Stinner in branch 'master':
bpo-38304: Remove PyConfig.struct_size (GH-16500) (GH-16508)
https://github.com/python/cpython/commit/3c30a76f3d3c0dcc1fb4de097fa4a3a4c92c0b0b
msg353656 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-10-01 10:06
New changeset 8462a4936b3a551dc546a6adea04a70b0a07ca67 by Victor Stinner in branch 'master':
bpo-38304: PyConfig_InitPythonConfig() cannot fail anymore (GH-16509)
https://github.com/python/cpython/commit/8462a4936b3a551dc546a6adea04a70b0a07ca67
msg353657 - (view) Author: miss-islington (miss-islington) Date: 2019-10-01 10:26
New changeset d49f096cc41f57155efe71cd089c29b38c218488 by Miss Islington (bot) in branch '3.8':
bpo-38304: PyConfig_InitPythonConfig() cannot fail anymore (GH-16509)
https://github.com/python/cpython/commit/d49f096cc41f57155efe71cd089c29b38c218488
msg353658 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-10-01 10:27
It was decided to abandon the idea of stable ABI for PyConfig: see bpo-38326. I now close this issue.
History
Date User Action Args
2019-10-01 10:27:28vstinnersetstatus: open -> closed
resolution: fixed
messages: + msg353658

stage: patch review -> resolved
2019-10-01 10:26:07miss-islingtonsetnosy: + miss-islington
messages: + msg353657
2019-10-01 10:06:35miss-islingtonsetpull_requests: + pull_request16100
2019-10-01 10:06:19vstinnersetmessages: + msg353656
2019-10-01 09:11:22vstinnersetpull_requests: + pull_request16099
2019-10-01 08:56:41vstinnersetmessages: + msg353652
2019-10-01 08:31:22vstinnersetmessages: + msg353651
2019-10-01 08:30:00vstinnersetstage: resolved -> patch review
pull_requests: + pull_request16098
2019-10-01 07:52:28lukasz.langasetmessages: + msg353644
2019-09-30 20:33:44vstinnersetstatus: closed -> open
resolution: fixed -> (no value)
messages: + msg353621
2019-09-30 20:26:30vstinnersetpull_requests: + pull_request16089
2019-09-30 10:53:20vstinnersetmessages: + msg353564
2019-09-30 10:52:37vstinnersetmessages: + msg353563
2019-09-30 10:34:45vstinnersetpull_requests: + pull_request16074
2019-09-30 10:31:01vstinnersetpull_requests: + pull_request16073
2019-09-29 21:21:55vstinnersetmessages: + msg353510
2019-09-29 21:19:10vstinnersetmessages: + msg353509
2019-09-29 07:03:41ned.deilysetnosy: + ned.deily
messages: + msg353490
2019-09-29 06:50:37ncoghlansetnosy: + ncoghlan
messages: + msg353487
2019-09-28 02:54:34vstinnersetstatus: open -> closed
priority: release blocker ->
messages: + msg353437

resolution: fixed
stage: patch review -> resolved
2019-09-28 02:50:46vstinnersetmessages: + msg353436
2019-09-28 02:30:21vstinnersetpull_requests: + pull_request16033
2019-09-28 02:28:37vstinnersetmessages: + msg353435
2019-09-28 02:18:02vstinnersetmessages: + msg353432
2019-09-28 02:08:42vstinnersetkeywords: + patch
stage: patch review
pull_requests: + pull_request16031
2019-09-28 00:32:45vstinnercreate