Modernizing: Restoring CI, Moving to pytest #136
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
chayim
changed the title
|
@illia-v So here we are. CI is better, migrations to pytest happened. You'll note that this now runs python 3.7-3.10, and on windows, mac, and linux. BUT: On python 3.8-3.10 core dumps after the tests complete. Any chance you can help out with that? |
|
@chayim I've found what caused this bug, below is the patch: diff --git a/src/reader.c b/src/reader.c
index bf99b58..b472c24 100644
--- a/src/reader.c
+++ b/src/reader.c
@@ -288,9 +288,12 @@ static int Reader_init(hiredis_ReaderObject *self, PyObject *args, PyObject *kwd
if (!_Reader_set_exception(&self->replyErrorClass, replyErrorClass))
return -1;
- if (notEnoughData)
+ if (notEnoughData) {
+ Py_DECREF(self->notEnoughDataObject);
self->notEnoughDataObject = notEnoughData;
-
+ Py_INCREF(self->notEnoughDataObject);
+ }
+
return _Reader_set_encoding(self, encoding, errors);
}
@@ -375,6 +378,7 @@ static PyObject *Reader_gets(hiredis_ReaderObject *self, PyObject *args) {
}
if (obj == NULL) {
+ Py_INCREF(self->notEnoughDataObject);
return self->notEnoughDataObject;
} else {
/* Restore error when there is one. */ |
|
First off awesome. But... We should contribute back to hiredis. Mind cross linking the PR there? @michael-grunder would probably love that? |
|
Always happy for PRs, but I don't see any changes to hiredis here. Unless I'm missing something, the above patch just modifies |
yes, it's hiredis-py/src/reader.c |
|
Sorry guys - this was on mobile, I didn't realize it was in src/reader.c. Ignore my comment. |
|
Okay.. the wheel list is very impressive here. I need a bit to figure out how to make this Github Actions-able, across all of these platforms. Unless someone else knows :p |
|
The newly built wheel list - what will now be uploaded. Note python 3.10, and 3.11 are now included. |
shacharPash
approved these changes
Dec 6, 2022
illia-v
reviewed
Dec 6, 2022
|
@DvirDukhan you might be interested in the GitHub CI side of this |
|
@ifduyue Given these changes, I'm voting for 2.1.0 on hiredis. I'll send up a PR after this one, to release. Fair? |
|
@chayim looks good, ship it! |
ad-m-ss
reviewed
Dec 12, 2022
| os: ['ubuntu-latest', 'windows-latest', 'macos-latest'] | ||
| fail-fast: false | ||
| env: | ||
| ACTIONS_ALLOW_UNSECURE_COMMANDS: true |
There was a problem hiding this comment.
Why do you need it? I don't see any need for that as no need to run deprecated commands set-env and add-path.
miketheman
added a commit
to miketheman/warehouse
that referenced
this pull request
Dec 16, 2022
Back in pypi#4584 we switched to using a source distribution. This was circa hiredis 0.2.0. As of hiredis 2.1.0, no source distributions are provided to PyPI, so this step fails to install any updated requirements. Refs: redis/hiredis-py#136 Signed-off-by: Mike Fiedler <[email protected]>
miketheman
added a commit
to miketheman/warehouse
that referenced
this pull request
Dec 16, 2022
Back in pypi#4584 we switched to using a source distribution. This was circa hiredis 0.2.0. As of hiredis 2.1.0, no source distributions are provided to PyPI, so this step fails to install any updated requirements. Refs: redis/hiredis-py#136 Signed-off-by: Mike Fiedler <[email protected]>
ewdurbin
pushed a commit
to pypi/warehouse
that referenced
this pull request
Dec 16, 2022
* Bump hiredis from 2.0.0 to 2.1.0 Bumps [hiredis](https://github.com/redis/hiredis-py) from 2.0.0 to 2.1.0. - [Release notes](https://github.com/redis/hiredis-py/releases) - [Changelog](https://github.com/redis/hiredis-py/blob/master/CHANGELOG.md) - [Commits](redis/hiredis-py@v2.0.0...v2.1.0) --- updated-dependencies: - dependency-name: hiredis dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> * fix: use compiled hiredis Back in #4584 we switched to using a source distribution. This was circa hiredis 0.2.0. As of hiredis 2.1.0, no source distributions are provided to PyPI, so this step fails to install any updated requirements. Refs: redis/hiredis-py#136 Signed-off-by: Mike Fiedler <[email protected]> Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: Mike Fiedler <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR modernizes the hiredis-py build and test cycles. Builds will now run tests, in parallel in pytest - replacing Travis.
This change also makes it possible to release, via the release drafter, to PyPI, once I add the appropriate secrets to repository. This follows the same method as redis-py, whereby making a release using the release drafter, triggers the pypi publish.
In the case, the release publish will in parallel, build wheels for all the associated platforms. These wheels are then collected, and uploaded to pypi. Testing everything other than the upload can be found in this action run.
closes #120
closes #122
closes #124
closes #129
closes #130
closes #121
closes #118