Re: [lldb-dev] [cfe-dev] [llvm-dev] [Github] RFC: linear history vs merge commits
I'm also in favor of linear history, option #1. FWIW I don't think lacking tight controls to prevent merges is going to be a huge deal. We already restrict who can commit, and there are lots of other rules you have to follow. We might get an accidental merge or two every once in a while, but I expect we'll all be able to live with that? On Wed, Jan 30, 2019 at 10:53 PM Mehdi AMINI via cfe-dev < cfe-...@lists.llvm.org> wrote: > > > On Wed, Jan 30, 2019 at 1:19 PM Eric Christopher via cfe-dev < > cfe-...@lists.llvm.org> wrote: > >> On Wed, Jan 30, 2019 at 12:42 PM David Greene via cfe-dev >> wrote: >> > >> > Bruce Hoult via llvm-dev writes: >> > >> > > How about: >> > > >> > > Require a rebase, followed by git merge --no-ff >> > > >> > > This creates a linear history, but with extra null merge commits >> > > delimiting each related series of patches. >> > > >> > > I believe it is compatible with bisect. >> > > >> > > https://linuxhint.com/git_merge_noff_option/ >> > >> > We've done both and I personally prefer the strict linear history by a >> > lot. It's just much easier to understand a linear history. >> > >> >> Agreed. Let's go with option #1. >> >> > What is the practical plan to enforce the lack of merges? When we looked > into this GitHub would not support this unless also forcing every change to > go through a pull request (i.e. no pre-receive hooks on direct push to > master were possible). Did this change? Are we hoping to get support from > GitHub on this? > > We may write this rule in the developer guide, but I fear it'll be hard to > enforce in practice. > > -- > Mehdi > > ___ > cfe-dev mailing list > cfe-...@lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev > ___ lldb-dev mailing list lldb-dev@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
Re: [lldb-dev] [cfe-dev] [llvm-dev] [Github] RFC: linear history vs merge commits
On 1 Feb 2019, at 22:48, Peter Wu via cfe-dev wrote: > > On caveat is that the test coverage would be limited or else the build > times would be very long. There are quite some builders for various > projects (llvm, cfe, compiler-rt, etc.) on a lot of different platforms > and targets (Linux, macOS, Windows, Android, etc.). > > With limited resources, Arthur's suggestion seems workable to me: > - Enable pre-commit testing of few configurations (in parallel). > - Encourage developers to wait for tests to pass before pushing changes. > - Do not block hard to avoid a situation where unrelated changes > (commits or build environment) cause failures. GitHub does let you block merges (by anyone who isn’t an administrator) of PRs that don’t meet certain requirements. For one project, we have it set up so that you need to have at least one reviewer saying approved, you have to have signed the CLA, and you have to pass all of the CI checks. It’s then easy to set up automatic merging when all of the prerequisites are met (you can get a notification and process it automatically). We allow self approval for small changes (not automatically enforced, this is a judgement call, but you can remove people who abuse it from the team quite easily and then they can’t approve changes). I’ve found it leads to a very nice workflow: developers aren’t in the loop for merges, they just prepare something that they think works, submit it, and get on with something else. If you’re lucky, someone approves it and you pass CI first go, then everything is fine. Reviewers can wait until CI is finished and not bother looking for things that are going to break. If the change introduces new tests, reviewers know that those tests are passing. If you broke a platform that you don’t test on locally, you get notified but no one else is spammed with breakage reports. Once you’ve fixed it, you get a review, and make any changes. The master branch is always buildable and passing CI. If your changes break CI, you don’t have a race to fix things before someone reverts your change, you just fix things in the PR and wait for it to be automatically merged. David ___ lldb-dev mailing list lldb-dev@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
Re: [lldb-dev] [cfe-dev] [llvm-dev] [Github] RFC: linear history vs merge commits
> -Original Message-
> From: cfe-dev [mailto:cfe-dev-boun...@lists.llvm.org] On Behalf Of Peter
> Wu via cfe-dev
> Sent: Friday, February 01, 2019 5:49 PM
> To: Arthur O'Dwyer
> Cc: llvm-...@lists.llvm.org; cfe-...@lists.llvm.org; openmp-
> d...@lists.llvm.org; lldb-dev@lists.llvm.org
> Subject: Re: [cfe-dev] [llvm-dev] [Github] RFC: linear history vs merge
> commits
>
> On Fri, Feb 01, 2019 at 12:41:05PM -0500, Arthur O'Dwyer via cfe-dev
> wrote:
> > I know GitHub can be configured to reject force-pushes to {any, a
> > specific} branch. I strongly suspect that if *the maintainers of
> > LLVM* asked nicely, GitHub would also be able to reject
> > merges to {any, a specific} branch.
>
> Anyone with admin permissions in a repo can add "branch protection
> rules" that prevent force pushes, there is no need to contact Github
> support for that.
>
> > - GitHub PRs have Travis integration so the reviewer can see if a
> > particular patch is actually compileable at all, before spending time
> > looking at it. I wish Phab had this feature (or maybe it exists and LLVM
> > doesn't use it?).
>
> This kind of pre-merge testing is very valuable, it could potentially
> prevent some unnecessary reverts by catching issues early.
>
> On caveat is that the test coverage would be limited or else the build
> times would be very long. There are quite some builders for various
> projects (llvm, cfe, compiler-rt, etc.) on a lot of different platforms
> and targets (Linux, macOS, Windows, Android, etc.).
Some platforms/projects are more prone to breakage than others. This
has been a particular pain point for my team lately, our auto-merge
hasn't been very auto for quite a while now due to frequent build breaks.
>
> With limited resources, Arthur's suggestion seems workable to me:
> - Enable pre-commit testing of few configurations (in parallel).
So, I've seen that Phabricator has something in the build-it-for-you
vein, which somebody explained not too long ago but I've forgotten the
details and a quick look doesn't turn up anything in my archive.
If that can be made to work generally, and especially if it could run
one each of Linux, Windows, and Mac, that would go a *long* way toward
addressing build breaks (for patches that go through Phab, anyway).
--paulr
> - Encourage developers to wait for tests to pass before pushing changes.
> - Do not block hard to avoid a situation where unrelated changes
> (commits or build environment) cause failures.
>
> I would also be in favor of linear history for reasons mentioned before
> (easier bisection, more readable logs). Though whatever choice happens
> (cherry-pick vs merge), I think it is important to keep the link between
> a change and the review. I have seen various strategies:
>
> - Phabricator: manually include a "Differential revision" link in the
> commit message.
> - Github (merge via web interface): automatically includes a reference
> to the "Pull Request".
> - Github (cherry-pick / rebase and merge with no merge commit):
> unfortunately loses the review information.
> - Gerrit: developers cannot merge directly, instead they use the web
> interface to submit a change. This will add a "Reviewed-on" link that
> references the review. (Used by Wireshark, Qt, boringssl, etc.)
>
> Projects that are use mailing lists to review patches (like Linux and
> QEMU) commonly include a Message-Id tag in the commit message that
> references the original mailing list discussion.
>
> The curl project also uses Github for reviews, but encourages developers
> with push permissions to manually fetch the commit, edit the commit
> message to reference the review and then push to the master branch (with
> no merge commits). Again, this is a manual process and new developers
> who are not familiar with this process accidentally create a merge
> commit anyway (or forget to reference the review).
>
> Ideally changes go through a single gatekeeper, a tool that recognizes
> comments to a merge request and subsequently tries to add extra info to
> a commit message (link to a review) and enforce a linear history (by
> cherry-picking changes or rebase + merge commits). This tool could be
> triggered by posting comments like "@name-of-the-bot merge this".
> (If necessary this could be combined with requiring tests to pass.)
>
> Such an extra indirection with a single gatekeeper could be a quite
> drastic change from the current development model though. It could
> reduce the number of broken builds and improve the quality of commit
> messages though.
>
> Just my two cents, hope it helps :)
> --
> Kind regards,
> Peter Wu
> https://lekensteyn.nl
> ___
> cfe-dev mailing list
> cfe-...@lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
___
lldb-dev mailing list
lldb-dev@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
Re: [lldb-dev] [cfe-dev] [llvm-dev] [Github] RFC: linear history vs merge commits
Oh, I'm completely in favor of making bad commits much less likely. I simply think there is a decent solution between "let everything in" and "don't let everything in unless its proven to work everywhere" that gets 90% of the improvement. The complexity of guaranteeing a buildable branch is high. If someone wants to take that on, great! I just don't think we should reasonably expect a group of volunteers to do it. :) -David Jeremy Lakeman writes: > I realise that llvm trunk can move fairly quickly. > So my original, but brief, suggestion was to merge the current set of > approved patches together rather than attempting them one at a time. > Build on a set of fast smoke test bots. If something breaks, it should > be possible to bisect it to reject a PR and make forward progress. > Occasionally bisecting a large set of PR's should still be less bot > time than attempting to build each of them individually. > Blocking the PR's due to target specific and or slow bot failures > would be a different question. > You could probably do this with a linear history, so long as the final > tree is the same as the merge commit, it should still build. > I'm just fond of the idea of trying to prevent bad commits from ever > being merged. Since they sometimes waste everyone's time. > > On Fri, 1 Feb 2019 at 04:02, David Greene wrote: > > writes: > > > Systems that I've seen will funnel all submitted PRs into a > single queue > > which *does* guarantee that the trial builds are against HEAD > and there > > are no "later commits" that can screw it up. If the trial build > passes, > > the PR goes in and becomes the new HEAD. > > The downside of a system like this is that when changes are going > in > rapidly, the queue grows very large and it takes forever to get > your > change merged. PR builds are serialized and if a "build" means > "make > sure it builds on all the Buildbots" then it takes a very long > time > indeed. > > There are ways to parallelize builds by speculating that PRs will > build > cleanly but it gets pretty complicated quickly. > > > But this would be a radical redesign of the LLVM bot system, and > would > > have to wait until we're done with the GitHub migration and can > spend > > a couple of years debating the use of PRs. :-) > > Heh. Fully guaranteeing buildability of a branch is not a trivial > task > and will take a LOT of thought and discussion. > > -David ___ lldb-dev mailing list lldb-dev@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
Re: [lldb-dev] [cfe-dev] [llvm-dev] [Github] RFC: linear history vs merge commits
> -Original Message- > From: cfe-dev [mailto:cfe-dev-boun...@lists.llvm.org] On Behalf Of David > Greene via cfe-dev > Sent: Thursday, January 31, 2019 4:56 PM > To: Roman Lebedev > Cc: llvm-dev; cfe-dev; openmp-dev (openmp-...@lists.llvm.org); LLDB Dev > Subject: Re: [cfe-dev] [llvm-dev] [Github] RFC: linear history vs merge > commits > > Roman Lebedev writes: > > > *Does* LLVM want to switch from phabricator to github pr's? > > I personally don't recall previous discussions. > > Personally, i hope not, i hope phabricator should/will stay. > > I find Phab pretty unintuitive. I just started using it in earnest > about four months ago, so that's one datapoint from people new to it. Heh. Years ago I described it as "actively user-hostile." Merely being unintuitive is a huge step forward. I don't know how much of that is the local LLVM instance being massaged toward usefulness as opposed to improvements in the underlying product. --paulr ___ lldb-dev mailing list lldb-dev@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
Re: [lldb-dev] [cfe-dev] [llvm-dev] [Github] RFC: linear history vs merge commits
I realise that llvm trunk can move fairly quickly. So my original, but brief, suggestion was to merge the current set of approved patches together rather than attempting them one at a time. Build on a set of fast smoke test bots. If something breaks, it should be possible to bisect it to reject a PR and make forward progress. Occasionally bisecting a large set of PR's should still be less bot time than attempting to build each of them individually. Blocking the PR's due to target specific and or slow bot failures would be a different question. You could probably do this with a linear history, so long as the final tree is the same as the merge commit, it should still build. I'm just fond of the idea of trying to prevent bad commits from ever being merged. Since they sometimes waste everyone's time. On Fri, 1 Feb 2019 at 04:02, David Greene wrote: > writes: > > > Systems that I've seen will funnel all submitted PRs into a single queue > > which *does* guarantee that the trial builds are against HEAD and there > > are no "later commits" that can screw it up. If the trial build passes, > > the PR goes in and becomes the new HEAD. > > The downside of a system like this is that when changes are going in > rapidly, the queue grows very large and it takes forever to get your > change merged. PR builds are serialized and if a "build" means "make > sure it builds on all the Buildbots" then it takes a very long time > indeed. > > There are ways to parallelize builds by speculating that PRs will build > cleanly but it gets pretty complicated quickly. > > > But this would be a radical redesign of the LLVM bot system, and would > > have to wait until we're done with the GitHub migration and can spend > > a couple of years debating the use of PRs. :-) > > Heh. Fully guaranteeing buildability of a branch is not a trivial task > and will take a LOT of thought and discussion. > > -David > ___ lldb-dev mailing list lldb-dev@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
Re: [lldb-dev] [cfe-dev] [llvm-dev] [Github] RFC: linear history vs merge commits
Roman Lebedev writes: > *Does* LLVM want to switch from phabricator to github pr's? > I personally don't recall previous discussions. > Personally, i hope not, i hope phabricator should/will stay. I find Phab pretty unintuitive. I just started using it in earnest about four months ago, so that's one datapoint from people new to it. -David ___ lldb-dev mailing list lldb-dev@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
Re: [lldb-dev] [cfe-dev] [llvm-dev] [Github] RFC: linear history vs merge commits
writes: > Systems that I've seen will funnel all submitted PRs into a single queue > which *does* guarantee that the trial builds are against HEAD and there > are no "later commits" that can screw it up. If the trial build passes, > the PR goes in and becomes the new HEAD. The downside of a system like this is that when changes are going in rapidly, the queue grows very large and it takes forever to get your change merged. PR builds are serialized and if a "build" means "make sure it builds on all the Buildbots" then it takes a very long time indeed. There are ways to parallelize builds by speculating that PRs will build cleanly but it gets pretty complicated quickly. > But this would be a radical redesign of the LLVM bot system, and would > have to wait until we're done with the GitHub migration and can spend > a couple of years debating the use of PRs. :-) Heh. Fully guaranteeing buildability of a branch is not a trivial task and will take a LOT of thought and discussion. -David ___ lldb-dev mailing list lldb-dev@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
Re: [lldb-dev] [cfe-dev] [llvm-dev] [Github] RFC: linear history vs merge commits
Mehdi AMINI writes: > What is the practical plan to enforce the lack of merges? When we > looked into this GitHub would not support this unless also forcing > every change to go through a pull request (i.e. no pre-receive hooks > on direct push to master were possible). Did this change? Are we > hoping to get support from GitHub on this? > > We may write this rule in the developer guide, but I fear it'll be > hard to enforce in practice. Again, changes aren't going through git yet, right? Not until SVN is decommissioned late this year (or early next). SVN requires a strict linear history so it handles the enforcement for now. My personal opinion is that when SVN is decomissioned we should use pull requests, simply because that's what's familiar to the very large development community outside LLVM. It will lower the bar to entry for new contributors. This has all sorts of implications we need to discuss of course, but we have some time to do that. If we don't use PRs, then we're pretty much on the honor system to disallow merges AFAIK. -David ___ lldb-dev mailing list lldb-dev@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
Re: [lldb-dev] [cfe-dev] [llvm-dev] [Github] RFC: linear history vs merge commits
> -Original Message- > From: cfe-dev [mailto:cfe-dev-boun...@lists.llvm.org] On Behalf Of David > Greene via cfe-dev > Sent: Wednesday, January 30, 2019 3:52 PM > To: Jeremy Lakeman > Cc: llvm-dev; LLDB Dev; cfe-dev; openmp-dev (openmp-...@lists.llvm.org) > Subject: Re: [cfe-dev] [llvm-dev] [Github] RFC: linear history vs merge > commits > > Jeremy Lakeman via llvm-dev writes: > > > 4. Each reviewed bug / feature must be rebased onto the current "known > > good" commit, merged into a "probably good" commit, tested by build > > bots, and only then pushed to trunk. Keeping trunk's history more > > usable, with most bad patches reworked and resubmitted instead of > > reverted. > > If you mean having a submitted PR trigger builds and only allow merging > if all builds pass, that may be doable. Of course by the time it's > merged it will be against some later commit (so it should be rebased) > and there's no guarantee it will build against *that* commit. But it > will tend to filter out most problems. Systems that I've seen will funnel all submitted PRs into a single queue which *does* guarantee that the trial builds are against HEAD and there are no "later commits" that can screw it up. If the trial build passes, the PR goes in and becomes the new HEAD. By the time a PR reaches the front of the build queue, it might not apply cleanly, in which case it gets bounced just like a failed build would. But this would be a radical redesign of the LLVM bot system, and would have to wait until we're done with the GitHub migration and can spend a couple of years debating the use of PRs. :-) --paulr ___ lldb-dev mailing list lldb-dev@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
Re: [lldb-dev] [cfe-dev] [llvm-dev] [Github] RFC: linear history vs merge commits
On Wed, Jan 30, 2019 at 12:42 PM David Greene via cfe-dev wrote: > > Bruce Hoult via llvm-dev writes: > > > How about: > > > > Require a rebase, followed by git merge --no-ff > > > > This creates a linear history, but with extra null merge commits > > delimiting each related series of patches. > > > > I believe it is compatible with bisect. > > > > https://linuxhint.com/git_merge_noff_option/ > > We've done both and I personally prefer the strict linear history by a > lot. It's just much easier to understand a linear history. > Agreed. Let's go with option #1. -eric ___ lldb-dev mailing list lldb-dev@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
