www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - What's up with pull request buildbots?

reply "Dylan Knutson" <tcdknutson gmail.com> writes:
Hello,
I'm a bit confused as to how the DMD buildbot is supposed to 
work: it seems like 50% of the time (ballparked from the first 15 
or so pull requests), the buildbot just doesn't report failures 
or successes. This bugs (heh) me a little bit because there are 
tons of months old pull requests just waiting in the pipeline, 
stuck at "Determining merge status". I don't know if this is part 
of the review process or caused by the yellow status, but for 
instance I've opened up a bug report a week or so prior:
http://d.puremagic.com/issues/show_bug.cgi?id=10113
and a few days afterwards, a pull request was submitted:
https://github.com/D-Programming-Language/dmd/pull/2080
which, turns out, was more or less a dup of another pull request, 
submitted 6 months ago:
https://github.com/D-Programming-Language/dmd/pull/1358

However, neither of these pull requests have been merged. And 
neither of the bugs have been fixed (my report was a dup of 
http://d.puremagic.com/issues/show_bug.cgi?id=2950, submitted 4 
years ago!).

So we're stuck with a buildbot system that leaves valuable pull 
requests from ever getting merged, and duplicate fixes being 
written because previous fixes were submitted and never accepted 
so long ago.

On a somewhat related note: Why is the Puremagic bugtracker still 
used, when Github has bug tracking with (IMO) better repo 
integration, discussion, and searching (which I think we can all 
agree on), on a repository *hosted at Github*? I can understand 
the reluctance to switch over, but what's keeping us from 
accepting new issues on Github, closing Puremagic down from 
accepting new requests, and working through the open requests on 
Puremagic until all can be taken care of have been?

Thanks for your time,
Dylan Knutson
Jun 02 2013
next sibling parent reply "Adam Wilson" <flyboynw gmail.com> writes:
On Sun, 02 Jun 2013 22:27:12 -0700, Dylan Knutson <tcdknutson gmail.com>  
wrote:

 Hello,
 I'm a bit confused as to how the DMD buildbot is supposed to work: it  
 seems like 50% of the time (ballparked from the first 15 or so pull  
 requests), the buildbot just doesn't report failures or successes. This  
 bugs (heh) me a little bit because there are tons of months old pull  
 requests just waiting in the pipeline, stuck at "Determining merge  
 status". I don't know if this is part of the review process or caused by  
 the yellow status, but for instance I've opened up a bug report a week  
 or so prior:
 http://d.puremagic.com/issues/show_bug.cgi?id=10113
 and a few days afterwards, a pull request was submitted:
 https://github.com/D-Programming-Language/dmd/pull/2080
 which, turns out, was more or less a dup of another pull request,  
 submitted 6 months ago:
 https://github.com/D-Programming-Language/dmd/pull/1358

I'll see if I can't give you a stab at an explanation. Note that I did not write the Auto-Tester, that is the work of Brad Anderson. Here are some facts about the AT as I understand them: - The AT works on a most current pull first basis, so a pull that is older than 6 months will naturally be scheduled much later. - The AT schedules work based on how many testing machines are available, so each pull is tested only once per listed platform, and when multiple build servers for a given platform are available, different pulls can be tested simultaneously on that platform. - Pull requests are tested by cloning [DMD/DRuntime/Phobos] master and merging the pull into it. This ensures a clean pull against master. - The AT must restart pull request testing every time code is merged into DMD/DRuntime/Phobos because of the above testing strategy. - If many pulls are merged into [DMD/DRuntime/Phobos] master in a given period of, for example a day, the AT will restart prior to completing a full test pass. - With the current number of open pulls the AT can complete a full testing pass in roughly 8 hours. - Each pull takes a rough average of 15 minutes to test. The lack of success/failure reports on older pulls is directly due to the above testing strategy, essentially the AT never made it that far before someone merged a pull into [DMD/DRuntime/Phobos] master and forced a restart.
 However, neither of these pull requests have been merged. And neither of  
 the bugs have been fixed (my report was a dup of  
 http://d.puremagic.com/issues/show_bug.cgi?id=2950, submitted 4 years  
 ago!).

 So we're stuck with a buildbot system that leaves valuable pull requests  
 from ever getting merged, and duplicate fixes being written because  
 previous fixes were submitted and never accepted so long ago.

The AT system is intentionally incapable of merging to master, so it itself is not leaving pulls behind. However if a pull is out-of-date it is likely that it will fail the AT anyways due the codebase delta in the target project. This is why pulls are tested in a newest first order.
 On a somewhat related note: Why is the Puremagic bugtracker still used,  
 when Github has bug tracking with (IMO) better repo integration,  
 discussion, and searching (which I think we can all agree on), on a  
 repository *hosted at Github*? I can understand the reluctance to switch  
 over, but what's keeping us from accepting new issues on Github, closing  
 Puremagic down from accepting new requests, and working through the open  
 requests on Puremagic until all can be taken care of have been?

According to the Bug Stats page (http://dlang.org/bugstats.php) there are nearly 3000 open issues in the BugZilla and over 7000 closed issues! Now I tend to agree that better systems than BugZilla exist. But consider the effort required to move all of those bugs to a new system, automated tooling or no, it wouldn't be easy or quick. And consider that GitHub issues lacks much of meta information capability that BugZilla has, we would loose a fantastic amount of information. Yes of course GitHub has the best integration with it's own issue tracker, but we've achieved a fairly high level of integration between GitHub and BugZilla, so that has become almost a non-issue these days.
 Thanks for your time,
 Dylan Knutson

-- Adam Wilson IRC: LightBender Project Coordinator The Horizon Project http://www.thehorizonproject.org/
Jun 02 2013
next sibling parent Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
On 6/4/13 3:52 AM, Robert wrote:
 On Mon, 2013-06-03 at 16:39 +0200, Dylan Knutson wrote:
 On a side note, there are a few (lots) of pull requests that are
 several months old, and master has changed too much for them to
 be compatible and/or relevant. Perhaps those should just be
 closed?

I have an open pull request which is already months old and no longer compatible to master. This is because I have not received any feedback since submission.

Apologies. What's the number? I'll look into it as soon as I can. Andrei
Jun 04 2013
prev sibling parent Walter Bright <newshound2 digitalmars.com> writes:
On 6/2/2013 10:49 PM, Adam Wilson wrote:
 Note that I did not write
 the Auto-Tester, that is the work of Brad Anderson.

Brad Roberts
Jun 04 2013
prev sibling next sibling parent Jonathan M Davis <jmdavisProg gmx.com> writes:
On Sunday, June 02, 2013 22:49:08 Adam Wilson wrote:
 I'll see if I can't give you a stab at an explanation. Note that I did not
 write the Auto-Tester, that is the work of Brad Anderson.

Brad Roberts actually, with some help from Daniel Murphy (who created the pull tester), and I don't know who else. - Jonathan M Davis
Jun 02 2013
prev sibling next sibling parent "Brad Anderson" <eco gnuk.net> writes:
On Monday, 3 June 2013 at 05:49:22 UTC, Adam Wilson wrote:
 On Sun, 02 Jun 2013 22:27:12 -0700, Dylan Knutson 
 <tcdknutson gmail.com> wrote:

 Hello,
 I'm a bit confused as to how the DMD buildbot is supposed to 
 work: it seems like 50% of the time (ballparked from the first 
 15 or so pull requests), the buildbot just doesn't report 
 failures or successes. This bugs (heh) me a little bit because 
 there are tons of months old pull requests just waiting in the 
 pipeline, stuck at "Determining merge status". I don't know if 
 this is part of the review process or caused by the yellow 
 status, but for instance I've opened up a bug report a week or 
 so prior:
 http://d.puremagic.com/issues/show_bug.cgi?id=10113
 and a few days afterwards, a pull request was submitted:
 https://github.com/D-Programming-Language/dmd/pull/2080
 which, turns out, was more or less a dup of another pull 
 request, submitted 6 months ago:
 https://github.com/D-Programming-Language/dmd/pull/1358

I'll see if I can't give you a stab at an explanation. Note that I did not write the Auto-Tester, that is the work of Brad Anderson. Here are some facts about the AT as I understand them:

I wish I could take credit for that. It's the work of the awesome Brad Roberts though.
Jun 02 2013
prev sibling next sibling parent "Dylan Knutson" <tcdknutson gmail.com> writes:
 - If many pulls are merged into [DMD/DRuntime/Phobos] master in 
 a given period of, for example a day, the AT will restart prior 
 to completing a full test pass.
 - With the current number of open pulls the AT can complete a 
 full testing pass in roughly 8 hours.
 - Each pull takes a rough average of 15 minutes to test.

Thanks for the explanation. I suppose the fix for my issue would be for reviewers to merge the small bugfixes a bit faster. I don't want to sound like I'm rushing the reviewers; as it is the project has amazing community contribution, and I'm constantly blown away at how much a group of volunteers can accomplish. On a side note, there are a few (lots) of pull requests that are several months old, and master has changed too much for them to be compatible and/or relevant. Perhaps those should just be closed?
 But consider the  effort required to move all of those bugs to 
 a new system, automated  tooling or no, it wouldn't be easy or 
 quick.

Sorry; let me clarify: I'd suggest that BZ stop accepting new issues, and use the GHI tracker for new bugs. Then, just use BZ as needed until all resolvable issues there have been resolved. As for loss of meta information on Github, eh, I suppose so, but GHI offers issue referencing and tagging, so that's something I guess? I don't expect to convince anyone to switch, just see why it isn't used in the first place. Thank you, Dylan Knutson
Jun 03 2013
prev sibling next sibling parent Robert <jfanatiker gmx.at> writes:
On Mon, 2013-06-03 at 16:39 +0200, Dylan Knutson wrote:
 On a side note, there are a few (lots) of pull requests that are 
 several months old, and master has changed too much for them to 
 be compatible and/or relevant. Perhaps those should just be 
 closed?

I have an open pull request which is already months old and no longer compatible to master. This is because I have not received any feedback since submission. It does not mean that it is irrelevant or that I am not willing to make it compatible again as soon as any reviewer has time for a review. So it has to be there, if not for anything else than just for for the sake of being there and being seen: - So that no-one else implements the same thing again for no reason - Maintainers know that there is something they need to take care of at some point. Pull requests should be closed by maintainers if the pull request won't get accepted even if improved quality wise or if the submitter is no longer available for making it ready, but not just because they are old. Best regards, Robert
Jun 04 2013
prev sibling next sibling parent Robert <jfanatiker gmx.at> writes:
 
 Apologies. What's the number? I'll look into it as soon as I can.
 
 Andrei
 

I will fix it according to changes in master ASAP. Best regards, Robert
Jun 04 2013
prev sibling next sibling parent Robert <jfanatiker gmx.at> writes:
 Apologies. What's the number? I'll look into it as soon as I can.
 
 Andrei

Wow, I have not anticipated this kind of reaction. Thanks a lot! It can be found here: https://github.com/D-Programming-Language/dmd/pull/1839 Best regards, Robert
Jun 04 2013
prev sibling next sibling parent Brad Roberts <braddr puremagic.com> writes:
On 6/4/13 12:52 AM, Robert wrote:
 On Mon, 2013-06-03 at 16:39 +0200, Dylan Knutson wrote:
 On a side note, there are a few (lots) of pull requests that are
 several months old, and master has changed too much for them to
 be compatible and/or relevant. Perhaps those should just be
 closed?

I have an open pull request which is already months old and no longer compatible to master. This is because I have not received any feedback since submission. It does not mean that it is irrelevant or that I am not willing to make it compatible again as soon as any reviewer has time for a review. So it has to be there, if not for anything else than just for for the sake of being there and being seen: - So that no-one else implements the same thing again for no reason - Maintainers know that there is something they need to take care of at some point. Pull requests should be closed by maintainers if the pull request won't get accepted even if improved quality wise or if the submitter is no longer available for making it ready, but not just because they are old. Best regards, Robert

Essentially, the submitter of the pull request needs to drive the change. The submission rate exceeds the attention span of the set of people with review and pull rights. So old stuff remains old unless brought up periodically. This tends to be the case with every large project.
Jun 04 2013
prev sibling next sibling parent "Jonathan M Davis" <jmdavisProg gmx.com> writes:
On Tuesday, June 04, 2013 09:52:57 Robert wrote:
 Pull requests should be closed by maintainers if the pull request won't
 get accepted even if improved quality wise or if the submitter is no
 longer available for making it ready, but not just because they are old.

I think that the problem is more that the Phobos maintainers are busy and don't necessarily do a good job as a group of reviewing pull requests in general, let alone making sure that each and every pull request gets reviewed and processed in a reasonable time frame. It's something that we need to work on. I think that it's rarely the case that a particular pull request has no comments on it because it's undesirable. It just hasn't been looked at yet like it should have been yet. If it were truly undesirable, it would at minimum have comments saying so if not have been closed. - Jonathan M Davis
Jun 04 2013
prev sibling next sibling parent "monarch_dodra" <monarchdodra gmail.com> writes:
On Tuesday, 4 June 2013 at 17:47:36 UTC, Jonathan M Davis wrote:
 On Tuesday, June 04, 2013 09:52:57 Robert wrote:
 Pull requests should be closed by maintainers if the pull 
 request won't
 get accepted even if improved quality wise or if the submitter 
 is no
 longer available for making it ready, but not just because 
 they are old.

I think that the problem is more that the Phobos maintainers are busy and don't necessarily do a good job as a group of reviewing pull requests in general, let alone making sure that each and every pull request gets reviewed and processed in a reasonable time frame. It's something that we need to work on. I think that it's rarely the case that a particular pull request has no comments on it because it's undesirable. It just hasn't been looked at yet like it should have been yet. If it were truly undesirable, it would at minimum have comments saying so if not have been closed. - Jonathan M Davis

Truth. I've had a pull closed in less than an hour once. On the other hand, I have one which has been open for about 7 months now. It's all a balance of importance vs resource/cost ratio, and reviewer interest. I've closed a few of my own ("valid") pulls myself: they weren't bad, just that I judge reviewer times would be better spent on more important things. It's a bit frustrating at times, but I find it more than balances when you realize that you actually *get* to participate at all, or even partake in specs discussion...
Jun 04 2013
prev sibling next sibling parent "Adam Wilson" <flyboynw gmail.com> writes:
On Sun, 02 Jun 2013 22:54:51 -0700, Jonathan M Davis <jmdavisProg gmx.com>  
wrote:

 On Sunday, June 02, 2013 22:49:08 Adam Wilson wrote:
 I'll see if I can't give you a stab at an explanation. Note that I did  
 not
 write the Auto-Tester, that is the work of Brad Anderson.

Brad Roberts actually, with some help from Daniel Murphy (who created the pull tester), and I don't know who else. - Jonathan M Davis

I keep missing this up. It doesn't help that I was talking to Brad Anderson (eco) on IRC. Sorry Brad Roberts! -- Adam Wilson IRC: LightBender Project Coordinator The Horizon Project http://www.thehorizonproject.org/
Jun 04 2013
prev sibling next sibling parent "Adam Wilson" <flyboynw gmail.com> writes:
On Tue, 04 Jun 2013 16:02:01 -0700, Walter Bright  
<newshound2 digitalmars.com> wrote:

 On 6/2/2013 10:49 PM, Adam Wilson wrote:
 Note that I did not write
 the Auto-Tester, that is the work of Brad Anderson.

Brad Roberts

*sigh* I've lost count of how many times I've made that goof. Sorry Mr. Roberts! -- Adam Wilson IRC: LightBender Project Coordinator The Horizon Project http://www.thehorizonproject.org/
Jun 04 2013
prev sibling next sibling parent Joseph Rushton Wakeling <joseph.wakeling webdrake.net> writes:
On 06/03/2013 07:49 AM, Adam Wilson wrote:
 - Pull requests are tested by cloning [DMD/DRuntime/Phobos] master and merging
 the pull into it. This ensures a clean pull against master.
 - The AT must restart pull request testing every time code is merged into
 DMD/DRuntime/Phobos because of the above testing strategy.
 - If many pulls are merged into [DMD/DRuntime/Phobos] master in a given period
 of, for example a day, the AT will restart prior to completing a full test
pass.
 - With the current number of open pulls the AT can complete a full testing pass
 in roughly 8 hours.
 - Each pull takes a rough average of 15 minutes to test.

What I don't understand is how the number of "pending" tests fluctuates. Not too long ago a pull request of mine to Phobos had passed 9, with 1 pending. Now it's pending 10. I have not pushed any changes in the interim ... !
Jun 05 2013
prev sibling next sibling parent "Brad Anderson" <eco gnuk.net> writes:
On Wednesday, 5 June 2013 at 16:22:19 UTC, Joseph Rushton 
Wakeling wrote:
 On 06/03/2013 07:49 AM, Adam Wilson wrote:
 - Pull requests are tested by cloning [DMD/DRuntime/Phobos] 
 master and merging
 the pull into it. This ensures a clean pull against master.
 - The AT must restart pull request testing every time code is 
 merged into
 DMD/DRuntime/Phobos because of the above testing strategy.
 - If many pulls are merged into [DMD/DRuntime/Phobos] master 
 in a given period
 of, for example a day, the AT will restart prior to completing 
 a full test pass.
 - With the current number of open pulls the AT can complete a 
 full testing pass
 in roughly 8 hours.
 - Each pull takes a rough average of 15 minutes to test.

What I don't understand is how the number of "pending" tests fluctuates. Not too long ago a pull request of mine to Phobos had passed 9, with 1 pending. Now it's pending 10. I have not pushed any changes in the interim ... !

Every commit to master causes retesting of all pull requests.
Jun 05 2013
prev sibling next sibling parent "Jonathan M Davis" <jmdavisProg gmx.com> writes:
On Wednesday, June 05, 2013 18:22:09 Joseph Rushton Wakeling wrote:
 What I don't understand is how the number of "pending" tests fluctuates. 
 Not too long ago a pull request of mine to Phobos had passed 9, with 1
 pending. Now it's pending 10. I have not pushed any changes in the
 interim ... !

Probably because another commit was merged into dmd, druntime, or Phobos, so it had to start again. - Jonathna M Davis
Jun 05 2013
prev sibling parent Joseph Rushton Wakeling <joseph.wakeling webdrake.net> writes:
On 06/05/2013 07:06 PM, Jonathan M Davis wrote:
 Probably because another commit was merged into dmd, druntime, or Phobos, so 
 it had to start again.

Ahh, OK. Thanks to you and Brad for the clarification! :-)
Jun 05 2013