Submit non-manifest changes in the right order. [chromiumos/chromite : master]

[Matt Tennant (Code Review)]

Matt Tennant has posted comments on this change.

Change subject: Submit non-manifest changes in the right order.
......................................................................


Patch Set 1: I would prefer that you didn't submit this

(2 inline comments)

....................................................
File buildbot/validation_pool.py
Line 240: return self.manifest.GetProjectPath(change.project, True)
I take it that it's ok if this method can return None now?


Line 432: return list(digraph.StronglyConnectedComponents(list(edges), edges)), failed
As we discussed in person, there still needs to be some kind of sorting of the disjointed transactions to make sure the submit order is valid. The digraph algorithm won't do that sorting.


--
To view, visit https://gerrit.chromium.org/gerrit/56325
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1b33b8c31705e5b48590472155f36695795a995d
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: David James <david...@chromium.org>
Gerrit-Reviewer: ChromeBot <chrom...@google.com>
Gerrit-Reviewer: Matt Tennant <mten...@chromium.org>
Gerrit-Reviewer: Mike Frysinger <vap...@chromium.org>

[David James (Code Review)]

David James has posted comments on this change.

Change subject: Submit non-manifest changes in the right order.
......................................................................

Patch Set 1: (2 inline comments)

....................................................
File buildbot/validation_pool.py
Line 240: return self.manifest.GetProjectPath(change.project, True)

Yes. Added a docstring to clarify.

Line 432: return list(digraph.StronglyConnectedComponents(list(edges), edges)), failed

Good catch. I found this in my testing but I couldn't figure out why until we discussed this.

[Matt Tennant (Code Review)]

Matt Tennant has posted comments on this change.

Change subject: Submit non-manifest changes in the right order.
......................................................................

Patch Set 2: (2 inline comments)

Let's clarify some variables and I'll review again.

....................................................
File buildbot/validation_pool.py
Line 446: groups = list(digraph.StronglyConnectedComponents(list(edges), edges))
The variable naming is confusing me. So now "groups" is what "plans" used to be, meaning they're clearly the same concept. The only distinction here is that one is ordered and one is not.

How about using "unordered_plans" instead of "groups" to illustrate the relationship?


Line 453: for x in deps[change]:
Can we use "dep" or "dep_plan" instead of "x"?

(Is "x" a "plan"? I think so, but I'm not positive. Then later it gets appended to the variable plan, which is a list... so again I'm not sure. Variable consistency would help here)

Is "x" assumed to also be its own entry in "groups"?

--
To view, visit https://gerrit.chromium.org/gerrit/56325
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1b33b8c31705e5b48590472155f36695795a995d

Gerrit-PatchSet: 2

[David James (Code Review)]

David James has posted comments on this change.

Change subject: Submit non-manifest changes in the right order.
......................................................................


Patch Set 2: (2 inline comments)

....................................................
File buildbot/validation_pool.py
Line 446: groups = list(digraph.StronglyConnectedComponents(list(edges), edges))

Done

Line 453: for x in deps[change]:

Done

[Matt Tennant (Code Review)]

Matt Tennant has posted comments on this change.

Change subject: Submit non-manifest changes in the right order.
......................................................................

Patch Set 3: Looks good to me, approved

(3 inline comments)

I believe this makes sense to me now. A couple comment nits, and a unittest nit that you can add if you want.

....................................................
File buildbot/validation_pool.py
Line 440: # Save off the dependencies of this change.
Put the word "ordered" in this comment.


Line 455: for change2 in deps[change]:
ah, I see. ok, how about s/change2/change_dep/

And put a comment above or below to say how this loop goes over the required CLs for the target "change" in dependency order.


....................................................
Commit Message
Line 20: TEST=Example Pre-CQ Launcher run with 2 disjoint patch series.
How about a unittest that replicates this scenario?

--
To view, visit https://gerrit.chromium.org/gerrit/56325
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1b33b8c31705e5b48590472155f36695795a995d

Gerrit-PatchSet: 3

[Mike Frysinger (Code Review)]

Mike Frysinger has posted comments on this change.

Change subject: Submit non-manifest changes in the right order.
......................................................................

Patch Set 3: Looks good to me, but someone else must approve

(2 inline comments)

....................................................
File buildbot/validation_pool.py

Line 235: Returns the branch if this project has a manifest. Otherwise returns None.
the proper format is:

Returns:
The branch if this project has a manifest. Otherwise returns None.


Line 247: Returns the project path if found. Returns None if the specified change is
same here

[David James (Code Review)]

David James has posted comments on this change.

Change subject: Submit non-manifest changes in the right order.
......................................................................

Patch Set 5: Looks good to me, approved; Ready; Verified

--
To view, visit https://gerrit.chromium.org/gerrit/56325
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1b33b8c31705e5b48590472155f36695795a995d

Gerrit-PatchSet: 5

Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: David James <david...@chromium.org>
Gerrit-Reviewer: ChromeBot <chrom...@google.com>

Gerrit-Reviewer: David James <david...@chromium.org>

[David James (Code Review)]

David James has posted comments on this change.

Change subject: Submit non-manifest changes in the right order.
......................................................................

Patch Set 3: (5 inline comments)

....................................................
File buildbot/validation_pool.py
Line 235: Returns the branch if this project has a manifest. Otherwise returns None.

Done

Line 247: Returns the project path if found. Returns None if the specified change is

Done

Line 440: # Save off the dependencies of this change.

Done

Line 455: for change2 in deps[change]:

Done

....................................................
Commit Message
Line 20: TEST=Example Pre-CQ Launcher run with 2 disjoint patch series.

We have a test for that in TestCreateDisjointTransactions

--
To view, visit https://gerrit.chromium.org/gerrit/56325
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1b33b8c31705e5b48590472155f36695795a995d

Gerrit-PatchSet: 3

[David James (Code Review)]

David James has posted comments on this change.

Change subject: Submit non-manifest changes in the right order.
......................................................................

Patch Set 5: Not Ready

--
To view, visit https://gerrit.chromium.org/gerrit/56325
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1b33b8c31705e5b48590472155f36695795a995d

Gerrit-PatchSet: 5

[David James (Code Review)]

David James has posted comments on this change.

Change subject: Submit non-manifest changes in the right order.
......................................................................

Patch Set 7:

Patchset 6 is just a rebase. The diff between patchset 6 and patchset 7 shows the fix to the exception while submitting changes in the commit queue.

--
To view, visit https://gerrit.chromium.org/gerrit/56325
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1b33b8c31705e5b48590472155f36695795a995d

Gerrit-PatchSet: 7

[David James (Code Review)]

David James has posted comments on this change.

Change subject: Submit non-manifest changes in the right order.
......................................................................

Patch Set 8: Verified

Trybots, local runs, and new unittests to verify submission all pass

--
To view, visit https://gerrit.chromium.org/gerrit/56325
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1b33b8c31705e5b48590472155f36695795a995d

Gerrit-PatchSet: 8

[Matt Tennant (Code Review)]

Matt Tennant has posted comments on this change.

Change subject: Submit non-manifest changes in the right order.
......................................................................

Patch Set 8: (2 inline comments)

....................................................
File buildbot/validation_pool.py
Line 572: manifest = self._forced_manifest
Isn't self._forced_manifest some False value here?

Edit: After discussion, it looks like the behavior of the previous code is not correctly mimic'ed here.


Line 577: finally:
Is the point to make sure self._manifest is reset to its original value? If that is true, then logic like this would be harder to get wrong:

original_manifest = self._manifest

...

finally:
self._manifest = original_manifest

That way it doesn't depend on how the logic above treats self._forced_manifest.

[Matt Tennant (Code Review)]

Matt Tennant has posted comments on this change.

Change subject: Submit non-manifest changes in the right order.
......................................................................

Patch Set 8: (4 inline comments)

....................................................
File buildbot/validation_pool_unittest.py
Line 84: class base(cros_test_lib.TestCase):
why is this class lower-case?


Line 94: # We have to use a custom mock class to fix some brain behaviour of
"brain behaviour"? not part of this CL, though


Line 98: mox_ = getattr(self, 'mox', None)
will the mox attribute ever be present now, since you changed the class to inherit from TestCase rather than MoxTestCase?

Edit: I see that sub-classes are inheriting from more than one class, including this one. Can I suggest that this logic involving a mox attribute be moved to a sub-class method somewhere that has reason to expect mox to exist? I don't know if that is easily done.


Line 1141: cros_build_lib_unittest.RunCommandTestCase):
Hmm. The concern here is that someone editing RunCommandTestCase, within cros_build_lib_unittest.py, is unlikely to think they are editing something that is being used elsewhere. Yes, they should check, but *_unittest.py files are not generally used like libraries. If this class is generally helpful maybe it should be moved elsewhere, like cros_test_lib.

[David James (Code Review)]

David James has posted comments on this change.

Change subject: Submit non-manifest changes in the right order.
......................................................................

Patch Set 8: (6 inline comments)

....................................................
File buildbot/validation_pool.py
Line 572: manifest = self._forced_manifest

Done


Line 577: finally:
Done

....................................................
File buildbot/validation_pool_unittest.py
Line 84: class base(cros_test_lib.TestCase):

Done

Line 94: # We have to use a custom mock class to fix some brain behaviour of

Done

Line 98: mox_ = getattr(self, 'mox', None)

Done


Line 1141: cros_build_lib_unittest.RunCommandTestCase):
That's a good refactoring but it'd affect a lot of unit tests. Suggest filing a bug?

[David James (Code Review)]

David James has posted comments on this change.

Change subject: Submit non-manifest changes in the right order.
......................................................................

Patch Set 9: Ready; Verified

--
To view, visit https://gerrit.chromium.org/gerrit/56325
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1b33b8c31705e5b48590472155f36695795a995d

Gerrit-PatchSet: 9

[Matt Tennant (Code Review)]

Matt Tennant has posted comments on this change.

Change subject: Submit non-manifest changes in the right order.
......................................................................

Patch Set 9: Looks good to me, approved

(1 inline comment)

....................................................
File buildbot/validation_pool.py
Line 255: self.manifest = None
Newline here would be nice.

[David James (Code Review)]

David James has posted comments on this change.

Change subject: Submit non-manifest changes in the right order.
......................................................................

Patch Set 9: (1 inline comment)

....................................................
File buildbot/validation_pool.py
Line 255: self.manifest = None

Done

[David James (Code Review)]

David James has posted comments on this change.

Change subject: Submit non-manifest changes in the right order.
......................................................................

Patch Set 10: Looks good to me, approved; Ready; Verified

OK, put in the newline :)

--
To view, visit https://gerrit.chromium.org/gerrit/56325
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1b33b8c31705e5b48590472155f36695795a995d

Gerrit-PatchSet: 10

[David James (Code Review)]

David James has posted comments on this change.

Change subject: Submit non-manifest changes in the right order.
......................................................................

Patch Set 10: Ready

[David James (Code Review)]

[David James (Code Review)]