Opened 8 years ago

Last modified 4 years ago

#708 new enhancement

Incorporate bzr_poller.py and add more docs

Reported by: dustin Owned by:
Priority: patches-accepted Milestone: 0.9.+
Version: master Keywords: bzr
Cc:

Description

It looks like the BzrPoller? is ready to be used as a changesource - I'm not sure why it's in contrib instead.

Change History (23)

comment:1 Changed 8 years ago by dustin

  • Keywords bzr added

comment:2 Changed 7 years ago by psykidellic

Hello

I tried to use the BZR poller and hit upon couple of issues for which I received help in the IRC channel.

The two change were:

  • Comment out line 243-247. I am not sure about the reason so I will be looking deeper into it soon. Mostly it looks like API mismatch.

#for change in reversed(self.parent.changes): # if change.branch == ourbranch: # self.last_revision = change.revision # break #else:

  • The API has changed from get_apparent_author to get_apparent_authors so I had to update line #170-172 to get_apparent_authors()[0].

I have hosted the changed file at https://bitbucket.org/psykidellic/buildbot-pollers/. I plan to update it as I find solutions. Once it reaches a clean version, maybe I can create a patch/diff for the actual repository.

comment:3 Changed 7 years ago by dustin

Sounds great - thanks for the updates!

comment:4 Changed 6 years ago by dustin

psykidellic - did you finish this?

comment:5 Changed 5 years ago by gracinet

Hi,

I just stumbled accross the same problems. psykidellic's version solves the startup problems, but there is another API mismatch (addChange)

We've had a few months ago a discussion (including Tom Prince) on the mailing list about BzrPoller.

The thing is, I really need that poller to work. As these issues demonstrate, the fact that it's in contrib is an impediment to its maintainability.

Dustin, do you mind if I start working on including it in the main tree ? Would that be suitable for 0.8.7 branch or should it be contributed for the current master ?

Regards,

Last edited 5 years ago by gracinet (previous) (diff)

comment:6 Changed 5 years ago by dustin

Yes, please do update it to include it!

I don't mind including that in 0.8.7, if the timing works out (it won't add instability), but I don't think we should delay the release for it.

So, yes, mainstreaming the poller, and adding comprehensive tests for it, will be a great help!

comment:7 Changed 5 years ago by gracinet

Fine, let's go, then !

By the way, bzr_buildbot's author is Gary Poster, from Canonical at that time, licensed under GPL3 and later versions

comment:8 Changed 5 years ago by tom.prince

If you are going to merge the existing code to master, you would need to get permission from all significant contributors to that file, to release under GPLv2.

Also, if you are updating the code, you should probably make it store state in the db (look at the current version of the git poller for an example).

comment:9 Changed 5 years ago by dustin

Oh, I hadn't checked the licenses. That's problematic -- if we include that in Buildbot proper, it can be construed to implicitly re-license the whole thing.

So we have two options. One is to contact the existing contributors as Tom suggests - probably best would be to start with Gary and see if Canonical is willing. I suspect the other contributors will be amenable.

The other option is a clean-room re-implementation, which could be a little bit tricky given your involvement with the existing poller. That said, I think there's enough work to do here that a complete rewrite would help. Among other things:

  • completely separate poller and hook (and note that hooks are going to be difficult, if not impossible, to test in buildbot)
  • storing state in the db
  • new addChange calling pattern
  • codebases
  • other features modeled on the GitPoller

comment:10 Changed 5 years ago by gracinet

I'm not a license freak, but if I understand the compatibility matrix correctly, that means that the current BzrPoller already has a license problem.

My involvment with the existing poller is rather low, I mostly took a look at the hook, not the poller.

An implementation from scratch would indeed be somewhat easier for me, since I modelled HgPoller on GitPoller already. Basing a new poller on the bzr executable rather than bzrlib would be a simple way to ensure that it's original work, and be somewhat simpler in terms of dependencies but

  • I can't be sure at this point that bzrlib is not the only way to get acceptable performance (a major issue in Bazaar context). To know that, I'll have to check in the existing code.
  • If a problem has but one solution, any code implementing it will look as an inclusion.

comment:11 Changed 5 years ago by dustin

IANAL, but that's one of the reasons it's in contrib/ - we don't distribute such stuff, even if it is tracked in our git repositories. So technically we've avoided the problem.

I don't think this is worth over-analyzing. We need to make a good-faith effort to re-implement. If that leaves a few passages looking similar, that's probably OK. But consulting the old code for lessons on performance is probably a step too far. Could you make some performance measurements directly, and perhaps include a comment in the updated source indicating the results?

As for hook vs. poller - by no means must you write both. However, as I indicated in comment 9, hooks are much harder to test, because they tend to assume they're run within a bzr environment, rather than in buildbot, and the buildbot tests do not require that bzr be installed. I'd be hesitant to put an untestable hook into the distribution, since it runs a very real risk of bitrot (similar to what the early parts of this bug show).

comment:12 Changed 5 years ago by tom.prince

I suggest just contacting Gary/Canonical? about relicensing. http://www.markshuttleworth.com/archives/687 suggests that it won't be an issue. https://launchpad.net/~gary

comment:13 Changed 5 years ago by dustin

I emailed Gary a few minutes ago.

comment:14 Changed 5 years ago by Dustin J. Mitchell

comment:15 Changed 5 years ago by dustin

So that takes care of that. Thanks, Gary!

comment:16 Changed 5 years ago by gracinet

Great, starting to work on it.

comment:17 Changed 5 years ago by Dustin J. Mitchell

comment:18 Changed 5 years ago by tom.prince

  • Priority changed from major to patches-accepted

comment:19 Changed 5 years ago by dustin

gracinet, any update here? Having asked for Gary's help, I think we have an obligation to see this through!

comment:20 Changed 5 years ago by gracinet

I do have a working version, but it does not have unit tests yet, and is not overall tested enough in the wild (only our testing server runs it).

I agree that it's been too long already, but am in a difficult position wrt other work that I need to sort out. Would somewhere around march 15 or 16 be an acceptable target ?

comment:21 Changed 5 years ago by gracinet

Current status (branch bzrpoller of my github fork)

  • This BzrPoller passed a real-life test
  • I have a first working unit test, relying on mock to replace bzrlib.

I'd need some adviced on the preferred way to proceed with those tests, here. Gary provided a separate generate_change function, primarily for code factorization between the hook and the poller.

The current (draft) unit tests replace all bzrlib calls, including those made by this generate_change by mocks. That's a bit cumbersome for the writer and the reader, and I don't think there's much value in going down to that detail: if bzrlib API changes, the tests won't see it anyway (of course).

We could very well mock the generate_change function instead. What do you think ?

comment:22 Changed 5 years ago by dustin

How likely is it that the bzrlib API will change? That's a real risk, but mocking it would have the advantage of letting us test against the "old" and "new" APIs if it did change.

I'd have to see your tests to give more specific advice, but it sounds like you're on the right track. Maybe clean it up and post a preliminary pull request so Tom and I can have a look?

comment:23 Changed 4 years ago by dustin

  • Milestone changed from 0.8.+ to 0.9.+

Ticket retargeted after milestone closed

Note: See TracTickets for help on using tickets.