lint: Report all lint errors instead of early exit #28862

1. 
   maflcko commented at 4:26 PM on November 13, 2023: member

   all-lint.py currently collects all failures. However, the 06_script.sh does not, since July this year (https://github.com/bitcoin/bitcoin/pull/28103#discussion_r1268115806).

   Fix this by printing all failures before exiting.

   Can be tested by modifying (for example) two subtrees in the same commit and then running the linters.

2. 
   DrahtBot commented at 4:26 PM on November 13, 2023: contributor

   <!--e57a25ab6845829454e8d69fc972939a-->

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

   <!--006a51241073e994b41acfe9ec718e94-->

   Code Coverage

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

   <!--021abf342d371248e50ceaed478a90ca-->

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	TheCharlatan, kevkevinpal

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

3. DrahtBot added the label Tests on Nov 13, 2023
4. lint: Report all lint errors instead of early exit faff3e3b46
5. ci: Add missing COPY for ./test/lint/test_runner fa01f884d3
6. maflcko force-pushed on Nov 13, 2023
7. fanquake requested review from laanwj on Nov 17, 2023
8. fanquake requested review from TheCharlatan on Nov 17, 2023
9. in test/lint/test_runner/src/main.rs:57 in fa01f884d3

     53 | +    }
     54 | +    if good {
     55 | +        Ok(())
     56 | +    } else {
     57 | +        Err("".to_string())
     58 | +    }
   

   ---

   ---

   

   TheCharlatan commented at 8:50 PM on November 19, 2023:

   Without state variables or ifs:

       [
           "src/crypto/ctaes",
           "src/secp256k1",
           "src/minisketch",
           "src/leveldb",
           "src/crc32c",
       ].iter().all(
           |&subtree| Command::new("test/lint/git-subtree-check.sh")
               .arg(subtree)
               .status()
               .expect("command_error")
               .success()
       ).then(|| ()).ok_or("".to_string())
   

   ---

   

   maflcko commented at 9:29 AM on November 20, 2023:

   Are you sure? all() short circuits (https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.all), but the goal of this pull is to report all lint errors, not only the first one.

   ---

   

   maflcko commented at 9:31 AM on November 20, 2023:

   Also, style-wise, some people prefer for over functional programming when the predicate has side effects. Happy to push what you prefer, but I wanted to mention that this was written on purpose in this way.

   ---

   

   TheCharlatan commented at 9:39 AM on November 20, 2023:

   Yeah, you're right. This is good as is.

10. in test/lint/test_runner/src/main.rs:56 in faff3e3b46 outdated

      52 | +            .success();
      53 | +    }
      54 | +    if good {
      55 | +        Ok(())
      56 | +    } else {
      57 | +        Err("".to_string())
    

    ---

    ---

    

    TheCharlatan commented at 8:53 PM on November 19, 2023:

    Maybe just add a "failure in subtree lint" (also for the others)?

    ---

    

    maflcko commented at 9:34 AM on November 20, 2023:

    I think this is already printed. For me:

    ...
    ^---- Failure generated from subtree check!
    ...
    

    ---

    

    TheCharlatan commented at 10:25 AM on November 20, 2023:

    It's a bit jarring to see Err(""), but I guess there is little point in adding a parser for the script output, or duplicating the failure reporting, so this can just be improved if/when the lint checks are converted to Rust.

    ---

    

    maflcko commented at 10:30 AM on November 20, 2023:

    yeah, my primary goal is to improve the experience when running the lint stuff (both locally and on CI). Anything else can be discussed and tackled in other threads, if there is a need and interest.

11. TheCharlatan approved
12. 
    TheCharlatan commented at 10:28 AM on November 20, 2023: contributor

    lgtm ACK fa01f884d3ac128f09bfa57ac2648a19a19d854e

    I checked that the CI can now correctly report multiple lint failures.

13. 
    kevkevinpal commented at 8:52 PM on November 20, 2023: contributor

    ACK fa01f88

    tested on my own fork and saw multiple lint errors https://github.com/kevkevinpal/bitcoin/pull/9/checks?check_run_id=18865861937

14. 
    maflcko commented at 7:50 AM on November 21, 2023: member

    tested on my own fork and saw multiple lint errors

    Yeah, I guess linters should enforce their rules globally, instead of only on the diff. Otherwise, one gets different results, depending on how the diff is calculated.

    Though, this is unrelated to this pull request and can be improved in the future, if there is need and interest.

15. fanquake merged this on Nov 22, 2023
16. fanquake closed this on Nov 22, 2023

    ---

17. maflcko deleted the branch on Nov 23, 2023
18. bitcoin locked this on Nov 22, 2024

Contributors

maflckoDrahtBotTheCharlatankevkevinpal

Review Requested
laanwj