CQ: Break out repo/path series application into a standalone... [chromiumos/chromite : master]
Brian Harring (Code Review)
Change subject: CQ: Break out repo/path series application into a standalone object.
......................................................................
CQ: Break out repo/path series application into a standalone object.
This is done so that
1) it's easier to actually test the machinery.
2) cbuildbot can make use of it.
3) to position the source such that future enhancements can be leveled-
better resolution, multiple runs, etc.
BUG=chromium-os:27415
TEST=cbuildbot x86-generic-full
TEST=run_tests.sh
Change-Id: I9b042c05f533eddef080949f1f1af30a77d4b4ff
---
M buildbot/validation_pool.py
M buildbot/validation_pool_unittest.py
2 files changed, 236 insertions(+), 173 deletions(-)
git pull ssh://gerrit.chromium.org:29418/chromiumos/chromite refs/changes/04/20504/1
--
To view, visit https://gerrit.chromium.org/gerrit/20504
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I9b042c05f533eddef080949f1f1af30a77d4b4ff
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: Brian Harring <ferr...@chromium.org>
Brian Harring (Code Review)
Change subject: CQ: Break out repo/path series application into a standalone object.
......................................................................
Patch Set 7: Verified
Ok folks... PTAL.
Should be good to go; as indicated, this rev shouldn't be achanging behaviour- just is code shuffling and rewiring.
--
To view, visit https://gerrit.chromium.org/gerrit/20504
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9b042c05f533eddef080949f1f1af30a77d4b4ff
Gerrit-PatchSet: 7
Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: Brian Harring <ferr...@chromium.org>
Gerrit-Reviewer: Brian Harring <ferr...@chromium.org>
Gerrit-Reviewer: Chris Sosa <so...@chromium.org>
Gerrit-Reviewer: David James <david...@chromium.org>
Brian Harring (Code Review)
Change subject: CQ: Break out repo/path series application into a standalone object.
......................................................................
Patch Set 9: Verified
Added some more docstrings, minor whitespace fix.
--
To view, visit https://gerrit.chromium.org/gerrit/20504
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9b042c05f533eddef080949f1f1af30a77d4b4ff
Gerrit-PatchSet: 9
Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: Brian Harring <ferr...@chromium.org>
Chris Sosa (Code Review)
Change subject: CQ: Break out repo/path series application into a standalone object.
......................................................................
Patch Set 9: I would prefer that you didn't submit this
(15 inline comments)
....................................................
File buildbot/validation_pool.py
Line 56: class HelperPool(object):
What is this helper pool? Can you please add an informative docstring?
Line 58: def __init__(self, internal=True, external=True):
Can you find better names for these variables? We use internal fairly consistently elsewhere to mean something different than this meaning. Perhaps enable_internal or disable_internal/external.
Line 69: if external == True:
if blah:
Line 88: raise Exception("Internal bug detected: change %s (internal? %r) asked "
Can you make a subclass of Exception for this.
Line 94: if helper is not None:
if helper
Line 106: if helper_pool is None:
if not helper_pool
Line 109: self.GetHelper = helper_pool.ForChange
I'd really rather avoid doing something like this. Please do so explicitly with a wrapper method
Line 115: Exception: If the appropriate GerritHelper could not be found.
This should be changed to reflect change away from Exception.
Line 134: True if we managed to apply any changes.
Please change returns to reflect what you return.
Line 137: # maintain ordering when applying changes.
Is this a direct move? Can you let me know explicitly what parts you changed here if any by adding Gerrit comments.
Line 265: Usage: Use ValidationPoo.AcquirePool -- a static
Can you modify docstring to reflect interaction with PatchSeries
Line 300: if helper_pool is None:
not helper_pool
Line 303: self._HelperFor = helper_pool.ForChange
Ditto with similiar comment about assigning methods
Line 325: def GetBuildDashboardForOverlays(overlays):
docstrings.
Line 532: applied, failed_tot, failed_inflight = \
\ unnecessary i think if you do the break at Apply(
--
To view, visit https://gerrit.chromium.org/gerrit/20504
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9b042c05f533eddef080949f1f1af30a77d4b4ff
Gerrit-PatchSet: 9
Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: Brian Harring <ferr...@chromium.org>
Brian Harring (Code Review)
Change subject: CQ: Break out repo/path series application into a standalone object.
......................................................................
Patch Set 9: (9 inline comments)
....................................................
File buildbot/validation_pool.py
Line 69: if external == True:
See the __init__ docs for internal/external; we explicitly allow external code to pass in their own instances (aka, unittests) so they can control/monitor the behaviour.
Doing as you suggest would disallow the unittests from having that sort of control. Documented this in __init__, but likely should expand it here.
Line 88: raise Exception("Internal bug detected: change %s (internal? %r) asked "
Frankly was more inclined to just level an assert there; that seems to be our norm.
Line 94: if helper is not None:
The only thing HelperPool knows is that it sets None if no instance is given; else it may generate a GerritHelper itself, may have a mock passed in, etc; specifically, it makes no assumptions of boolean protocol support of that object, hence being written this way.
So... that's why it's written this way- it's written explicitly to check the only assumption this code makes about the instance. And yes, that's being fairly paranoid, but it's basically muscle memory at this point to write logic checking only the assumptions we explicitly enforce.
Can change it, but figured I might as well explain the reasoning...
Line 106: if helper_pool is None:
No. Think through if someone screws up usage of this api, and passes in () for helper_pool.
Your version would silently convert that to an external/internal; my version makes it blow up.
I *want* the blow up if people write code that misuses the API.
Line 109: self.GetHelper = helper_pool.ForChange
Grumble. Happen to like that :/
Will use the less advanced version for these spots.
Line 137: # maintain ordering when applying changes.
Direct move, minus the decoupling. No logic changes- those come in a later rev.
The only behavioural changes that could occur for CQ is if it internally ran ApplyPoolIntoRepo multiple times (although I'm fairly sure it wouldn't quite behave if it did).
Line 265: Usage: Use ValidationPoo.AcquirePool -- a static
I could, but tbh I didn't since I view the PatchSeries as an internal detail- external code shouldn't know about it, nor care about it, should only make assumptions about the public api's we expose.
To that end I probably should make .patch_series ._patch_series.
Line 300: if helper_pool is None:
Same reasoning given for GerritHelper.__init__ .
When doing default arguments/fallbacks like this, one needs to be *extremely* explicit in the check else you can get bit in the ass pretty easily.
Consider the following prototype:
def DoSomeStuff(myargs, cache_override=None):
if not cache_override:
cache_override = self._internal_cache
self._ReallyDoStuff(myargs, cache_override)
It's subtle, but that block of code will incorrectly fallback to the internal cache if the passed in cache_override is {} (which may be entirely intentional- to force a run w/out using internal caches values).
If you use the 'is None', check, it handles it correctly. The only ass biter in scenarios like that is when you need to detect if None was explicitly passed in, in which case you use an object() singleton to detect it.
Line 532: applied, failed_tot, failed_inflight = \
Hmm?
To be clear, the point of this stub was to break out the logic of ApplyPoolIntoRepo into a contained/decoupled block, without breaking existing ValidationPool behaviour.
Longer term, this stub (and ValidationPool) should be refactored- right now the point of this patch was to carve out exactly what I needed to isolate for fixing CQ issues and getting cherry-pick deps sorted.
--
To view, visit https://gerrit.chromium.org/gerrit/20504
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9b042c05f533eddef080949f1f1af30a77d4b4ff
Gerrit-PatchSet: 9
Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: Brian Harring <ferr...@chromium.org>
Brian Harring (Code Review)
Change subject: CQ: Break out repo/path series application into a standalone object.
......................................................................
Patch Set 11: Verified
Cleaned up the bits that aren't in debate, flushed out docstrings (some of these are almost the same # of lines as the code :/), and generally polished it while rebasing it.
Possible I missed something, but the review issues that were valid should be addressed.
Finally, note that same as last time, nothing moved from ValidationPool.ApplyPoolIntoRepo into PatchSeries.Apply has changed- same code/logic, just now contained/decoupled in it's own object.
PTAL.
--
To view, visit https://gerrit.chromium.org/gerrit/20504
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9b042c05f533eddef080949f1f1af30a77d4b4ff
Gerrit-PatchSet: 11
Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: Brian Harring <ferr...@chromium.org>
Chris Sosa (Code Review)
Change subject: CQ: Break out repo/path series application into a standalone object.
......................................................................
Patch Set 9: (6 inline comments)
....................................................
File buildbot/validation_pool.py
Line 69: if external == True:
Ah I see the issue.
I don't like the idea of False, True, or Object. It tends not to work well and be used wrong by others trying to maintain the code.
You are forgetting that unittests can do magical things to give you the functionality you want in Python.
mock_helper = self.mox.CreateMock(helper)
p = HelperPool()
p._external = mock_helper
Remember referencing and setting private variables in unittests is a-ok style-wise for Python if it makes your implementation more clear.
Line 88: raise Exception("Internal bug detected: change %s (internal? %r) asked "
Assert it.
Line 94: if helper is not None:
I'm not that strong about this one. In general good python style dictates to keep things as short and clear as possible. If you strongly feel this is correct / true in this case I won't object :)
Line 109: self.GetHelper = helper_pool.ForChange
The key issue is you kill any help(validiation_pool) usage or any pydoc generating things as this method won't be documented/found.
I agree it's a nice short-hand but I prefer if we only leave those shorthands in unittests.
Line 265: Usage: Use ValidationPoo.AcquirePool -- a static
Hai.
Line 532: applied, failed_tot, failed_inflight = \
Alas you misread my comment. I explicitly meant avoid using the '\' character here :)
--
To view, visit https://gerrit.chromium.org/gerrit/20504
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9b042c05f533eddef080949f1f1af30a77d4b4ff
Gerrit-PatchSet: 9
Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: Brian Harring <ferr...@chromium.org>
David James (Code Review)
Change subject: CQ: Break out repo/path series application into a standalone object.
......................................................................
Patch Set 11: (11 inline comments)
....................................................
File buildbot/validation_pool.py
Line 88: (primarily useful for test code to pass in mock ups).
There should be no need for this -- pymox is actually fully capable of mucking with the helper without adding an API for this.
If you are going to add an API, no need to use the True/False/object API -- you can split up into multiple parameters.
Line 112: raise NoHelperAvailable(
As sosa pointed out, assert is probably enough for this. That kills the need for the NoHelperAvailable class.
Line 212: apply_chain = False
Why are you setting apply_chain = False ? It looks like it's not ever used in this case...
Line 220: if not helper.IsChangeCommitted(dep, must_match=False):
Any chance we could set must_match=True ? If the change doesn't exist, it's a bit misleading to say it's not ready to be committed... it would be a lot better to catch "QueryHasNoResults" and return a specific error message in that case.
Line 231: '%s matching multiple branches.' % (change.id, dep))
This can also happen if it matches multiple projects.
Line 251: elif change in changes_that_failed_to_apply_to_tot:
What if change is in changes_that_failed_to_apply_against_other_changes ? (E.g. if we encountered the same change twice in the change stack)
Would be weird to report 2 errors for the same patch in a single run, or, worse, to fail and succeed in a single run...
Line 256: trivial = False if dryrun else not self.IsContentMerging(change)
Everything above here can move up, above the try, right? It's cleaner to keep unnecessary stuff out of the try.
Line 279: self.failed.extend(changes_that_failed_to_apply_to_tot)
What is self.applied and self.failed intended to be used for? I don't see these variables used anywhere in your change.
Line 291: Usage: Use ValidationPoo.AcquirePool -- a static
s/ValidationPoo/ValidationPool/
Line 359: if overlays in [constants.PUBLIC_OVERLAYS, constants.BOTH_OVERLAYS, False]:
False is not a valid value for overlays, is it? push_overlays can be None, but overlays needs to be one of the valid overlays ('both', 'public', or 'private')
....................................................
File buildbot/validation_pool_unittest.py
Line 63: def SetPoolsContentMergingProjects(pool, *projects):
pool is unused now -- time to remove the arg?
--
To view, visit https://gerrit.chromium.org/gerrit/20504
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9b042c05f533eddef080949f1f1af30a77d4b4ff
Gerrit-PatchSet: 11
Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: Brian Harring <ferr...@chromium.org>
David James (Code Review)
Change subject: CQ: Break out repo/path series application into a standalone object.
......................................................................
Patch Set 11: Fails
This fails with the following error message for me:
cbuildbot: Unhandled exception:
Traceback (most recent call last):
File "./cbuildbot", line 67, in <module>
ret = target()
File "/usr/local/google2/chrome-internal/chromite/scripts/cbuildbot.py", line 1029, in main
_RunBuildStagesWrapper(options, build_config)
File "/usr/lib/python2.6/contextlib.py", line 34, in __exit__
self.gen.throw(type, value, traceback)
File "/usr/local/google2/chrome-internal/chromite/lib/cros_build_lib.py", line 1130, in NoOpContextManager
yield
File "/usr/local/google2/chrome-internal/chromite/scripts/cbuildbot.py", line 1029, in main
_RunBuildStagesWrapper(options, build_config)
File "/usr/local/google2/chrome-internal/chromite/scripts/cbuildbot.py", line 561, in _RunBuildStagesWrapper
if not buildbot.Run():
File "/usr/local/google2/chrome-internal/chromite/scripts/cbuildbot.py", line 316, in Run
success = self._ReExecuteInBuildroot(sync_instance)
File "/usr/local/google2/chrome-internal/chromite/scripts/cbuildbot.py", line 287, in _ReExecuteInBuildroot
vp_file = sync_instance.SaveValidationPool()
File "/usr/local/google2/chrome-internal/chromite/buildbot/cbuildbot_stages.py", line 410, in SaveValidationPool
cPickle.dump(self.pool, p_file, protocol=cPickle.HIGHEST_PROTOCOL)
cPickle.PicklingError: Can't pickle <type 'instancemethod'>: attribute lookup __builtin__.instancemethod failed
Command-line: ./cbuildbot x86-generic-paladin -p chromiumos/chromite -r /usr/local/google/trybot --buildbot --debug
--
To view, visit https://gerrit.chromium.org/gerrit/20504
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9b042c05f533eddef080949f1f1af30a77d4b4ff
Gerrit-PatchSet: 11
Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: Brian Harring <ferr...@chromium.org>
Brian Harring (Code Review)
Change subject: CQ: Break out repo/path series application into a standalone object.
......................................................................
Patch Set 11: Fails
(7 inline comments)
Noted the failure spot, and recorded the results of a bit of IM conversation, while addressing remaining comments/issues.
Refresh sometime tonight/morn.
....................................................
File buildbot/validation_pool.py
Line 88: (primarily useful for test code to pass in mock ups).
Per IM conversation, recording the reasoning this was done here.
I *intentionally* did it this way despite knowing pymox api's; usual pymox usage here results in stubbing the class. I want (and specifically will need down the line for full internal/external query tests) control over each instance. Thus class level stubbing does not fly (nor should it be used in the other scenarios where this occurs, even if it is, for the same reasons I gave- the point is to ensure it accesses the right *instance*, not just *a* instance).
So... I can make the tests reach in and drop a mock in- this however means we're not actually testing the proper ValidationPool -> PatchSeries propagation of GerritHelpers (which we need to have coverage on for sane internal/external testing, as mention). Ok, reach in, know the exact pathway w/in this instance, and stub the instance manually, each test.
Beyond being nasty to do at each test case, when you're at that point, it's better to just treat it as a proper unittest and pass the sucker done- and is better to design API's that way so they're actually testable (rather than the semi-mox encouraged "stub random shit in the environment eliminating large swaths of code coverage in the process"). Specifically, we have to pass it down through ValidationPool anyways (to do checks on the dashboard handling, and general querying), thus it's better to write the code so it's testable in that fashion, which is what I did here.
The API sucks a bit, which I'll address via using classmethods; that said, still will be passing Helpers in from ValidationPool (meaning threading the mock's down through it) to keep the coverage the current form has.
Line 212: apply_chain = False
Per commit/comments, this is (or at least *should* be) a direct move of the existing ApplyPoolIntoRepo code; intentionally not touching anything in that move, just decoupling that code from the surrounding ValidationPool logic.
All issues reported in this block of code are addressed in full in the next rev (https://gerrit.chromium.org/gerrit/#change,20811, although to be clear I need to refresh that rev).
Line 279: self.failed.extend(changes_that_failed_to_apply_to_tot)
Pardon, should've left a comment in this particular rev explaining it; they're state tracking for the instance.
In this rev, it doesn't do anything- next rev, it allows for multiple Apply invocations via remembering its state across calls.
Will attempt to pull that from this change in the next refresh.
Line 291: Usage: Use ValidationPoo.AcquirePool -- a static
Heh. I like that name personally.
Will sort it in the next refresh.
Line 326: self._HelperFor = helper_pool.ForChange
This is what caused the pickling failure david reported; it's left over cruft from refactoring of this, isn't intended to have been in there.
Line 359: if overlays in [constants.PUBLIC_OVERLAYS, constants.BOTH_OVERLAYS, False]:
Nfc tbh; this logic I didn't modify at all, purely pulled it out of the previous inlining w/in __init__, putting it into staticmethods so the classmethods can use it.
The original spot for this code s on the left at line #114; while it's undergone a superficial change (namely the addition of HelperPool), the logic itself is exactly the same, including the resultant GerritHelpers configured.
@Sosa: any comment?
....................................................
File buildbot/validation_pool_unittest.py
Line 63: def SetPoolsContentMergingProjects(pool, *projects):
Rather wire it properly manipulates the appropriate GerritHelper (this version as it is now makes it very hard to properly control/test internal vs external helpers).
--
To view, visit https://gerrit.chromium.org/gerrit/20504
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9b042c05f533eddef080949f1f1af30a77d4b4ff
Gerrit-PatchSet: 11
Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: Brian Harring <ferr...@chromium.org>
Chris Sosa (Code Review)
Change subject: CQ: Break out repo/path series application into a standalone object.
......................................................................
....................................................
File buildbot/validation_pool.py
Line 74: """Raised when a change asks for a helper, but the request helper
Bad docstring style.
Line 88: (primarily useful for test code to pass in mock ups).
Line 291: Usage: Use ValidationPoo.AcquirePool -- a static
Line 359: if overlays in [constants.PUBLIC_OVERLAYS, constants.BOTH_OVERLAYS, False]:
--
To view, visit https://gerrit.chromium.org/gerrit/20504
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Gerrit-Change-Id: I9b042c05f533eddef080949f1f1af30a77d4b4ff
Gerrit-PatchSet: 11
Gerrit-Branch: master
Gerrit-Owner: Brian Harring <ferr...@chromium.org>
Brian Harring (Code Review)
Change subject: CQ: Break out repo/path series application into a standalone object.
......................................................................
All nits addressed as of patchset 17
....................................................
File buildbot/validation_pool.py
The reason I'm being a major stick in the mud here is that of just plain different approaches to testing- mox is fine, but we use it entirely too much to suppress pathways where code should actually be executed as part of the tests.
Thus the preference to explicitly pass down objects, ensuring it's getting what we want. I could stub out the parts of GerritHelper I care about (at the instance level, since class level makes it impossible to track internal vs external access), but in doing so the rest of GerritHelper's api's are still accessible.
I could make the tests know and reach in and suppress the specific attribute, but in doing so the test winds up w/ knowledge about the code that is undesirable- undesirable in that it makes the tests more likely to break for code shift (the tests should break when API gurantees or functionality are broken, not when one changes an internal private var).
So... that's why I'm being stubborn.
Line 112: raise NoHelperAvailable(
Done.
Note the current refactoring can't induce it, but part of the reason an exception seems better is that I have a sneaking suspicion at some point in the future a unified CQ will manage to trigger this, specifically doing so live.
Catching AssertionError sucks a bit... will deal with it when it shows however.
Line 220: if not helper.IsChangeCommitted(dep, must_match=False):
Basically, those issues will be addressed, but this isn't the rev for it (else the resultant rev turns into a >1kloc monstrosity).
Line 279: self.failed.extend(changes_that_failed_to_apply_to_tot)
addressed in the new rev; specifically dropped the attributes since they're relevant only for the next patch.
Line 291: Usage: Use ValidationPoo.AcquirePool -- a static
Line 359: if overlays in [constants.PUBLIC_OVERLAYS, constants.BOTH_OVERLAYS, False]:
Plus, I'd rather that go in separate so that if it's fine it stays, or if it needs reversion, it gets pulled (just that), versus if there are issues in this CL (unlikely, but potential) resulting in the change you suggested being reverted along w/ it.
--
To view, visit https://gerrit.chromium.org/gerrit/20504
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Gerrit-Change-Id: I9b042c05f533eddef080949f1f1af30a77d4b4ff
Gerrit-PatchSet: 11
Gerrit-Branch: master
Gerrit-Owner: Brian Harring <ferr...@chromium.org>
Brian Harring (Code Review)
......................................................................
PTAL.
--
To view, visit https://gerrit.chromium.org/gerrit/20504
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Gerrit-Change-Id: I9b042c05f533eddef080949f1f1af30a77d4b4ff
Gerrit-PatchSet: 17
Gerrit-Branch: master
Gerrit-Owner: Brian Harring <ferr...@chromium.org>
David James (Code Review)
Change subject: CQ: Break out repo/path series application into a standalone object.
......................................................................
(1 inline comment)
Can you test with --buildbot --debug next time?
Here's the failure I get locally:
$ ./cbuildbot x86-generic-paladin -r /usr/local/google/trybot --buildbot --debug --noclean
self._PerformStage()
File "/usr/local/google2/chrome-internal/chromite/buildbot/cbuildbot_stages.py", line 489, in _PerformStage
ManifestVersionedSyncStage._PerformStage(self)
File "/usr/local/google2/chrome-internal/chromite/buildbot/cbuildbot_stages.py", line 306, in _PerformStage
next_manifest = self.GetNextManifest()
File "/usr/local/google2/chrome-internal/chromite/buildbot/cbuildbot_stages.py", line 452, in GetNextManifest
self._options.buildnumber, self.builder_name, self._options.debug)
File "/usr/local/google2/chrome-internal/chromite/buildbot/validation_pool.py", line 456, in AcquirePool
raw_changes = helper.GrabChangesReadyForCommit()
AttributeError: 'bool' object has no attribute 'GrabChangesReadyForCommit'
....................................................
File buildbot/validation_pool.py
Line 456: raw_changes = helper.GrabChangesReadyForCommit()
File "/usr/local/google2/chrome-internal/chromite/buildbot/builderstage.py", line 243, in Run
self._PerformStage()
File "/usr/local/google2/chrome-internal/chromite/buildbot/cbuildbot_stages.py", line 489, in _PerformStage
ManifestVersionedSyncStage._PerformStage(self)
File "/usr/local/google2/chrome-internal/chromite/buildbot/cbuildbot_stages.py", line 306, in _PerformStage
next_manifest = self.GetNextManifest()
File "/usr/local/google2/chrome-internal/chromite/buildbot/cbuildbot_stages.py", line 452, in GetNextManifest
self._options.buildnumber, self.builder_name, self._options.debug)
File "/usr/local/google2/chrome-internal/chromite/buildbot/validation_pool.py", line 456, in AcquirePool
raw_changes = helper.GrabChangesReadyForCommit()
AttributeError: 'bool' object has no attribute 'GrabChangesReadyForCommit'
--
To view, visit https://gerrit.chromium.org/gerrit/20504
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Gerrit-Change-Id: I9b042c05f533eddef080949f1f1af30a77d4b4ff
Gerrit-PatchSet: 17
Gerrit-Branch: master
Gerrit-Owner: Brian Harring <ferr...@chromium.org>
Brian Harring (Code Review)
Change subject: CQ: Break out repo/path series application into a standalone object.
......................................................................
I can't make it fail, nor am I seeing issues in looking this over (yet again).
David, your turn to manage it in a single well timed run.
--
To view, visit https://gerrit.chromium.org/gerrit/20504
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Gerrit-Change-Id: I9b042c05f533eddef080949f1f1af30a77d4b4ff
Gerrit-PatchSet: 18
Gerrit-Branch: master
Gerrit-Owner: Brian Harring <ferr...@chromium.org>
Chris Sosa (Code Review)
Change subject: CQ: Break out repo/path series application into a standalone object.
......................................................................
(5 inline comments)
Just nits
....................................................
File buildbot/validation_pool.py
Line 73: class HelperPool(object):
Seems strange to call this a HelperPool as I think it overloads the term pool a bit with the ValidationPool. I can't think of a better name atm though. Perhaps HelperInterface?
Line 102:
Remove extra line.
Line 108: If no helper is configured, an Exception is raised.
Document the type of exception?
Line 361:
Seems strange to have a line break here but not after external = True
Line 552: def ApplyPoolIntoRepo(self, buildroot):
Docstring for all public methods.
--
To view, visit https://gerrit.chromium.org/gerrit/20504
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Gerrit-Change-Id: I9b042c05f533eddef080949f1f1af30a77d4b4ff
Gerrit-PatchSet: 18
Gerrit-Branch: master
Gerrit-Owner: Brian Harring <ferr...@chromium.org>
Brian Harring (Code Review)
Change subject: CQ: Break out repo/path series application into a standalone object.
......................................................................
nits will be sorted in the likely inevitable next patch (or if this patch stops hating me, the inherited LGTM).
Class name, have a few thoughts in it- at this point I'm inclined to keep the beast mostly in it's current form, sans a rename, mainly since cross gerrit deps I'm still a bit unsure of at the guts level (how best to do it while ensuring it CQ doesn't try submitting to a gerrit instance it isn't allow to).
....................................................
File buildbot/validation_pool.py
Line 73: class HelperPool(object):
A better name is AllowedGerritHelpers I suspect.
--
To view, visit https://gerrit.chromium.org/gerrit/20504
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Gerrit-Change-Id: I9b042c05f533eddef080949f1f1af30a77d4b4ff
Gerrit-PatchSet: 18
Gerrit-Branch: master
Gerrit-Owner: Brian Harring <ferr...@chromium.org>
David James (Code Review)
Change subject: CQ: Break out repo/path series application into a standalone object.
......................................................................
Still fails immediately, but with a different message. (I ran the same command I used last time.)
$ ./cbuildbot x86-generic-paladin -r /usr/local/google/trybot --buildbot --debug --noclean
cbuildbot: Unhandled exception:
ret = target()
File "/usr/local/google/trybot/chromite/scripts/cbuildbot.py", line 1119, in main
File "/usr/lib/python2.6/contextlib.py", line 34, in __exit__
self.gen.throw(type, value, traceback)
yield
File "/usr/local/google/trybot/chromite/scripts/cbuildbot.py", line 1119, in main
_RunBuildStagesWrapper(options, build_config)
File "/usr/local/google/trybot/chromite/scripts/cbuildbot.py", line 573, in _RunBuildStagesWrapper
if not buildbot.Run():
File "/usr/local/google/trybot/chromite/scripts/cbuildbot.py", line 306, in Run
sync_instance.Run()
File "/usr/local/google/trybot/chromite/buildbot/builderstage.py", line 230, in Run
self.HandleSkip()
File "/usr/local/google/trybot/chromite/buildbot/cbuildbot_stages.py", line 424, in HandleSkip
self.LoadValidationPool(self._options.validation_pool)
File "/usr/local/google/trybot/chromite/buildbot/cbuildbot_stages.py", line 418, in LoadValidationPool
CommitQueueSyncStage.pool = cPickle.load(p_file)
AttributeError: 'module' object has no attribute 'HelperPool'
--
To view, visit https://gerrit.chromium.org/gerrit/20504
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Gerrit-Change-Id: I9b042c05f533eddef080949f1f1af30a77d4b4ff
Gerrit-PatchSet: 18
Gerrit-Branch: master
Gerrit-Owner: Brian Harring <ferr...@chromium.org>
Brian Harring (Code Review)
Change subject: CQ: Break out repo/path series application into a standalone object.
......................................................................
drop the --noclean (not a bug in the CL, you're just not testing in a safe manner).
--
To view, visit https://gerrit.chromium.org/gerrit/20504
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Gerrit-Change-Id: I9b042c05f533eddef080949f1f1af30a77d4b4ff
Gerrit-PatchSet: 18
Gerrit-Branch: master
Gerrit-Owner: Brian Harring <ferr...@chromium.org>
David James (Code Review)
Change subject: CQ: Break out repo/path series application into a standalone object.
......................................................................
(2 inline comments)
....................................................
File buildbot/validation_pool.py
Line 74:
Line 78: """Initialize this instance w/ the given handlers.
s{w/}{with}g
--
To view, visit https://gerrit.chromium.org/gerrit/20504
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Gerrit-Change-Id: I9b042c05f533eddef080949f1f1af30a77d4b4ff
Gerrit-PatchSet: 18
Gerrit-Branch: master
Gerrit-Owner: Brian Harring <ferr...@chromium.org>
Brian Harring (Code Review)
Change subject: CQ: Break out repo/path series application into a standalone object.
......................................................................
....................................................
File buildbot/validation_pool.py
--
To view, visit https://gerrit.chromium.org/gerrit/20504
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Gerrit-Change-Id: I9b042c05f533eddef080949f1f1af30a77d4b4ff
Gerrit-PatchSet: 18
Gerrit-Branch: master
Gerrit-Owner: Brian Harring <ferr...@chromium.org>
David James (Code Review)
Change subject: CQ: Break out repo/path series application into a standalone object.
......................................................................
(1 inline comment)
....................................................
File buildbot/validation_pool.py
Line 127:
Still got an extra newline here.
--
To view, visit https://gerrit.chromium.org/gerrit/20504
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Gerrit-Change-Id: I9b042c05f533eddef080949f1f1af30a77d4b4ff
Gerrit-PatchSet: 19
Gerrit-Branch: master
Gerrit-Owner: Brian Harring <ferr...@chromium.org>
Brian Harring (Code Review)
Change subject: CQ: Break out repo/path series application into a standalone object.
......................................................................
(1 inline comment)
....................................................
File buildbot/validation_pool.py
Line 127:
--
To view, visit https://gerrit.chromium.org/gerrit/20504
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Gerrit-Change-Id: I9b042c05f533eddef080949f1f1af30a77d4b4ff
Gerrit-PatchSet: 19
Gerrit-Branch: master
Gerrit-Owner: Brian Harring <ferr...@chromium.org>
Brian Harring (Code Review)
......................................................................
Bug smoked out by CQ http://build.chromium.org/p/chromiumos/builders/x86%20generic%20paladin/builds/4168/steps/CommitQueueCompletion/logs/stdio
--
To view, visit https://gerrit.chromium.org/gerrit/20504
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Gerrit-Change-Id: I9b042c05f533eddef080949f1f1af30a77d4b4ff
Gerrit-PatchSet: 19
Gerrit-Branch: master
Gerrit-Owner: Brian Harring <ferr...@chromium.org>
Brian Harring (Code Review)
......................................................................
Rebased to the new pickling method, and explicitly passing _helper_pool down (that wasn't the cause of the previous issue, but is being done for correctness reasons).
inheriting lgtm...
--
To view, visit https://gerrit.chromium.org/gerrit/20504
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Gerrit-Change-Id: I9b042c05f533eddef080949f1f1af30a77d4b4ff
Gerrit-PatchSet: 20
Gerrit-Branch: master
Gerrit-Owner: Brian Harring <ferr...@chromium.org>
Brian Harring (Code Review)
......................................................................
--
To view, visit https://gerrit.chromium.org/gerrit/20504
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Gerrit-Change-Id: I9b042c05f533eddef080949f1f1af30a77d4b4ff
Gerrit-PatchSet: 20
Gerrit-Branch: master
Gerrit-Owner: Brian Harring <ferr...@chromium.org>
David James (Code Review)
Change subject: CQ: Break out repo/path series application into a standalone object.
......................................................................
(2 inline comments)
....................................................
File buildbot/validation_pool.py
Don't you want a comma here, not a period?
Line 374: self._helper_pool))
W0212:373:ValidationPool.__reduce__: Access to a protected member _helper_pool of a client class
David James (Code Review)
Change subject: CQ: Break out repo/path series application into a standalone object.
......................................................................
Could you test with --buildbot --debug prior to marking verified?
....................................................
File buildbot/validation_pool.py
Line 373: self.changes_that_failed_to_apply_earlier.
cbuildbot: Unhandled exception:
Traceback (most recent call last):
ret = target()
File "/usr/local/google2/chrome-internal/chromite/scripts/cbuildbot.py", line 1119, in main
File "/usr/lib/python2.6/contextlib.py", line 34, in __exit__
self.gen.throw(type, value, traceback)
yield
File "/usr/local/google2/chrome-internal/chromite/scripts/cbuildbot.py", line 1119, in main
_RunBuildStagesWrapper(options, build_config)
File "/usr/local/google2/chrome-internal/chromite/scripts/cbuildbot.py", line 573, in _RunBuildStagesWrapper
if not buildbot.Run():
success = self._ReExecuteInBuildroot(sync_instance)
File "/usr/local/google2/chrome-internal/chromite/scripts/cbuildbot.py", line 287, in _ReExecuteInBuildroot
vp_file = sync_instance.SaveValidationPool()
cPickle.dump(self.pool, p_file, protocol=cPickle.HIGHEST_PROTOCOL)
File "/usr/local/google2/chrome-internal/chromite/buildbot/validation_pool.py", line 373, in __reduce__
self.changes_that_failed_to_apply_earlier.
AttributeError: 'list' object has no attribute 'self'
Brian Harring (Code Review)
Change subject: CQ: Break out repo/path series application into a standalone object.
......................................................................
Patchset19 exposed a separate CQ bug in the pickling pathways; that was fixed via Iaa68e523 (and tested via I9b3b8c6f), the rebasing is what led to the breakage of 20 (one liner typo, annoyingly the __reduce__ modification wasn't even necessary).
Suckers been tested, works, tests pass (issue in the pickling tests was spotted that can only be smoked out via pickle versions changes in HEAD vs tot); compatibility code for the pickling tests was dropped since it's no longer necessary.
Intending on landing this since it's a fat chunk of code I'm sick of lugging around for bits later in the series...
--
To view, visit https://gerrit.chromium.org/gerrit/20504
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9b042c05f533eddef080949f1f1af30a77d4b4ff
David James (Code Review)
Change subject: CQ: Break out repo/path series application into a standalone object.
......................................................................
(5 inline comments)
....................................................
File buildbot/validation_pool.py
Line 110: elif self._external:
Any reason why you're testing self._external here, and self._external is None on line 120? Pick your favorite form... no reason to use both forms.
....................................................
File buildbot/validation_pool_unittest.py
Line 371: internal=False))
Shouldn't this be None, not False?
Line 395: internal=False))
Shouldn't this be None, not False?
Line 566: '%s/chromiumos/chromite' % constants.GIT_HTTP_URL,
Maybe the CloneGitRepo function is broken, but it doesn't clone 'master' for me -- it just clones my local branch to the tmpdir, hence not really doing what it says.
If I os._exit(1) after this and look in the tmpdir, it's got my local commits in there -- oops?
Line 569: env = os.environ.copy()
This is unused... you're modifying it right below, but it's not passed anywhere...
David James (Code Review)
Change subject: CQ: Break out repo/path series application into a standalone object.
......................................................................
....................................................
File buildbot/validation_pool_unittest.py
Brian Harring (Code Review)
Change subject: CQ: Break out repo/path series application into a standalone object.
......................................................................
Nits sorted, tests still passing (including manual validation).
--
To view, visit https://gerrit.chromium.org/gerrit/20504
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9b042c05f533eddef080949f1f1af30a77d4b4ff
David James (Code Review)
Change subject: CQ: Break out repo/path series application into a standalone object.
......................................................................
Brian Harring (Code Review)
Change subject: CQ: Break out repo/path series application into a standalone object.
......................................................................
Brian Harring (Code Review)
Change subject: CQ: Break out repo/path series application into a standalone object.
......................................................................
Brian Harring (Code Review)
Change subject: CQ: Break out repo/path series application into a standalone object.
......................................................................
Flushed a bug via CQ for this during the submission phase (only triggerable in a live run); logic inversion, corrected:
http://build.chromium.org/p/chromiumos/builders/x86%20generic%20paladin/builds/4492/steps/CommitQueueCompletion/logs/stdio
Inheriting, but will land a bit later (during off hours specifically should this flush anything else)...
--
To view, visit https://gerrit.chromium.org/gerrit/20504
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9b042c05f533eddef080949f1f1af30a77d4b4ff
Brian Harring (Code Review)
Change subject: CQ: Break out repo/path series application into a standalone object.
......................................................................