-
Notifications
You must be signed in to change notification settings - Fork 69
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
Python2 codepath is incorrect as check_call() does NOT capture output. #33
Closed
ShaheedHaque
wants to merge
2
commits into
acesseonline:master
from
ShaheedHaque:fix_subprocess_output_handling
Closed
Python2 codepath is incorrect as check_call() does NOT capture output. #33
ShaheedHaque
wants to merge
2
commits into
acesseonline:master
from
ShaheedHaque:fix_subprocess_output_handling
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
Also, the Python2/3 exception handling path is suboptimal as firstly, logger.exception() outputs the exception info already (since exc_info is True by default), and secondly, in both cases it has access to any output generated which may as well be the msg argument. (I note that my previous PR#28 didn't address this wider point). For example, without the fix, the logged output is something like this: ========= [2018-07-02 09:14:55,059: ERROR/ForkPoolWorker-15] Command '/usr/local/lib/python3.6/dist-packages/pyreportjasper/jasperstarter/bin/jasperstarter --locale en_GB process "/main/srhaque/Innovatieltd/testdjango/paiyroll/report/JasperReports/pre_approval.jrxml" -o "/tmp/Acme Accountants/Advanced Mechanics/m1/2018-07-28/pre_approval" -f pdf -t jsonql --data-file '/tmp/Acme Accountants/Advanced Mechanics/m1/2018-07-28/snapshot.json' --jsonql-query 'employee.*'' returned non-zero exit status 1. Traceback (most recent call last): File "/usr/local/lib/python3.6/dist-packages/pyreportjasper/jasperpy.py", line 188, in execute self.command, shell=True, check=True).returncode File "/usr/lib/python3.6/subprocess.py", line 418, in run output=stdout, stderr=stderr) subprocess.CalledProcessError: Command '/usr/local/lib/python3.6/dist-packages/pyreportjasper/jasperstarter/bin/jasperstarter --locale en_GB process "/main/srhaque/Innovatieltd/testdjango/paiyroll/report/JasperReports/pre_approval.jrxml" -o "/tmp/Acme Accountants/Advanced Mechanics/m1/2018-07-28/pre_approval" -f pdf -t jsonql --data-file '/tmp/Acme Accountants/Advanced Mechanics/m1/2018-07-28/snapshot.json' --jsonql-query 'employee.*'' returned non-zero exit status 1. [2018-07-02 09:14:55,062: ERROR/ForkPoolWorker-15] Unhandled exception Traceback (most recent call last): File "/usr/local/lib/python3.6/dist-packages/pyreportjasper/jasperpy.py", line 188, in execute self.command, shell=True, check=True).returncode File "/usr/lib/python3.6/subprocess.py", line 418, in run output=stdout, stderr=stderr) subprocess.CalledProcessError: Command '/usr/local/lib/python3.6/dist-packages/pyreportjasper/jasperstarter/bin/jasperstarter --locale en_GB process "/main/srhaque/Innovatieltd/testdjango/paiyroll/report/JasperReports/pre_approval.jrxml" -o "/tmp/Acme Accountants/Advanced Mechanics/m1/2018-07-28/pre_approval" -f pdf -t jsonql --data-file '/tmp/Acme Accountants/Advanced Mechanics/m1/2018-07-28/snapshot.json' --jsonql-query 'employee.*'' returned non-zero exit status 1. During handling of the above exception, another exception occurred: Traceback (most recent call last): ... File "/usr/local/lib/python3.6/dist-packages/pyreportjasper/jasperpy.py", line 151, in process return self.execute() File "/usr/local/lib/python3.6/dist-packages/pyreportjasper/jasperpy.py", line 193, in execute raise NameError('Your report has an error and couldn ' NameError: Your report has an error and couldn 't be processed!\ Try to output the command using the attribute `command;` and run it manually in the console! ========= (Notice how the word "Command" appears 3 times). With the fix the output is the less redundant: ======== [2018-07-02 09:06:44,706: ERROR/ForkPoolWorker-2] None Traceback (most recent call last): File "/usr/local/lib/python3.6/dist-packages/pyreportjasper/jasperpy.py", line 188, in execute self.command, shell=True, check=True).returncode File "/usr/lib/python3.6/subprocess.py", line 418, in run output=stdout, stderr=stderr) subprocess.CalledProcessError: Command '/usr/local/lib/python3.6/dist-packages/pyreportjasper/jasperstarter/bin/jasperstarter --locale en_GB process "/main/srhaque/Innovatieltd/testdjango/paiyroll/report/JasperReports/pre_approval.jrxml" -o "/tmp/Acme Accountants/Advanced Mechanics/m1/2018-07-28/pre_approval" -f pdf -t jsonql --data-file '/tmp/Acme Accountants/Advanced Mechanics/m1/2018-07-28/snapshot.json' --jsonql-query 'employee.*'' returned non-zero exit status 1. [2018-07-02 09:06:44,708: ERROR/ForkPoolWorker-2] Unhandled exception Traceback (most recent call last): File "/usr/local/lib/python3.6/dist-packages/pyreportjasper/jasperpy.py", line 188, in execute self.command, shell=True, check=True).returncode File "/usr/lib/python3.6/subprocess.py", line 418, in run output=stdout, stderr=stderr) subprocess.CalledProcessError: Command '/usr/local/lib/python3.6/dist-packages/pyreportjasper/jasperstarter/bin/jasperstarter --locale en_GB process "/main/srhaque/Innovatieltd/testdjango/paiyroll/report/JasperReports/pre_approval.jrxml" -o "/tmp/Acme Accountants/Advanced Mechanics/m1/2018-07-28/pre_approval" -f pdf -t jsonql --data-file '/tmp/Acme Accountants/Advanced Mechanics/m1/2018-07-28/snapshot.json' --jsonql-query 'employee.*'' returned non-zero exit status 1. During handling of the above exception, another exception occurred: Traceback (most recent call last): ... File "/usr/local/lib/python3.6/dist-packages/pyreportjasper/jasperpy.py", line 151, in process return self.execute() File "/usr/local/lib/python3.6/dist-packages/pyreportjasper/jasperpy.py", line 193, in execute raise NameError('Your report has an error and couldn ' NameError: Your report has an error and couldn 't be processed!\ Try to output the command using the attribute `command;` and run it manually in the console! ======== (Notice how the first "Command" has been replaced by "None", and would have been replaced by any output from the command for both Python2/3).
Pull Request Test Coverage Report for Build 275
💛 - Coveralls |
I should mention that you might prefer the line:
to be something like:
Let me know if so, and I'll happily oblige. |
I realised I did something stupid. Sorry. You are actually trying to return the returncode, and completely ignoring the command output. I will rework and do a new PR. |
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.
Also, the Python2/3 exception handling path is suboptimal as firstly,
logger.exception() outputs the exception info already (since exc_info is
True by default), and secondly, in both cases it has access to any output
generated which may as well be the msg argument.
(I note that my previous PR#28 didn't address this wider point). For example,
without the fix, the logged output is something like this:
(Notice how the word "Command" appears 3 times). With the fix the output is the less redundant:
(Notice how the first "Command" has been replaced by "None", and would have been
replaced by any output from the command for both Python2/3).