Skip to content

backport 15166, 15257, 16667, 16186, partial 16465, 16907, 16921, 16961, partial 19348, 21096 - #4487

Merged
UdjinM6 merged 11 commits into
dashpay:developfrom
UdjinM6:fix_linter_p1
Oct 5, 2021
Merged

backport 15166, 15257, 16667, 16186, partial 16465, 16907, 16921, 16961, partial 19348, 21096#4487
UdjinM6 merged 11 commits into
dashpay:developfrom
UdjinM6:fix_linter_p1

Conversation

@UdjinM6

@UdjinM6 UdjinM6 commented Oct 5, 2021

Copy link
Copy Markdown

Backports from #4405

MarcoFalke and others added 11 commits October 5, 2021 20:33
…itian descriptor

bd3f5a9 build: remove mingw linker workaround from win gitian descriptor (fanquake)

Pull request description:

  This workaround was added as part of the switch to gitian building using Ubuntu 14.04 (dashpay#6900).
  However, it should no longer be required, as we have switched to Bionic (bitcoin#13171) and that
  has a far newer version of binutils.

  Original discussion: bitcoin#6900
  binutils patch: https://sourceware.org/bugzilla/show_bug.cgi?id=16192

ACKs for top commit:
  MarcoFalke:
    ACK bd3f5a9
  theuni:
    ACK bd3f5a9
  laanwj:
    ACK bd3f5a9

Tree-SHA512: 01a5789994decf8cdedf7aaa0a449d2100a77e2e6b422d6b9dd5a4ac3e2e0b538c3d43aae4a1c3713614782f3c6b09d8d8bb21c20e86ce3c1734183dedd02d0c
a517541 Remove no longer needed shellcheck suppressions (practicalswift)
0b7196e Fix warnings introduced in shellcheck v0.6.0 (practicalswift)
07a53dc Remove repeated suppression. Fix indentation. (practicalswift)
638e53b Pin shellcheck version to v0.6.0 (practicalswift)

Pull request description:

  Pin `shellcheck` version.

Tree-SHA512: 996e438e424020fe888de1d77ffd33fa32848332febfffbc21a842784aee339332c79c41687c9c577ba1206eb20674623157d584a072e8ae88ae086ee2277bc8
…spell 1.15.0

b748bf6 Fix spelling errors identified by codespell 1.15.0 (Ben Woosley)

Pull request description:

  Note all changes are to comments / documentation.

  After this commit, the only remaining output is:

  ```
    $ test/lint/lint-spelling.sh
    src/test/base32_tests.cpp:14: fo  ==> of, for
    src/test/base64_tests.cpp:14: fo  ==> of, for
    ^ Warning: codespell identified likely spelling errors. Any false positives? Add them to the list of ignored words in test/lint/lint-spelling.ignore-words.txt
  ```

  Note:
  * I ignore several valid alternative spellings ~, but changed homogenous
  to homogeneous as the latter is a more specific term according to the
  Google dictionary definitions I found~
  * homogenous is present in tinyformat, hence should be addressed upstream
  * process' is correct only if there are plural processes

ACKs for commit b748bf:
  practicalswift:
    ACK b748bf6
  fanquake:
    ACK bitcoin@b748bf6

Tree-SHA512: 9add7044643ce015e0a44d8b27a3f300d72c485ffff550fb6491a17f14528085289ec5caddfe02f291ea9b2cded38a0dd3079652a054e2d7fe2ff4f7b53db5d7
…pressions

72a18a7 tests: Add information on how to add Vulture suppressions (practicalswift)

Pull request description:

  Add information on how to add `vulture` suppressions.

  As requested by MarcoFalke in bitcoin#16906 (comment) -- your wish is my command! :)

ACKs for top commit:
  fanquake:
    ACK 72a18a7 - similar sort of message as in [lint-spelling.sh](https://github.com/bitcoin/bitcoin/blob/master/test/lint/lint-spelling.sh).

Tree-SHA512: b347f8cea33d4b0ba987a972979b0ac3423938084fea923a2c457a8081bc839a94ad818689d147477104b9197dc35be413f76a96026cd1507b4411d7513e3464
…list

fac35b2 test: lint: Add DisabledOpcodeTemplates to whitelist (MarcoFalke)

Pull request description:

  Fixes bitcoin#16906

Top commit has no ACKs.

Tree-SHA512: 98b175bb062425fd3a8bd0d0258f4c0e0d5106980f1e037df7c2b2b2e5aa6031b11b582c026265d7b2de56049ccbadb0b7add9130d323f15886f681c6268ba0a
f4beb49 test: Remove python dead code linter (Wladimir J. van der Laan)

Pull request description:

  Primarily I'd like to remove this because it is very imprecise, due to Python's dynamic nature, giving it a large list of false positives that need to be listed as exceptions. See for example bitcoin#16906.

  It's also a frequent source of complaints. I'm doubtful of the usefulness of checking for dead code in a linter in the first place.
  Having some dead code in the test framework for a while is not a
  disaster.

ACKs for top commit:
  sdaftuar:
    utACK f4beb49
  practicalswift:
    ACK f4beb49 -- diff looks correct
  jamesob:
    ACK bitcoin@f4beb49

Tree-SHA512: 329b1555210311d5d15799fd2cb794b3208b0ac4d8a2ffaf4dece1bcc3e0e8b1fe952d5e7a394f94a98919cab579fb579eae7db2a796cc9a1a42ef495dd17507
3f8776a Re-add dead code detection (flack)

Pull request description:

  This re-adds unreachable code detection for Python based on `vulture`.

  Effectively, this reverts f4beb49. The difference to the previous version is that this runs with the `--min-confidence 100` setting. From https://pypi.org/project/vulture/:

  > Use `--min-confidence 100` to only report code that is guaranteed to be unused within the analyzed files.

  So this should avoid the previous issues where static analysis had wrong positives due to the dynamic nature of Python code by only reporting things that are unambiguous (such as code after a `return` statement). As such, there is not suppressions list.

  My motivation was mainly bitcoin#21081 which would have been caught by this (as can be seen by the CI run failing). This is still marked as draft because bitcoin#21081 is needed to get the linter to pass. Also, there is a second problem that this found (see https://github.com/bitcoin/bitcoin/pull/19509/files#r571454691). From what I can tell, this is a spurious type comment that could just be removed (or if that line has no side effects it could also be deleted altogether?). I could add a commit here to fix it, but I wanted to see if there is interest in having this linter again in the first place

ACKs for top commit:
  practicalswift:
    ACK 3f8776a

Tree-SHA512: 52314ad4f627d969de1eb15375ca677ed86a2e816fe773756a1ce22421214ba407b5a09a4bf701a3aab1a10c7b336f548e4cef3327edf154acba55e987db21f6
3d0a82c devtools: Accomodate block-style copyright blocks (Ben Woosley)
0ef0e51 lint: Bump flake8 to 3.7.8 (Ben Woosley)
8389207 lint: Disable flake8 W504 warning (Ben Woosley)
b21680b test/contrib: Fix invalid escapes in regex strings (Ben Woosley)

Pull request description:

  This is a second go at bitcoin#15221, fixing new lints in:
  W504 line break after binary operator
  W605 invalid escape sequence
  F841 local variable 'e' is assigned to but never used

  This time around:
  * One commit per rule, for easier review
  * I went with the PEP-8 style of breaking before binary operators
  * I looked into the raw regex newline issue, and found that raw strings with newlines embedded do work appropriately. E.g. run `re.match(r" \n ", " \n ")` to check this for yourself. `re.MULTILINE` exists to modify `^` and `$` in multiline scenarios, but  all of these searches are per-line.

ACKs for top commit:
  practicalswift:
    ACK 3d0a82c -- diff looks correct

Tree-SHA512: bea0c144cadd72e4adf2e9a4b4ee0535dd91a8e694206924cf8a389dc9253f364a717edfe9abda88108fbb67fda19b9e823f46822d7303c0aaa72e48909a6105
39d526b test: Bump linter versions (Duncan Dean)

Pull request description:

  As per bitcoin#19346, `mypy==0.700` was incompatible with Python 3.8.

  I've bumped the versions of all the linters to their latest stable versions.

  Checked with both Python 3.7 and 3.8 and everything still seems to work fine.

ACKs for top commit:
  hebasto:
    ACK 39d526b, I have reviewed the code and it looks OK, I agree it can be merged.

Tree-SHA512: f3ee7fda8095aa25aa68685e863076d52a6b82649770d24b0064d652763c0ceb8ebcbf9024fc74fca45c754e67b2a831dd070b3af23bc099140e6d27e89a5319
Fixes
```
contrib/auto_gdb/log_size.py:22: unused variable 'from_tty' (100% confidence)
contrib/auto_gdb/used_size.py:29: unused variable 'from_tty' (100% confidence)
```
@UdjinM6 UdjinM6 added this to the 18 milestone Oct 5, 2021
@UdjinM6
UdjinM6 marked this pull request as ready for review October 5, 2021 19:38

@PastaPastaPasta PastaPastaPasta left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK for merging via merge commit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants