Merge lp://staging/~spiv/bzr-builder/merge-subdirs-479705 into lp://staging/~james-w/bzr-builder/trunk-old

This fixes bug 479705. It allows a merge line of a builder recipe to specify a subdirectory to merge, rather than the whole named branch. The obvious application of this is to allow merging just the debian/ directory from a branch that contains more than just a debian/ directory.

Hi,

Thanks for working on this.

I have some concerns about the utility of it. If the branch
you are merging doesn't make any modifications outside of
./debian/ then this makes no difference, therefore it must
be for the case where it does.

Is this then to take the packaging without any modifications
to the source, so that it is a vanilla packaging?

If so, then it is just assumed that this won't be done if
some of the modifications are needed, or if the packaging
depends on some of the modifications?

That would be acceptable, but I wonder why we want to encourage
people to do things this way, instead of say, layering another
branch on top that undoes the unwanted things? Obviously the two
are not equivalent, but the latter seems to fit better with the
model to me.

As for a code review, two things mean that this is not mergeable
as is:

  * Needs a format bump.
  * Needs an update to the documentation so that people know it is possible.

Other than that the changes look fine with one obvious exception:

45 + :param subpath: XXX

Thanks,

James

> Hi,
>
> Thanks for working on this.
>
> I have some concerns about the utility of it. If the branch
> you are merging doesn't make any modifications outside of
> ./debian/ then this makes no difference, therefore it must
> be for the case where it does.

This is false. If the branches are not related, merging a subdir can avoid very many unnecessary conflicts while still generating a single tree.

> Is this then to take the packaging without any modifications
> to the source, so that it is a vanilla packaging?

Or to merge unrelated branches.

> As for a code review, two things mean that this is not mergeable
> as is:
>
> * Needs a format bump.

I don't see why it needs this: old versions will error appropriately, new versions will work.

If old versions would misbehave then I'd agree on the need to bump the format.

On Tue Nov 24 00:59:18 UTC 2009 Robert Collins wrote:
> > Hi,
> >
> > Thanks for working on this.
> >
> > I have some concerns about the utility of it. If the branch
> > you are merging doesn't make any modifications outside of
> > ./debian/ then this makes no difference, therefore it must
> > be for the case where it does.
>
> This is false. If the branches are not related, merging a subdir can avoid very
> many unnecessary conflicts while still generating a single tree.

If the branches are not related there is no way in builder to merge
them at this point anyway (no way to specify a merge base). I haven't
tested to make sure that the API works the same as the UI here, so
apologies if I mispoke.

I appreciate trying to improve this situation, but I'm not sure that
this is the correct fix, at least at this time. I would like to know
more about the design though, as I feel that there is something I am
missing.

Thanks,

James

> > This is false. If the branches are not related, merging a subdir can avoid
> very
> > many unnecessary conflicts while still generating a single tree.
>
> If the branches are not related there is no way in builder to merge
> them at this point anyway (no way to specify a merge base). I haven't
> tested to make sure that the API works the same as the UI here, so
> apologies if I mispoke.

You're right about that. It would be easy to catch UnrelatedBranches and if a subpath is specified try again with a base of NULL_REVISION. Or we could add a 'mergeunrelated' directive. Or further extend the syntax to allow specifying bases.

Do you have a preference for one of those options, or do you feel this is the wrong track entirely? It seems to me that however it is specified a partial merge is a fairly neat way to provide "please give me this branch, plus the debian/ dir from another branch."

I'm sure there are many cases where blindly combining a debian/ dir from an existing package with an arbitrary version of upstream won't work. I think it's likely it will
work ok for many nightly packages though: have a recipe that specifies "current trunk + packaging from $recent-packaging". I'm not certain which use-case Robert had in mind, but that's what I'm thinking of.

Hi,

I'm sorry the discussion on this has stalled, but I'm not sure what to do
about it.

I'd rather know the concrete use cases for the change and how this would fix it
before merging so that I can better support them in future. Help understanding that
would be greatly appreciated.

Thanks,

James

So, in two parts:
 - Andrew is looking at making sure it works with unrelated branches.

Use cases: as we discussed, for launchpads 'build from branch' to be successful for upstreams, they need easy access to debian/ in package branches that are likely to have no history relationship: This syntax allows them to specify that that is what they want without us resorting to guessing.

Hi,

I think I would like to see catching UnrelatedBranches, and then if a subdir
is specified, try again with NULL_REVISION as the base. If there are then
any conflicts it should bail out as normal.

This should have it work for the simple case, which is a start.

Why specify the subdirectory separate from the URL? Could we not do an
open_containing and use the subpath returned from that?

Thanks,

James

Hi Andrew,

This is great now, thanks.

merge-part NAME BRANCH REVISION SUBDIR [TARGET-DIR]

I think I might prefer

merge-part NAME BRANCH SUBDIR REVISION [TARGET-DIR]

(Maybe even:

merge-part NAME BRANCH SUBDIR [REVISION [TARGET-DIR]]

or

merge-part NAME BRANCH SUBDIR [TARGET-DIR [REVISION]])

We can't do

from bzrlib.cleanup import OperationWithCleanups

unfortunately, as it is too new for the bzr that we need to work with.
We could backport newer bzr's for building older distroseries, but that's
quite a bit of work.

MergeIntoMerger only seems to be in newer bzr's too? Is there any way that
we can avoid depending on that?

108 + if target_subdir is None:
109 + target_subdir = os.path.basename(subpath)

I wouldn't expect that, I would think that it would just be "subpath".

I'm happy to do the work to merge this in, the only blocker right now
is MergeIntoMerger, so if you could comment on that then we can move
forward.

Thanks,

James

Right, sorry, I now see that MergeIntoMerger is proposed
for adding to bzr.

I think that's the right way to go as it should be in bzr,
but it does make this hard for deploying on older distroseries
in Launchpad.

Thanks,

James

James Westby wrote:
> Right, sorry, I now see that MergeIntoMerger is proposed
> for adding to bzr.
>
> I think that's the right way to go as it should be in bzr,
> but it does make this hard for deploying on older distroseries
> in Launchpad.

Ah, I didn't realise that was a requirement. I can extract that out in a way
that will work with older bzrs without much trouble. Would you like that in
bzr-builder or a separate plugin?

Also, what's the oldest bzr version that bzr-builder wants to work with?

On Fri, 09 Jul 2010 00:49:38 -0000, Andrew Bennetts <email address hidden> wrote:
> James Westby wrote:
> > Right, sorry, I now see that MergeIntoMerger is proposed
> > for adding to bzr.
> >
> > I think that's the right way to go as it should be in bzr,
> > but it does make this hard for deploying on older distroseries
> > in Launchpad.
>
> Ah, I didn't realise that was a requirement. I can extract that out in a way
> that will work with older bzrs without much trouble. Would you like that in
> bzr-builder or a separate plugin?

Great. bzr-builder is easier.

Do you think it should still go in bzr with a "backport" inside
bzr-builder?

> Also, what's the oldest bzr version that bzr-builder wants to work with?

Well, dapper is still "supported" so 0.8.2.

Really though we're probably only going to have hardy and later,
meaning 1.3.1.

Previously bzr-builder used very simple APIs, so this wasn't really an
issue. If we keep running in to this we can review our deployment and
perhaps use a newer bzr as well as a newer bzr-builder (given that the
plugin isn't even in hardy).

Thanks,

James

James Westby wrote:
> On Fri, 09 Jul 2010 00:49:38 -0000, Andrew Bennetts <email address hidden> wrote:
[...]
> Great. bzr-builder is easier.
>
> Do you think it should still go in bzr with a "backport" inside
> bzr-builder?

Yes, exactly.

> > Also, what's the oldest bzr version that bzr-builder wants to work with?
>
> Well, dapper is still "supported" so 0.8.2.

Ouch!

> Really though we're probably only going to have hardy and later,
> meaning 1.3.1.
>
> Previously bzr-builder used very simple APIs, so this wasn't really an
> issue. If we keep running in to this we can review our deployment and
> perhaps use a newer bzr as well as a newer bzr-builder (given that the
> plugin isn't even in hardy).

Considering that so many branches on Launchpad are 2a format now it
seems to me that bzr >= 2.0 is almost a requirement anyway. Please tell
me I'm right ;)

A compromise be to say "merge-part" is not available unless you
are using a new-enough bzr. (Again, probably >= 2.0 with the help of a
backport in bzr-builder.)

On Fri, 09 Jul 2010 05:25:53 -0000, Andrew Bennetts <email address hidden> wrote:
> Considering that so many branches on Launchpad are 2a format now it
> seems to me that bzr >= 2.0 is almost a requirement anyway. Please tell
> me I'm right ;)

That's a good point. It may be that the archive we are using to backport
bzr-builder already has a bzr in for other reasons, and so that is
getting picked up.

Checking...

It does not, so indeed the newer branch formats can't be built on older
releases.

Filed

  https://bugs.edge.launchpad.net/launchpad-code/+bug/603615

about this.

Depending on how we fix this we could then not put the new classes in to
bzr-builder.

> A compromise be to say "merge-part" is not available unless you
> are using a new-enough bzr. (Again, probably >= 2.0 with the help of a
> backport in bzr-builder.)

I don't fancy that, but it would be an option, yes.

Thanks,

James

I am looking forward to using this, but wonder why it used with branching ("merge") and not nesting ("nest")?

Andrew wrote:
> have a recipe that specifies "current trunk + packaging from $recent-packaging".

That's the most common use case and looks like "nesting", not merging.

As far as I understand the following will take of this / make it behave like "nest":
> I think I would like to see catching UnrelatedBranches, and then if
> a subdir is specified, try again with NULL_REVISION as the base.

Daniel: I agree, nest-part is probably a better description for users. It happens that the implementation in bzr reuses a bunch of merge infrastructure, but I think it's fair to say that's an implementation detail that is irrelevant to users. So I've updated the patch accordingly, thank you!

James: So what is the plan for dealing or not with older bzr versions for now? The relevant bzr change is with PQM for landing in 2.2 right now. I'll going resubmit this patch for merging without any backwards compat for bzr < 2.2 included, but if you do want it I can make an additional proposal to copy the MergeIntoMerge bits into bzr-builder. At this point I'm basically just asking for a clear "Yes, do that" or "No, don't bother" instruction and then I'll go and do as you say, the current plan is a bit unclear with bug 603615 not going anywhere atm.

(Because the trunk branch has changed since I first created this proposal I'm going to mark this one as rejected and create a new one. But I really just resubmitting this patch.)

The new merge proposal is at <https://code.edge.launchpad.net/~spiv/bzr-builder/merge-subdirs-479705/+merge/31251>. For some reason LP won't let me mark this proposal as Rejected, though :(