Hi,

I've made some comments inline below. Also I'm going to do a bit more
work on these patches and then send separate merge requests for
separate branches to keep things a bit simpler.


On Wed, Oct 21, 2009 at 12:31:34AM -0000, Jamu Kakar wrote:
> [1]
> 
> The tests haven't been updated to match the changes and, as a
> result, there are many failures.  We'll need to fix that before the
> branch can be merged.  You can run the tests with 'make check' to
> see what-all is breaking.  I can help with this (though probably not
> until the end of this week, realistically).

Opps should have checked the tests. I'll sort that out.

> I see that you've use bzr pipeline and I'm guessing from the log
> (haven't looked at the individual pipes) that these changes came
> from jelmer's build-deb branch.

Most of them. I had to fix them up a bit as his was based on a much
older revision.

I'm using pipes for other reasons which aren't related :)

> 
> [2]
> 
> A minor issue is that the version number format has changed.  What
> you've changed it to is close to what it was before the change to
> the current format.  I think it makes sense to follow the PPA
> guidelines, so I'm in favour of the change, but some users will
> probably be unhappy about it.  Is this change necessary for your use
> case?

I prefer my version :) I noticed later there was a bug to implement
version templates. I'll do some work on that instead and resubmit so
we can support both.

> [3]
> 
> Also, there are some minor cosmetic issues:
> 
> 1. Please use double-quotes instead of single-quotes.
> 
> 2. Whole words are preferred to abbreviations.  'branch_wt', for
>    example, would be better as 'branch_working_tree' or
>    'working_tree'.
> 
> 3. There are some lines that are longer than 80 characters that need
>    wrapping.

ok No problems.

> 
> 
> [4]
> 
> +import shutil
> 
> This import is unnecessary.

OK.

> 
> 
> [5]
> 
> Something I'll need to double check is copyright assignment
> requirements, given that this is a Canonical project.  Have you
> signed the contributor agreement?

I need to do that for some bzr patches as well. So I'll get it sorted
shortly.

> [6]
> 
> +        # TODO: This merge doesn't make any sense...
> 
> I don't understand this comment.  The merge does make sense, as far
> as I understand it...?  Am I missing something?

That was one of jelmer's comments. I'll look into it a bit further.

> 
> [7]
> 
> +            # Sometimes we might have a rules.autoppa and no rules file.
> +            # We bzr mv .autoppa in place so as to keep perms right
> +            self.working_tree.remove(path[:-len(".autoppa")])
> +            self.working_tree.rename_one(path[start:end] + '.autoppa', path[start:end])
> 
> I don't think this is correct.  This will effectively cause the
> .autoppa file to "disappear" when/if the build branch is merged back
> into the source branch, which will likely break subsequent builds.

OK I'll check that out.

> 
> 
> [8]
> 
> +        cmd_builddeb().run([self.working_tree.basedir], ['-sa'], source=True)
> 
> Yay for reuse!  One thing to note is that the bzrlib.ui module isn't
> setup properly.  I guess if you've been using this branch, it
> doesn't matter, but I do wonder if the output would be better with a
> properly-initialized bzrlib.ui.  The code required to do this is:
> 
>     import bzrlib.ui
>     bzrlib.ui.ui_factory = bzrlib.ui.make_ui_for_terminal(
>         sys.stdin, sys.stdout, sys.stderr)
> 
> I think this really ought to go in Commandant.  Anyway, just thought
> I'd mention it in case doing this would improve the user experience.

ok I'll check that out

> 
> 
> [9]
> 
> === added file 'debian/pycompat'
> === added file 'debian/watch'
> 
> I'm not a packaging expert and so I don't really understand what
> problem the addition of these two files solves.  Would you mind
> explaining, please?

What Robert said :)

> [10]
> 
> Thanks for merging in jelmer's build-deb branch.  I didn't realize
> there were outstanding merge proposals because Launchpad doesn't
> tell me by default.  I'll fix that now.

no problems. I also had a bit of a whinge about that bug.


-- 
John
Blog                             http://www.inodes.org
LCA2010                          http://www.lca2010.org.nz