On Mon, 30 Sep 2019 at 09:37, Victor Stinner <vstinner@python.org> wrote:
Le lun. 30 sept. 2019 à 00:33, Nick Coghlan <ncoghlan@gmail.com> a écrit :
As noted above, despite what I wrote on BPO, you no longer need to persuade me that the version check is desirable, only that a narrow check on specific struct sizes is preferable to a broad check on the expected API version.
I understand that your main motivation to use the Python version number rather than sizeof(PyConfig) is the error message.
No, my main motivation is to create an API that can emit a useful error message on *ALL* version conflicts between an embedding application and the embedded runtime, not just version conflicts involving versions that change the size of the config structs. The latter option is a good one if all we want to version is the struct itself, but I don't think that's what we really want here: I think we want to version check the entire API/ABI. The improved error message is just a nice bonus, as is the fact that I think the following code is more self-explanatory than the size checks: PyConfig config; config.header_version = PY_VERSION_HEX; PyStatus status = PyConfig_InitPythonConfig(&config); The documentation would say something like "header_version should specify the version of the CPython headers that was used to build and link the embedding application. The simplest way to set it correctly is to set it to ``PY_VERSION_HEX``".
If we implement support for older PyConfig ("stable ABI"), you will simply never see this error: it will just work transparently.
But that's true regardless of whether the consistency check is based on the struct size or the header version.
IMHO the current error message is good enough:
if (config->struct_size != sizeof(PyConfig)) { return _PyStatus_ERR("unsupported PyConfig structure size " "(Python version mismatch?)"); }
That error message won't trigger if the public API struct that changed size is PyTypeObject or PyObject (or any of the other structs in the full API/ABI), whereas a PY_VERSION_HEX based check can be made to fail based on any policy we choose to enforce. For example, we could make the runtime enforce the following pair of rules: * if both the header version and runtime version are final releases or release candidates, then only the major and minor version fields need to match (the X.Y in X.Y.Z) * if either the header version or the runtime version is an alpha or beta release (or an unknown release type), then the entire version number must match exactly Or we could be more permissive, and only ever enforce the first rule, even for alpha and beta releases.
I wrote a proof-of-concept to check if it would be doable to support multiple versions (sizes) of PyConfig: it's doable and it's quite easy to implement, a few lines of code. For example, support Python 3.8 PyConfig in Python 3.9.
That doesn't change all that much with a Py_VERSION_HEX based check - it just needs an extra table for range based lookups to find the relevant struct size for the given version, together with keeping definitions of the old config structs around. typedef struct { uint32_t firstVersion; Py_ssize_t structSize; } _Py_versionToSize; static _Py_versionToSize _PyConfigStructSizes { {0x03090000, sizeof(_PyConfig_3_9)}, {0, sizeof(_PyConfig_3_8)} } If the given version matched, you wouldn't need to do the size lookup. You also wouldn't be constrained to having the lookup table be from versions to struct sizes - it could instead be from versions to function pointers, or some other such thing, based on whatever made the most sense given how the rest of the config system worked in any given release. Cheers, Nick. -- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia