engine: build and deployers should return results for targets they built, and#3374
Conversation
b30b89d to
16b9304
Compare
|
rebased! |
| @@ -23,6 +23,10 @@ type BuildAndDeployer interface { | |||
| // | |||
There was a problem hiding this comment.
I think you could compress these comments (from "Returns a..." onward). Something like:
Returns a BuildResultSet containing output (build result and associated file changes) for each target built in this call. (The BuildResultSet only contains results for newly built targets--if a target was clean and didn't need to be built, it doesn't appear in the result set.)
| func (q *TargetQueue) Results() store.BuildResultSet { | ||
| return q.results | ||
| // New results that we built with a BuildHandler. Omits results | ||
| // that were re-used previous builds. |
There was a problem hiding this comment.
I don't care HOW we built them (aren't all results built with a BuildHander?), I care WHEN we built them. "Results that were build in the current call (omits results...)" or something?
| func (q *TargetQueue) NewResults() store.BuildResultSet { | ||
| newResults := store.BuildResultSet{} | ||
| for id, result := range q.results { | ||
| if q.IsDirty(id) { |
There was a problem hiding this comment.
I understand why this works but the verbiage is confusing ("IsDirty" makes me think it STILL needs a build??). Not a big deal to leave as-is but i wonder if there exists a comment that would clarify?
| if q.IsDirty(id) { | |
| if q.IsDirty(id) { // i.e. WASDirty() -- if True, we built this target afresh |
There was a problem hiding this comment.
changed it to a private method and renamed it isBuilding
| newResults := q.NewResults() | ||
| if err != nil { | ||
| return store.BuildResultSet{}, buildcontrol.WrapDontFallBackError(err) | ||
| return newResults, buildcontrol.WrapDontFallBackError(err) |
There was a problem hiding this comment.
hmmm so this results set ought to only contain results for targets we actually built this time, IF we built them successfully, right? (just checking my understanding)
…ilt, and not return results for targets they didn't build. This should make #3362 much simpler and easy to reason about. Also adds test harnesses for different build graph configurations.
2 participants
Hello @landism, @maiamcc,
Please review the following commits I made in branch nicks/noop:
a935df3 (2020-05-22 18:03:01 -0400)
engine: build and deployers should return results for targets they built, and not return results for targets they didn't build.
This should make #3362
much simpler and easy to reason about.
Also adds test harnesses for different build graph configurations.
4fb8808 (2020-05-22 16:40:23 -0400)
engine: remove last build state from the target queue build handlers
Code review reminders, by giving a LGTM you attest that: