Port lint scripts to Python

laanwj commented at 11:46 am on April 6, 2022: member

To prevent endless issues when running the linters locally (say, on specific versions of bash, or system tools having certain names, or a certain implementation of sed, etc) , as well as for maintainability/reviewability, it’s preferred for linter scripts to be written in Python.

Here is a list of scripts, feel free to pick up any that haven’t been ported:

test/lint/check-doc.py (added in #7280, was always Python)
test/lint/git-subtree-check.sh (in progress in #25039)
test/lint/lint-all.sh (ported in #24982)
test/lint/lint-assertions.sh (ported in #24856)
test/lint/lint-circular-dependencies.sh (ported in #24915)
test/lint/lint-cpp.sh (removed in #24785)
test/lint/lint-files.py (added in #21740, was always Python)
test/lint/lint-format-strings.sh (ported in #24802)
test/lint/lint-git-commit-check.sh (ported in #24853)
test/lint/lint-include-guards.sh (ported in #24902)
test/lint/lint-includes.sh (ported in #24895)
test/lint/lint-locale-dependence.sh (ported in #24932)
test/lint/lint-logs.sh (ported in #24849)
test/lint/lint-python-dead-code.py (ported in #24778)
test/lint/lint-python-mutable-default-parameters.sh (ported in #24800)
test/lint/lint-python.sh (ported in #24794)
test/lint/lint-python-utf8-encoding.sh (ported in #24916)
test/lint/lint-qt.sh (removed in #24790)
test/lint/lint-shell-locale.sh (ported in #24929)
test/lint/lint-shell.sh (ported in #24840)
test/lint/lint-spelling.py (ported in #24766)
test/lint/lint-submodule.sh (ported in #24803)
test/lint/lint-tests.sh (ported in #24815)
test/lint/lint-whitespace.sh (ported in #24844)
test/lint/run-lint-format-strings.py (added in #13705, was always Python)

Although I try to keep this post up to date, before starting on a linter, please first do a github search in PRs to see if a PR for that already exists.

Guidelines

Don’t forget to update test/README.md when the name of a script changes.
Avoid shell=true on subprocess calls. Remember that the point here is to avoid shell ambiguities.
If possible, avoid calling out to subprocesses at all and use what Python APIs provide (say, grep sed awk sha256sum). An exception if this would introduce further dependencies that need to be installed. These are OK to use subprocess for:
- git subcommands including git grep—the alternative would be to require an extra dependency on a Python git module, also, git can be assumed to be the same implementation everywhere
- Other scripts in the source tree.
- External linters (vulture, codespell, etc) that provide only a command-based interface.
Some further recommendations about Python usage here: #24766#pullrequestreview-932953476

Useful skills:

Python
Being able to read shell script

Want to work on this issue?

For guidance on contributing, please read CONTRIBUTING.md before opening your pull request.

laanwj added the label Tests on Apr 6, 2022

laanwj added the label good first issue on Apr 6, 2022

fanquake deleted a comment on Apr 7, 2022

Eunoia1729 commented at 6:27 am on April 7, 2022: contributor

Picking lint-format-strings.sh to start with.

laanwj commented at 11:56 am on April 7, 2022: member

Picking lint-format-strings.sh to start with.

Thanks! It should be a fairly straightforward one, as it already calls into a Python script, run-lint-format-strings.py, to do the brunt of the work. Could turn that into a module instead of subprocess. Mostlyt a matter of replacing the grep with Python code.

KevinMusgrave commented at 3:59 pm on April 7, 2022: contributor

I’ll try lint-python-mutable-default-parameters.sh

MarcoFalke commented at 8:05 am on April 8, 2022: member

Reminder that review is more important than porting, so I encourage and recommend that anyone who opened a pull request to port to also review at least one other port pull request.

Kvaciral commented at 1:33 pm on April 8, 2022: contributor

I’d like to port lint-whitespace.sh

KevinMusgrave commented at 10:03 pm on April 9, 2022: contributor

I’ll do lint-tests.sh

Eunoia1729 commented at 4:33 am on April 10, 2022: contributor

Picking lint-shell-locale.sh next

whiteh0rse commented at 10:15 pm on April 10, 2022: contributor

I’ll give a stab at test/lint/lint-shell.sh.

MarcoFalke referenced this in commit 22e3b6f4d5 on Apr 11, 2022

sidhujag referenced this in commit 50195ebdc9 on Apr 11, 2022

MarcoFalke commented at 7:31 am on April 13, 2022: member

Also, if you port a script, make sure to test that a failure looks similar before and after the change. Ideally, leave a comment with the output copy-pasted or a screenshot in the pull request.

Kvaciral commented at 1:41 am on April 14, 2022: contributor

Giving a port of /test/lint/lint-git-commit-check.sh a go next..

MarcoFalke referenced this in commit d0f7493b6c on Apr 14, 2022

hiagopdutra commented at 7:28 pm on April 14, 2022: contributor

I’ll port the test/lint/lint-assertions.sh

Kvaciral commented at 1:42 pm on April 15, 2022: contributor

Working on a port of /test/lint/lint-includes.sh now..

anibilthare commented at 4:12 pm on April 17, 2022: none

Since python 3.5, run() API is available in subprocess module. In a situation where one can use both the APIs run() and check_output() is there a preference to one over the other? Backward compatibility maybe?

brydinh commented at 4:24 pm on April 17, 2022: contributor

Giving test/lint/lint-include-guards.sh a shot.

laanwj commented at 1:29 pm on April 18, 2022: member

Since python 3.5, run() API is available in subprocess module. In a situation where one can use both the APIs run() and check_output() is there a preference to one over the other? Backward compatibility maybe?

It’s fine to use either imo. But the general tendency I’m seeing is that check_output is used in the linters when stdout is needed. run is more flexible, and also allows for other combinations of settings.

laanwj commented at 1:57 pm on April 18, 2022: member

Although I try to keep the OP up to date, before starting on a linter, please first do a github search in PRs to see if a PR for that already exists.

laanwj referenced this in commit 3059d4dd72 on Apr 18, 2022

laanwj referenced this in commit 5fdf37e14b on Apr 18, 2022

Smlep commented at 9:48 pm on April 18, 2022: contributor

Hey 👋 I’ll port test/lint/lint-circular-dependencies.sh

Edit: done (https://github.com/bitcoin/bitcoin/pull/24915)

sidhujag referenced this in commit d100a1c133 on Apr 19, 2022

Kvaciral commented at 12:02 pm on April 19, 2022: contributor

I’ll be working on the port of lint-locale-depence.sh next..

MarcoFalke referenced this in commit fc99f8c09e on Apr 20, 2022

laanwj commented at 12:49 pm on April 20, 2022: member

I’ve removed commit-script-check.sh from the list as it’s a) not a linter b) messes with the git tree c) intentionally invokes shell script, so there’s little to pythonize about it unless we change scripted-diffs to use Python, which is way outside scope of this issue.

MarcoFalke commented at 12:57 pm on April 20, 2022: member

Not sure what the status of “extended-lint” is. Can this be removed?

laanwj commented at 1:21 pm on April 20, 2022: member

Not sure what the status of “extended-lint” is. Can this be removed?

Good point. It doesn’t seem to be invoked as part of the CI at all?

I guess the cppcheck linter can be part of the normal lints, or removed completely? I’m a bit confused. I would have assumed it was still active as of #20530, when the cppcheck version was upgraded. But also looks like fa2941bbf47a8a6b79b8db4a87e1aedcf6a29a5e already removed it from the CI in 2019.

Anyhow, removed from the list, there’s no point in wasting anyone’s time porting it if it’s probably going away.

MarcoFalke commented at 1:25 pm on April 20, 2022: member

I don’t have access to logs at this point, but I am assuming I removed it because it was failing.

goldeneagle3636 commented at 0:57 am on April 21, 2022: none

I’d like to convert test/lint/git-subtree-check.sh.

laanwj referenced this in commit 2513499348 on Apr 21, 2022

sidhujag referenced this in commit 57d8585eac on Apr 22, 2022

laanwj referenced this in commit 7134327be5 on Apr 25, 2022

laanwj referenced this in commit c90b42bcdb on Apr 25, 2022

laanwj referenced this in commit 8b686776ef on Apr 25, 2022

laanwj referenced this in commit 777b89b300 on Apr 25, 2022

laanwj referenced this in commit 0342ae1d39 on Apr 25, 2022

laanwj referenced this in commit 9eedbe98c8 on Apr 25, 2022

laanwj referenced this in commit 16fa967d3c on Apr 25, 2022

laanwj referenced this in commit 1e7db37e76 on Apr 25, 2022

laanwj commented at 5:50 pm on April 25, 2022: member

Looks like we’re almost done here!

hiagopdutra commented at 6:14 pm on April 25, 2022: contributor

I’ll convert the test/lint/lint-all.sh to finish it!

sidhujag referenced this in commit 03867261ad on Apr 26, 2022

sidhujag referenced this in commit ea3961c0ac on Apr 26, 2022

sidhujag referenced this in commit d4371f17a5 on Apr 26, 2022

sidhujag referenced this in commit 80390805ab on Apr 26, 2022

laanwj referenced this in commit 85aea18ae6 on Apr 28, 2022

laanwj removed the label good first issue on May 9, 2022

laanwj commented at 9:23 am on May 9, 2022: member

I’m going to close this issue. The test/lint/git-subtree-check.sh port is still under review, but all the linters have been ported and merged, so there’s no need for this issue to attract attention anymore.

Thanks everyone for the help!

laanwj closed this on May 9, 2022

DrahtBot locked this on May 9, 2023

Port lint scripts to Python #24783

Guidelines

Useful skills:

Want to work on this issue?