Skip to content

Conversation

@knst
Copy link
Collaborator

@knst knst commented May 15, 2024

Issue being fixed or feature implemented

Backports from bitcoin v22+ for a new linter for filenames and file permissions

What was done?

backports:

How Has This Been Tested?

Run new linter: test/lint/lint-files.py

Breaking Changes

N/A

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • 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

laanwj and others added 6 commits May 16, 2024 02:09
…and permissions

46b025e test: add new python linter to check file names and permissions (windsok)
6f6bb3e test: fix file permissions on various scripts (windsok)

Pull request description:

  Adds a new python linter test which tests for correct filenames and file permissions in the repository.

  Replaces the existing tests in the `test/lint/lint-filenames.sh` and `test/lint/lint-shebang.sh` linter tests, as well as adding some new and increased testing. This increased coverage is intended to catch issues such as in bitcoin#21728 and https://github.com/bitcoin/bitcoin/pull/16807/files#r345547050

  Summary of tests:
  * Checks every file in the repository against an allowed regexp to make sure only lowercase or uppercase alphanumerics (a-zA-Z0-9), underscores (_), hyphens (-), at (@) and dots (.) are used in repository filenames.

  * Checks only source files (*.cpp, *.h, *.py, *.sh) against a stricter allowed regexp to make sure only lowercase alphanumerics (a-z0-9), underscores (_), hyphens (-) and dots (.) are used in source code filenames. Additionally there is an exception regexp for directories or files which are excepted from matching this regexp (This should replicate the existing `test/lint/lint-filenames.sh` test)

  * Checks all files in the repository match an allowed executable or non-executable file permission octal. Additionally checks that for executable files, the file contains a shebang line.

  * Checks that for executable `.py` and `.sh` files, the shebang line used matches an allowable list of shebangs (This should replicate the existing `test/lint/lint-shebang.sh` test)

  * Checks every file that contains a shebang line to ensure it has an executable permission

  Additionally updates the permissions on various files to comply with the new tests.

  Fixes bitcoin#21729

ACKs for top commit:
  practicalswift:
    cr re-ACK 46b025e: patch still looks correct
  kiminuo:
    code review ACK 46b025e if `contrib/gitian-descriptors/assign_DISTNAME` permission change is deemed OK.
  laanwj:
    Code review ACK 46b025e

Tree-SHA512: 1c8201a2cee0d9cbce15652b68cec9a6458a8b493fcd5392f98560aca0b1a12e668baab65a47100f116f626dadc3f591deb47f7368468c6a46c6c712c2533455
…r test

2227fc4 test: minor fixes & improvements for files linter test (windsok)

Pull request description:

  Couple of minor fixes & improvements for files linter test added in bitcoin#21740

  - Use a context manager when opening files, so that files are closed are we are done with them

  - Use the `-z` flag when shelling out to `git ls-files` so that we can catch newlines and other weird control characters in filenames.

  From the `git ls-files` manpage:
  ```
  -z \0 line termination on output and do not quote filenames. See OUTPUT below for more information.

  Without the -z option, pathnames with "unusual" characters are quoted as explained for the configuration variable
  core.quotePath (see git-config(1)). Using -z the filename is output verbatim and the line is terminated by a NUL byte.
  ```

ACKs for top commit:
  MarcoFalke:
    cr ACK 2227fc4
  practicalswift:
    cr ACK 2227fc4: patch looks correct

Tree-SHA512: af059a805f4a7614162de85dea856052a45ab531895cb0431087e7fc9e037513fa7501bb5eb2fe43238adf5f09e77712ebdbb15b1486983359ad3661a3da0c60
fae211c lint: Start to use py lint scripts (MarcoFalke)
fa82e89 Move lint script and data file to avoid lint- prefix (MarcoFalke)

Pull request description:

ACKs for top commit:
  fjahr:
    tACK fae211c

Tree-SHA512: f8272a1bab9efb8203cac121710baae68f01f79e520ad71ff15aa516d19763d61c088b411b019de105a6a30e7ee3c274814d59963f6ac22ba1084560fb601f45
908fb7e test: Use permissions from git in `lint-files.py` (laanwj)
48d2e80 test: Don't use shell=True in `lint-files.py` (laanwj)

Pull request description:

  Improvements to the `lint-files.py` script:

  - Avoid use of `shell=True`.
  - Check the permissions in git's metadata instead of in the filesystem. This stops the umask or filesystem from interfering. It's also more efficient as it only needs a single call to `git ls-files`.

  (what triggered this change was `File "..." contains a shebang line, but has the file permission 775 instead of the expected executable permission 755.` errors running the script locally).

ACKs for top commit:
  vincenzopalazzo:
    re-tACK bitcoin@908fb7e

Tree-SHA512: 2eaf868c55a9c3508b12658a5b3ac429963fd0551e645332d0ac54be56fefccee95115e4667386df24b46b545593cb0d0bf8c6cecab73f9cb19d37ddf704c614
Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

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

utACK e0ad143

@knst knst requested a review from UdjinM6 May 16, 2024 06:34
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

@PastaPastaPasta PastaPastaPasta merged commit 95c3532 into dashpay:develop May 17, 2024
@UdjinM6 UdjinM6 added this to the 21 milestone May 18, 2024
PastaPastaPasta added a commit that referenced this pull request Dec 4, 2024
, bitcoin#24844, bitcoin#24853, bitcoin#24895, bitcoin#24802, bitcoin#24932, partial bitcoin#23212, bitcoin#23462 (lint backports)

852f55e merge bitcoin#24932: Convert lint-locale-dependence.sh to Python (Kittywhiskers Van Gogh)
f745b7f merge bitcoin#24802: convert format strings linter test to python (Kittywhiskers Van Gogh)
a0b051b partial revert dash#4807: enable more multi-threading and caching in linters (Kittywhiskers Van Gogh)
7864c21 merge bitcoin#24895: Convert lint-includes.sh to Python (Kittywhiskers Van Gogh)
f63bafe merge bitcoin#24853: Convert lint-git-commit-check.sh to Python (Kittywhiskers Van Gogh)
d463d7d fix: clone full history on GitHub Actions build job, track `develop` (Kittywhiskers Van Gogh)
d23b661 merge bitcoin#24844: Convert lint-whitespace.sh to Python (Kittywhiskers Van Gogh)
fe16516 merge bitcoin#24849: Convert lint-logs.sh to Python (Kittywhiskers Van Gogh)
dbfd0b0 merge bitcoin#24766: convert spellchecking lint test to python (Kittywhiskers Van Gogh)
5c23add merge bitcoin#24778: Convert Python dead code linter test to Python (Kittywhiskers Van Gogh)
829dcfb partial bitcoin#23462: Enable SC2046 and SC2086 shellcheck rules (Kittywhiskers Van Gogh)
df6be0e partial bitcoin#23212: enable mypy import checking (Kittywhiskers Van Gogh)
f77eca9 fix: make sure that parallelized `lint-all` runs incl. python scripts (Kittywhiskers Van Gogh)
e220175 merge bitcoin#22861: Update test README and lint script (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional Information

  * Dependency for #6429

  * Even though support for Python scripts was extended to `lint-all` in [bitcoin#24762](f226e8d) ([dash#6023](#6023)), the changes needed were not extended to parallelized linting added in [dash#4637](#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](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](https://github.com/dashpay/dash/blob/f9d044d5ec7cc492d10fd4a89f85927b00305e83/test/functional/test_framework/crypto/bip324_cipher.py#L16-L17) and [here](https://github.com/dashpay/dash/blob/f9d044d5ec7cc492d10fd4a89f85927b00305e83/test/functional/test_framework/crypto/muhash.py#L9).

    <details>

    <summary>Lint errors:</summary>

    ```
    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)
    ```

    </details>

    And upgrading to the latest version of `mypy` used upstream (1.4.1, [source](https://github.com/bitcoin/bitcoin/blob/f7144b24be09e7db2173a0b15d73f99a08b98118/ci/lint/04_install.sh#L52)) brings syntax awareness but new errors.

    <details>

    <summary>Lint errors:</summary>

    ```
    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)
    ```

    </details>

  * [bitcoin#23462](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](https://github.com/dashpay/dash/blob/4aaa314a575a318683f3d15f16f396ac1a44a913/test/lint/lint-git-commit-check.py#L45)) doesn't play nice with it (see [actions/checkout#423](https://github.com/actions/checkout/discussions/423)).

    No such issue exists on GitLab ([build](https://gitlab.com/dashpay/dash/-/jobs/8456260939#L113)), but it remains present on GitHub Actions ([build](https://github.com/dashpay/dash/actions/runs/11996049800/job/33440106874?pr=6428#step:7:103)). 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](#6428 (comment)))

  * Changes to `run-lint-format-strings.py` made in [dash#4807](#4807) were reverted as they were interfering with `lint-format-strings.py`

    <details>

    <summary>Lint error:</summary>

    ```
    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
    ```

    </details>

  ## 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:
  knst:
    utACK 852f55e
  UdjinM6:
    utACK 852f55e

Tree-SHA512: e508311c00249e7e48f91ca23e6f62117c07497f9c4471d07659b233e43a9f4d09ee2bfe0dd76f0c9746209cdba5895cb002a708924daa19d7aa76869815a7d9
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