fix mercurial checkouts w/ revision

[bhearsum]

Mercurial checkouts that specify a revision *always* clobber. This is really terrible for large trees. By separating 'pull' and 'update' we can update to a certain revision without clobbering.

Through my own tests and asking around in #mercurial on Freenode I've confirmed that 'hg clone --rev [num]' and 'hg pull' && 'hg update --rev [num]' leave the working directory in the exact same state. (When doing a clone --rev you won't have any revisions later than the specified one in your repository, but I don't think we care about that.)

Patch incoming.

[bhearsum]

fix mercurial checkouts with a revision

[bhearsum]

I was going to write a test for this but I'm pretty sure it gets tested as part of Mercurial.test_checkout. self.do_vctest() seems to do a lot of things, including adding revisions and checking out.

[bhearsum]

Just wanted to note that we've been running this for the past few weeks without issue.

[bhearsum]

Any chance this will make 0.7.8?

[warner]

I'll look at it

[dustin]

warner, does this look OK? I don't have hg installed, and don't want to go down that particular rabbit hole, so I haven't committed it yet.

[bsmedberg]

I've tried this patch and have the following problem. If a pull occurs and there are no changes, the build fails, because the code in _handleEmptyUpdate is operating on the wrong command (the update command, instead of the pull command). I'm going to try and upload a patch in a bit.

[bsmedberg]

patch for commands.py

[bsmedberg]

The patch attached makes the following changes from the previous patch:

1) the ordering is doVCUpdate/_handleEmptyUpdate/_doUpdate 2) _doUpdate doesn't set self.command because AFAICT the only reason we set self.command was so _handleEmptyUpdate would have the correct command log to grep 3) when no revision is specified, explicitly specify 'default' to match the default behavior of a revision-less clone... if the previous checkout was on a branch, we want to revert to the default branch

[dustin]

The first of these patches (Ben's) applies, but the second (bsmedberg) doesn't.

 http://repo.or.cz/w/buildbot.git?a=shortlog;h=refs/remotes/dustin/bug277

bsmedberg, can you update it?

[even]

The bsmedberg's patch includes the Ben's patch so if you tried to apply them one after one, the second one logically fail. I tried to apply the bsmedberg's patch on a 0.7.8 base and it applied without problem.

[bsmedberg]

Updated patch

[even]

I marked that bug to be on the 0.7.10 Milestone since it has been waiting too long and that a lot of people looks like to be waiting for it (including me).

Adding keyword review since it should work now.

[dustin]

in my branch

[even]

Why did you let the bug open ? Is there a specific reason for it to stay unresolved ?

[dustin]

nope, thanks

[dustin]

Actually, this change is failing tests on my system, so I've reverted it in my branch, but it's available at

 http://github.com/djmitche/buildbot/tree/bug277

if you want to fork the repository and hack on it. I'll attach test.log.

[marcusl]

I did some related work when fixing #187, which has been merged. Perhaps someone could look at that and see how it works?

[dustin]

I rebased my branch onto master and re-ran the tests, and I'm still seeing failures.

[FAIL]: buildbot.test.test_vc.MercurialInRepo.testCheckoutHTTP

Traceback (most recent call last):
  File "/home/dustin/devel/projects/buildbot/t/buildbot/buildbot/test/test_vc.py", line 546, in _do_vctest_clobber_1
    self.checkGotRevisionIsLatest(bs)
  File "/home/dustin/devel/projects/buildbot/t/buildbot/buildbot/test/test_vc.py", line 492, in checkGotRevisionIsLatest
    self.checkGotRevision(bs, expected)
  File "/home/dustin/devel/projects/buildbot/t/buildbot/buildbot/test/test_vc.py", line 486, in checkGotRevision
    self.failUnlessEqual(bs.getProperty("got_revision"), str(expected))
twisted.trial.unittest.FailTest: not equal:
a = '35bf7e302caf'
b = '33e6135447df'
===============================================================================
[ERROR]: buildbot.test.test_vc.MercurialInRepo.testCheckoutBranch

Traceback (most recent call last):
Failure: twisted.internet.defer.TimeoutError: <buildbot.test.test_vc.MercurialInRepo testMethod=testCheckoutBranch> (testCheckoutBranch) still running at 120.0 secs

There are more, but I think they're fallout from these failures.

[even]

Replying to dustin:

I rebased my branch onto master and re-ran the tests, and I'm still seeing failures. There are more, but I think they're fallout from these failures.

Can we see the protocol of the failing test and the description of the expected result since I read the patch and started to believe that the test is wrong and not the patch.

[dustin]

Replying to even:

Can we see the protocol of the failing test and the description of the expected result since I read the patch and started to believe that the test is wrong and not the patch.

I'm not sure what you mean by "protocol". The code is in

 http://github.com/djmitche/buildbot/commit/116e0808017f218abc57cf3bcfaff535572e1633

As for the expected results -- I don't know. Marcus?

[marcusl]

dustin: I'll see if I can run som tests and see what happens.

even: The actual test class (MercurialInRepo?) is in buildbot/test/test_vc.py, but most of it happens in BaseHelper? in the same file.

[even]

I'm reading the tests done on Mercurial and I'm starting to think that the whole thing looks inconsistent.

I can see in the code of the test that the whole branching is done using different directories (which is ugly since Mercurial has its own branching system, but I will let that aside since it has nothing to do with the current issue). What looks strange is in buildbot/test/test_vc.py line 2629. Since you are using directories to do the branching, why is there "inrepo" written there and not "dirname". I really don't understand the logic here.

I don't know if this is the reason for the whole thing failing though. It would really be great to have the complete log of the test (what commands are executed, what their result is, what the expected result was, etc) to know if the error is coming from the mercurial object or from the test.

Is it possible to retrieve those informations ?

[marcusl]

Mercurial supports branches in two ways, inrepo or as separate dirs, so BuildBot supports both of these (with inrepo being the latest addition, that has received a lot of attention lately).

If you look at the tests in Dustin's repo, he has merged #187 which added extra support for inrepo branches and an extra testcase, (written by me, being the first real attempt at hacking BuildBot), that creates branches in the same repo then uses 'inrepo' to tell the Mercurial source step to work with that.

So, the MercurialInRepo? test ckass uses inrepo branches. The Mercurial class uses branch names.

Hope it clears things up.

[marcusl]

Oh, and logging is available by uncommenting some lines at the top of test_vc.py, then re-running the tests.

[marcusl]

The #277 fix didn't take the branch value into account, but always used 'default', which then made the testCheckoutBranch loop forever, as it failed to clobber back and find the correct repo.

I've attached a patch that fixes this. Basically, use my _update() which does 'hg identify --num --branch' before deciding to clobber or updating, instead of the provided _doUpdate().

I was a bit worried that we've uncovered another bug, namely eternal looping if a requested branch isn't found. But that causes '_update2' to fail instead. (I've checked this.)

[marcusl]

Fix #277 -patch to work with #187 fix

[even]

Ok. That's why this one is failing. It can't just be applied on #187. But #187 already do what we want to do in this. No need to apply the given patch since update is called whatever. I'm closing this : changes needed are done in #187.

[marcusl]

You're right. I didn't spot that I already did that.

Too much context switching lately. :)

[even]

A change in another bug makes me repon this bug. Since there's a really trivial patch to this, I'll post it in a few minutes.

[even]

Remove an useless test causing the bug.

[even]

Here, I just added the patch I mentioned earlier. Also adding review keyword since I just tested it and it works well.

[marcusl]

Right. That shouldn't be there. I actually thought this was fixed, so thanks for spotting it.

[marcusl]

Fixed in  389f9c251b11:

    Fixes #277: Remove unnecessary test

@@ -2444,10 +2444,7 @@ class Mercurial(SourceBase):
         if os.path.exists(os.path.join(self.builder.basedir,
                                        self.srcdir, ".buildbot-patched")):
             return False
-        # like Darcs, to check out a specific (old) revision, we have to do a
-        # full checkout. TODO: I think 'hg pull' plus 'hg update' might work
-        if self.revision:
-            return False
+
         return os.path.isdir(os.path.join(self.builder.basedir,
                                           self.srcdir, ".hg"))