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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove `+` character from platform releases string #895

Merged
merged 3 commits into from Nov 29, 2017

Conversation

Projects
None yet
4 participants
@avinassh
Contributor

avinassh commented Nov 28, 2017

There is an issue when pika is used within GCP(Google Cloud Platform) and following patch tries to fix that.

Details:

pika has a method get_linux_version which returns Linux platform information tuple. However, on GCP, it fails. Since that method fails, its not possible to use pika and hence its makes impossible to use pika on GCP.

This is what one of the GCP vm instance returns when asked for platform information:

$ python
Python 2.7.10 (default, Aug 30 2017, 20:23:35)
[GCC 4.9.x 20150123 (prerelease)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import platform
>>> platform.release()
'4.4.64+'
>>>

Due + character, get_linux_version fails and thus, we cannot import pika:

$ python
Python 2.7.13 (default, Sep 14 2017, 23:47:22)
[GCC 4.9.2] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import pika
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/local/lib/python2.7/site-packages/pika/__init__.py", line 17, in <module>
    from pika.connection import ConnectionParameters
  File "/usr/local/lib/python2.7/site-packages/pika/connection.py", line 18, in <module>
    from pika import callback
  File "/usr/local/lib/python2.7/site-packages/pika/callback.py", line 8, in <module>
    from pika import frame
  File "/usr/local/lib/python2.7/site-packages/pika/frame.py", line 7, in <module>
    from pika import spec
  File "/usr/local/lib/python2.7/site-packages/pika/spec.py", line 16, in <module>
    from pika import data
  File "/usr/local/lib/python2.7/site-packages/pika/data.py", line 10, in <module>
    from pika.compat import PY2, PY3
  File "/usr/local/lib/python2.7/site-packages/pika/compat.py", line 143, in <module>
    LINUX_VERSION = get_linux_version(platform.release())
  File "/usr/local/lib/python2.7/site-packages/pika/compat.py", line 134, in get_linux_version
    return tuple(map(int, ver_str.split('.')[:3]))
ValueError: invalid literal for int() with base 10: '70+'

Steps to reproduce:

I couldn't figure out a way to reproduce this bug outside GCP, on my local machine etc. So, to reproduce this bug I think one needs an access to GCP. Create a VM instance and try above. I created a Kubernetes installation (which does the VMs installations too) and tried this. This platform information is also passed to Docker, hence my Docker containers are also failing.

Let me know if any information is needed. Or a better way to fix this.

Thank you 馃槂

@lukebakken

This comment has been minimized.

Contributor

lukebakken commented Nov 28, 2017

Could you see if there are other Python projects that have this issue on GCP and what their workaround is?

@avinassh

This comment has been minimized.

Contributor

avinassh commented Nov 28, 2017

@lukebakken I don't know of any packages which rely on platform info. Do you happen to know any? If so, I will start from there.

@codecov

This comment has been minimized.

codecov bot commented Nov 28, 2017

Codecov Report

Merging #895 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #895      +/-   ##
==========================================
+ Coverage   81.92%   81.96%   +0.03%     
==========================================
  Files          21       21              
  Lines        3752     3759       +7     
  Branches      554      555       +1     
==========================================
+ Hits         3074     3081       +7     
  Misses        527      527              
  Partials      151      151
Impacted Files Coverage 螖
pika/compat.py 96.92% <100%> (+0.37%) 猬嗭笍

Continue to review full report at Codecov.

Legend - Click here to learn more
螖 = absolute <relative> (impact), 酶 = not affected, ? = missing data
Powered by Codecov. Last update 9a03d44...d339793. Read the comment docs.

@avinassh

This comment has been minimized.

Contributor

avinassh commented Nov 28, 2017

Updated PR, added a test for the same.

@ofpiyush

This comment has been minimized.

ofpiyush commented Nov 28, 2017

Our workaround was to clean it ourselves and monkey patch this line on platform just in case something else assumed platform.release() to be in X.X.X-something format.

I am looking for the source of why platform.release() is assumed to be in that format.

@katajakasa any ideas?

@katajakasa

This comment has been minimized.

Contributor

katajakasa commented Nov 28, 2017

Interesting, I expected the release to return a clean version string since that is what it shows in python documentation (https://docs.python.org/3/library/platform.html#platform.release).

As the LINUX_VERSION is only checked if python does not natively support TCP_USER_TIMEOUT, it might be bet to just wrap the contents of the function to a try-except and catch ValueError, then return None on exception. This might make old kernels with weird release strings disable the TCP_USER_TIMEOUT support, but that is probably preferable to completely blowing up pika with new kernels.

I'd offer a patch but I'm currently away from my dev machine, sorry ;)

@katajakasa

This comment has been minimized.

Contributor

katajakasa commented Nov 28, 2017

Actually, looks like py-amqp has a solution for this: https://github.com/celery/py-amqp/blob/master/amqp/platform.py#L24

Edit: Also, py-amqp has had the exact same problem: celery/py-amqp#119

@avinassh

This comment has been minimized.

Contributor

avinassh commented Nov 29, 2017

@katajakasa Thank you! I have updated the PR + added some more tests

@ofpiyush

FWIW LGTM

@katajakasa

Looks good here too.

@lukebakken

This comment has been minimized.

Contributor

lukebakken commented Nov 29, 2017

Thanks everyone for the PR and the input.

@lukebakken lukebakken merged commit 387cb32 into pika:master Nov 29, 2017

4 checks passed

codecov/patch 100% of diff hit (target 81.92%)
Details
codecov/project 81.96% (+0.03%) compared to 9a03d44
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@lukebakken lukebakken added this to the 0.11.2 milestone Nov 29, 2017

@avinassh avinassh deleted the avinassh:version-fix branch Nov 29, 2017

gbartl pushed a commit to gbartl/pika that referenced this pull request Jan 27, 2018

Merge pull request pika#895 from avinassh/version-fix
Remove `+` character from platform releases string
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment