New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add bors integration for merging PRs #1068

Merged
merged 3 commits into from Jul 16, 2018

Conversation

Projects
None yet
4 participants
@ppannuto
Copy link
Member

ppannuto commented Jul 3, 2018

Rust has spun out their automated build and merge tool bors, as a service. It looks pretty easy to integrate. I believe basically the contents of this PR plus flipping a switch on the bors website.

The big change that bors provides over our current system is that currently Travis builds when branches are pushed, which considers whether the code will build if merged with master at the time the PR is created. It does not test again at the time the PR is approved, which means if another PR is merged in the interim, merging a PR that passed checks could break the build. This is unlikely to happen often, but has happened to use before. (And, happens to be currently set up to happen again with the split OptionalCell PRs).

The greater motivation for me is that bors also brings a bit more clarity of intent to approvals. In particular, I tend to view a github 'approval' as 'this is clear to merge right now', but I don't think that's a universal sentiment (e.g. any P-Upkeep that was approved and not immediately merged, or many of the OptionalCell PRs that are currently approved but have not yet been tested on hardware). Using bors affords us the opportunity to disentangle approving a set of changes from an intent to merge. GitHub approvals can continue to work as they do without change, which is nice as it lets us keep our P-Significant workflow the same. However, now there is an option for an explicit 'intent to merge' in bors r+.

It looks like there is a healthy number of other projects (google) using bors, though interestingly a number of them seem to be the embedded rust folks (or google's just biasing my search results). I've looked into this a few times before, but it was too much manual effort to set up an get working before. It looks like bors-as-a-service has finally matured to a point where it's stable enough that I think we should consider adopting it.

@ppannuto ppannuto added the rfc label Jul 3, 2018

@niklasad1

This comment has been minimized.

Copy link
Member

niklasad1 commented Jul 3, 2018

+1 from my side

@alevy

This comment has been minimized.

Copy link
Member

alevy commented Jul 3, 2018

I'm strongly in favor. Before making the switch, we should have documentation somewhere obvious that explains how to use bors and how it affects the process. I have a general sense, but wouldn't know how to use it right off the bat

@bradjc

This comment has been minimized.

Copy link
Contributor

bradjc commented Jul 3, 2018

I'm a little unclear what this buys us. My two concerns:

  • bors makes sense for rust because their full Travis build takes two hours or so, and it's not worth running that on every commit. Ours, in contrast, takes about 6 minutes.
  • the bors interface is half documented and overly cryptic. There is a homu page that describes most (but certainly not all) commands in the most terse way possible, and doesn't explain the options those commands can take.

If all we want is to check that a build still succeeds all that is needed is to click "retry build" in the Travis page before clicking merge. And if someone approves with the intent to merge it's just mark approve and then click merge.

bors also splits approvals, you could approve in the GitHub sense, or in the bors sense, and I believe those don't show up in the same ui element.

@alevy

This comment has been minimized.

Copy link
Member

alevy commented Jul 3, 2018

How would we feel about giving this a test run in a more malleable repo? E.g. Signpost, or tock-sensortag, or libtock-c or something?

@ppannuto

This comment has been minimized.

Copy link
Member

ppannuto commented Jul 3, 2018

How would we feel about giving this a test run in a more malleable repo? E.g. Signpost, or tock-sensortag, or libtock-c or something?

That seems like a good idea, I'll put together a similar PR for libtock-c.

bors makes sense for rust because their full Travis build takes two hours or so, and it's not worth running that on every commit. Ours, in contrast, takes about 6 minutes.

I'm advocating for running travis more often, not less. (Though to that end I should/will uncomment the master in the travis config from this PR in the interest of catching trivial errors faster)

the bors interface is half documented and overly cryptic. There is a homu page that describes most (but certainly not all) commands in the most terse way possible, and doesn't explain the options those commands can take.

The documentation is a little terse, but it's target audience is developers, I don't think it's unreasonable. It lists all the commands as I can see it; I don't think there are a lot of options, it's a simple tool. I'm not sure what homu page you're referring to. The documentation here is from the documentation tab of the same bors link I put at the opening of this PR. I think bors used to be pretty cryptic and over the last few months folks have put effort into pulling it out of the rust core, documenting it, and making it usable for other projects.

If all we want is to check that a build still succeeds all that is needed is to click "retry build" in the Travis page before clicking merge.

Yes, but I would advocate that this should be automated, not manual. Hence bors. (Incidentally, that same argument is what led to the creation of bors in the first place)

bors also splits approvals, you could approve in the GitHub sense, or in the bors sense, and I believe those don't show up in the same ui element.

Yep. That's a bit unfortunate, but not a deal-breaker I don't think.

@alevy

This comment has been minimized.

Copy link
Member

alevy commented Jul 4, 2018

Now that it's up. Let's give this a couple weeks on libtock-c and revisit here.

@bradjc

This comment has been minimized.

Copy link
Contributor

bradjc commented Jul 6, 2018

Homu: https://buildbot2.rust-lang.org/homu/ It's the first thing that comes up when I google rust bors.

I'll note I'm very skeptical of the value here. It seems like adding an entire tool that would have only caught one bug for us over 806 pull requests doesn't make sense.

If we got the benefit that rust does where we could run a few checks on every commit, and had a super test suite that took hours to run that had to pass before merging, then this seems like an obvious benefit to me. But, we don't. If we had no way to run travis at arbitrary points, this would seem like an obvious benefit to me, but we can run travis at will.

@bradjc
Copy link
Contributor

bradjc left a comment

See comment

@ppannuto

This comment has been minimized.

Copy link
Member

ppannuto commented Jul 6, 2018

If we got the benefit that rust does where we could run a few checks on every commit, and had a super test suite that took hours to run that had to pass before merging, then this seems like an obvious benefit to me.

Except that we have a history of working hard to shorten travis builds because we currently have to babysit checks before merging (#1055 only builds one debug config to keep travis fast, #622 killed off all mac builds, the recent LLD toolchain PR celebrating faster travis builds). This isn't to say we should have a license to make tests take forever, but we shouldn't be optimizing for babysitting PRs.

but we can run travis at will.

Yes, we could also run all tests manually as well. We automate things because computers are better are following rules diligently than humans.

I'll note I'm very skeptical of the value here. It seems like adding an entire tool that would have only caught one bug for us over 806 pull requests doesn't make sense.

For me it's less about the bug-catching (though that is a perk) than it is the workflow enhancement. It's obnoxious to have PRs that are "approved but for this doc typo / broken link / whatever", that you then fix, approve again, and then sit like a monkey waiting for travis to re-build what you know is going to succeed because you just changed a comment, all so you can click the merge button.

It also resolves the distinction between "Approving" a PR and indicating that a PR should be merged (because the current system cannot always capture intent. You may have "Approved with intent to merge" but CI was still running so you couldn't click the merge button). With bors, if you intend for something to merge you say so explicitly.

@bradjc

This comment has been minimized.

Copy link
Contributor

bradjc commented Jul 6, 2018

Longer travis times that don't test more isn't a goal either. As a developer, I care a lot about checking that travis succeeds when I submit a PR. Shorter travis times means I have to do less baby sitting my PRs because I know if they pass without having to come back much later. I care much more about optimizing that than I do about optimizing the reviewer workflow.

We can get the best of both worlds, but it means having a quick check started by commits and a long check started by bors.

ppannuto added a commit that referenced this pull request Jul 6, 2018

fix dangling reference to NumCell
For the record, I swear I did not plan this, but this is exactly the
problem that #1068 would fix.

@ppannuto ppannuto referenced this pull request Jul 6, 2018

Merged

fix dangling reference to NumCell #1078

2 of 2 tasks complete

ppannuto added a commit that referenced this pull request Jul 7, 2018

sam4l: fixup master from conflicting PRs
This was predicted by #1068, but I forgot about it because this had
been sitting for a while, sorry.

@ppannuto ppannuto referenced this pull request Jul 7, 2018

Merged

sam4l: fixup master from conflicting PRs #1096

2 of 2 tasks complete
# List of commit statuses that must pass on the merge commit before it is
# pushed to master.
status = [
"continuous-integration/travis-ci/push",

This comment has been minimized.

@bradjc

bradjc Jul 11, 2018

Contributor

Should this be continuous-integration/travis-ci/pr? I don't think all PRs get /push.

This comment has been minimized.

@ppannuto

ppannuto Jul 11, 2018

Member

No. The way bors works is it creates a test branch it owns (using the commits from the PR, but otherwise wholly independent from the travis runs on the PR that github shows) and pushes that branch. Travis runs a job on the 'pushed' branch, which must pass.

The PR test is the test that runs when the PR is created, which is not the test we're interested in.

@bradjc bradjc dismissed their stale review Jul 11, 2018

This could be helpful, and shouldn't be hurtful.

@ppannuto ppannuto changed the title [RFC] add bors integration Add bors integration for merging PRs Jul 13, 2018

@ppannuto

This comment has been minimized.

Copy link
Member

ppannuto commented Jul 13, 2018

I think this should be ready to go. I added some documentation on how we plan to use bors based on the discussion from the call.

I think we broadly came to consensus on bors during the last call, so I've tagged this last-call, with a plan to merge around Sunday night / Monday morning unless someone objects.

bors: Test PRs for fail-fast behavior as well
From the libtock-c trial, we determined it's useful to have PRs tested
as well so that CI errors can be caught and handled early. Our test
suite is short and fast enough that this short not be a large burden.
@alevy

alevy approved these changes Jul 13, 2018

@ppannuto ppannuto merged commit 0079e68 into master Jul 16, 2018

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details

@ppannuto ppannuto deleted the bors branch Jul 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment