Add Travis check for unused Python imports #11835

Add Travis check for unused Python imports.

$ contrib/devtools/lint-python.sh
./test/functional/example_test.py:18:1: F401 'test_framework.mininode.NODE_NETWORK' imported but unused
./test/functional/test_framework/messages.py:27:1: F401 'test_framework.util.wait_until' imported but unused
./test/functional/test_framework/test_framework.py:16:1: F401 'traceback' imported but unused

Cool. I hope this will bring down nit comments on functional test pulls.

utACK 998cdc958f4e50efceb5f5c01f0002506a9d863c

Travis now fails as expected:

$ if [ "$CHECK_DOC" = 1 -a "$TRAVIS_EVENT_TYPE" = "pull_request" ]; then contrib/devtools/lint-all.sh; fi
./test/functional/example_test.py:18:1: F401 'test_framework.mininode.NODE_NETWORK' imported but unused
./test/functional/test_framework/test_framework.py:16:1: F401 'traceback' imported but unused
./test/functional/test_framework/messages.py:27:1: F401 'test_framework.util.wait_until' imported but unused

^---- failure generated from contrib/devtools/lint-python.sh

The command "if [ "$CHECK_DOC" = 1 -a "$TRAVIS_EVENT_TYPE" = "pull_request" ]; then contrib/devtools/lint-all.sh; fi" failed and exited with 1 during .

Your build has been stopped.

I will now add a commit fixing those three unused imports to verify that Travis passes.

Now passing Travis and thus working as expected! :-)

Concept ACK, sounds useful

Thanks for tagging me @MarcoFalke . You know how much I love python linting :)

This looks good to me. Example of this failing is here: https://travis-ci.org/bitcoin/bitcoin/jobs/312082939.

I have one comment inline, and one general comment that it makes more sense to me to reverse the order of the commits (fix the warnings first and then add the checker).

The only potential problem is if there are open pull requests which would silently conflict with this (ie PRs which introduce unused imports or change python modules without removing no-longer-required imports). @ajtowns was adding some checking in a different PR here: https://github.com/jnewbery/bitcoin/pull/4 and used a magic script to check open PRs for conflicts. Perhaps he could share that and we could do the same here?

Longer term, I'd like to add more flake8 warnings to this, as long as we can all agree on which ones. Good ones could be:

• W291 trailing whitespace
• W191 indentation contains tabs
• F821 undefined name

The only potential problem is if there are open pull requests which would silently conflict with this

Good catch. However, instead of adding temporary code to allow for leeway, I'd rather just reset all arm_doc_jobs on travis. Otherwise we'd have to drag along the leeway code for an unspecific amount of time.

• W291 trailing whitespace
• W191 indentation contains tabs
• F821 undefined name

We already check for trailing whitespace and tab charactes in https://github.com/bitcoin/bitcoin/blob/4ef4dfebbc07d93d72899f60e01ca77a280c9122/contrib/devtools/lint-whitespace.sh#L36 https://github.com/bitcoin/bitcoin/blob/4ef4dfebbc07d93d72899f60e01ca77a280c9122/contrib/devtools/lint-whitespace.sh#L63

Though, the check is only done on the diff of the pull request (ignoring preexisting lint). However, I don't see an issue fixing existing lint errors in python code and enabling those checks in this pull.

instead of adding temporary code to allow for leeway

Sorry - I wasn't suggesting going that far - just running a check and then adding a note to those PRs and manually restarting the travis jobs.

I don't see an issue fixing existing lint errors in python code and enabling those checks in this pull.

I think it's better to keep this PR focused on adding the lint-python.sh infrastructure, since it's small and easy to review. Future PRs can add additional flake8 rules.

Agree, makes sense.

@jnewbery Thanks for reviewing! Added a copyright header and changed the commit ordering. Looks good? :-)

utACK d60b32074098d50b04e408c1304dd6f6120654ed

utACK d60b32074098d50b04e408c1304dd6f6120654ed (I've run the new lint script locally, but haven't verified the travis changes work)

The PR checking I did for the naming convention patches mentioned above was just on filenames ("git diff --name-status $(git merge-base $PR master)..$PR"), checking to see what PRs would break this checks is a bit harder. The following looks okay:

total=$(echo $PULLS | wc -w)
sofar=0
for a in $PULLS; do
   sofar=$((sofar + 1))
   echo >&2 "$a ($sofar/$total)"
   (
     (git checkout -q pull/origin/$a && flake8 --ignore=B,C,E,F,I,N,W --select=F401 --format '%(path)s:-:%(col)d: %(code)s %(text)s') | sed 'p'
     (git checkout -q $(git merge-base origin/master pull/origin/$a) && flake8 --ignore=B,C,E,F,I,N,W --select=F401 --format '%(path)s:-:%(col)d: %(code)s %(text)s')
   ) | sed "s/^/$a: /" | sort | uniq -c | sed -n 's/^ *2 //p'
done | tee ../LOG

(To fetch pull requests, you need a entry in .git/config like:

[remote "origin"]
url = https://github.com/bitcoin/bitcoin.git
fetch = +refs/heads/*:refs/remotes/origin/*
fetch = +refs/pull/*/head:refs/pull/origin/*

)

Reformatting the output above suggests the following PRs introduce unreferenced imports:

#11796: ./test/functional/test_framework/blocktools.py:

F401 '.script.OP_EQUAL' imported but unused
F401 '.script.OP_HASH160' imported but unused

#11774: ./test/functional/test_framework/blocktools.py

F401 '.script.OP_EQUAL' imported but unused
F401 '.script.OP_HASH160' imported but unused

#11771: ./test/functional/test_framework/mininode.py

F401 'test_framework.util.assert_equal' imported but unused

#11708: ./test/functional/signrawtransactions.py

F401 'test_framework.script.OP_0' imported but unused

#11666: ./test/functional/signinput.py

F401 'time' imported but unused

#11653: ./test/functional/getsignaturehash.py

F401 'time' imported but unused

#11647: ./test/functional/minchainwork.py

F401 'test_framework.util.sync_blocks' imported but unused

#10947: ./test/functional/segwit2.py

F401 'io.BytesIO' imported but unused
F401 'test_framework.address.key_to_p2pkh' imported but unused
F401 'test_framework.address.script_to_p2sh' imported but unused
F401 'test_framework.mininode.COIN' imported but unused
F401 'test_framework.mininode.COutPoint' imported but unused
F401 'test_framework.mininode.CTransaction' imported but unused
F401 'test_framework.mininode.CTxIn' imported but unused
F401 'test_framework.mininode.CTxOut' imported but unused
F401 'test_framework.mininode.FromHex' imported but unused
F401 'test_framework.mininode.sha256' imported but unused
F401 'test_framework.mininode.ToHex' imported but unused
F401 'test_framework.script.CScript' imported but unused
F401 'test_framework.script.hash160' imported but unused
F401 'test_framework.script.OP_0' imported but unused
F401 'test_framework.script.OP_1' imported but unused
F401 'test_framework.script.OP_2' imported but unused
F401 'test_framework.script.OP_CHECKMULTISIG' imported but unused
F401 'test_framework.script.OP_CHECKSIG' imported but unused
F401 'test_framework.script.OP_DUP' imported but unused
F401 'test_framework.script.OP_EQUAL' imported but unused
F401 'test_framework.script.OP_EQUALVERIFY' imported but unused
F401 'test_framework.script.OP_HASH160' imported but unused
F401 'test_framework.script.OP_TRUE' imported but unused

#10554: ./test/functional/zmq_test.py

F401 'test_framework.util.assert_not_equal' imported but unused

#10470: ./test/functional/listsinceblock.py

F401 'test_framework.util.start_nodes' imported but unused
F401 'test_framework.util.sync_mempools' imported but unused

#10437:

./test/functional/bip135-grace.py
  F401 'tempfile' imported but unused
  F401 'test_framework.script.CScript' imported but unused
  F401 'test_framework.script.OP_1NEGATE' imported but unused
  F401 'test_framework.script.OP_CHECKSEQUENCEVERIFY' imported but unused
  F401 'test_framework.script.OP_DROP' imported but unused
  F401 'test_framework.util.sync_blocks' imported but unused
./test/functional/bip135-threshold.py
  F401 'tempfile' imported but unused
  F401 'test_framework.script.CScript' imported but unused
  F401 'test_framework.script.OP_1NEGATE' imported but unused
  F401 'test_framework.script.OP_CHECKSEQUENCEVERIFY' imported but unused
  F401 'test_framework.script.OP_DROP' imported but unused
  F401 'test_framework.util.sync_blocks' imported but unused

#10366:

./test/functional/bip9-softforks.py
  F401 'test_framework.util.stop_nodes' imported but unused
./test/functional/mempool_persist.py
  F401 'test_framework.util.connect_nodes_bi' imported but unused
./test/functional/merkle_blocks.py
  F401 'test_framework.util.assert_raises' imported but unused
  F401 'test_framework.util.JSONRPCException' imported but unused
./test/functional/mining.py
  F401 'hashlib.sha256' imported but unused
  F401 'struct.pack' imported but unused
./test/functional/smartfees.py
  F401 'test_framework.util.assert_equal' imported but unused

#9483: ./qa/rpc-tests/spv.py

F401 'pprint.pprint' imported but unused

#9152: ./test/functional/sweepprivkeys.py

F401 'decimal' imported but unused
F401 'math' imported but unused
F401 'json' imported but unused

#8755: ./qa/rpc-tests/sighashlimit.py

F401 'test_framework.script.OP_1' imported but unused

Thanks for writing and running the script @ajtowns

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

utACK d60b32074098d50b04e408c1304dd6f6120654ed
-----BEGIN PGP SIGNATURE-----

iQIcBAEBCgAGBQJaLcS6AAoJENLqSFDnUoslH9gP/0PiY6125k0oO/yI2I8C8LmX
VuQJY5JgdH0apA3mjWYoVLY0mC3vIa1HeOPPBUwXibEDBrO3rBd+OnzW2ZY6FvJ9
KwViKukjtpK51Kt1AxJN2BBmr/W4M9nCUJ3EaQHdhmKPnrIPcKIqlkufqbpRYDFq
QaA57ly/FHgQfuYEh4cvGZeO6k8RvQcGwnWx4jyQbj/bvYfZwbuHQZp78OChf3Eb
VnspOmy7PGn9vTpa/4qqs1SM6s5/cZJ6JRF1UP2abS49/EP8fbAWFqpdFWCrBvh0
qoLfhfGeSeQG2t+9LDUjoIMUhbt7rqzd5oPIwVNL8fxU5Wpa8aY/SHix0Gj3gEZA
xt1OcsCMWLHhTPygxF6s7UgSqvoh2oGoic4PZyY7ffWA+SciBue7tsS4PlV7KLfN
4lwDNw8rDmykmteDb9Q0g852WYahXRpRWBTswtRy1C+xe5KwusZZeSWpVF4RtkfK
ao1MYY/1W/fO71y8fMtJFmNYnvwrwp680ZQ5TMCvtjP6VrSq3bGdEvvijR1j+UgA
z51/mSAVupZsZX/MYKzn7hcwVHV/96ACZeiXL+r1CSDGcOPSZeUfwzHz3JKA7xu5
bFsv8PnQseBdjpIxbxRLZKx4niWGOjk/lUKArbv45uEXzVeQ9TJKDJ5Pnkal92IB
85anKK9+nHkEs0DVdfn+
=Br1P
-----END PGP SIGNATURE-----

Awesome. Thanks @ajtowns . @MarcoFalke - do you plan to comment on those PRs or would you like me to do it?

I checked that there are comments on all that are mergeable to master. The three ones by you and @ajtowns don't need a comment, I presume. (#11796, #11774, #11771)