Skip to content

Conversation

@jamadden
Copy link
Member

Fixes #77

@jamadden
Copy link
Member Author

rebased on master to resolve conflicts with #106

Copy link
Member

@icemac icemac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

},
install_requires=[
'zope.interface',
"cffi ; platform_python_implementation == 'CPython'",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That requires a "recent" (2 years) setuptools. It's the (preferred, IIRC) replacement for the "empty extra" syntax.

@jamadden
Copy link
Member Author

Thanks!

@dataflake
Copy link
Member

@jamadden I know this is 7 years after the fact, but...

In #77 you were confident that CFFI is just a lightweight dependency and not problematic. From several years of experience we know this is not the case. It has been a never ending bugbear, especially around the time we are trying to implement support for newer Pythons and CFFI just doesn't keep up and breaks in unexpected ways. This is happening once again for 3.15 and we have to go through the many repositories that depend on persistent and remove testing for 3.15 until CFFI has caught up and we can enable it again. When that happens, who knows.

My suggestion would be to revert this PR at least partially to remove the dependency. Are there any really important reasons for this change other than the code simplification and test coverage you mentioned in #77?

I am volunteering to do this reversion because I am fed up with the constant issues.

@jamadden
Copy link
Member Author

I completely agree, CFFI has become much heavier weight than expected, and I have no objections to seeing it made optional or removed entirely, so long as a PURE_PYTHON mode is still functional --- its major advantage was performance in that mode, and, aside from debugging, the main need for performance in PURE_PYTHON mode was PyPy, but my understanding is that PyPy isn't expecting to make releases for Python 3.12+ so its relevance is diminishing.

@dataflake
Copy link
Member

So I have started working on this by taking the Python implementation _DequeRing as it existed before #107 and tried to add the _CFFIRing implementation as it exist today under the assumption that it's better than the version before #107.

The unit tests are a problem, a lot of tests for the ring class itself as well as those for the pickle cache seem to assume internals and APIs that exist on _CFFIRing but not on _DequeRing, which sticks to the basic IRing API that's in the same ring.py module. These additional methods make sense for the CFFI implementation, but I'm not sure what they should even do for the simple Python implementation.

I have created #227 with the current state of work and some example tracebacks.

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 this pull request may close these issues.

Can we require CFFI for PURE_PYTHON usage on CPython?

4 participants