It is a good property for github history to have each commit buildable and runnable on its own. Bors only checks the PR as one big change before landing. Would it be possible in the future to ask bors to test each commit before merging?
1 Like
Unfortunately, I don't think that this feature is compatible with how Bors works.
As far as I can tell, Bors does not use the CI statuses on any of the commits on your branch. Rather, it only uses at the CI statuses on the merge commits that it creates.
For example, suppose that your Git history looks like this, with a default branch named master and one feature branch named myfeature that contains a single commit:
A——B——C <——master
\
D <——myfeature
If you open a pull request to merge myfeature into master and comment bors r+ on that pull request, then Bors will create a merge commit M by merging myfeature into master and will force-push the branch staging to M:
A——B——C———M <—— staging
\ /
D
Now, your CI system will test the staging branch, which is equivalent to testing the M commit. Bors waits for your CI to finish. If CI fails on staging, Bors posts a Build failed comment. If all CI checks pass on staging, Bors will fast-forward master to point to M:
A——B——C———M <—— master
\ /
D
So, as you can see, the CI status on the single commit D on the myfeature branch is never used by Bors. Bors only uses the CI status on the staging branch (i.e. the merge commit M) to decide whether a build succeeds or fails.
Here is an example of a single feature branch with three commits:
A——B——C <——master
\
D——E——F <——myfeature
A——B——C—————————M <——staging
\ /
D——E——F
A——B——C—————————M <——master
\ /
D——E——F
Here is an example of two feature branches, each with one commit:
A——B——C <——master
|\
| \
| \
\ D <——feature—1
\
\
E <——feature—2
A——B——C———————M <—— staging
|\ /|
| \ |
| \ / |
\ D /
\ /
\ /
E
A——B——C———————M <—— master
|\ /|
| \ |
| \ / |
\ D /
\ /
\ /
E
And here is an example of two feature branches, each with three commits:
A——B——C <——master
|\
| \
| \
\ D——E——F <——feature—1
\
\
G——H——I <——feature—2
A——B——C—————————————M <——staging
|\ /|
| \ / |
| \ / |
\ D——E——F /
\ /
\ /
G——H——I
A——B——C—————————————M <——master
|\ /|
| \ / |
| \ / |
\ D——E——F /
\ /
\ /
G——H——I
Notice in all of these examples that Bors only uses the CI statuses on the merge commit M.
@notriddle Can you take a look at this and make sure that I am explaining Bors correctly?
If you want to enforce the property that every commit is buildable and runnable on its own, you can accomplish this, but not within Bors. Also, it will be very inconvenient.
The basic idea is:
- Enforce the property that each pull request contains only a single commit.
- Require that both the
Travis CI - BranchandTravis CI - Pull Requestchecks pass on each pull request. - If a branch only contains one commit, and the
Travis CI - Branchcheck has passed on that branch, then we know that the single commit in that branch passes.
To implement number 1, add code like this to your Travis script:
if [[ "$TRAVIS_PULL_REQUEST" == "false" ]]; then
else
export NUMBER_OF_COMMITS=$(git rev-list --count $TRAVIS_COMMIT_RANGE)
echo $NUMBER_OF_COMMITS
if [ "$NUMBER_OF_COMMITS" -eq "1" ]; then
echo "There is exactly one commit in this pull request. Success."
else
echo "There is NOT exactly one commit in this pull request. Failing..."
exit 1
fi
fi
To implement number 2, make sure that the Travis CI - Branch, Travis CI - Pull Request, and bors checks are all required in your GitHub branch protection rule for the master branch:
Additionally, you should add the Travis CI - Pull Request to the pr_status list in bors.toml:
delete_merged_branches = true
pr_status = ["Travis CI - Pull Request"]
status = ["Travis CI - Branch"]
Also, you should make sure that your .travis.yml is configured to build all branches except staging.tmp and trying.tmp:
branches:
except:
- staging.tmp
- trying.tmp
That's all true, but I'm not sure it's relevant. Nothing prevents us from modifying bors so that it'll scan through all the old commits and check their statuses.
What really makes this inconvenient is that most GitHub CI products only build the tip commit when you push (you'd notice this if you push a pull request with multiple commits, only the last one has a status). So, to make this work, bors would have to take care of ensuring that all the intermediate commits got built.
If it did this as part of its existing queue, then the queue is now going to take twice as long or more. If it does this in parallel with its existing queue, then you risk causing a build timeout, because there's no way to make Travis prioritize staging the way we want.
1 Like
How would this work? Bors would go through each commit on the branch, one by one, see if it had a status, and if not, call e.g. the Travis API to trigger a build on that commit?
Pretty much, yeah.
I am really interested in this feature as well. I don't think it needs to slow down testing too much, because unlike bors' final test of the merge, the test of the intermediate commits don't need to be re-run.
Restricting PRs to single commits seems like a mistake.
Would I just go and code up the featuer and create a PR to bors-ng itself?
Personally, I would rather not accept such a PR.
For one thing, it doesn't actually need to be a part of bors-ng. You could write something that tests all the intermediate commits on a PR, then posts a commit status on that PR, and set the pr_status config option to require it.
For the other thing, it seems really complicated to actually do it right. As I said before, you really need to make sure that staging builds never wait on pull request builds, but there's no good API to do that.
1 Like