Bug 85887

Created attachment 140724 [details] WIP patch We finished setting up an SVN mirror 2 weeks ago and use it on http://build.webkit.sed.hu (14 buildslaves) with this patch adapted to our master, and it works very stable. The new wait-for-SVN-server.py ensures that svn update never start before mirror is ready and in sync with svn.webkit.org. It protects buildslaves from "rm -f" because of unavailable SVN server or invalid SVN revision passed with force build button. In my patch this script only runs when SVN mirror is added. But it can be useful for svn.webkit.org SVN server too to avoid "rm -rf" because of unavailable server.

I forgot to mention that the origin of "SVN server XML call" part of Tools/BuildSlaveSupport/wait-for-SVN-server.py is buildbot code: https://github.com/buildbot/buildbot/blob/buildbot-0.8.2/master/contrib/svnpoller.py

Adding dpranke@ and maruel@ for a Chromium opinion, also given this thread which outlines our GIT mirror: https://lists.webkit.org/pipermail/webkit-dev/2012-March/019698.html

Comment on attachment 140724 [details] WIP patch I have no useful knowledge to contribute here. Sounds useful, though :) The code looks fine to me, but you probably want wms or lforschler to review it.

Is there a corresponding change to the config.json which will point your slaves to the mirror?

Comment on attachment 140724 [details] WIP patch Can we unittest this in master_cfg_unittest.py?

Created attachment 140874 [details] Add SVN mirror for Qt buildslaves (In reply to comment #5)> Is there a corresponding change to the config.json which will point your slaves to the mirror?Sure. After the general patch landed, I'm going to add this change to config.json

(In reply to comment #6) > (From update of attachment 140724 [details]) > Can we unittest this in master_cfg_unittest.py? There isn't any checked in unittest for CheckOutSource and *Factory classes. But let's give it a try.

Created attachment 149537 [details] Patch updated to ToT, I'm going to add unit test too.

Created attachment 149753 [details] proposed patch I updated it to ToT and addded a unit test. ( It won't apply until https://bugs.webkit.org/show_bug.cgi?id=90072 landed )

View in context: https://bugs.webkit.org/attachment.cgi?id=149753&action=review > Tools/BuildSlaveSupport/build.webkit.org-config/config.json:182 > + "SVNMirror": "svn://rain.inf.u-szeged.hu:3389/", Why is this different from the rest? > Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:104 > + self.addFactoryArguments(platform=platform, configuration=configuration, architecture=architecture, buildOnly=buildOnly, SVNMirror = SVNMirror) No need for the spaces around the last equal sign (just like the others). > Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:685 > + self.addStep(WaitForSVNServer, doStepIf = bool(SVNMirror), hideStepIf = bool(SVNMirror)) Won't this hide the step if there is an SVNMirror specified? Is this what you wanted? (Also no need for the spaces around the equal signs) > Tools/BuildSlaveSupport/wait-for-SVN-server.py:31 > + response = commands.getoutput("svn log --non-interactive --verbose --xml --limit=1 " + SVNServer) Use % formatter if you use it already elsewhere, just to be consistent. > Tools/BuildSlaveSupport/wait-for-SVN-server.py:52 > + except exceptions.ValueError: 'ValueError' should be enough, no need for the exceptions module. > Tools/BuildSlaveSupport/wait-for-SVN-server.py:69 > +parser = OptionParser() > +parser.add_option("-r", "--revision", dest="revision", help="SVN revision number") > +parser.add_option("-s", "--svn-server", dest="SVNServer", help="SVN server") > +(options, args) = parser.parse_args() > +waitForSVNRevision(options.SVNServer, options.revision) This should be in an if __name__ == "__main__": block. Also add a type argument for the revision like: type="int", so you won't need to check if the revision is a number or not. The OptionParser will do it for you.

(In reply to comment #11) > View in context: https://bugs.webkit.org/attachment.cgi?id=149753&action=review > > > Tools/BuildSlaveSupport/build.webkit.org-config/config.json:182 > > + "SVNMirror": "svn://rain.inf.u-szeged.hu:3389/", > > Why is this different from the rest? Because the machine run this bot on is out of our firewall and it can see only 3389 TCP port. Which is restricded for this machine only. :) > > Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:104 > > + self.addFactoryArguments(platform=platform, configuration=configuration, architecture=architecture, buildOnly=buildOnly, SVNMirror = SVNMirror) > > No need for the spaces around the last equal sign (just like the others). Fixed. > > Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:685 > > + self.addStep(WaitForSVNServer, doStepIf = bool(SVNMirror), hideStepIf = bool(SVNMirror)) > > Won't this hide the step if there is an SVNMirror specified? Is this what you wanted? (Also no need for the spaces around the equal signs) Good point. But we shouldn't add this step at all if bool(SVNMirror) is false. > > Tools/BuildSlaveSupport/wait-for-SVN-server.py:31 > > + response = commands.getoutput("svn log --non-interactive --verbose --xml --limit=1 " + SVNServer) > > Use % formatter if you use it already elsewhere, just to be consistent. > > > Tools/BuildSlaveSupport/wait-for-SVN-server.py:52 > > + except exceptions.ValueError: > > 'ValueError' should be enough, no need for the exceptions module. Removed, because type="int" argument check works for us with empty revision too. (force build without revision) > > Tools/BuildSlaveSupport/wait-for-SVN-server.py:69 > > +parser = OptionParser() > > +parser.add_option("-r", "--revision", dest="revision", help="SVN revision number") > > +parser.add_option("-s", "--svn-server", dest="SVNServer", help="SVN server") > > +(options, args) = parser.parse_args() > > +waitForSVNRevision(options.SVNServer, options.revision) > > This should be in an if __name__ == "__main__": block. > Also add a type argument for the revision like: type="int", so you won't need to check if the revision is a number or not. The OptionParser will do it for you. Fixed.

Created attachment 149942 [details] proposed patch Updated patch based on Peter's review.

Created attachment 158304 [details] proposed patch

Comment on attachment 158304 [details] proposed patch Updated to ToT, use subprocess.Popen instead of commands.getoutput to make wait-for-SVN-server.py work on Windows too.

Comment on attachment 158304 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=158304&action=review > Tools/BuildSlaveSupport/build.webkit.org-config/mastercfg_unittest.py:350 > + # SVN mirror feature isn't unittestable now with source.oldsource.SVN(==source.SVN) , only with source.svn.SVN(==SVN) > + # https://bugs.webkit.org/show_bug.cgi?id=85887 > + if issubclass(CheckOutSource, source.SVN): > + return Unfortunately it isn't unittestable now with source.oldsource.SVN, because SVN.baseURL is set to _ComputeRepositoryURL(baseURL) in SVN.__init__ and _ComputeRepositoryURL needs build object to work, so we got the following error if we tried to run unittest with source.oldsource.SVN: FAIL: test_CheckOutSource (__main__.SVNMirrorTest) ---------------------------------------------------------------------- Traceback (most recent call last): File "./mastercfg_unittest.py", line 358, in test_CheckOutSource self.assertEquals(CheckOutSourceInstance.baseURL, self.get_SVNMirrorFromConfig(builder['name'])) AssertionError: <buildbot.steps.source.oldsource._ComputeRepositoryURL object at 0x26aa110> != 'http://svn.webkit.org/repository/webkit/' You can try this unittest with the newer source.svn.SVN if you apply the following patch: https://bugs.webkit.org/attachment.cgi?id=158307 (https://bugs.webkit.org/show_bug.cgi?id=90072) But unfortunately we can't migrate to source.svn.SVN for some reason now.

Created attachment 163563 [details] Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163563&action=review > Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:710 > + if bool(SVNMirror): No need for the bool() call.

(In reply to comment #17) > Created an attachment (id=163563) [details] > Patch Updated to ToT. When the connection is going broken during svn update, buildbot does "rm -rf" and then a new checkout. Transatlantic checkout is _impossible_, because it would take half a day, but it is absolutely _impossible_ without connection lost. In this case now I have to stop the slave, checkout from our local mirror, svn switch to svn.webkit.org and restart the slave. I got fed up with it. I won't do this sisyphus work anymore. Please review this patch as soon as possible. We really need it.

Created attachment 163574 [details] Patch

Comment on attachment 163574 [details] Patch looks plausible. good luck.

(In reply to comment #21) > (From update of attachment 163574 [details]) > looks plausible. good luck. Thanks, I'm going to land it in an IDLE time, maybe tomorrow morning in CET timezone (night in PDT)

Comment on attachment 163574 [details] Patch Clearing flags on attachment: 163574 Committed r128399: <http://trac.webkit.org/changeset/128399>

All reviewed patches have been landed. Closing bug.

It seems automatic master restart didn't work. Bill or Lucas, could you restart the master, please?

Master has been restarted. There was a stale lockfile from the last time I was working on the restart process.

Created attachment 163934 [details] Patch The script was added to a different folder than the one specified in master.cfg. I'm moving it to BuildSlaveSupport/ to keep together with the other scripts and avoid changing master.cfg.

Comment on attachment 163934 [details] Patch Rejecting attachment 163934 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: which does not exist! Applying it anyway. patching file Tools/BuildSlaveSupport/build.webkit.org-config/wait-for-SVN-server.py Hunk #1 FAILED at 1. 1 out of 1 hunk FAILED -- saving rejects to file Tools/BuildSlaveSupport/build.webkit.org-config/wait-for-SVN-server.py.rej patching file Tools/BuildSlaveSupport/wait-for-SVN-server.py Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Dirk Pranke']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue Full output: http://queues.webkit.org/results/13835704

It seems Ossy already fixed it :) http://trac.webkit.org/changeset/128493

Thanks for everyone. After master restarting I noticed some annoying bug: - first run is broken, because master.cfg wanted to run wait-for-SVN-sever.py before svn update ... at that time wait-for-SVN-sever.py doesn't exist. :-/ - problem for a new slave - problem after "rm -rf" after an svn fail or mirror changes in config.json - possible solution: check the existance of wait-for-SVN-sever.py from master.cfg - true: run it - false: sleep for X seconds to let SVN mirror have time for sync - popen in wait-for-SVN-sever.py is buggy on windows - type="int" check for revision in wait-for-SVN-sever.py is incorrect, because it can be empty string if force build button is pushed without adding revision. I leave this bug open now and will file bug reports about these bugs tomorrow morning. Of course, I'll fix them as soon as possible.

(In reply to comment #30) > Thanks for everyone. > > After master restarting I noticed some annoying bug: > - first run is broken, because master.cfg wanted to run wait-for-SVN-sever.py > before svn update ... at that time wait-for-SVN-sever.py doesn't exist. :-/ > - problem for a new slave > - problem after "rm -rf" after an svn fail or mirror changes in config.json > - possible solution: check the existance of wait-for-SVN-sever.py from master.cfg > - true: run it > - false: sleep for X seconds to let SVN mirror have time for sync new bug report: https://bugs.webkit.org/show_bug.cgi?id=98013 > - popen in wait-for-SVN-sever.py is buggy on windows false positive alarm :) We had a sophisticated svn.bat SVN wrapper script on our Windows bot, and popen couldn't find the SVN binary. > - type="int" check for revision in wait-for-SVN-sever.py is incorrect, because it can be empty string if force build button is pushed without adding revision. new bug report: https://bugs.webkit.org/show_bug.cgi?id=98014 I close this bug, because it is solved and it works fine. The mentioned minor bugs will be fixed in separated bug reports.

After removing Qt bots in https://trac.webkit.org/changeset/156771/webkit , SVNMirror code is completely unused. It seems to be unused for last 5 years. Should we just remove this dead code?

(In reply to Aakash Jain from comment #32) > After removing Qt bots in https://trac.webkit.org/changeset/156771/webkit , > SVNMirror code is completely unused. It seems to be unused for last 5 years. > > Should we just remove this dead code? Deleting this dead code in https://bugs.webkit.org/show_bug.cgi?id=217823