The lint commit-script-check.sh can not be called from the test_runner at all and must be called manually. Also, some checks require COMMIT_RANGE to be set.
Fix all issues by moving two lint checks into the test_runner. Also, the proper commit range is passed to the checks by the test_runner, so that the user no longer has to do it.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31653.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | kevkevinpal, willcl-ark |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
723@@ -650,6 +724,10 @@ fn main() -> ExitCode {
724 };
725
726 let git_root = get_git_root();
727+ let commit_range = commit_range();
728+ let commit_log = check_output(git().args(["log", "--no-merges", "--oneline", &commit_range]))
729+ .expect("git error");
730+ println!("Checking commit range ({commit_range}):\n{commit_log}\n");
looks like this adds a new log added to the CI (which I think is fine)
For other reviewers this is what this log looks like in Cirrus CI
0[12:23:15.442] Checking commit range (HEAD~..HEAD):
1[12:23:15.442] fa42b4aa5e lint: Call lint_commit_msg from test_runner
2[12:23:15.442] faee335285 lint: Call lint_scripted_diff from test_runner
looks like this adds a new log added to the CI (which I think is fine)
It shouldn’t add a new log to the CI. The commits were logged in the CI before this pull request (See the echo).
The goal of this pull request is to move the logic into the test_runner, so that it is easier to run the test_runner locally and replicate the CI.
I guess it doesnt add a new line but modify’s what is already there
adding Checking commit range
example from #31690
0[20:29:08.514] + echo
1[20:29:08.514]
2[20:29:08.514] + git log --no-merges --oneline HEAD~..HEAD
3[20:29:08.516] 2db6923332 [doc] Amend notes on benchmarking
either way looks good to me!
723@@ -650,6 +724,10 @@ fn main() -> ExitCode {
724 };
725
726 let git_root = get_git_root();
727+ let commit_range = commit_range();
728+ let commit_log = check_output(git().args(["log", "--no-merges", "--oneline", &commit_range]))
729+ .expect("git error");
commit_range there is also a .expect which has the text “git command failed”
it would make sense to have the same text for consistency, but feel free to ignore
git log to fail, but since RUST_BACKTRACE=1 is set, the error should be self-explanatory anyway. Possibly expect could be replaced by unwrap?
crACK fa42b4a
I think this is a good change moves some of our python linting scripts into our rust lint runner which reduces the number of python files we need to maintain and does the same thing.
Looks like this change adds another log which lists the commits being linted and their oneline which I think is fine
Allowing to call the check from the test_runner allows for consistent
error messages and better UX by having a single test_runner for all
checks.
This requires the env var to be set for now. The next commit makes the
commit range optional.
Having a single test_runner for all logic improves the consistency and
UX.
Allowing to call the check from the test_runner allows for consistent
error messages. Also, manually setting the commit range is no longer
needed.
18- # Otherwise, assume that a merge commit exists. This merge commit is assumed
19- # to be the base, after which linting will be done. If the merge commit is
20- # HEAD, the range will be empty.
21- COMMIT_RANGE="$( git rev-list --max-count=1 --merges HEAD )..HEAD"
22 fi
23 export COMMIT_RANGE
I think we can delete this line and then do the following on line 12
export COMMIT_RANGE="HEAD~..HEAD"
since now this export sometimes is empty
tACK faf8fc5487d409eeff7b7b260eabb6929a7b7a5f
Makes sense to move this into test_runner.
Worked correctly for me, although I did forget that we currently can’t run the linter (and local CI) in a worktree, which I was using.
I’ll try and open a PR to fix that shortly.
Labels
Tests