-
Notifications
You must be signed in to change notification settings - Fork 915
Fix memory leak in msg.headers() #458
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
Conversation
17845b1
to
1488fb2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd add a quick update to the RELEASE.md to ensure we don't forget to keep the CP version up to date.
Will merge as soon as these packages are verified. |
@rnpridgeon Might want to re-review this since there's been changes, thanks |
Added an additional step to validate interceptor packaging in the wheels. I did not add this as an |
.travis.yml
Outdated
@@ -66,7 +66,8 @@ script: | |||
- if [[ -z $CIBW_BEFORE_BUILD ]]; then py.test -v --timeout 20 --ignore=tmp-build --import-mode append ; fi | |||
- if [[ -n $TRAVIS_TAG && -n $CIBW_BEFORE_BUILD ]]; then cibuildwheel --output-dir wheelhouse1 && tools/fixup-wheels.sh wheelhouse1 wheelhouse ; fi | |||
- if [[ -n $TRAVIS_TAG && $TRAVIS_OS_NAME == linux && -n $CIBW_BEFORE_BUILD ]]; then tools/test-manylinux.sh ; fi | |||
|
|||
- if [[ -n $TRAVIS_TAG && -n $CIBW_BEFORE_BUILD ]]; then pip install --no-index -f wheelhouse/ confluent-kafka;\ | |||
py.test -v --timeout 20 --ignore=tmp-build --import-mode append; tests/test_misc.py::test_unordered_dict; fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... append; tests/test_misc...
Should there really be a ; in there?
Also, the tab indent is very red in the diff
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should probably use &&
instead of ;
between pip install and py.test.
Another thing to note is that it is error prone to run the tests from within the confluent-kafka-python source tree since import confluent_kafka
may pick up the local directory, better to cd out to the parent directory.
.travis.yml
Outdated
@@ -66,8 +66,7 @@ script: | |||
- if [[ -z $CIBW_BEFORE_BUILD ]]; then py.test -v --timeout 20 --ignore=tmp-build --import-mode append ; fi | |||
- if [[ -n $TRAVIS_TAG && -n $CIBW_BEFORE_BUILD ]]; then cibuildwheel --output-dir wheelhouse1 && tools/fixup-wheels.sh wheelhouse1 wheelhouse ; fi | |||
- if [[ -n $TRAVIS_TAG && $TRAVIS_OS_NAME == linux && -n $CIBW_BEFORE_BUILD ]]; then tools/test-manylinux.sh ; fi | |||
- if [[ -n $TRAVIS_TAG && -n $CIBW_BEFORE_BUILD ]]; then pip install --no-index -f wheelhouse/ confluent-kafka;\ | |||
py.test -v --timeout 20 --ignore=tmp-build --import-mode append; tests/test_misc.py::test_unordered_dict; fi | |||
- if [[ -n $TRAVIS_TAG && -n $CIBW_BEFORE_BUILD ]]; then pip install --no-index -f wheelhouse/ confluent-kafka && cd .. && pytest -v --timeout 20 --ignore=tmp-build --import-mode append confluent-kafka-python/tests/test_misc.py::test_unordered_dict; fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put the cd and the command in a subshell, or cd .. back out, otherwise any sub-sequent commands will fail because the directory changed:
- if [[ -n $TRAVIS_TAG && -n $CIBW_BEFORE_BUILD ]]; then pip install --no-index -f wheelhouse/ confluent-kafka && (cd .. && pytest -v --timeout 20 --ignore=tmp-build --import-mode append confluent-kafka-python/tests/test_misc.py::test_unordered_dict); fi
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, I guess there are no guarantees this will always be the last script executed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, it will help the next poor chap that adds a command here.
Moved interceptor inclusion work to own branch