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

gkrizek commented at 4:53 AM on January 18, 2019: contributor

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 commented at 4:55 AM on January 18, 2019: contributor

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 commented at 4:58 AM on January 18, 2019: contributor

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

fanquake added the label Scripts and tools on Jan 18, 2019

in test/lint/check-doc.py:33 in bf6f7e9d2b outdated

  25 | @@ -26,12 +26,8 @@
  26 |  
  27 |  
  28 |  def main():
  29 | -    if sys.version_info >= (3, 6):
  30 | -        used = check_output(CMD_GREP_ARGS, shell=True, universal_newlines=True, encoding='utf8')
  31 | -        docd = check_output(CMD_GREP_DOCS, shell=True, universal_newlines=True, encoding='utf8')
  32 | -    else:
  33 | -        used = check_output(CMD_GREP_ARGS, shell=True, universal_newlines=True) # encoding='utf8'

laanwj commented at 7:08 AM on January 18, 2019:

Let's change only the < sys.version_info >= (3, 6) case here, and keep the 3.6+ one for when 3.6 support is the minimum.

in contrib/devtools/optimize-pngs.py:27 in bf6f7e9d2b outdated

  26 | @@ -27,7 +27,7 @@ def content_hash(filename):
  27 |  pngcrush = 'pngcrush'

laanwj commented at 7:08 AM on January 18, 2019:

I think it's fine to keep this rarely-used utility Python 3.6+ only.

laanwj commented at 7:10 AM on January 18, 2019: member

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 commented at 2:05 PM on January 18, 2019: contributor

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 renamed this:
~~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
on Jan 18, 2019

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

fdf82ba181

gkrizek force-pushed on Jan 18, 2019

gkrizek commented at 3:39 PM on January 18, 2019: contributor

@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 commented at 3:50 PM on January 18, 2019: contributor

@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 commented at 4:07 PM on January 18, 2019: contributor

@gkrizek Sounds goods. Thanks!

gkrizek commented at 4:52 PM on January 18, 2019: contributor

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 commented at 7:35 PM on January 21, 2019: member

utACK fdf82ba1813cf12e2794bbe20f7d002eaf4279fc

ken2812221 commented at 1:27 AM on January 24, 2019: contributor

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

gkrizek commented at 3:39 AM on January 24, 2019: contributor

@ken2812221 Thanks for testing this!

jonasschnelli commented at 6:42 AM on January 24, 2019: contributor

utACK fdf82ba1813cf12e2794bbe20f7d002eaf4279fc

jonasschnelli merged this on Jan 24, 2019

jonasschnelli closed this on Jan 24, 2019

jonasschnelli referenced this in commit 73a6bac9ff on Jan 24, 2019

MarcoFalke commented at 12:20 AM on January 25, 2019: member

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 deleted the branch on Jan 25, 2019

MarcoFalke referenced this in commit 904129b35d on Mar 29, 2019

linuxsh2 referenced this in commit 791a234bba on Jul 30, 2021

linuxsh2 referenced this in commit c2b8e5f0c9 on Aug 3, 2021

PastaPastaPasta referenced this in commit 96426b5057 on Sep 11, 2021

DrahtBot locked this on Dec 16, 2021