bzr branch fixes #439

Merged
merged 5 commits into from Jul 7, 2012

Projects

None yet

3 participants

@licquia
licquia commented Jun 2, 2012

At the moment, branches in bzr are separate trees at separate locations. The buildbot bzr source step allows branch logic to work with the "baseURL" and "defaultBranch" parameters, which are used to create repository URLs for different branches at build time.

Unfortunately, for new-style source steps, this is completely broken. The step code always uses defaultBranch, rather than the branch included as part of the ChangeSource that triggers the build.

My changes fix this, and add tests for this functionality.

Tested on the buildbot setup for the LSB (www.linuxbase.org/buildbot).

Jeff Licquia added some commits Jun 1, 2012
Jeff Licquia Fix bzr branch handling.
Branches in bzr are separate URLs at the moment; handling branches
amounts to appending to a base URL (the "baseURL" parameter).  The
original code did this on factory creation; we need it to happen
at build time instead (when the bzr step is actually run on the slave).
3f40589
Jeff Licquia Add tests for bzr baseURL; fix combining baseURL w/ branch. 38906c4
@tomprince tomprince and 1 other commented on an outdated diff Jun 2, 2012
master/buildbot/steps/source/bzr.py
self.revision = revision
self.method = self._getMethod()
self.stdio_log = self.addLog("stdio")
+ if self.repourl is None:
+ if self.baseURL[-1] == '/':
@tomprince
tomprince Jun 2, 2012 Member

This should use self.build.path_module.join. Or maybe just os.path.join.

@tomprince
tomprince Jun 2, 2012 Member

In fact, this should perhaps just append the branch, in case the URLs are /path/to/repo-branch1 and /path/to/repo-branch2.

@licquia
licquia Jun 2, 2012

In theory, people could append branches that way; in practice, bzr strongly encourages "directory <-> branch" thinking in the way shared repos are set up and in other ways. I thought the more likely error would be for people to omit the trailing slash on baseURL. It's a judgment call, though, so either way.

Totally cool with using the join method, though; will push something here in a bit.

@tomprince
tomprince Jun 2, 2012 Member

Well, so I realized, after I wrote my original comment, that it is likely a remote URL, rather than a path on the slave. (I guess I should have been clearer).

@licquia
licquia Jun 4, 2012

Per djmitche's comment, switched to using os.path.join().

@tomprince tomprince commented on the diff Jun 2, 2012
master/buildbot/steps/source/bzr.py
assert self.mode in ['incremental', 'full']
if self.mode == 'full':
assert self.method in ['clean', 'fresh', 'clobber', 'copy', None]
def startVC(self, branch, revision, patch):
- self.branch = branch or 'master'
+ if branch:
@tomprince
tomprince Jun 2, 2012 Member

Is this change necessary? The branch argument defaults to self.branch, which might be None. (This is handled in base.Source.)

@licquia
licquia Jun 2, 2012

If we're sure that branch defaults to self.branch, then yeah. OTOH, if we're sure, why do the "or 'master'"? Using 'master' as the fallback is almost certainly wrong here.

@tomprince
tomprince Jun 2, 2012 Member

Well, is there a sensible default for branch? If there isn't, perhaps defaultBranch needs to be mandatory, if baseURL is used. In any case, we can perhaps just use branch, instead of updating self.branch, since the only use is in this method, anyway.

@djmitche
djmitche Jun 2, 2012 Member

It does - see start in master/buildbot/steps/source/base.py.

The original (branch or 'master') is intended to implement the "default" branch, for when a source stamp comes in with no branch specified. The name varies per VC (default on hg, trunk on svn, master on git). Is master wrong for Bzr? If so, what's reasonable?

@licquia
licquia Jun 4, 2012

bzr doesn't (or didn't; see below) really have a sense of the "default branch name", because of the whole directory <-> branch thing. Imagine, in git, if all branch operations didn't exist, so you had to use "clone" to create branches; that's how bzr works. (It's a little more complicated, in that you can share the repo metadata between branches, but that's largely an implementation detail and not relevant to what buildbot needs.)

The 2.5 release of bzr introduced support for what bzr calls "colocated branches", which work like branches in git. It's still very new; there's no default branch name yet, for example, and no tools really understand colocated branches yet. So I'd assume that "traditional" bzr branching will be the norm for some time.

There's also no convention for branch naming as there is in svn, so it's hard to say what the default should be. I'd probably just say that using "baseURL" requires using "defaultBranch" as well, and rely on that. For the "repourl" case, I think we can ignore the "branch" parameter entirely.

@tomprince
tomprince Jun 4, 2012 Member

In that case, defaultBranch should be checked in the constructor, and a call to buildbot.config.error if it isn't provided (in the baseURL case). Then branch could just be used, and self.branch ignored here.

I wonder, though, if branchType option of mercurial should perhaps be used instead, with just repourl, and using the logic of baseURL if branchType="dirname"?

@tomprince
Member

This looks good. Thanks for catching and fix this.

@djmitche

If these are really URLs, we shouldn't be using the slave's path module. I think @tomprince may have sent you on a false lead here :(

@djmitche
Member
djmitche commented Jul 6, 2012

@licquia, any progress on this?

@licquia
licquia commented Jul 7, 2012

Sorry about that. I had it in my mind that this was done, and now see I had left one detail open.

I've pushed that change. I elected to merely add the check for defaultBranch, and not alter the logic in startVC(). My reasoning: we still need to use self.branch if branch is not set, and rather than do something elaborate with if statements, it seemed easiest to simply overwrite the default stored in self.branch as needed and rely on it from that point on.

As for the suggestion regarding branchType: I think it might be a bit early to do that, and would take a bit of work to implement properly (handling errors on "old-style" single-branch branches, etc.). It's worth doing at some point, but the changes here will at any rate need to be done anyway, and I suspect bzr 2.5 will need to be deployed a bit more widely before this feature will become important for buildbot users.

Finally, rather than use buildbot.config.error, I copied the existing error logic in the constructor, which raises ValueError on invalid constructor arguments. If that's wrong, it will probably need to be changed in the other two places it's done as well, and I decided to bet on "(possibly) foolish consistency".

@djmitche
Member
djmitche commented Jul 7, 2012

Thanks, and well done resisting feature creep :)

@djmitche djmitche merged commit 802b85e into buildbot:master Jul 7, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment