Skip to content
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

[test]: Update all subprocess.check_output functions to be Python 3.4 compatible #15196

Merged
merged 1 commit into from Jan 24, 2019

Conversation

Projects
None yet
7 participants
@gkrizek
Copy link
Contributor

commented Jan 18, 2019

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.

@gkrizek

This comment has been minimized.

Copy link
Contributor Author

commented Jan 18, 2019

Just to make a note: This was pretty hard to find because it seems the travis_wait command in CI doesn't print the output of the script it's calling. At least not stderr.

@gkrizek

This comment has been minimized.

Copy link
Contributor Author

commented Jan 18, 2019

Like I mentioned, this only fails on the Cron CI runs because of this if statement: https://github.com/bitcoin/bitcoin/blob/master/.travis/lint_06_script.sh#L21

@laanwj

This comment has been minimized.

Copy link
Member

commented Jan 18, 2019

Thanks! Concept ACK.

I think in principle the changes here could be restricted to contrib/verify-commits/verify-commits.py, as that's the only one launched from Travis.

@practicalswift

This comment has been minimized.

Copy link
Member

commented Jan 18, 2019

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 gkrizek changed the title Update all subprocess.check_output functions to be Python 3.4 compatible [test]: Update all subprocess.check_output functions to be Python 3.4 compatible Jan 18, 2019

Update all subprocess.check_output functions in CI scripts to be Pyth…
…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

@gkrizek gkrizek force-pushed the gkrizek:cron-ci-fix branch from bf6f7e9 to fdf82ba Jan 18, 2019

@gkrizek

This comment has been minimized.

Copy link
Contributor Author

commented Jan 18, 2019

@laanwj Just pushed an update. I kept the function changes to only contrib/verify-commits/verify-commits.py and kept the if statement in test/lint/check-doc.py.

@gkrizek

This comment has been minimized.

Copy link
Contributor Author

commented Jan 18, 2019

@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 travis_wait command in CI would have given output about the failure. I'm going to try to talk to the Travis team to see what's up with that.

@practicalswift

This comment has been minimized.

Copy link
Member

commented Jan 18, 2019

@gkrizek Sounds goods. Thanks!

@gkrizek

This comment has been minimized.

Copy link
Contributor Author

commented Jan 18, 2019

Talked with someone at Travis and the reason it doesn't output the logs is because we see errexit on the script. So it exits as soon as a command fails and doesn't get a chance to output the log.

https://github.com/bitcoin/bitcoin/blob/master/.travis.yml#L53

We could stop using travis_wait and execute the verify-commits.py script directly. It has to log something at least once every 10 minutes to not timeout. I can look into making that change, but I'll do it on a separate issue.

@laanwj

This comment has been minimized.

Copy link
Member

commented Jan 21, 2019

utACK fdf82ba

@ken2812221

This comment has been minimized.

Copy link
Member

commented Jan 24, 2019

Tested ACK fdf82ba
The verify-commits.py script passed on travis: https://travis-ci.org/ken2812221/bitcoin/jobs/483691019

@gkrizek

This comment has been minimized.

Copy link
Contributor Author

commented Jan 24, 2019

@ken2812221 Thanks for testing this!

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Jan 24, 2019

utACK fdf82ba

@jonasschnelli jonasschnelli merged commit fdf82ba into bitcoin:master Jan 24, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

jonasschnelli added a commit that referenced this pull request Jan 24, 2019

Merge #15196: [test]: Update all subprocess.check_output functions 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
@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Jan 25, 2019

Thanks for debugging this

It has to log something at least once every 10 minutes to not timeout. I can look into making that change, but I'll do it on a separate issue.

I wouldn't object such a change.

@gkrizek gkrizek deleted the gkrizek:cron-ci-fix branch Jan 25, 2019

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Mar 29, 2019

Merge bitcoin#15255: [tests] Remove travis_wait from lint script
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.