Opened 11 years ago

Last modified 16 months ago

#97 reopened enhancement

Use svn switch to improve efficiency

Reported by: retracile Owned by:
Priority: major Milestone: 0.9.+
Version: 0.8.3 Keywords: svn, sprint
Cc: duncanphilipnorman@…, pmatos@…

Description (last modified by dustin)

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.

Attachments (2)

buildbot-svn-switch.patch (1.4 KB) - added by retracile 11 years ago.
use svn switch instead of svn update
buildbot-slave-0.8.3-svn_switch.patch (1.6 KB) - added by duncanphilipnorman 7 years ago.
Patch against 0.8.3.

Download all attachments as: .zip

Change History (16)

Changed 11 years ago by retracile

use svn switch instead of svn update

comment:1 follow-up: Changed 9 years ago by dustin

  • Resolution set to wontfix
  • Status changed from new to closed

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.

comment:2 in reply to: ↑ 1 Changed 7 years ago by duncanphilipnorman

  • Cc duncanphilipnorman@… added
  • Resolution wontfix deleted
  • Status changed from closed to reopened

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.)

comment:3 follow-up: Changed 7 years ago by 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!

comment:4 in reply to: ↑ 3 ; follow-up: Changed 7 years ago by 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?

comment:5 in reply to: ↑ 4 ; follow-ups: Changed 7 years ago by 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.

comment:6 in reply to: ↑ 5 Changed 7 years ago by 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, my original buildbot-slave-0.8.3-svn_switch.patch was buggy, but I've replaced it with a patch I've actually used that works, in case someone out there is looking for this functionality slave-side.

Last edited 7 years ago by duncanphilipnorman (previous) (diff)

Changed 7 years ago by duncanphilipnorman

Patch against 0.8.3.

comment:7 Changed 6 years ago by dustin

  • Keywords svn added
  • Milestone changed from undecided to 0.8.+
  • Version changed from 0.7.5 to 0.8.3

comment:8 follow-up: Changed 6 years ago by dustin

  • Keywords sprint added

duncanphilipnorman - any update on this bug?

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

comment:9 in reply to: ↑ 8 Changed 6 years ago by 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.

comment:10 in reply to: ↑ 5 Changed 5 years ago by dcoshea

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 note that in 0.8.6p1, a slave-side 'svn update' can "silently" fail if there are unversioned files in the working copy which were added in the repository - I've seen the following appear in the log for the SVN step:

svn: warning: Failed to add file '[...]': an unversioned file of the same name already exists

but the step was marked as passed, resulting in hard-to-diagnose build failures later.

It seems to me, then, that doing an 'svn switch' isn't more risky than doing an 'svn update', and in either case, telling the step to do a clean checkout every time is a way to avoid these problems.

Replying to dustin: [...]

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.

Doesn't "oldsource" mean "slave side"?

comment:11 Changed 5 years ago by dustin

  • Description modified (diff)

Is the update failure because 'svn' returns a zero (successful) exit status in that case?

Regarding oldsource - you're right. When I said "yes" I should have said "no" :/

comment:12 Changed 4 years ago by dustin

  • Milestone changed from 0.8.+ to 0.9.+

Ticket retargeted after milestone closed

comment:13 Changed 21 months ago by pmatos

  • Cc pmatos@… added
Note: See TracTickets for help on using tickets.