[webkit-reviews] review denied: [Bug 31744] BuildQueue should check if the tree is currently buildable : [Attachment 43612] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Adam Barth <abarth at webkit.org>'s
request for review:
Bug 31744: BuildQueue should check if the tree is currently buildable
https://bugs.webkit.org/show_bug.cgi?id=31744

Attachment 43612: Patch
https://bugs.webkit.org/attachment.cgi?id=43612&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
"clean" build is overloaded.  Not what you want.

Probably just "build"

 152	     Command.__init__(self, "Updates working copy and does a clean
build.", "tt", options)


 790		 self.run_bugzilla_tool(["clean-build", self.port.flag(),
"--force-clean", "--quiet", "--ignore-builders"])

shouldn't need --ignore builders, no?  why would "build" ever look at builders?


I'm not sure that build-attachment needs to look at builders either.  Maybe it
should.  Although it seems like you don't really want "build-attachment" here
so much as you want an apply step, followed by a build step.  We seem to be
conflating two problems.  1. the need to run in a separate process.  2.  the
want to control whcih specific steps are run.  Let's talk in person.

error is evil!
 39 from modules.logging import error, log, tee

There is already a --no-update flag elsewhere:
 81		make_option("--no-update", action="store_false", dest="update",
default=True, help="Do not update the working copy."),

perhaps we should just copy or move that one?