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

Add the ability to force use of C extensions. #132

Merged
merged 2 commits into from
Mar 3, 2020
Merged

Add the ability to force use of C extensions. #132

merged 2 commits into from
Mar 3, 2020

Conversation

jamadden
Copy link
Member

With PURE_PYTHON=0, like in zope.interface.

Also always require all three extensions. This solves mysterious issues you can get if you wind up mixing and matching (#124).

Fixes #131

Add travis and tox tests for this.

if (!TimeStamp)
{
PyObject* ts_module;
ts_module = PyImport_ImportModule("persistent._timestamp");
Copy link
Member Author

Choose a reason for hiding this comment

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

This had to move from module initialization time because it created a cycle.

Note that it's importing the C version only now.

Also note the new error handling at the end (if !TimeStamp). That turned out to be a problem that resulted in a SystemError at one point; that shouldn't be possible now that we're getting the C implementation, however.

persistent/persistence.py Outdated Show resolved Hide resolved
Copy link
Member

@mgedmin mgedmin left a comment

Choose a reason for hiding this comment

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

The diff is too big for me right now, but I skimmed some of it

.travis.yml Outdated Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
@jamadden jamadden force-pushed the issue131 branch 3 times, most recently from 4192cd3 to 67e410f Compare February 26, 2020 00:55
@jamadden
Copy link
Member Author

Rebased on master.

@icemac
Copy link
Member

icemac commented Feb 26, 2020

TravisCI is green now (after restarting some jobs). Is there still something that needs to be done?

@jamadden
Copy link
Member Author

Awaiting review.

@mgedmin
Copy link
Member

mgedmin commented Feb 27, 2020

I still don't have the energy for reviewing this, sorry!

@icemac
Copy link
Member

icemac commented Feb 27, 2020

Same here, sorry, too.

With PURE_PYTHON=0, like in zope.interface.

Also always require all three extensions. This solves mysterious issues you can get if you wind up mixing and matching (#124).

Fixes #131

Add travis and tox tests for this.
@jamadden
Copy link
Member Author

I found a way to make the diff much smaller and simpler by avoiding the need to rename Persistent to PersistentPy. (use_c_impl updates the globals of the Python functions so that the name the class is defined by is still the name the class bodies see when they execute.) Hopefully that helps.

Copy link
Member

@mgedmin mgedmin left a comment

Choose a reason for hiding this comment

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

LGTM!

CHANGES.rst Show resolved Hide resolved
persistent/timestamp.py Outdated Show resolved Hide resolved
persistent/tests/test_timestamp.py Outdated Show resolved Hide resolved
persistent/tests/test_timestamp.py Outdated Show resolved Hide resolved
persistent/tests/test_timestamp.py Outdated Show resolved Hide resolved
persistent/picklecache.py Outdated Show resolved Hide resolved
persistent/persistence.py Outdated Show resolved Hide resolved
persistent/persistence.py Show resolved Hide resolved
persistent/_compat.py Outdated Show resolved Hide resolved
persistent/_compat.py Show resolved Hide resolved
- Let an empty value of PURE_PYTHON mean the same thing as it did before (no preference)
- Add tests for the functions that handle PURE_PYTHON
- Clean up imports in various places
- Add more tests for C TimeStamp hash code.
- Add some comments.
@jamadden jamadden merged commit 49c9ffb into master Mar 3, 2020
@jamadden jamadden deleted the issue131 branch March 3, 2020 14:37
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.

Support ability to require use of C extensions
3 participants