Skip to content
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

Build broken on Python 3.10 #46

Closed
njsmith opened this issue May 28, 2020 · 7 comments · Fixed by #52
Closed

Build broken on Python 3.10 #46

njsmith opened this issue May 28, 2020 · 7 comments · Fixed by #52

Comments

@njsmith
Copy link

njsmith commented May 28, 2020

python/cpython#20429

  immutables/_map.c: In function ‘map_node_bitmap_new’:
  immutables/_map.c:574:19: error: lvalue required as left operand of assignment
    574 |     Py_SIZE(node) = size;
        |                   ^
  immutables/_map.c: In function ‘map_node_collision_new’:
  immutables/_map.c:1359:19: error: lvalue required as left operand of assignment
   1359 |     Py_SIZE(node) = size;
        |                   ^
  error: command '/usr/bin/gcc' failed with exit code 1

@vstinner If the motivation is to make PyObject* opaque in the limited ABI, then why did you also make this change in the non-limited ABI?

@vstinner
Copy link
Contributor

You should use Py_SET_SIZE() which is available since Python 3.9. To support Python 3.8 and older, you can use:

#ifndef Py_SET_SIZE
#  define Py_SET_SIZE(obj, size) do { Py_SIZE(obj) = (size); } while (0)
#endif

@vstinner If the motivation is to make PyObject* opaque in the limited ABI, then why did you also make this change in the non-limited ABI?

My goal is to move slowly C extensions towards the limited C API, and so use the stable ABI.

For the rationale, see:

@vstinner
Copy link
Contributor

vstinner commented Jun 3, 2020

Sorry, it's not a good idea to use #ifndef Py_SET_SIZE. Tomorrow, Py_SET_SIZE may become a function (I have a plan like that, especially for the limited C API). A better fix is:

#if PY_VERSION_HEX < 0x030900A4
#  define Py_SET_TYPE(obj, size) do { Py_TYPE(obj) = (size); } while (0)
#endif

I proposed python/cpython#20610 to enhance the documentation to explain how to port existing code to Py_SET_SIZE(): to propose this macro for backward compatibility.

@vstinner
Copy link
Contributor

vstinner commented Dec 4, 2020

I created PR #52 to add Python 3.10 support to immutables.

@1st1 1st1 closed this as completed in #52 Dec 7, 2020
@vstinner
Copy link
Contributor

vstinner commented Dec 7, 2020

I tested manually on Fedora 33 with Python 3.10.0a2:

$ python3.10 -m venv ENV
$ ENV/bin/python setup.py install
$ ENV/bin/python -m pip install pytest
$ ENV/bin/python -m pytest

It's now possible to build immutables on Python 3.10 and the test suite pass ("75 passed, 77 skipped in 4.45s").

@1st1
Copy link
Member

1st1 commented Jan 5, 2021

77 skipped

That's probably the C tests that were skipped. So there might be still some problem why the C extension wasn't built under 3.10

@vstinner
Copy link
Contributor

vstinner commented Jan 5, 2021

That's probably the C tests that were skipped. So there might be still some problem why the C extension wasn't built under 3.10

Oh. When tests are run from the source tree, "immutables._map" cannot be imported.

If I create a venv and run the venv Python outside the source tree, I can import the extension.

So the issue is just how I ran tests.

immutables are not installed by pip. So I'm not sure what is the safe way to install it and then run tests? I had to remove immutables/ directory from the source tree to keep tests and be able to import immutables._map.

@1st1
Copy link
Member

1st1 commented Jan 5, 2021

If I create a venv and run the venv Python outside the source tree, I can import the extension.

I usually create a venv and do pip install -e . in it. Everything then imports and tests perfectly from anywhere.

fantix added a commit that referenced this issue Feb 9, 2021
New Features

* Add support for Python 3.10 and more tests
  (by @vstinner in 45105ec for #46, @hukkinj1 in d7f3eeb, f0b4fd4)

* Make __repr__ more similar to other mapping types
  (by @ofek in 8af1502 for #17)

Misc

* Minor docs and CI fixes
  (by @MisterKeefe in 76e491c for #32, @fantix in 1282379 for #39)
@fantix fantix mentioned this issue Feb 9, 2021
fantix added a commit that referenced this issue Feb 9, 2021
New Features

* Add support for Python 3.10 and more tests
  (by @vstinner in 45105ec for #46, @hukkinj1 in d7f3eeb, f0b4fd4)

* Make __repr__ more similar to other mapping types
  (by @ofek in 8af1502 for #17)

Misc

* Minor docs and CI fixes
  (by @MisterKeefe in 76e491c for #32, @fantix in 1282379 for #39)
fantix added a commit that referenced this issue Feb 10, 2021
New Features

* Add support for Python 3.10 and more tests
  (by @vstinner in 45105ec for #46, @hukkinj1 in d7f3eeb, f0b4fd4)

* Make __repr__ more similar to other mapping types
  (by @ofek in 8af1502 for #17)

Misc

* Minor docs and CI fixes
  (by @MisterKeefe in 76e491c for #32, @fantix in 1282379 for #39)
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 a pull request may close this issue.

3 participants