Skip to content
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

stop using bors #1304

Closed
StefanKarpinski opened this issue Aug 16, 2019 · 11 comments
Closed

stop using bors #1304

StefanKarpinski opened this issue Aug 16, 2019 · 11 comments
Labels

Comments

@StefanKarpinski
Copy link
Member

@StefanKarpinski StefanKarpinski commented Aug 16, 2019

I think that using bors on this repo was an interesting experiment, but I feel that it is a failed one. I find interacting with bors incredibly annoying and confusing and I think the benefits are negligible and far outweighed by the downsides. For example, @staticfloat and I both ended up giving up on trying to convince bors to run CI on various PRs and just yolo merged instead. The only things that bors is actually good for are:

  1. Merging a PR whenever it passes tests. There are various GitHub integrations that can do this without being as annoying to use as bors.

  2. Making sure that the build doesn't break when someone merges a PR and someone else wants to merge another PR that passes tests independently, doesn't have any merge conflicts, yet somehow manages to break the build. This has happened maybe two, three times in the entire history of Julia development on https://github.com/JuliaLang/julia. I doubt that Pkg is any more prone to this problem than Julia itself is. Maybe Rust code has this problem more and actually needs bors, but Julia code doesn't seem to—if two changes both pass tests and don't conflict, the chances of their merge breaking tests is vanishing.

So I propose that we stop using bors and use normal CI like Julia itself does.

@KristofferC

This comment has been minimized.

Copy link
Collaborator

@KristofferC KristofferC commented Aug 16, 2019

I find 1. really convenient so would be good if we could find a replacement for that.

I think Bors itself has worked pretty well (with the exception of no squashing functionality), it is just that:

  1. Our Windows CI runs serially for many Julia versions and the tests are slow so CI takes hours to run.
  2. We hit the network a lot and any connection error means the CI will fail.

So it feels a little bit that perhaps Bors is taking a bit of the blame here for the fact that our CI will be annoying no matter what system we use. In the beginning, when we didn't have that many tests it felt that bors worked quite smoothly which kind of corroborates the above statement.

For 1, #1297 should help. For 2, not sure what to do.

@StefanKarpinski

This comment has been minimized.

Copy link
Member Author

@StefanKarpinski StefanKarpinski commented Aug 16, 2019

Even just getting bors to rerun CI is a problem though, whereas without it, I can at least just restart individual CI runs. Bors also majorly increases the chances of accidentally merging without squashing.

@KristofferC

This comment has been minimized.

Copy link
Collaborator

@KristofferC KristofferC commented Aug 16, 2019

Yeah, the flakiness combined with that it is hard to restart individual CI runs (+ that CI takes a long time) is a good argument that causes the bors workflow to be kind of annoying.

@KristofferC

This comment has been minimized.

Copy link
Collaborator

@KristofferC KristofferC commented Aug 16, 2019

Maybe something like https://github.com/apps/mergewhenready to merge when CI passes.

@StefanKarpinski

This comment has been minimized.

Copy link
Member Author

@StefanKarpinski StefanKarpinski commented Aug 16, 2019

Yeah, I've been looking at a few options. That's one, here are some others:

I haven't found any that let you easily choose whether to squash or merge a given PR though, which seems like pretty essential functionality to me. I would be ok with using labels to control which one should be done on merge, but that doesn't seem to be a thing.

@DilumAluthge

This comment has been minimized.

Copy link
Contributor

@DilumAluthge DilumAluthge commented Aug 16, 2019

Bors also majorly increases the chances of accidentally merging without squashing.

Bors will have automatic squash-merging relatively soon. See the RFC here and the pull request here.

Of course, that doesn’t solve the other problems that Stefan listed.

@DilumAluthge

This comment has been minimized.

Copy link
Contributor

@DilumAluthge DilumAluthge commented Aug 16, 2019

Here’s another option for automatic merging:

The README for bulldozer says that you can select “squash” as your merge method, but that setting seems to be per-branch, not per-PR.

@DilumAluthge

This comment has been minimized.

Copy link
Contributor

@DilumAluthge DilumAluthge commented Aug 16, 2019

Another option would be to develop one of those newfangled “GitHub Actions” to automatically squash-merge on Pkg.jl.

Probably easier to use a prebuilt solution though.

@notriddle

This comment has been minimized.

Copy link

@notriddle notriddle commented Aug 19, 2019

One thing that has helped me with nasty CI setups (WebDriver, in my case) is to wrap the test suite in a bounded retry-loop. If your tests are flaky, that's probably a good idea no matter what you do with bors.

@tkf

This comment has been minimized.

Copy link
Contributor

@tkf tkf commented Jan 5, 2020

I haven't found any that let you easily choose whether to squash or merge a given PR though, which seems like pretty essential functionality to me.

@StefanKarpinski FYI, it looks like you can do this in mergify with some configuration. Here is an example .mergify.yml:

pull_request_rules:
  - name: automatic squash-merge when CI passes
    conditions:
      - base=master
      - status-success=continuous-integration/travis-ci/pr
      - label=ready-to-merge:squash
    actions:
      merge:
        method: squash
  - name: automatic merge when CI passes
    conditions:
      - base=master
      - status-success=continuous-integration/travis-ci/pr
      - label=ready-to-merge:merge
    actions:
      merge:
        method: merge

and usages:

tkf/MyPlayground.jl#10
tkf/MyPlayground.jl#11

@StefanKarpinski

This comment has been minimized.

Copy link
Member Author

@StefanKarpinski StefanKarpinski commented Jan 5, 2020

Cool, thanks for the info, @tkf.

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

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.