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

backport: merge bitcoin#24778, #24766, #24849, #24844, #24853, #24895, #24802, #24932, partial bitcoin#23212, #23462 (lint backports) #6428

Merged
merged 14 commits into from
Dec 4, 2024

Conversation

kwvg
Copy link
Collaborator

@kwvg kwvg commented Nov 24, 2024

Additional Information

  • Dependency for backport: merge bitcoin#24794, #23524, #24902, #24915, #24916, #24929, #23506, #24840, #24982, partial bitcoin#25288 (lint backports: part 2) #6429

  • Even though support for Python scripts was extended to lint-all in bitcoin#24762 (dash#6023), the changes needed were not extended to parallelized linting added in dash#4637.

    This meant the match expression didn't include non-shell scripts and additionally, parallel interpreted all scripts as Bash scripts. This has been resolved in this PR.

  • bitcoin#23212 is partial as --ignore-missing-imports has not been removed from mypy arguments as the version of mypy used in the PR (0.910) isn't syntax aware to imports like here and here.

    Lint errors:
    dash@96b217d539f7:/src/dash$ ./test/lint/lint-python.sh
    Consider install flake8-cached for cached flake8 results.
    [...]
    test/functional/test_framework/crypto/muhash.py:9: error: Skipping analyzing ".chacha20": found module but no type hints or library stubs  [import]
    test/functional/test_framework/crypto/ellswift.py:15: error: Cannot find implementation or library stub for module named "test_framework.crypto.secp256k1"  [import]
    test/functional/test_framework/crypto/bip324_cipher.py:16: error: Skipping analyzing ".chacha20": found module but no type hints or library stubs  [import]
    test/functional/test_framework/crypto/bip324_cipher.py:17: error: Skipping analyzing ".poly1305": found module but no type hints or library stubs  [import]
    test/functional/test_framework/messages.py:29: error: Cannot find implementation or library stub for module named "test_framework.crypto.siphash"  [import]
    test/functional/test_framework/messages.py:32: error: Cannot find implementation or library stub for module named "dash_hash"  [import]
    test/functional/test_framework/script.py:20: error: Cannot find implementation or library stub for module named "test_framework.crypto.ripemd160"  [import]
    test/functional/test_framework/key.py:16: error: Cannot find implementation or library stub for module named "test_framework.crypto"  [import]
    test/functional/test_framework/v2_p2p.py:9: error: Cannot find implementation or library stub for module named "test_framework.crypto.bip324_cipher"  [import]
    test/functional/test_framework/v2_p2p.py:10: error: Cannot find implementation or library stub for module named "test_framework.crypto.chacha20"  [import]
    test/functional/test_framework/v2_p2p.py:11: error: Cannot find implementation or library stub for module named "test_framework.crypto.ellswift"  [import]
    test/functional/test_framework/v2_p2p.py:12: error: Cannot find implementation or library stub for module named "test_framework.crypto.hkdf"  [import]
    test/functional/p2p_v2_encrypted.py:22: error: Cannot find implementation or library stub for module named "test_framework.crypto.chacha20"  [import]
    test/functional/feature_utxo_set_hash.py:12: error: Cannot find implementation or library stub for module named "test_framework.crypto.muhash"  [import]
    Found 17 errors in 12 files (checked 275 source files)
    

    And upgrading to the latest version of mypy used upstream (1.4.1, source) brings syntax awareness but new errors.

    Lint errors:
    dash@96b217d539f7:/src/dash$ ./test/lint/lint-python.sh
    Consider install flake8-cached for cached flake8 results.
    [...]
    test/functional/test_framework/coverage.py:23: error: Incompatible default for argument "coverage_logfile" (default has type "None", argument has type "str")  [assignment]
    test/functional/test_framework/coverage.py:23: note: PEP 484 prohibits implicit Optional. Accordingly, mypy has changed its default to no_implicit_optional=True
    test/functional/test_framework/coverage.py:23: note: Use https://github.com/hauntsaninja/no_implicit_optional to automatically upgrade your codebase
    test/functional/test_framework/util.py:322: error: Incompatible default for argument "timeout" (default has type "None", argument has type "int")  [assignment]
    test/functional/test_framework/util.py:322: note: PEP 484 prohibits implicit Optional. Accordingly, mypy has changed its default to no_implicit_optional=True
    test/functional/test_framework/util.py:322: note: Use https://github.com/hauntsaninja/no_implicit_optional to automatically upgrade your codebase
    test/functional/test_framework/util.py:322: error: Incompatible default for argument "coveragedir" (default has type "None", argument has type "str")  [assignment]
    test/functional/test_framework/messages.py:32: error: Cannot find implementation or library stub for module named "dash_hash"  [import]
    test/functional/test_framework/test_framework.py:122: note: By default the bodies of untyped functions are not checked, consider using --check-untyped-defs  [annotation-unchecked]
    test/functional/test_framework/test_framework.py:123: note: By default the bodies of untyped functions are not checked, consider using --check-untyped-defs  [annotation-unchecked]
    test/functional/test_framework/test_framework.py:124: note: By default the bodies of untyped functions are not checked, consider using --check-untyped-defs  [annotation-unchecked]
    test/functional/test_framework/test_framework.py:125: note: By default the bodies of untyped functions are not checked, consider using --check-untyped-defs  [annotation-unchecked]
    test/functional/p2p_message_capture.py:48: note: By default the bodies of untyped functions are not checked, consider using --check-untyped-defs  [annotation-unchecked]
    Found 6 errors in 5 files (checked 275 source files)
    
  • bitcoin#23462 is partial as neither SC2046 nor SC2086 have been enabled. This backport was done for the sole purpose of backporting changes to shell scripts that would be replaced with their Python counterparts in later backports.

    The necessary changes needed to enable SC2046 and SC2086 will be done in a later pull request.

  • actions/checkout does not do a full clone nor tracks the default branch by default. It is already documented that git merge-base (used by lint-git-commit-check.py and lint-whitespace.py, source) doesn't play nice with it (see actions/checkout#423).

    No such issue exists on GitLab (build), but it remains present on GitHub Actions (build). This PR does a full clone, switches to the develop branch, then switches back to the intended commit, as a workaround (see working build here)

  • Changes to run-lint-format-strings.py made in dash#4807 were reverted as they were interfering with lint-format-strings.py

    Lint error:
    dash@96b217d539f7:/src/dash$ ./test/lint/lint-format-strings.py
    Traceback (most recent call last):
      File "/src/dash/test/lint/run-lint-format-strings.py", line 308, in <module>
        main()
      File "/src/dash/test/lint/run-lint-format-strings.py", line 304, in main
        sys.exit(max(exit_codes))
    ValueError: max() arg is an empty sequence
    Traceback (most recent call last):
      File "/src/dash/test/lint/run-lint-format-strings.py", line 308, in <module>
        main()
      File "/src/dash/test/lint/run-lint-format-strings.py", line 304, in main
        sys.exit(max(exit_codes))
    ValueError: max() arg is an empty sequence
    

Breaking Changes

None expected.

Checklist

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas (note: N/A)
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation
  • I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

@kwvg kwvg added this to the 22.1 milestone Nov 24, 2024
@kwvg kwvg force-pushed the shtopy branch 3 times, most recently from 94b14ea to 4aaa314 Compare November 24, 2024 12:18
@kwvg
Copy link
Collaborator Author

kwvg commented Nov 24, 2024

GitHub Actions run for 4aaa314, https://github.com/kwvg/dash/actions/runs/11996049561.

knst
knst previously approved these changes Nov 26, 2024
Copy link
Collaborator

@knst knst left a comment

Choose a reason for hiding this comment

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

LGTM 4aaa314

Copy link

This pull request has conflicts, please rebase.

@kwvg
Copy link
Collaborator Author

kwvg commented Nov 27, 2024

GitHub Actions run for 852f55e, https://github.com/kwvg/dash/actions/runs/12044056813.

@kwvg kwvg requested a review from knst November 27, 2024 09:35
Copy link
Collaborator

@knst knst left a comment

Choose a reason for hiding this comment

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

utACK 852f55e

Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

utACK 852f55e

(side note: there is no mapfile on macOS so some linters no longer work locally for me😞 )

"src/immer/",
"src/util/expected.h",
"doc/release-notes/",
"src/qt/locale"]
Copy link

Choose a reason for hiding this comment

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

24844: note/nit: "src/qt/locale" was added in 16988 but that's a "translations update" PR (i.e. DNM) so it's probably ok to simply fix it here.

Copy link
Collaborator Author

@kwvg kwvg Dec 4, 2024

Choose a reason for hiding this comment

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

Also, mapfile shouldn't be a problem after #6429, the only shell scripts that'll remain then are commit-script-check.sh, git-subtree-check.sh and lint-cppcheck-dash.sh (former two remain shell scripts upstream[1] and the latter is a Dash-specific script). macOS won't be left out! 🙂

[1] - commit-script-check.sh, git-subtree-check.sh

@PastaPastaPasta PastaPastaPasta merged commit eb88589 into dashpay:develop Dec 4, 2024
18 of 23 checks passed
PastaPastaPasta added a commit that referenced this pull request Dec 4, 2024
, bitcoin#24915, bitcoin#24916, bitcoin#24929, bitcoin#23506, bitcoin#24840, bitcoin#24982, partial bitcoin#25288 (lint backports: part 2)

2fa480a partial bitcoin#25288: Reliably don't start itself (lint-all.py runs all tests twice) (Kittywhiskers Van Gogh)
bda1e03 merge bitcoin#24982: Port `lint-all.sh` to `lint-all.py` (Kittywhiskers Van Gogh)
b054a0d merge bitcoin#24840: port `lint-shell.sh` to python (Kittywhiskers Van Gogh)
973ca7b merge bitcoin#23506: Make more shell scripts verifiable by the `shellcheck` tool (Kittywhiskers Van Gogh)
694c1a4 merge bitcoin#24929: convert shell locale linter test to Python (Kittywhiskers Van Gogh)
2a7d32a merge bitcoin#24916: Convert lint-python-utf8-encoding.sh to Python (Kittywhiskers Van Gogh)
0321fa0 merge bitcoin#24915: Convert lint-circular-dependencies.sh to Python (Kittywhiskers Van Gogh)
e3dc4b1 merge bitcoin#24902: Convert lint-include-guards.sh to Python (Kittywhiskers Van Gogh)
fc48a13 merge bitcoin#23524: Fix typos in endif header comments (Kittywhiskers Van Gogh)
1f8c3b5 merge bitcoin#24794: Convert Python linter to Python (Kittywhiskers Van Gogh)
110b6ac partial revert dash#4807: enable more multi-threading and caching in linters (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional Information

  * Depends on #6428

  * The introduction in `flake8-cached` for `lint-python.sh` in [dash#4807](#4807) was reverted as this logic wasn't going to be ported over to the Python replacement as the `flake8-cached` repo has been archived since April 2023 ([source](https://github.com/jnoortheen/flake8-cached)) and we don't use it in CI through GitLab ([build](https://gitlab.com/dashpay/dash/-/jobs/8456994796#L144)) or GitHub Actions ([build](https://github.com/dashpay/dash/actions/runs/11981121905/job/33406844883#step:7:75)).

  * [bitcoin#25288](bitcoin#25288) has been marked as partial as the change of the glob pattern from `{mod_path}/lint-*` to `{mod_path}/lint-*.py` as we still have `lint-cppcheck-dash.sh` around ([source](https://github.com/dashpay/dash/blob/b88d9910a8362ad7a47cf2ea822a00649793350b/test/lint/lint-cppcheck-dash.sh)) (and the original `cppcheck` linter upstream was removed in [bitcoin#25091](bitcoin#25091)).

    A Python port of that linter would allow for completing [bitcoin#25288](bitcoin#25288).

  ## Breaking Changes

  None expected.

  ## Checklist

  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [x] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  UdjinM6:
    utACK 2fa480a
  PastaPastaPasta:
    utACK 2fa480a

Tree-SHA512: 48ddf11be11232df26051b39dfadac9f363d2f201b9f303cad6ddd54550e2f1881947061155da9d4eaf3f5a87cdd371368dc36b4d70eb81ff4c48a7a93af63ae
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants