Use Py_SET_SIZE() in _pickle_33.c #64

vstinner · 2022-01-27T19:51:03Z

Use Python 3.9 Py_SET_SIZE() function to set an object size. Add a
Py_SET_SIZE() implementation for Python 3.8 and older.

On Python 3.11, Py_SIZE() can no longer be used as an l-value to set
an object size:

Use the Py_REFCNT() and Py_TYPE() macros rather than accessing
directly to ob_refcnt and ob_type members of the PyObject structure.

Use Python 3.9 Py_SET_SIZE() function to set an object size. Add a Py_SET_SIZE() implementation for Python 3.8 and older. On Python 3.11, Py_SIZE() can no longer be used as an l-value to set an object size: * https://docs.python.org/dev/c-api/structures.html#c.Py_SIZE * https://docs.python.org/dev/whatsnew/3.11.html#id2 Use the Py_REFCNT() and Py_TYPE() macros rather than accessing directly to ob_refcnt and ob_type members of the PyObject structure.

vstinner · 2022-01-27T19:54:46Z

src/zodbpickle/_pickle_33.c already supports Python 3.10 thanks to this macro added by commit 8d99afc:

#if (PY_VERSION_HEX >= 0x30A00B1) /* 3.10.0b1 */
#ifndef Py_SIZE
#define Py_SIZE(ob) (((PyVarObject*)(ob))->ob_size)
#endif
...

The intent of Py_SET_SIZE() (added to Python 3.9) is to hide implementation details, whereas the redefined Py_SIZE() macro can be used again as an l-value to set directly the PyObject.ob_size member.

See my draft https://www.python.org/dev/peps/pep-0674/ for the rationale, and https://bugs.python.org/issue39573 for the issue which changed the Py_SIZE() macro to disallow using it as an l-value.

In Python 3.11, Py_SIZE() is modified to disallow using it as an l-value.

This requires the changes from #64 to run successfully.

icemac · 2022-01-31T08:49:25Z

@vstinner Thank you for your contribution.

Running your commit on Python 3.11.0a4 on MacOS 11.6 I get the following errors:

$ tox -repy311
py311 recreate: /.../zodbpickle/.tox/py311
py311 develop-inst: /.../zodbpickle
ERROR: invocation failed (exit code 1), logfile: /.../zodbpickle/.tox/py311/log/py311-1.log
====================================================================== log start =======================================================================
Looking in indexes: https://pypi.org/simple, https://development....de/devpi/my/+simple/
Obtaining file:///.../zodbpickle
  Preparing metadata (setup.py): started
  Preparing metadata (setup.py): finished with status 'done'
Requirement already satisfied: setuptools in ./.tox/py311/lib/python3.11/site-packages (from zodbpickle==2.2.1.dev0) (58.3.0)
Collecting zope.testrunner
  Using cached https://development....de/devpi/my/%2Bf/ae7/fbeb862a36083/zope.testrunner-5.4.0-py2.py3-none-any.whl (216 kB)
Collecting six
  Using cached https://development....de/devpi/root/pypi/%2Bf/8ab/b2f1d86890a2d/six-1.16.0-py2.py3-none-any.whl (11 kB)
Collecting zope.interface
  Using cached zope.interface-5.4.0-cp311-cp311-macosx_11_0_x86_64.whl
Collecting zope.exceptions
  Using cached https://development....de/devpi/my/%2Bf/bb9/8cc07e90ebe59/zope.exceptions-4.4-py2.py3-none-any.whl (18 kB)
Installing collected packages: zope.interface, zope.exceptions, six, zope.testrunner, zodbpickle
  Running setup.py develop for zodbpickle
    ERROR: Command errored out with exit status 1:
     command: /.../zodbpickle/.tox/py311/bin/python -c 'import io, os, sys, setuptools, tokenize; sys.argv[0] = '"'"'/.../zodbpickle/setup.py'"'"'; __file__='"'"'/.../zodbpickle/setup.py'"'"';f = getattr(tokenize, '"'"'open'"'"', open)(__file__) if os.path.exists(__file__) else io.StringIO('"'"'from setuptools import setup; setup()'"'"');code = f.read().replace('"'"'\r\n'"'"', '"'"'\n'"'"');f.close();exec(compile(code, __file__, '"'"'exec'"'"'))' develop --no-deps
         cwd: /.../zodbpickle/
    Complete output (34 lines):
    running develop
    /.../zodbpickle/.tox/py311/lib/python3.11/site-packages/setuptools/command/easy_install.py:156: EasyInstallDeprecationWarning: easy_install command is deprecated. Use build and pip and other standards-based tools.
      warnings.warn(
    /.../zodbpickle/.tox/py311/lib/python3.11/site-packages/setuptools/command/install.py:34: SetuptoolsDeprecationWarning: setup.py install is deprecated. Use build and pip and other standards-based tools.
      warnings.warn(
    running egg_info
    writing src/zodbpickle.egg-info/PKG-INFO
    writing dependency_links to src/zodbpickle.egg-info/dependency_links.txt
    writing requirements to src/zodbpickle.egg-info/requires.txt
    writing top-level names to src/zodbpickle.egg-info/top_level.txt
    reading manifest file 'src/zodbpickle.egg-info/SOURCES.txt'
    reading manifest template 'MANIFEST.in'
    adding license file 'LICENSE.txt'
    writing manifest file 'src/zodbpickle.egg-info/SOURCES.txt'
    running build_ext
    building 'zodbpickle._pickle' extension
    /usr/bin/clang -fno-common -dynamic -DNDEBUG -g -fwrapv -O3 -Wall -pipe -Os -isysroot/Library/Developer/CommandLineTools/SDKs/MacOSX11.sdk -I/.../zodbpickle/.tox/py311/include -I/opt/local/Library/Frameworks/Python.framework/Versions/3.11/include/python3.11 -c src/zodbpickle/_pickle_33.c -o build/temp.macosx-11.0-x86_64-3.11/src/zodbpickle/_pickle_33.o
    src/zodbpickle/_pickle_33.c:272:23: error: expression is not assignable
        return self->data[--Py_SIZE(self)];
                          ^ ~~~~~~~~~~~~~
    src/zodbpickle/_pickle_33.c:282:29: error: expression is not assignable
        self->data[Py_SIZE(self)++] = obj;
                   ~~~~~~~~~~~~~^
    src/zodbpickle/_pickle_33.c:1698:13: error: implicit declaration of function '_PyFloat_Pack8' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
            if (_PyFloat_Pack8(x, (unsigned char *)&pdata[1], 0) < 0)
                ^
    src/zodbpickle/_pickle_33.c:1757:29: warning: result of comparison of constant 128 with expression of type 'char' is always false [-Wtautological-constant-out-of-range-compare]
            if (ch < 0x20 || ch >= 0x80 || ch == '\'' || ch == '\\') {
                             ~~ ^  ~~~~
    src/zodbpickle/_pickle_33.c:4250:9: error: implicit declaration of function '_PyFloat_Unpack8' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
        x = _PyFloat_Unpack8((unsigned char *)s, 0);
            ^
    1 warning and 4 errors generated.
    error: command '/usr/bin/clang' failed with exit code 1
    ----------------------------------------
ERROR: Command errored out with exit status 1: /.../zodbpickle/.tox/py311/bin/python -c 'import io, os, sys, setuptools, tokenize; sys.argv[0] = '"'"'/.../zodbpickle/setup.py'"'"'; __file__='"'"'/.../zodbpickle/setup.py'"'"';f = getattr(tokenize, '"'"'open'"'"', open)(__file__) if os.path.exists(__file__) else io.StringIO('"'"'from setuptools import setup; setup()'"'"');code = f.read().replace('"'"'\r\n'"'"', '"'"'\n'"'"');f.close();exec(compile(code, __file__, '"'"'exec'"'"'))' develop --no-deps Check the logs for full command output.
WARNING: You are using pip version 21.3.1; however, version 22.0.2 is available.
You should consider upgrading via the '/.../zodbpickle/.tox/py311/bin/python -m pip install --upgrade pip' command.

======================================================================= log end ========================================================================
_______________________________________________________________________ summary ________________________________________________________________________
ERROR:   py311: InvocationError for command /.../zodbpickle/.tox/py311/bin/python -m pip install --exists-action w -e '/.../zodbpickle[test]' (exited with code 1)

Am I doing something wrong there?

vstinner · 2022-01-31T13:07:27Z

Oh right, I missed two functions: fixed in my second commit.

This change is not enough to fully support Python 3.11. save_float() and load_binfloat() use private _PyFloat_Pack8() and _PyFloat_Unpack8() functions which moved to the internal C API in Python 3.11.

src/zodbpickle/_pickle_33.c: In function ‘save_float’:
src/zodbpickle/_pickle_33.c:1700:13: warning: implicit declaration of function ‘_PyFloat_Pack8’ [-Wimplicit-function-declaration]
 1700 |         if (_PyFloat_Pack8(x, (unsigned char *)&pdata[1], 0) < 0)
      |             ^~~~~~~~~~~~~~

src/zodbpickle/_pickle_33.c: In function ‘load_binfloat’:
src/zodbpickle/_pickle_33.c:4252:9: warning: implicit declaration of function ‘_PyFloat_Unpack8’; did you mean ‘PySlice_Unpack’? [-Wimplicit-function-declaration]
 4252 |     x = _PyFloat_Unpack8((unsigned char *)s, 0);
      |         ^~~~~~~~~~~~~~~~
      |         PySlice_Unpack

This function moved to the internal C API in Python 3.11: python/cpython@0a883a7

Maybe this function should be copied to zodbpickle as well.

Rather than forking the _pickle module, maybe the noload operation should be contributed to the upstream Python project, in the stdlib pickle module? I don't know if it makes sense. I don't know zodbpickle nor noload.

vstinner · 2022-01-31T13:18:20Z

This change is not enough to fully support Python 3.11. save_float() and load_binfloat() use private _PyFloat_Pack8() and _PyFloat_Unpack8() functions which moved to the internal C API in Python 3.11.

I would prefer to address that in a separated PR.

jamadden · 2022-01-31T13:20:41Z

maybe the noload operation should be contributed to the upstream Python project, in the stdlib pickle module

Indeed, noload did exist in upstream stdlib. However, noload support was deliberately crippled in Python 2.7 (http://bugs.python.org/issue1101399); the version used here does what was done in Python 2.6. While I would love to have a functional noload in core Python, I doubt it would be accepted unless the problems described in that issue can be fixed

vstinner · 2022-01-31T13:51:50Z

Indeed, noload did exist in upstream stdlib. However, noload support was deliberately crippled in Python 2.7 (http://bugs.python.org/issue1101399); the version used here does what was done in Python 2.6. While I would love to have a functional noload in core Python, I doubt it would be accepted unless the problems described in that issue can be fixed

Oh, that's an old issue. You should maybe propose again the idea? If it's useful to you, maybe other people outside Zope would find it useful?

icemac · 2022-02-01T07:08:33Z

As this PR drastically improves the Python 3.11 situation, I'd suggest to merge it and fix the remaining _PyFloat_Pack8/_PyFloat_Unpack8 issue separately.

@jamadden Do you think a signed contributor agreement is needed to merge this PR?

jamadden · 2022-02-01T11:25:13Z

Do you think a signed contributor agreement is needed to merge this PR?

I am not a lawyer, but: yes, I think so. The Plone Contributors Agreement FAQ seems pretty firm:

Do I have to sign the contributor's agreement to make checkins to the Plone core codebase?
Yes.

Only the Plone Release Manager can grant waivers:

Unless, in the judgement of the Plone Release Manager, the patch is so tiny as not to constitute a "creative work."

While this is small, it introduces new functions/function like macros and calls them, replacing direct inline code. I'm also not the release manager, but that seems creative to me.

vstinner · 2022-02-01T14:53:05Z

Do you think a signed contributor agreement is needed to merge this PR?

Should I sign or was the question for @icemac? The README file doesn't mention this document, and the project has no CONTRIBUTING document. If I have to sign it, I don't know how to sign it.

jugmac00 · 2022-02-01T14:57:38Z

The instruction would be here
https://www.zope.dev/developer/becoming-a-committer.html

P.S.: @icemac sounds like a good idea to add a contributor's document via the meta tool.

icemac · 2022-02-02T07:08:06Z

@vstinner @jugmac00 It seems like be a problem in GitHub: According to https://docs.github.com/en/communities/setting-up-your-project-for-healthy-contributions/creating-a-default-community-health-file it is possible to have a CONTRIBUTING.md in a .github repository in the organization to be used for each repository which does not have its own CONTRIBUTING.md.

We have https://github.com/zopefoundation/.github/blob/master/CONTRIBUTING.md. But maybe this only shows up at the right when creating the first PR.

@vstinner Yes, please sign the contributor agreement.

vstinner · 2022-02-02T13:26:01Z

We have https://github.com/zopefoundation/.github/blob/master/CONTRIBUTING.md. But maybe this only shows up at the right when creating the first PR.

I understood that https://github.com/zopefoundation/zodbpickle/ should have in the root directory, in a .github or a docs sub-directory. IMO the root directory is a good place. But it should not be a project called .github.

https://docs.github.com/en/communities/setting-up-your-project-for-healthy-contributions/creating-a-default-community-health-file#about-default-community-health-files

jugmac00 · 2022-02-02T13:32:27Z

We have https://github.com/zopefoundation/.github/blob/master/CONTRIBUTING.md. But maybe this only shows up at the right when creating the first PR.

I understood that https://github.com/zopefoundation/zodbpickle/ should have in the root directory, in a .github or a docs sub-directory. IMO the root directory is a good place. But it should not be a project called .github.

https://docs.github.com/en/communities/setting-up-your-project-for-healthy-contributions/creating-a-default-community-health-file#about-default-community-health-files

https://github.blog/changelog/2019-02-21-organization-wide-community-health-files/ and https://docs.github.com/en/communities/setting-up-your-project-for-healthy-contributions/creating-a-default-community-health-file - so in theory it should have worked

vstinner · 2022-02-02T14:13:46Z

@vstinner Yes, please sign the contributor agreement.

I read it and I'm sorry, I don't want to sign it. I don't want to transfer the rights of my header file to Plone, just to port zodbpickle to Python 3.11.

If zodbpickle cannot use pythoncapi_compat.h, I suggest you to copy/paste the code provided in What's New in Python 3.10 and What's New in Python 3.11 and write a new PR based on that.

vstinner · 2022-02-02T14:21:31Z

If zodbpickle cannot use pythoncapi_compat.h, I suggest you to copy/paste the code provided in What's New in Python 3.10 and What's New in Python 3.11 and write a new PR based on that.

What's New in Python 3.x code is distributed under the Zero Clause BSD License. See the notice at the bottom of the doc:

Examples, recipes, and other code in the documentation are additionally licensed under the Zero Clause BSD License.

vstinner · 2022-03-12T00:22:26Z

src/zodbpickle/_pickle_33.c:1700:13: warning: implicit declaration of function ‘_PyFloat_Pack8’ [-Wimplicit-function-declaration]
src/zodbpickle/_pickle_33.c:4252:9: warning: implicit declaration of function ‘_PyFloat_Unpack8’; did you mean ‘PySlice_Unpack’? [-Wimplicit-function-declaration]

I added new public functions to Python 3.11 alpha7 for zodbpickle: see #67

vstinner · 2022-03-12T00:23:45Z

If zodbpickle cannot use pythoncapi_compat.h, I suggest you to copy/paste the code provided in What's New in Python 3.10 and What's New in Python 3.11 and write a new PR based on that.

In the meanwhile, the pythoncapi_compat project moved under the Python organization on GitHub: https://github.com/python/pythoncapi_compat and its license changed from MIT to Zero Clause BSD (0BSD) license.

icemac · 2022-03-14T07:13:57Z

That's nice to hear. Thank you.

icemac pushed a commit that referenced this pull request Jan 31, 2022

Add support for Python 3.11.0a4.

a4c1902

This requires the changes from #64 to run successfully.

icemac mentioned this pull request Jan 31, 2022

Add support for Python 3.11.0a7. #65

Merged

4 tasks

Fix also Pdata_pop() and Pdata_push()

0a32c07

icemac mentioned this pull request Feb 2, 2022

Add CONTRIBUTING.md to the root of the target repository zopefoundation/meta#133

Merged

vstinner closed this Feb 2, 2022

vstinner deleted the py_set_size branch February 2, 2022 14:14

vstinner mentioned this pull request Mar 12, 2022

Status of 'noload' in Python 3 stdlib ? (i.e. let's try to kill this module for Python 3) #35

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Py_SET_SIZE() in _pickle_33.c #64

Use Py_SET_SIZE() in _pickle_33.c #64

vstinner commented Jan 27, 2022

vstinner commented Jan 27, 2022

icemac commented Jan 31, 2022

vstinner commented Jan 31, 2022

vstinner commented Jan 31, 2022

jamadden commented Jan 31, 2022

vstinner commented Jan 31, 2022

icemac commented Feb 1, 2022

jamadden commented Feb 1, 2022

vstinner commented Feb 1, 2022

jugmac00 commented Feb 1, 2022

icemac commented Feb 2, 2022

vstinner commented Feb 2, 2022

jugmac00 commented Feb 2, 2022

vstinner commented Feb 2, 2022

vstinner commented Feb 2, 2022

vstinner commented Mar 12, 2022

vstinner commented Mar 12, 2022

icemac commented Mar 14, 2022

Use Py_SET_SIZE() in _pickle_33.c #64

Use Py_SET_SIZE() in _pickle_33.c #64

Conversation

vstinner commented Jan 27, 2022

vstinner commented Jan 27, 2022

icemac commented Jan 31, 2022

vstinner commented Jan 31, 2022

vstinner commented Jan 31, 2022

jamadden commented Jan 31, 2022

vstinner commented Jan 31, 2022

icemac commented Feb 1, 2022

jamadden commented Feb 1, 2022

vstinner commented Feb 1, 2022

jugmac00 commented Feb 1, 2022

icemac commented Feb 2, 2022

vstinner commented Feb 2, 2022

jugmac00 commented Feb 2, 2022

vstinner commented Feb 2, 2022

vstinner commented Feb 2, 2022

vstinner commented Mar 12, 2022

vstinner commented Mar 12, 2022

icemac commented Mar 14, 2022