Bug 32406 – [bzt] Convert rollout to StepSequence

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Adam Barth]

and make it work in the process.

[Adam Barth]

Created attachment 44663 [details]
Patch

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44663 [details] without any errors.

[Eric Seidel (no email)]

Comment on attachment 44663 [details]
Patch

I'm having trouble soaking in this patch.  I think I may either need to see you in person or see this in smaller hunks.

[Adam Barth]

Basically, instead of caching the patch object on the step, we carry a "state" dictionary along while running the steps.  Various steps can read or write this state dictionary as appropriate.

In this case, Rollout kicks adds the revision number to the state so that the ApplyReverseDiffStep can know what revision to revert.  This system also let's us carry the commit_text from CommitStep to ClosePatchStep, letting us remove the last piece of custom logic in the LandingSequence.

Finally, instead of making RolloutStep a one-off, I generalized it with MetaStep that lets you aggregate a bunch of smaller steps together into a bigger step.  This is needed because RolloutStep wants to check for --complete-rollout before building or committing.  We could have taught BuildStep and CommitStep about --complete-rollout, but that didn't seem to be the right layer of abstraction...

We can discuss further if you like.

[Eric Seidel (no email)]

Comment on attachment 44663 [details]
Patch

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 {}

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.  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 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

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.

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

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

[Adam Barth]

(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.

[Adam Barth]

Comment on attachment 44663 [details]
Patch

I'm going to try again with smaller patches.

[Adam Barth]

Simpler patch in Bug 32464.

[Adam Barth]

Created attachment 44729 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 44729 [details]
Patch

test_complete_rollout is clearly wrong.

We should just do:
from modules.buildsteps import *

If we want to restrict what * means, we can define a __ALL__ in buildsteps.py:
"The public names defined by a module are determined by checking the module’s namespace for a variable named __all__; if defined, it must be a sequence of strings which are names defined or imported by that module. The names given in __all__ are all considered public and are required to exist. If __all__ is not defined, the set of public names includes all names found in the module’s namespace which do not begin with an underscore character ('_'). __all__ should contain the entire public API. It is intended to avoid accidentally exporting items that are not part of the API (such as library modules which were imported and used within the module)."
http://docs.python.org/reference/simple_stmts.html#import

This is probably wrong, yes:
 355         self._tool.bugs.reopen_bug(bug_id, state["commit_text"])

it should be the bug link, whatever that is.  Do we need a step to convert from commit text output to revisions?  Or maybe we have a class to do that already...

This feels like this should be done by some StepSequence member:
 336         collected_options = cls._collect_options_from_steps(cls.substeps)
I remember you were explaining to me at some point why that was not possible?

Otherwise looks OK.  r- for the broken rollout test (which perhaps we should just remove?)

[Adam Barth]

Created attachment 44841 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 44841 [details]
Patch

OK.

[Adam Barth]

Committed r52130: <http://trac.webkit.org/changeset/52130>