442823 - hgpoller + new pushlog picks up wrong changeset

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[Benjamin Smedberg]

The hgpoller picks up the wrong changeset with the new pushlog: I pushed two commits at once, 3590fdfcfe26 and 6d88fe34dc34. The new pushlog shows the first changeset pushed, not the second one, which causes the buildbot to build the wrong one. This could be pretty bad when pushing large branches.

http://hg.mozilla.org/mozilla-central/index.cgi/pushloghtml

I think the pushlog hook really needs to record the last changeset pushed, not the first one, the way it used to.

Comment 1

[(not currently active) Ted Mielczarek]

Are you sure the pushlog ever worked this way? The documentation says that the node parameter is "The changeset ID of the first changeset in the group that was added. All changesets between this and tip, inclusive, were added by a single “hg pull”, “hg push” or “hg unbundle”."

See http://hgbook.red-bean.com/hgbookch10.html section 10.9.1

We could certainly change the pushlog to record all the changesets pushed in the pushlog if that makes more sense.

Comment 2

[Benjamin Smedberg]

Yes, because it didn't use "node", it used "tip":

http://hg.mozilla.org/users/bsmedberg_mozilla.com/hghooks/index.cgi/file/e01e14e32151/hg_record_changeset_info

Comment 3

[Mike Shaver (:shaver emeritus)]

It's pretty bad if you ever push with a merge changeset, because it picks up the first one (pre-merge), which means you're effectively running with the merge-inducing "trunk" changes backed out!  And then we don't notice and start another build later with the right changeset, so you basically need to always push a single changeset at a time right now for build-on-commit to be useful.

I discovered this the hard way, did not enjoy it, please let me know if I can help rather than starting to litter my repo with spurious build-inducing commits!

Comment 4

[(not currently active) Ted Mielczarek]

I am probably halfway to fixing this. Because my changes to the pushlog changed the semantics, I've rewritten the convert-pushlog-db script we used to import from the flatfile to the db. It will now migrate the contents of the db to a new schema, and read the old pushlog to know which pushes were logged with which system. The new db schema is going to have two tables, a pushlog table that logs (user, date), and a changesets table that logs (pushid, node) for *every* changeset pushed with this push. This will make some code in the pushlog web interface easier to deal with as well.

Comment 5

[(not currently active) Ted Mielczarek]

Attachment: pushlog revamp, redux

Ok. This rewrites the pushlog converter script and the pushlog hook as described in my previous comment. I've tested it on a test repo with a combination of groups of changesets and individual changesets being pushed in both the old pushlog and the new pushlog, and it appears to do the right thing. (Correctly stores all changesets in the db grouped correctly into pushes.)

The modified pushlog hook logs each push and all associated changesets, to avoid any ambiguity here.

Comment 6

[Benjamin Smedberg]

Comment on attachment 329087 [details] [diff] [review]
pushlog revamp, redux

Hrm, I'd love a testcase... e.g. a shell script or python script which creates a new repo, pushes things to it, and runs sqlite3 to verify that the data is what we expect.

Comment 7

[(not currently active) Ted Mielczarek]

Yeah, I've done some manual testing, I can create an automated test, although you have to have the module installed first.

Comment 8

[Benjamin Smedberg]

Could the manual test munge PYTHONPATH so you can test with the local version?

Comment 9

[(not currently active) Ted Mielczarek]

Attachment: [checked in] hook/converter changes, with tests

Ok, this adds a test for the hook, and a test for the converter script. I modified the schema slightly to include the revision number in the changesets table, for easier ordering.

Comment 10

[(not currently active) Ted Mielczarek]

Comment on attachment 329548 [details] [diff] [review]
[checked in] hook/converter changes, with tests

Pushed in changeset 17acc0151c12

Comment 11

[(not currently active) Ted Mielczarek]

Attachment: pushlog-feed changes

Here's the corresponding changes to the pushlog-feed extension. It also adds browsing forwards/backwards on the pushloghtml page.

Comment 12

[(not currently active) Ted Mielczarek]

Attachment: template changes

I changed pushloghtml to use a template so it's less broken. This patch contains the template changes.

Comment 13

[(not currently active) Ted Mielczarek]

Attachment: slightly tweaked template changes

I changed the JS-date stuff a little bit in the template, I think it might be confusing to do that by default, especially when I get around to adding query-by-date.

Comment 14

[(not currently active) Ted Mielczarek]

Aravind: are you interested in reviewing these changes, or should I ask bsmedberg? (Or does anyone really care?)

Comment 15

[Aravind Gottipati [:aravind]]

(In reply to comment #14)
> Aravind: are you interested in reviewing these changes, or should I ask
> bsmedberg? (Or does anyone really care?)
> 

Nope.. I am cool if it works for you guys.  One Q. though, do I need to do any back processing to fix/convert the old sqlite dbs?  Or is this just a fix in the pushlog-feed thingy?

Comment 16

[(not currently active) Ted Mielczarek]

This is going to require running the converter script again. I discovered that they way I was logging data in the new pushlog was inconsistent with the old pushlog, hence this bug. We'll need to update the hook script, run the converter, and then update the templates and the pushlog-feed extension. I'll push all these changes out, then file an IT bug for you for deployment.

Comment 17

[(not currently active) Ted Mielczarek]

Pushed the other changes:
http://hg.mozilla.org/users/bsmedberg_mozilla.com/hgpoller/index.cgi/rev/646e1437fccf
http://hg.mozilla.org/index.cgi/hg_templates/rev/b9416f6d97ce

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So I wish this problem had been publicized more broadly and/or fixed faster.  I wasted a good bit of time on bug 363706 (see bug 363706 comment 44 through bug 363706 comment 57) because I didn't know that this was happening when multiple changesets were pushed together.

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think you should announce this on dev-planning (or someplace else appropriate) so that anyone else who backed out changes due to unexplained unit test failures caused by this bug knows that this is a possible cause of those failures (and that it might be safe to repush the changes in question one changeset at a time).

Comment 20

[(not currently active) Ted Mielczarek]

Filed bug 445399 on IT deployment of this code to hg.mozilla.org.

dbaron: I'm really sorry about that. I wish this had been discovered earlier, or I had had more time to fix it after it was discovered. Hopefully we can get this deployed soon and stop wasting developer's time. I'll post something to dev.planning if you think that'd be useful.

Comment 21

[Mike Shaver (:shaver emeritus)]

I should have raised the severity when I realized what it was causing, too.  A post to dev.planning would be good.

Comment 22

[(not currently active) Ted Mielczarek]

http://groups.google.com/group/mozilla.dev.planning/browse_thread/thread/1b2fc2eacb619630#

Comment 24

[Gregory Szorc [:gps]]

https://hg.mozilla.org/hgcustom/version-control-tools/rev/cb3ffb82c6df

Comment 25

[Gregory Szorc [:gps]]

https://hg.mozilla.org/hgcustom/version-control-tools/rev/66240ecdf62e