[Webkit-unassigned] [Bug 32406] [bzt] Convert rollout to StepSequence

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=32406





--- Comment #6 from Adam Barth <abarth at webkit.org>  2009-12-11 15:51:53 PST ---
(In reply to comment #5)
> (From update of attachment 44663 [details])
> Why is {} necessary here?  it seems wrong:
>  270         EnsureBuildersAreGreenStep(tool, options).run({})
> 
> run should have a default arguemtn of None, and then if passed None should use
> a {}

This is just an artifact of the transition to steps.  I don't think it's worth
optimizing for calling the steps individually.  None should do that.  The code
that checks for None and makes a new dictionary is in StepSequence, who should
be the only one running steps.

> It seems that would require step to have a base-class "run" wrapper which knew
> how to call some _run_internal with the proper args.  not sure.

Right, that's StepSequence.

> I also wonder
> if the various run methods shouldn't return some dictionary instead, which the
> wrapping run() call could use to update the existing dictionary.  That makes it
> easier to test the inputs/outputs of the various run internal functions, no?

I'm not sure it matters if we return the new state or side-effect the
parameter.  How would we merge the two if we used the return formulation? 
Adding and overwriting is easy, but how could you delete (i.e., consume) a
parameter?

> I shoudl fix this:
> log("\nNOTE: Rollout support is experimental.\nPlease verify the rollout diff
> and use \"bugzilla-tool land-diff %s\" to commit the rollout." % bug_id)
> it's actually better to just re-run rollout with --complete-rollout and
> --force-clean

Sure, but we can do that in a separate patch.  Nothing in this patch affects
that.

> I think this whole thing could have been done in smaller pieces, by first
> moving some of the Rollout stuff into steps, and then dealing with the separate
> state question.

The problem is that rollout operates on a revision.  I can change thing to use
state and then do Rollout in a separate patch.  That might be easier to review.

> I think MetaStep is useful.  StepSequence could be written on top of it, or
> MetaStep could use StepSequence.  Either way they should share code.

Yes.  I added a FIXME to this effect.  I couldn't quite figure out the relation
between them, but they're clearly related.

> I think it makes the most sense for MetaStep to just have a step sequence as an
> member and use that.

We can do that, but StepSequence doesn't expose the AbstractStep API, which
MetaStep needs to in order to actually be a step.  It doesn't need the
run_and_handle_errors method, which is really why StepSequence exists...  I'm
inclined to do a few more conversions and hope that the right answer reveals
itself.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.