Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

List all changes related to a build. #1305

Merged
merged 3 commits into from Nov 25, 2014
Merged

Conversation

delanne
Copy link
Contributor

@delanne delanne commented Oct 28, 2014

  • to do this, the column 'parent_changeid' was added to the table changes

Note:

  • I added a tab 'changes' for the build webpage. I think this should be modified in order to have something more readable (but my angularJS skills are very limited).

order_by=sa.desc(changes_tbl.c.changeid),
limit=1)
return conn.scalar(q)
d = self.db.pool.do(thd)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why create a separate variable -- d -- if its value immediately returned?

@sa2ajj
Copy link
Contributor

sa2ajj commented Oct 28, 2014

👍 from me. I'd let others look at as well.

return self.db.pool.do(thd)

@defer.inlineCallbacks
def getChanges4Build(self, buildid):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We typically spell out "For" in methods like this.

@sa2ajj
Copy link
Contributor

sa2ajj commented Oct 28, 2014

(Interesting, I cancel builds for e9665b0 but they still show pending)

td Date
td Files
tr(ng-repeat="change in changes | orderBy:'changeid':true")
td {{ change.changeid }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's by far not limited to this commit or this place, but I don't believe ids have anything to do on the web interface. They are just internal data representation details ...

@sa2ajj
Copy link
Contributor

sa2ajj commented Oct 30, 2014

Side note: it looks like the base branch (master) moved on beyond a simple merge. This branch needs to be rebased.

@delanne
Copy link
Contributor Author

delanne commented Oct 31, 2014

before I continue to work on this, we need on aggreement on this PR.

@benallard
Copy link
Contributor

I believe I'm missing something big here, as the correct fix just looks so natural to me.

My point is: at some moment in time, we do have all the necessary information at hand:

  • which change(s) lead to which sourcestamp,
  • which sourcestamp(s) lead to a buildset,
  • which buildset(s) to a buildrequest,
  • and in case of collapsed build requests, we do even know the list of buildrequests we are skipping.

The reason why I'm wondering why we are 'forgetting' this information, just in order to try to get it back via complicated ways (full commit tree in DB) later on ... To me it looks like the natural way is to write all those relations to the DB, just in order to read them later on.

Please let me know what's the silly part there.

@tardyp
Copy link
Member

tardyp commented Oct 31, 2014

I've been thinking about this as well, and I agree with @benallard that it is dangerous to make assumptions on build continuity, and that would be better to have more support on the core for change hierarchy.

This is a tough problem which will be hard to discuss on IRC or via github. I would propose to make another hangout attempt, and try to brainstorm there, and make a decision.

@djmitche
Copy link
Member

djmitche commented Nov 1, 2014

Yes, this is complicated. The bit I'd disagree with is "which change(s) lead to which sourcestamp". We know one change which led to that sourcestamp; that change represents a delta from the sourcstamp before it. With a 'parents' field for changes, we also know which changes led to that parent sourcstamp (or in case of a merge, sourcestamps), and so on.

In the current arrangement, the only way that we associate multiple changes with one sourcestamp is if the scheduler aggregates them during the treeStableTimer. treeStableTimer made sense for CVS, where commits were per-file and not atomic at the repository level, but even for SVN it doesn't make sense. This arrangement misses buildrequest merging, retries, and a number of other phomena because it tries to predict -- before any builds have taken place -- the range of sourcestamps between the last known-good commit and this one.

The approach I'm suggesting instead leaves that determination until the build is complete and it's time to report on its status. In the final implementation, I think that would traverse the change graph backward from the current build until it found a change for which a successful build on the same builder had been performed. This way, we use the most up-to-date information to determine who might be to blame. Fast builders which never collapse requests will always blame a single change, while slower builders which do collapse requests will have longer blamelists.

This is a lot of machinery to build, and I don't think we need to build it for 0.9.0, but I feel strongly that it's the right long-term goal.

@jaredgrubb
Copy link
Member

I think these are definitely good things to think about. I know I've wanted buildbot to be a bit smarter about knowing how builds are related. But I agree that it can wait until after 0.9.0. Good things to think about though :)

@benallard
Copy link
Contributor

If the only missing part is indeed "which change(s) lead to which sourcestamp", then I think that this should be fixed first.

I agree that the only scenario where this come in place is the treeStableTimer. But I don't think it's a thing of the past. We use it daily to collapse the commits contained in a push action ! In a multi-repository setup, you want to get the commit to the lib and the commit to the app in one build.

I'm afraid I don't fully understand the goal of the 'parent' field. Is it meant to point to the sourcestamp we made before the scheduler kicks in ? As in: the parent commit ? Does this means that you aim at replicating the whole commit tree in the DB ? With support for merge, forced push, ammends, and darcs dark concepts ? I would say that this, without direct connection to the VCS tool is a lost battle.

We have that old concept of "Problem" if I remember correctly, where all blame list are concatenated until the build pass again.

We should aim at solving the troubles one at a time I believe, and this means first being able to give the blamelist for one particular build, based on collapsed buildrequests, and treeStableTimers's changes.

@delanne
Copy link
Contributor Author

delanne commented Nov 4, 2014

  1. The branch was rebased
  2. the blame list is construct as dustin suggested (EG: traverse the change graph backward from the current build until it found a change for which a successful build on the same builder had been performed.)

@sa2ajj
Copy link
Contributor

sa2ajj commented Nov 5, 2014

I'd appreciate a couple of reviewers for this PR :)

@@ -45,6 +45,26 @@ def getBuildByNumber(self, builderid, number):
(self.db.model.builds.c.builderid == builderid)
& (self.db.model.builds.c.number == number))

def getPrevSuccessfulBuild(self, builderid, number):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can return a build on any repository, on any branch. (Yes, I do have builders generic enough to be able to handle events from multiple repositories). Not necessary related to the current one !

@delanne delanne force-pushed the Changes4Build branch 2 times, most recently from 0713b95 to 4c66990 Compare November 5, 2014 14:41
@djmitche
Copy link
Member

djmitche commented Nov 7, 2014

@benallard yes, this means replicating the version-control hierarchy in the DB. Trac does this, too, actually. It's a little scary for darcs, but we'll leave that to the Darcs maintainers (maybe changes don't have parents, so you don't get a blamelist beyond one change?)

Support for treeStableTimer won't go away -- it will lose the behavior of determining the set of changes "responsible for" a particular build, but will continue to determine when a scheduler produces a buildset. In this sense, it's an early, simpler version of build request collapsing (avoiding creating lots of build requests that will just be merged later).

@sa2ajj
Copy link
Contributor

sa2ajj commented Nov 14, 2014

I think it conflicts with GH-1334

@delanne
Copy link
Contributor Author

delanne commented Nov 14, 2014

Yes. And I need to add more tests. I'll continue this PR next week.

Sent from my iPhone

On 14 nov. 2014, at 17:44, Mikhail Sobolev notifications@github.com wrote:

I think it conflicts with GH:1334


Reply to this email directly or view it on GitHub.

 - to do this, the column 'parent_changeid' was added to the table changes
 - traverse the change graph backward from the current build until it found a change for which a successful build on the same builder had been performed.
 - use repository/branch/codebase of sourcestamps as a criteria to found the previous build
 - getParentChangeId to getParentChangeIds
 - parent_changeid to parent_changeids
 - update documentation
@delanne
Copy link
Contributor Author

delanne commented Nov 25, 2014

rebase from master + fix conflict. Should be OK for reviewed.

(tbl.c.results == 0)),
offset=offset,
limit=10)
if len(prevBuilds) == 0:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if not prevBuilds:
   ...

?

@sa2ajj
Copy link
Contributor

sa2ajj commented Nov 25, 2014

LGTM, though I feel uneasy about really short variable names. For example, I understood cb as a callback, though later it became obvious it stands for codebase.

Release notes are missing.

@sa2ajj
Copy link
Contributor

sa2ajj commented Nov 25, 2014

I still expect the release notes.

@sa2ajj
Copy link
Contributor

sa2ajj commented Nov 25, 2014

And the identation "fix" causes pep8 problems. :(

master/buildbot/test/fake/fakedb.py:799:5: E129 visually indented line with same indent as next logical line

change['codebase'] == codebase):

^

E129 visually indented line with same indent as next logical line

1

make: *** [pep8] Error 1

Edit: added the error but formatting sucks.

@delanne
Copy link
Contributor Author

delanne commented Nov 25, 2014

Yes, my first commit was reindent with autopep8, and you asked about indentation. I repush indentation as it was in the first commit.

@sa2ajj
Copy link
Contributor

sa2ajj commented Nov 25, 2014

Sorry about that. I'm really curious about this particular case. I'll look at it after it lands in master.

@benallard
Copy link
Contributor

The dark side of git ...

@delanne
Copy link
Contributor Author

delanne commented Nov 25, 2014

travis is failing ... IMO, this is not related to this PR (as I did not modify any buildslave code). Also, I'm unable to reproduce the error on my desktop. maybe a race condition was introduced by #1370 ?

@sa2ajj
Copy link
Contributor

sa2ajj commented Nov 25, 2014

They are the same as in TRAC-3066

@sa2ajj
Copy link
Contributor

sa2ajj commented Nov 25, 2014

(It's flaky flaky)

sa2ajj pushed a commit that referenced this pull request Nov 25, 2014
List all changes related to a build.
@sa2ajj sa2ajj merged commit 2eff249 into buildbot:master Nov 25, 2014
@sa2ajj
Copy link
Contributor

sa2ajj commented Nov 25, 2014

Per comment, no release notes are required for this kind of change.

@benallard
Copy link
Contributor

Still, @delanne, I already mentioned the advantage of small commits to you ... So I won't repeat myself.

Your main commit here is touching the db layer, the data layer, the web interface, and other bits. All in 1 commit.That's too much.

@benallard
Copy link
Contributor

The karma test is failing since this:

    Expected { files : [ 'myfiles' ], category : 'mycategory', parent_changeids : [ 2 ], repository : 'myrepository', author : 'myauthor', project : 'myproject', comments : 'mycomments', changeid : 2, codebase : 'mycodebase', branch : 'mybranch', sourcestamp : { codebase : 'mycodebase', ssid : 2, repository : 'myrepository', created_at : 2, patch : { body : 'mybody', comment : 'mycomment', patchid : 2, level : 2, author : 'myauthor', subdir : 'mysubdir' }, project : 'myproject', branch : 'mybranch', revision : 'myrevision' }, revision : 'myrevision', revlink : 'myrevlink', properties : { prop : [ 'value', 'source' ] }, when_timestamp : 2 } to equal { files : [ 'myfiles' ], category : 'mycategory', repository : 'myrepository', author : 'myauthor', project : 'myproject', comments : 'mycomments', changeid : 2, codebase : 'mycodebase', branch : 'mybranch', sourcestamp : { codebase : 'mycodebase', ssid : 2, repository : 'myrepository', created_at : 2, patch : { body : 'mybody', comment : 'mycomment', patchid : 2, level : 2, author : 'myauthor', subdir : 'mysubdir' }, project : 'myproject', branch : 'mybranch', revision : 'myrevision' }, revision : 'myrevision', revlink : 'myrevlink', properties : { prop : [ 'value', 'source' ] }, when_timestamp : 2 }.
    Error: Expected { files : [ 'myfiles' ], category : 'mycategory', parent_changeids : [ 2 ], repository : 'myrepository', author : 'myauthor', project : 'myproject', comments : 'mycomments', changeid : 2, codebase : 'mycodebase', branch : 'mybranch', sourcestamp : { codebase : 'mycodebase', ssid : 2, repository : 'myrepository', created_at : 2, patch : { body : 'mybody', comment : 'mycomment', patchid : 2, level : 2, author : 'myauthor', subdir : 'mysubdir' }, project : 'myproject', branch : 'mybranch', revision : 'myrevision' }, revision : 'myrevision', revlink : 'myrevlink', properties : { prop : [ 'value', 'source' ] }, when_timestamp : 2 } to equal { files : [ 'myfiles' ], category : 'mycategory', repository : 'myrepository', author : 'myauthor', project : 'myproject', comments : 'mycomments', changeid : 2, codebase : 'mycodebase', branch : 'mybranch', sourcestamp : { codebase : 'mycodebase', ssid : 2, repository : 'myrepository', created_at : 2, patch : { body : 'mybody', comment : 'mycomment', patchid : 2, level : 2, author : 'myauthor', subdir : 'mysubdir' }, project : 'myproject', branch : 'mybranch', revision : 'myrevision' }, revision : 'myrevision', revlink : 'myrevlink', properties : { prop : [ 'value', 'source' ] }, when_timestamp : 2 }.
        at /home/benoit/buildbot/buildbot/www/base/buildbot_www/static/tests.js:2667
        at /home/benoit/buildbot/buildbot/www/base/node_modules/guanlecoja/node_modules/karma-jasmine/lib/boot.js:126
        at /home/benoit/buildbot/buildbot/www/base/node_modules/guanlecoja/node_modules/karma-jasmine/lib/adapter.js:171
        at http://localhost:9876/karma.js:189
        at http://localhost:9876/context.html:45

note the missing oparent_changeids in the second part.

@benallard
Copy link
Contributor

Just opened TRAC-3071 with an error when visiting the new data endpoint. (also posting it here to prevent it for being unseen.)

@benallard benallard mentioned this pull request Nov 27, 2014
@benallard
Copy link
Contributor

BTW, This is a behavioral change (if I understand it correctly), as previously only the direct changes for a build were shown, now all changes since the previously sucessfull build are shown. And I can imagine that some people rather want the previous behavior.

So i believe a release note update would have been required.

@djmitche
Copy link
Member

djmitche commented Dec 8, 2014

Mentioning in the release notes is not a bad idea, but I think that this is the expected behavior. Of our userbase, I'd guess that 50% thought it worked this way already, 40% knew it didn't but were annoyed at the weird behavior, and 10% liked it.

@benallard
Copy link
Contributor

Fair enough, see #1423 and TRAC-3098 for further improvement of this feature.

@delanne delanne deleted the Changes4Build branch October 20, 2015 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants