For the most part, I think committers need to build every PR they 
review, for a couple of reasons:

1) Smoke testing and edge case testing is part of the review process. If 
you don't build it, you obviously aren't running it and making sure it 
works. Code reviews that only involve reading code tend to drift toward 
mere enforcement of style standards, which is necessary, but not 
sufficient. We adopted code reviews because there was a time when a lot 
of unfinished, untested work was getting committed.

2) We have a lot of commits going in, and most builds have multiple 
commits. If a build breaks, everyone who committed in that build needs 
to stop their work and check the build. And everyone unlucky enough to 
pull from master during the broken build time will have their work 
interrupted. That amounts to more people and more time lost than if the 
committer runs a build.

Now, there are occasional exceptions -- I just merged a pull from Zach 
that I didn't build. But it was a 2-line change that just introduced the 
use of a constant in place of a numeric literal. Not the kind of thing 
that really needed a second compile. (Of course, if the constant was 
misspelled, I'll have egg on my face in a little while... but then, so 
will Zach, for submitted an uncompiled change!)

One thing that would save a lot of my time is if I had a second box to 
do builds on. I should look into getting an EC2 instance to do builds 
on. I think my usage might fit into the free usage tier.

-chris


On 6/22/12 9:39 AM, Lance Speelmon wrote:
> Is it correct in assuming we have CI builds for every commit in projects?  If 
> so, could I suggest that people's time is too valuable to spend building 
> every PR manually?
>
> WDYT?  Thanks, L
> _______________________________________________
> oae-dev mailing list
> [email protected]
> http://collab.sakaiproject.org/mailman/listinfo/oae-dev
_______________________________________________
oae-dev mailing list
[email protected]
http://collab.sakaiproject.org/mailman/listinfo/oae-dev

Reply via email to