and make it work in the process.
Created attachment 44663 [details] Patch
style-queue ran check-webkit-style on attachment 44663 [details] without any errors.
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.
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.
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.
(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.
Comment on attachment 44663 [details] Patch I'm going to try again with smaller patches.
Simpler patch in Bug 32464.
Created attachment 44729 [details] Patch
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?)
Created attachment 44841 [details] Patch
Comment on attachment 44841 [details] Patch OK.
Committed r52130: <http://trac.webkit.org/changeset/52130>