ci, lint: Remove usage of TRAVIS_COMMIT_RANGE #20071

pull fjahr wants to merge 3 commits into bitcoin:master from fjahr:commit_range changing 3 files +29 −8

fjahr commented at 4:16 PM on October 3, 2020: member

This is causing problems again, very similar to #19654.

UPDATE: This now removes all remaining usages of TRAVIS_COMMIT_RANGE and instead uses TRAVIS_BRANCH for the range, including lint-git-commit-check where TRAVIS_COMMIT_RANGE had already been removed. For builds triggered by a pull request, TRAVIS_BRANCH is the name of the branch targeted by the pull request. In the linters there is still a fallback that assumes master as the target branch.
DrahtBot added the label Tests on Oct 3, 2020
hebasto commented at 5:33 PM on October 3, 2020: member

Concept ACK.

As this is the second similar issue, it is natural to wonder whether other TRAVIS_COMMIT_RANGE use cases are safe?
fjahr commented at 5:35 PM on October 3, 2020: member

Concept ACK.

As this is the second similar issue, it is natural to wonder whether other TRAVIS_COMMIT_RANGE use cases are safe?

The only other place seems to be the whitespace linter, I am looking into it :) Since this is an acute issue I just submitted it alone so it could be merged faster.
fjahr force-pushed on Oct 3, 2020
fjahr renamed this:
~~ci: Don't use TRAVIS_COMMIT_RANGE for commit-script-check~~
ci, lint: Remove usage of TRAVIS_COMMIT_RANGE
on Oct 3, 2020
hebasto commented at 6:40 PM on October 3, 2020: member

@fjahr

While you are working on this: detailed comments will be much appreciated :)
in ci/lint/06_script.sh:10 in 25aeb0e4b0 outdated
```
   6 | @@ -7,7 +7,9 @@
   7 |  export LC_ALL=C
   8 |  
   9 |  if [ "$TRAVIS_EVENT_TYPE" = "pull_request" ]; then
  10 | -  test/lint/commit-script-check.sh $TRAVIS_COMMIT_RANGE
  11 | +  MERGE_BASE=$(git merge-base HEAD master)
```
sipa commented at 6:42 PM on October 3, 2020:

I think you want to use $TRAVIS_BRANCH here instead of master, as that would be wrong for non-master PRs.

Also, I think this can be simplified to just COMMIT_RANGE="$TRAVIS_BRANCH..HEAD" then.

fjahr commented at 7:46 PM on October 3, 2020:

done, I have also introduced $TRAVIS_BRANCH to lint-git-commit-check.sh where I had previously removed TRAVIS_COMMIT_RANGE
fjahr force-pushed on Oct 3, 2020
lint: Don't use TRAVIS_COMMIT_RANGE for commit-script-check 1b41ce8f5f
lint: Don't use TRAVIS_COMMIT_RANGE in whitespace linter c11dc995c9
lint: Use TRAVIS_BRANCH in lint-git-commit-check.sh a91ab86fae
fjahr force-pushed on Oct 3, 2020
fjahr commented at 7:59 PM on October 3, 2020: member

@fjahr

While you are working on this: detailed comments will be much appreciated :)

True, I added some on TRAVIS_BRANCH and a little note where I still use MERGE_BASE. Let me know if you see other places where I should add some more.
sipa commented at 9:59 PM on October 3, 2020: member

ACK a91ab86fae91d416d664d19d2f482a8d19c115a6. See test I tried in #20075.

The AppVeyor failure is expected, see #20066.
in test/lint/lint-git-commit-check.sh:37 in a91ab86fae
```
  32 | +
  33 |  if [ -z "${COMMIT_RANGE}" ]; then
  34 |      if [ -n "$1" ]; then
  35 |        COMMIT_RANGE="HEAD~$1...HEAD"
  36 |      else
  37 | +      # This assumes that the target branch of the pull request will be master.
```
sipa commented at 10:09 PM on October 3, 2020:

Why is this a reasonable assumption?

fjahr commented at 10:57 PM on October 3, 2020:

I assume this will mostly be used when people run the linters locally after they have worked on a pull request and before they push and open the PR (this is what I do). And I assume most people work on PRs against master most of the time and those that don't probably are not at a stage where they care about linters. If not, I can't think of a better way that just works for everyone without passing in more parameters. And that is already possible with COMMIT_RANGE which I assume people will use if they work against a different branch and want to run the linter. If this would just be set to "HEAD" here for example as the fallback then the linter would run through all commits in history and find some old commits that violate this rule. We can think of other fallbacks but this one seems most reasonable.

But also this was the way it already worked before I first touched this file in my earlier change, so I did not overthink it TBH: https://github.com/bitcoin/bitcoin/pull/19654/commits/72351784b3df21a89f79076f4b814a6e700b6469

sipa commented at 11:13 PM on October 3, 2020:

I see, this isn't used in any automated CI.

fjahr commented at 11:23 PM on October 3, 2020:

That's my understanding, that the linters only run on Travis.
MarcoFalke merged this on Oct 4, 2020
MarcoFalke closed this on Oct 4, 2020
DrahtBot locked this on Feb 15, 2022

Contributors

Labels

Linked (view graph)

#19654 lint: Don't use TRAVIS_COMMIT_RANGE in commit message linter