llvm dev - Jul 2016 - [RFC] One or many git repositories?

On 25 Jul 2016, at 11:19, Daniel Sanders via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> > > On Fri, Jul 22, 2016 at 3:40 PM, Daniel Sanders via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
> > > Do you have an example in mind? I'd expect them to rely on
each 'master' being
> > > an improvement on 'master^'. I wouldn't expect them
to be interested in how
> > > 'master^' became 'master'.
> > 
> > I would love it if each master commit was an improvement on the
previous commit, or at last
> > was virtually guaranteed to be not broken. It's most annoying that
the existing LLVM history
> > has a lot of examples of commits being reversed by a later commit.
> > 
> > The ease in git of branching -- and more importantly rebasing the
branch on a later state of master
> > -- means that you can run buildbots for all the different platforms on
each pull request BEFORE
> > merging it to master.
>  
> I'd like to see this too. I'd prefer not to make it a requirement
for _all_ buildbots to pass before commit though. I'm thinking that we could
make passing 'fast' buildbots mandatory before commit but leave
'slow' buildbots in the current post-commit testing scheme. That way
targets that can provide fast buildbots will win additional stability guarantees
but the community isn't held back by the slowest buildbot. We'd have to
discuss where we'd draw the line between 'fast' and 'slow'.
How difficult would it be with GitHub to set up something to automatically merge
pull requests if they keep the buildbots happy?  The workflow would then be to
send a pull request annotated by a committer as ready to merge (i.e. reviewed /
due for post-commit review).  If it passed, then no more human interaction would
be required - once the buildbots had finished testing it, it would be the new
master (effectively, we’d merge to a speculative-master and then perform a
simple fast-forward push from there to the real master if the tests passed).  If
they failed, then drop it from the queue and add a note on the pull request
indicating what failed.

The workflow would be exactly the same for committers and non-committers, except
that committers would be able to tag their own pull requests as ready to merge.

David

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 3719 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20160725/08834bed/attachment.bin>

On 25 July 2016 at 11:59, David Chisnall via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
> How difficult would it be with GitHub to set up something to automatically
merge pull requests if they keep the buildbots happy?
It's not just about passing the tests...

Currently, post-commit review committers have the ability to commit in
areas where they are comfortable with or they own, not the entire
project. The whole area is very fuzzy, and there's no automated way of
doing that at the moment.

A pull request is a request for *comment*, and we currently express
that via a Phab review. If I'm comfortable with the patch, I commit
directly, if I want others opinions, I create a review. If we move as
you propose, I'd lose the second option.

I also don't think pre-commit review committers should be just relying
on the fast bots passing. There are many other issues like code
quality, formatting, tests, etc. that need to be done properly.

If we go this route, we'll be effectively flattening out everyone that
has commit access into one big pool, and that would be a massive
change in how our community operates (for better or worse).

Unless you're also proposing that only the "core team" (code
owners,
high committers, etc) get commit access in the new style.

> The workflow would be exactly the same for committers and non-committers,
except that committers would be able to tag their own pull requests as ready to
merge.
Can this be controlled via a server hook? One that would be supported by GitHub?

cheers,
--renato

On 25 Jul 2016, at 12:09, Renato Golin <renato.golin at linaro.org>
wrote:
> 
> A pull request is a request for *comment*, and we currently express
> that via a Phab review. If I'm comfortable with the patch, I commit
> directly, if I want others opinions, I create a review. If we move as
> you propose, I'd lose the second option.
A pull request is a mechanism.  The community can decide what the policy is.
> I also don't think pre-commit review committers should be just relying
> on the fast bots passing. There are many other issues like code
> quality, formatting, tests, etc. that need to be done properly.
I agree, but it’s much harder to automate that (though having clang-format
automatically applied might be nice).
> If we go this route, we'll be effectively flattening out everyone that
> has commit access into one big pool, and that would be a massive
> change in how our community operates (for better or worse).
That’s not what I’m suggesting.  Pull requests provide a convenient staging
mechanism.  On the back end, they’re simply git branches in a namespace that
won’t be automatically fetched by clients that do a clone.
> Unless you're also proposing that only the "core team" (code
owners,
> high committers, etc) get commit access in the new style.
I’d suggest that no one should have direct push access (or, at least, the no one
should *use* the direct access - presumably some people in what GitHub calls the
admin groups would retain access for dealing with problems).
>> The workflow would be exactly the same for committers and
non-committers, except that committers would be able to tag their own pull
requests as ready to merge.
> 
> Can this be controlled via a server hook? One that would be supported by
GitHub?
GitHub supports everything that is needed to implement this suggestion.  From
the developers’ perspective, the workflow is:

- User sends pull request.

- User from a list of people that correspond to our current set of committers
labels the request as ‘approved’ (GitHub labels can only be applied by members
of the organisation).

In the case of committers who are committing to areas where they are comfortable
with post-commit review, these two users can be the same person, so you’d send a
pull request and add the tag yourself.

The buildbot master then:

- Collects all of the pull requests (git fetch)

- Uses the GitHub API to filter the ones that have been approved

- Merges these, in turn, to its local master branch

- Sends those changes to the builders

- If they succeed, pushes master to GitHub

In idle time (when not busy with approved patches), it should also try other
pull requests and add a label saying that they’ve passed the tests.

There are some obvious refinements to this, for example only kicking the fast
buildbots when there are several patches outstanding and then running the slower
ones with the combined master, then backtracking and applying them one at a time
if this fails.

Longer term, it would be nice to integrate things like running the static
analyser into this workflow.

I don’t have particularly strong feelings about this, I just wanted to point out
that it’s possible with GitHub and a little bit of client code talking to the
GitHub APIs.  There’s a slightly more complex workflow for people to push code,
but in exchange for that we’d never be in a state where anything pushed to
master introduces regressions for which tests exist.

David

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 3719 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20160725/dc741896/attachment.bin>