test: Refactor subtree exclusion in lint tests

BrandonOdiwuor commented at 10:38 am on February 26, 2024: contributor

Fixes #17413

Refactor subtree exclusion in lint tests to one place

Second attempt after PR: https://github.com/bitcoin/bitcoin/pull/24435

DrahtBot commented at 10:39 am on February 26, 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	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 added the label Tests on Feb 26, 2024

BrandonOdiwuor force-pushed on Feb 26, 2024

DrahtBot added the label CI failed on Feb 26, 2024

DrahtBot commented at 10:46 am on February 26, 2024: contributor

🚧 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

BrandonOdiwuor force-pushed on Feb 26, 2024

BrandonOdiwuor marked this as ready for review on Feb 26, 2024

DrahtBot removed the label CI failed on Feb 26, 2024

in test/lint/lint-python-utf8-encoding.py:15 in b4d7b7a355 outdated

11@@ -12,11 +12,10 @@
12 
13 from subprocess import check_output, CalledProcessError
14 
15-EXCLUDED_DIRS = ["src/crc32c/", "src/secp256k1/"]

fjahr commented at 5:36 pm on February 26, 2024:

This is not the same list as what’s in the file so you are now excluding more than before

BrandonOdiwuor commented at 4:23 pm on March 11, 2024:

fixed

fjahr commented at 5:40 pm on February 26, 2024: contributor

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.

BrandonOdiwuor force-pushed on Mar 11, 2024

DrahtBot added the label CI failed on Mar 11, 2024

DrahtBot commented at 4:19 pm on March 11, 2024: contributor

🚧 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 requested review from fjahr on Mar 11, 2024

BrandonOdiwuor commented at 4:25 pm on March 11, 2024: contributor

@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

in test/lint/lint_ignore_dirs.py:1 in 3df289a48f outdated

0@@ -0,0 +1,5 @@
1+EXCLUDED_DIRS = ["src/leveldb/",

fjahr commented at 4:28 pm on March 11, 2024:

Please name it SHARED_EXCLUDED_DIRS (or something similar), that is clearer and I don’t like that the same var name is reused in the whitespace and includes linters (also means it’s harder to grep for).

BrandonOdiwuor commented at 4:42 pm on March 11, 2024:

renamed EXCLUDED_DIRS to SHARED_EXCLUDED_DIRS

fjahr commented at 4:30 pm on March 11, 2024: contributor

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

BrandonOdiwuor force-pushed on Mar 11, 2024

BrandonOdiwuor requested review from fjahr on Mar 11, 2024

fjahr commented at 4:44 pm on March 11, 2024: contributor

utACK 6e80169227ec7c8e624308c5e97a68760e10c231

DrahtBot removed the label CI failed on Mar 12, 2024

DrahtBot added the label Needs rebase on Mar 15, 2024

hebasto commented at 9:23 pm on March 15, 2024: member

Concept ACK.

in test/lint/lint_ignore_dirs.py:1 in 6e80169227 outdated

0@@ -0,0 +1,5 @@
1+SHARED_EXCLUDED_DIRS = ["src/leveldb/",

maflcko commented at 8:29 am on March 25, 2024:

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 commented at 10:51 am on March 26, 2024:

updated the var to SHARED_EXCLUDED_SUBTREES

maflcko commented at 8:31 am on March 25, 2024: member

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.

BrandonOdiwuor force-pushed on Mar 26, 2024

test: Refactor subtree exclusion in lint tests 80fa7da21c

BrandonOdiwuor force-pushed on Mar 26, 2024

BrandonOdiwuor requested review from maflcko on Mar 26, 2024

DrahtBot removed the label Needs rebase on Mar 26, 2024

BrandonOdiwuor commented at 1:37 pm on March 26, 2024: contributor

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

fjahr commented at 1:58 pm on March 26, 2024: contributor

re-ACK 80fa7da21c470302165c47cc4a6a62fb44f997ef

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.

DrahtBot requested review from hebasto on Mar 26, 2024

maflcko commented at 6:36 pm on March 26, 2024: member

lgtm ACK 80fa7da21c470302165c47cc4a6a62fb44f997ef

maflcko commented at 7:01 pm on March 26, 2024: member

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 commented at 10:36 pm on March 26, 2024: contributor

ACK https://github.com/bitcoin/bitcoin/commit/80fa7da21c470302165c47cc4a6a62fb44f997ef

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

fjahr commented at 10:47 pm on March 26, 2024: contributor

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 commented at 8:15 am on March 27, 2024: member

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.

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

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.

fanquake merged this on Mar 27, 2024

fanquake closed this on Mar 27, 2024

test: Refactor subtree exclusion in lint tests #29479

Code Coverage

Reviews

Conflicts