[test]: Update all subprocess.check_output functions to be Python 3.4 compatible#15196
[test]: Update all subprocess.check_output functions to be Python 3.4 compatible#15196jonasschnelli merged 1 commit intobitcoin:masterfrom
Conversation
|
Just to make a note: This was pretty hard to find because it seems the |
|
Like I mentioned, this only fails on the Cron CI runs because of this |
|
Thanks! Concept ACK. I think in principle the changes here could be restricted to |
|
Concept ACK @gkrizek Can you think of a way to test/lint for this to make sure we don't introduce a regression in the future? :-) |
gkrizek
changed the title
…on 3.4 compatible Removing the 'universal_newlines' and 'encoding' args from the subprocess.check_outputs fuction. 'universal_newlines' is supported in 3.4, but 'encoding' is not. Without specifying 'encoding' it will make a guess at encoding, which can break things on BSD systems. We must handle encoding/decoding ourselves until we can use Python 3.6
|
@laanwj Just pushed an update. I kept the function changes to only |
|
@practicalswift That's a good question and I tried to think about that as I fixed this. Ultimately, I think this is a pretty far off edge case. We should not be downgrading our Python version in CI very often at all (only upgrading obviously). So to take the time to write tests to check for features that aren't available in the running Python version is a waste of time I think because it happens so infrequently. It seems like CI was set to 3.6 on accident without actually checking what the minimum supported version was. I think this would have been found much sooner if the |
|
@gkrizek Sounds goods. Thanks! |
|
Talked with someone at Travis and the reason it doesn't output the logs is because we see https://github.com/bitcoin/bitcoin/blob/master/.travis.yml#L53 We could stop using |
|
utACK fdf82ba |
|
Tested ACK fdf82ba |
|
@ken2812221 Thanks for testing this! |
|
utACK fdf82ba |
… be Python 3.4 compatible fdf82ba Update all subprocess.check_output functions in CI scripts to be Python 3.4 compatible (Graham Krizek) Pull request description: CI is failing the `lint` stage on every Cron run (regular PR/Push runs still pass). The failure was introduced in 74ce326 and has been broken since. The Python version running in CI was downgraded to 3.4 from 3.6. There were a couple files that were using the `encoding` argument in the `subprocess.check_output` function. This was introduced in Python 3.6 and therefore broke the scripts that were using it. The `universal_newlines` argument was used as well, but in order to use it we must be able to set encoding because of issues on some BSD systems. To get CI to pass, I removed all `universal_newline` and `encoding` args to the `check_ouput` function. Then I decoded all `check_output` return values. This should keep the same behavior but be Python 3.4 compatible. Tree-SHA512: f5e5885e98cf4777be9cc254446a873eedb03bdccbd8e06772a964db95e9fcf46736aa9cdcab1d8f123ea9f4947ed6020679898d8b2f47ffb1d94c21a4b08209
|
Thanks for debugging this
I wouldn't object such a change. |
8b8d8ee Remove travis_wait from lint script (Graham Krizek) Pull request description: Using the `travis_wait` command in conjunction with `set -o errexit` causes problems. The `travis_wait` command will correctly log the command's output if successful, but if the command fails the process exits before the `travis_wait` command can dump the logs. This will hide important debugging information like error messages and stack traces. We ran into this in bitcoin#15196 and it was very hard to debug because output was being suppressed. `travis_wait` was being used because the `contrib/verify-commits/verify-commits.py` script can sometimes run for a long time without producing any output. If a script runs for 10 minutes without logging anything, the CI run times out. The `travis_wait` command will extend this timeout by logging a message for you, while sending stderr and stdout to a file. This PR removes the `travis_wait` command from our CI system and adds additional logging to the `verify-commits.py` script so it doesn't make Travis timeout. ACKs for commit 8b8d8e: MarcoFalke: utACK 8b8d8ee Tree-SHA512: 175a8dd3f4d4e03ab272ddba94fa8bb06875c9027c3f3f81577feda4bc8918b5f0e003a19027f04f8cf2d0b56c68633716a6ab23f95b910121a8d1132428767d
…ions to be Python 3.4 compatible fdf82ba Update all subprocess.check_output functions in CI scripts to be Python 3.4 compatible (Graham Krizek) Pull request description: CI is failing the `lint` stage on every Cron run (regular PR/Push runs still pass). The failure was introduced in 74ce326 and has been broken since. The Python version running in CI was downgraded to 3.4 from 3.6. There were a couple files that were using the `encoding` argument in the `subprocess.check_output` function. This was introduced in Python 3.6 and therefore broke the scripts that were using it. The `universal_newlines` argument was used as well, but in order to use it we must be able to set encoding because of issues on some BSD systems. To get CI to pass, I removed all `universal_newline` and `encoding` args to the `check_ouput` function. Then I decoded all `check_output` return values. This should keep the same behavior but be Python 3.4 compatible. Tree-SHA512: f5e5885e98cf4777be9cc254446a873eedb03bdccbd8e06772a964db95e9fcf46736aa9cdcab1d8f123ea9f4947ed6020679898d8b2f47ffb1d94c21a4b08209
8b8d8ee Remove travis_wait from lint script (Graham Krizek) Pull request description: Using the `travis_wait` command in conjunction with `set -o errexit` causes problems. The `travis_wait` command will correctly log the command's output if successful, but if the command fails the process exits before the `travis_wait` command can dump the logs. This will hide important debugging information like error messages and stack traces. We ran into this in bitcoin#15196 and it was very hard to debug because output was being suppressed. `travis_wait` was being used because the `contrib/verify-commits/verify-commits.py` script can sometimes run for a long time without producing any output. If a script runs for 10 minutes without logging anything, the CI run times out. The `travis_wait` command will extend this timeout by logging a message for you, while sending stderr and stdout to a file. This PR removes the `travis_wait` command from our CI system and adds additional logging to the `verify-commits.py` script so it doesn't make Travis timeout. ACKs for commit 8b8d8e: MarcoFalke: utACK 8b8d8ee Tree-SHA512: 175a8dd3f4d4e03ab272ddba94fa8bb06875c9027c3f3f81577feda4bc8918b5f0e003a19027f04f8cf2d0b56c68633716a6ab23f95b910121a8d1132428767d
7 participants
CI is failing the
lintstage on every Cron run (regular PR/Push runs still pass). The failure was introduced in 74ce326 and has been broken since. The Python version running in CI was downgraded to 3.4 from 3.6. There were a couple files that were using theencodingargument in thesubprocess.check_outputfunction. This was introduced in Python 3.6 and therefore broke the scripts that were using it. Theuniversal_newlinesargument was used as well, but in order to use it we must be able to set encoding because of issues on some BSD systems.To get CI to pass, I removed all
universal_newlineandencodingargs to thecheck_ouputfunction. Then I decoded allcheck_outputreturn values. This should keep the same behavior but be Python 3.4 compatible.