Use svn switch to improve efficiency

[retracile]

The SVN operation uses svn update, and deletes its working copy if the branch changes. However, Subversion is able to move between branches much more efficiently using 'svn switch'.

Attached is a lightly tested patch that uses 'svn switch'. It will always work for the 'copy' mode; other modes are less tested. An 'svn switch' of a working copy containing unversioned files can in some cases fail; that is not handled in this patch.

[retracile]

use svn switch instead of svn update

[dustin]

You write:

An 'svn switch' of a working copy containing unversioned files can in some cases fail; that is not handled in this patch.

which is what worries me. I'm always in favor of burning a few more cycles over introducing spurious build failures.

[duncanphilipnorman]

Replying to dustin:

You write:

An 'svn switch' of a working copy containing unversioned files can in some cases fail; that is not handled in this patch.

which is what worries me. I'm always in favor of burning a few more cycles over introducing spurious build failures.

I'm using a similar patch against 0.8.3, which I'll attach as buildbot-slave-0.8.3-svn_switch.patch .

Here's why I think the patch (or the one above) should be reconsidered, despite svn switch sometimes failing:

1. Saves a ton of resources when using a single builder for multiple branches (common situation).
2. Low risk, since SourceBaseCommand.doVC uses SourceBaseCommand.maybeDoVCFallback if the update fails.
   • maybeDoVCFallback calls doClobber and then doVCFull.
   • In particular, if the update/switch fails, then we're getting the old behaviour (clobber and checkout). A build step that succeeds without this patch will succeed with this patch (possible coding bugs aside).

(I hope it's okay to reopen this ticket; let me know if I should just start a new one with the same summary and description.)

[dustin]

Reopening is fine. However, this should be implemented in the new master-side step, rather than in the slave-side step. On the master-side step, this should probably be a new method for the svn step.

Thanks for continuing to work on this - let me know if you need any pointers!

[duncanphilipnorman]

Replying to dustin:

Reopening is fine. However, this should be implemented in the new master-side step, rather than in the slave-side step. On the master-side step, this should probably be a new method for the svn step.

Ah, I see that the code has been refactored since the 0.8.3. A few things.

1. After a quick glance, it looks like svn switch could be used to implement an argument like clobberOnBranchChange=False. This would apply to mode=incremental as well as many of the methods in mode=full.
2. I found a Source in steps/source/oldsource.py... is that the one that the master-side SVN inherits from?
3. SVN._sourcedirIsUpdatable checks whether the .svn exists, but it doesn't check if the branch has changed. I can't find this check elsewhere (even in the Source class). Where is that logic?
4. In 0.8.3, if an update operation failed, there was a fallback to do clobber and full checkout. I don't see that in the 0.8.5 documentation... and I also don't see it in the current source. What am I missing?

[dustin]

Replying to duncanphilipnorman:

1. After a quick glance, it looks like svn switch could be used to implement an argument like clobberOnBranchChange=False. This would apply to mode=incremental as well as many of the methods in mode=full.

I'd rather that be "switch_branches" or something like that. You're right that it applies to both modes, and multiple behaviors.

1. I found a Source in steps/source/oldsource.py... is that the one that the master-side SVN inherits from?

Yes.

1. SVN._sourcedirIsUpdatable checks whether the .svn exists, but it doesn't check if the branch has changed. I can't find this check elsewhere (even in the Source class). Where is that logic?

That may be missing in the master-side step. In the slave-side steps, that's stored in sourcedata, which is mostly handled in the base class (see writeSourcedata and readSourcedata).

1. In 0.8.3, if an update operation failed, there was a fallback to do clobber and full checkout. I don't see that in the 0.8.5 documentation... and I also don't see it in the current source. What am I missing?

Hm, I don't see that either. I could have sworn that got implemented, but perhaps that, too, is missing in the master-side commands.

[duncanphilipnorman]

Replying to dustin:

Replying to duncanphilipnorman:

1. After a quick glance, it looks like svn switch could be used to implement an argument like clobberOnBranchChange=False. This would apply to mode=incremental as well as many of the methods in mode=full.

I'd rather that be "switch_branches" or something like that. You're right that it applies to both modes, and multiple behaviors.

Okay, that name makes sense. I can implement the other missing stuff, too. Unfortunately, I'm quite busy, so I can't promise any sort of timeline; we'll see when I get to it. (Anyone else should feel free to jump in... I'll add a comment when I actually start working so we don't duplicate effort.)

In the meantime, buildbot-slave-0.8.3-svn_switch.patch was buggy, so anyone with permissions can go ahead and delete it. I've actually used buildbot-slave-0.8.3-svn_switch-actually_works.patch, which I'll attach in a moment, so if someone out there is looking for this functionality slave-side it's a better bet.

[duncanphilipnorman]

Patch against 0.8.3.

[dustin]

duncanphilipnorman - any update on this bug?

For someone interested in subversion support, perhaps this could be finished up during the PyCon sprints?

[duncanphilipnorman]

Replying to dustin:

duncanphilipnorman - any update on this bug?

For someone interested in subversion support, perhaps this could be finished up during the PyCon sprints?

No progress, as my job has kept me too busy. I can confirm that the patch works well against 0.8.3, as we've been using it live since I posted it here.

If anyone else has interest in porting it to 0.8.5, I strongly encourage it! If I do find time to start working on this, I'll comment here first so that we're not duplicating effort.