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

Use Py_SET_REFCNT(self, refcnt); instead of Py_REFCNT(self) = refcnt;. #271

Closed
polkovnikov opened this issue Nov 8, 2021 · 11 comments
Closed

Comments

@polkovnikov
Copy link

Starting from Python 3.10 your library doesn't compile, it has just single compile issue, in file handle.c you do
Py_REFCNT(self) = refcnt;, but you should instead do Py_SET_REFCNT(self, refcnt);.

Starting from Python 3.10 it is not valid to set refcnt directly by assignment operator. Because now Py_REFCNT() is a function that returns l-value which is not assignable.

Py_SET_REFCNT() for setting refcount was present in all Python versions, so it is safe to use this setter function instead of direct assignment for all Python versions.

More than that in Python 3.10 it was the only compilation issue (at least on Windows), after fixing it wheel was built successfully from current repository trunk.

Also would be nice if you release prebuilt binary wheels on PyPi for all Python versions, currently it looks like that there are no prebuilt wheels for Python versions >= 3.7.

BTW, Thanks for great Python package!

@oliver-sanders
Copy link

BTW, Thanks for great Python package!

Thanks from me too.

Also would be nice if you release prebuilt binary wheels on PyPi for all Python versions

See #270. This could be fixed with a new release.

Starting from Python 3.10 your library doesn't compile

See also this comment on the Conda Forge feedstock:

conda-forge/pyuv-feedstock#12 (comment)

Unfortunately the original author no longer has the time to work on this project. Would you be able to propose this change in a PR?

@saghul
Copy link
Owner

saghul commented Nov 25, 2021

Unfortunately the original author no longer has the time to work on this project. Would you be able to propose this change in a PR?

Happy to give the commit bit to someone who has the time to drive it forward.

@oliver-sanders
Copy link

Might be a good idea to add the "maintainer wanted" topic to the repo - https://github.com/topics/maintainer-wanted

@bjornfor
Copy link

@polkovnikov: Can you turn your proposed fix into a PR please? (If not, let me know and I can probably make some copy-pasta PR and give you credit in the commit message.)

Why I'm here: my tmux uses powerline which uses pyuv -- I either have to remove powerline or fix pyuv.

@polkovnikov
Copy link
Author

@bjornfor Please do this PR instead of me, just replace this line with Py_SET_REFCNT(self, refcnt);.

This change was enough for me to make this project compilable.

You may even do this without any credit to me. But as you wish.

@bjornfor
Copy link

bjornfor commented Jul 1, 2022

FYI, I added the change and started rebuilding pyuv against python3.7/3.8/3.9/3.10 but got build error undefined symbol: Py_SET_REFCNT for python<3.9. Python documentation confirms this observation.

@polkovnikov
Copy link
Author

polkovnikov commented Jul 2, 2022

@bjornfor Current (unfixed) code should work for Python < 3.9, but new code with Py_SET_REFCNT should work for Python >= 3.9.

Near python.h file there is file patchlevel.h which has:

#define PY_MAJOR_VERSION        3
#define PY_MINOR_VERSION        9

so you can replace line with:

#if PY_MAJOR_VERSION * 100 + PY_MINOR_VERSION >= 309
    Py_SET_REFCNT(self, refcnt);
#else
    Py_REFCNT(self) = refcnt;
#endif

You may even implement a helper inline function that does fixed code above. But right now this code is used only in 1 line of whole project, so might be that helper is not needed, although should look nice if implemented.

@bjornfor
Copy link

bjornfor commented Jul 3, 2022

@polkovnikov: Thanks! (I actually came up with something similar on my own.)

Now the only remaining question is which branch should I make the PR against, @saghul ? v1.x or master?

It seems v1.x has two commits missing from master, not sure why. The last release seems to have been made from master branch (although the tag is reachable from both branches).

@saghul
Copy link
Owner

saghul commented Jul 3, 2022

master please.

bjornfor added a commit to bjornfor/pyuv that referenced this issue Jul 3, 2022
Specifically this build error:

  In file included from src/pyuv.c:8:
  src/handle.c: In function ‘resurrect_object’:
  src/handle.c:17:21: error: lvalue required as left operand of assignment
     17 |     Py_REFCNT(self) = refcnt;
        |                     ^

Fixes saghul#271.

Co-authored-by: @polkovnikov
@bjornfor
Copy link

bjornfor commented Jul 3, 2022

PR ready: #275

bjornfor added a commit to bjornfor/pyuv that referenced this issue Jul 3, 2022
Specifically this build error:

  In file included from src/pyuv.c:8:
  src/handle.c: In function ‘resurrect_object’:
  src/handle.c:17:21: error: lvalue required as left operand of assignment
     17 |     Py_REFCNT(self) = refcnt;
        |                     ^

Fixes saghul#271.

Co-authored-by: @polkovnikov
saghul pushed a commit that referenced this issue Jul 4, 2022
Specifically this build error:

  In file included from src/pyuv.c:8:
  src/handle.c: In function ‘resurrect_object’:
  src/handle.c:17:21: error: lvalue required as left operand of assignment
     17 |     Py_REFCNT(self) = refcnt;
        |                     ^

Fixes #271.

Co-authored-by: @polkovnikov
@bjornfor
Copy link

bjornfor commented Jul 4, 2022

This can be closed now. (I thought my "Fixes ..." comment in the commit message would auto-close, but it didn't.)

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

No branches or pull requests

4 participants