test: Refactor subtree exclusion in lint tests

[BrandonOdiwuor]

Fixes #17413

Refactor subtree exclusion in lint tests to one place

Second attempt after PR: #24435

[DrahtBot]

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	fjahr, maflcko, davidgumberg
Concept ACK	hebasto

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #29744 (lint: Rewrite spelling, includes, and include guards linters in Rust by davidgumberg)
• #28981 (Replace Boost.Process with cpp-subprocess by hebasto)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

[DrahtBot]

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

_{Debug: https://github.com/bitcoin/bitcoin/runs/21976874466}

[fjahr]

This doesn't feel like much of an improvement as is. We are trading a tiny bit of deduplication for the txt parsing code that is much harder to reason about than the straightforward lists. Maybe there is a simpler way of doing this through a shared python module or so.

This would also need documentation so that people know where to add which dirs for exclusion if there are changes in the future.

[DrahtBot]

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

_{Debug: https://github.com/bitcoin/bitcoin/runs/22521444236}

[BrandonOdiwuor]

@fjahr I have changed this to a python script as suggested which is relatively easy to import and is more straight forward

• also added a doc

[fjahr]

Approach ACK, thanks, I like this a lot more.

[fjahr]

utACK 6e80169

[hebasto]

Concept ACK.

[maflcko]

There is also fn get_pathspecs_exclude_subtrees() -> Vec<String>. The changes here seem fine. However, an alternative might be to rewrite the 3 lint checks to rust, so that they can use the already existing function. No strong opinion, though.

[maflcko]

I'd say this should be have "subtree" in the name, or is there a reason to globally exclude a folder that is not a subtree?

[BrandonOdiwuor]

updated the var to SHARED_EXCLUDED_SUBTREES

[BrandonOdiwuor]

@maflcko is the ultimate goal to run all lint checks from rust?

[fjahr]

re-ACK 80fa7da

However, an alternative might be to rewrite the 3 lint checks to rust, so that they can use the already existing function.

Is there any other benefit to the rewrite, aside from reusing that function? These linters were just recently rewritten to Python and I think a rewrite to rust would be a step back in some regard because we have fewer contributors in the project that are familiar with it.

[maflcko]

lgtm ACK 80fa7da

[maflcko]

Is there any other benefit to the rewrite, aside from reusing that function?

Rust is type-safe, so many issues that are found after code is written, reviewed, and merged just can not happen and will be caught, ideally before a pull is reviewed and merged. For example #29068 (comment) would have failed early in Rust (error[E0308]: mismatched types: expected i32, found bool). Let me know if I should provide more examples that were hit in this project. I know that python has type annotations, but if they are not used (like in the past where type-bugs have bitten this project), they are useless. In Rust, it is impossible to forget to annotate the type.

[davidgumberg]

ACK 80fa7da

I've written a draft PR to explore rewriting these tests in Rust: #29744

[fjahr]

Rust is type-safe, so many issues that are found after code is written, reviewed, and merged just can not happen and will be caught, ideally before a pull is reviewed and merged. For example #29068 (comment) would have failed early in Rust (error[E0308]: mismatched types: expected i32, found bool). Let me know if I should provide more examples that were hit in this project. I know that python has type annotations, but if they are not used (like in the past where type-bugs have bitten this project), they are useless. In Rust, it is impossible to forget to annotate the type.

I am aware of the general differences between Rust and Python. If there is consensus that rewriting Python programs to Rust is considered an improvement in general just because of the different language, then we should start with the functional test framework because that sees a lot more usage in terms of runs and code changes, no?

But I was rather asking about other arguments that are specific to the linters as was discussed, like the reuse of the function mentioned already.

[maflcko]

If there is consensus that rewriting Python programs to Rust is considered an improvement in general just because of the different language, then we should start with the functional test framework because that sees a lot more usage in terms of runs and code changes, no?

This is just my opinion, but I think the benefit of rewriting existing python scripts to rust is limited. Obviously, when a shortcoming is detected, or code needs to be re-structured anyway for another reason, the author can decide to write it in rust, if they prefer. I mostly see it as a style-question, which are generally left up to each pull request author themselves.

I am not sure about the benefits of rewriting the functional tests to rust. The review effort on a pull that changes 50'000+ lines of code would be impossible to gather.

$ cat ./test/functional/*py|wc -l 
56846

But I was rather asking about other arguments that are specific to the linters as was discussed, like the reuse of the function mentioned already.

I don't think there are other meaningful benefits, other than the benefits offered by the language or being able to re-use code. A programming language is just a tool to achieve a goal, so the choice of language shouldn't be a priority or blocker and more be seen as a style-question.

In any case, this trivial refactoring pull request of the lint tests here seems ready to merge with 3 ACKs.