lint: Misc improvements for lint runner #29537

pull fjahr wants to merge 3 commits into bitcoin:master from fjahr:2024-03-lint-runner changing 3 files +7 −2

fjahr commented at 8:46 PM on March 2, 2024: contributor
1. Document the dependency to rust being installed locally
2. Add the build output directory to gitignore
3. Clean up the build output directory when running make clean
DrahtBot commented at 8:47 PM on March 2, 2024: contributor



The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Code Coverage

For detailed information about the code coverage, see the test coverage report.



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK maflcko, TheCharlatan

Stale ACK ismaelsadeeq

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
DrahtBot added the label Tests on Mar 2, 2024

Type	Reviewers
ACK	maflcko, TheCharlatan
Stale ACK	ismaelsadeeq

in Makefile.am:341 in 6321d06153 outdated

 337 | @@ -338,7 +338,7 @@ clean-docs:
 338 |  clean-local: clean-docs
 339 |  	rm -rf coverage_percent.txt test_bitcoin.coverage/ total.coverage/ fuzz.coverage/ test/tmp/ cache/ $(OSX_APP)
 340 |  	rm -rf test/functional/__pycache__ test/functional/test_framework/__pycache__ test/cache share/rpcauth/__pycache__
 341 | -	rm -rf osx_volname dist/
 342 | +	rm -rf osx_volname dist/ test/lint/test_runner/target/

TheCharlatan commented at 12:36 PM on March 6, 2024:

We also don't delete the test/lint/__pycache__, maybe a good opportunity to add it?

fjahr commented at 11:55 PM on March 6, 2024:

yepp, thanks, added!

fjahr force-pushed on Mar 6, 2024
in test/lint/README.md:20 in 3d44a26152 outdated
```
  15 | @@ -16,13 +16,15 @@ result is cached and it prevents issues when the image changes.
  16 |  test runner
  17 |  ===========
  18 |  
  19 | -To run all the lint checks in the test runner outside the docker, use:
  20 | +To run all the lint checks in the test runner outside the docker you first need
  21 | +to [install the rust toolchain](https://www.rust-lang.org/tools/install) locally.
```
maflcko commented at 11:41 AM on March 11, 2024:

Not sure about prescribing a specific installation method. Anyone should be free to use whatever method they like the most.

fjahr commented at 3:39 PM on March 11, 2024:

That was also my intention with this link. It recommends one installation method but makes clear this is just a recommendation and also links to other installation methods available. What alternative solution would you suggest?

Also, I think generally in the docs we often just suggest one specific installation method for dependencies, like brew for macos or apt-get for Ubuntu etc. although there are other options available.

maflcko commented at 3:51 PM on March 11, 2024:

What alternative solution would you suggest?

Not adding a link? Seems unlikely that someone doesn't know how to install packages, and at the same time can not use a search engine? https://duckduckgo.com/?t=ffab&q=install+the+rust+toolchain

apt-get for Ubuntu

Yeah, using the distro-shipped package manager seems to be normally the first choice and the most likely method to work in most machine-settings?

(No strong opinion in any case)

fjahr commented at 4:03 PM on March 11, 2024:

Not adding a link? Seems unlikely that someone doesn't know how to install packages, and at the same time can not use a search engine?

Anyone using a search engine will either end up on this page (first result) or another page that suggests the same install method, so we may save people that indirection and keep them safer in the process too.

How about just copying from the experts? https://github.com/rust-bitcoin/rust-bitcoin?tab=readme-ov-file#installing-rust, i.e. this: "Rust can be installed using your package manager of choice or rustup.rs. The former way is considered more secure since it typically doesn't involve trust in the CA system. But you should be aware that the version of Rust shipped by your distribution might be out of date."

maflcko commented at 4:11 PM on March 11, 2024:

version of Rust shipped by your distribution might be out of date.

Not sure. I disagree that a version of a rust compiler shipped with a commonly used distro can be "out of date". This opinion is based on the broken assumption that developers only ever use the latest toolchain.

It would be better to support as many compiler versions as possible, if reasonable.

fjahr commented at 4:19 PM on March 11, 2024:

Ok, then I am keeping this as is for now but I will remove the link if more reviewers share your opinion that it should be removed.
maflcko commented at 5:17 PM on March 11, 2024:

nit (feel free to ignore)

to install the rust toolchain using your package manager of choice or [rustup](https://www.rust-lang.org/tools/install).
fjahr commented at 8:24 PM on March 17, 2024:

done
ismaelsadeeq approved
ismaelsadeeq commented at 4:06 PM on March 11, 2024: member

utACK 3d44a2615269df061fcfa9b317132a7fcfedd366
in test/lint/README.md:27 in 3d44a26152 outdated
```
  24 |  ```sh
  25 |  ( cd ./test/lint/test_runner/ && cargo fmt && cargo clippy && RUST_BACKTRACE=1 cargo run )
  26 |  ```
  27 |  
  28 | -#### Dependencies
  29 | +#### Python dependencies
```
maflcko commented at 4:36 PM on March 11, 2024:

Not sure if this is correct. For example, ShellCheck is written in Haskell, not Python.

fjahr commented at 4:41 PM on March 11, 2024:

dropped that change
fjahr force-pushed on Mar 11, 2024
maflcko approved
maflcko commented at 5:18 PM on March 11, 2024: member

lgtm
lint: Clarify lint runner rust dependency fad7f42324
lint: Add lint runner build dir to gitignore cfa057b86d
lint: Add lint runner build dir and lint pycache to clean task 742d2b9347
fjahr force-pushed on Mar 17, 2024
fjahr commented at 8:25 PM on March 17, 2024: contributor

Rebased to give the CI a chance to show green and took the suggested rewording from @maflcko .
maflcko commented at 7:10 AM on March 18, 2024: member

ACK 742d2b93473a856786e32c5e35e3b6ce2a95000f
DrahtBot requested review from ismaelsadeeq on Mar 18, 2024
TheCharlatan approved
TheCharlatan commented at 7:22 AM on March 18, 2024: contributor

ACK 742d2b93473a856786e32c5e35e3b6ce2a95000f
fanquake merged this on Mar 18, 2024
fanquake closed this on Mar 18, 2024
bitcoin locked this on Mar 18, 2025

Contributors

Review Requested
ismaelsadeeq

Labels

Tests

lint: Misc improvements for lint runner #29537

Code Coverage

Reviews