-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
[scripts] Fix bin/ycsb for Python 2.6 and bin/ycsb check_output #709
Conversation
I didn't get to try this on Windows. I tried this with the following against the basic db. YCSB commands
Ubuntu 14.04
CentOS 6
|
if p.returncode: | ||
raise subprocess.CalledProcessError(p.returncode, cmd) | ||
return stdout | ||
# From https://gist.github.com/edufelipe/1027906 |
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.
we'll need to know the license for the backport. I'll leave a comment on the gist
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.
Maybe it makes sense to pull straight from the source instead?
https://github.com/python/cpython/blob/2.7/Lib/subprocess.py#L545
It would be under the license here: https://github.com/python/cpython/blob/2.7/LICENSE
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.
Let's give it till Tuesday or Wednesday for the gist author to respond.
it fails on windows the same way it used to fail on windows, so no worries there. :) |
two weeks is a long time for no response. which one of us should do the reimplementation? :) |
@busbey I think I should have time to update this today. |
feb128a
to
0ea0637
Compare
@busbey updated the check_output method using the one from Python. Minor modifications like adding subprocess. before a few items and fixing the CalledProcessError output. Tested on Ubuntu and CentOS again as outlined above. |
+1, presuming travis checks go fine (I don't think this changes anything they cover anyways). |
Thanks @busbey Travis passed |
Fix checking of argparse module and custom check_output to not return None for err.output.
Fixes #708