hgpoller + new pushlog picks up wrong changeset

RESOLVED FIXED

Status

Release Engineering
General
RESOLVED FIXED
9 years ago
3 years ago

People

(Reporter: bsmedberg, Assigned: ted)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

9 years ago
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.
(Assignee)

Comment 1

9 years ago
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.

(Reporter)

Comment 2

9 years ago
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
(Assignee)

Updated

9 years ago
Assignee: nobody → ted.mielczarek
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!
(Assignee)

Comment 4

9 years ago
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.
Status: NEW → ASSIGNED
(Assignee)

Comment 5

9 years ago
Created attachment 329087 [details] [diff] [review]
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.
Attachment #329087 - Flags: review?(benjamin)
(Reporter)

Comment 6

9 years ago
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.
Attachment #329087 - Flags: review?(benjamin) → review+
(Assignee)

Comment 7

9 years ago
Yeah, I've done some manual testing, I can create an automated test, although you have to have the module installed first.
(Reporter)

Comment 8

9 years ago
Could the manual test munge PYTHONPATH so you can test with the local version?
(Assignee)

Comment 9

9 years ago
Created attachment 329548 [details] [diff] [review]
[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.
Attachment #329087 - Attachment is obsolete: true
(Assignee)

Comment 10

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

Pushed in changeset 17acc0151c12
Attachment #329548 - Attachment description: with tests → [checked in] hook/converter changes, with tests
(Assignee)

Comment 11

9 years ago
Created attachment 329681 [details] [diff] [review]
pushlog-feed changes

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

Comment 12

9 years ago
Created attachment 329682 [details] [diff] [review]
template changes

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

Comment 13

9 years ago
Created attachment 329717 [details] [diff] [review]
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.
Attachment #329682 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Attachment #329717 - Attachment is patch: true
Attachment #329717 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Comment 14

9 years ago
Aravind: are you interested in reviewing these changes, or should I ask bsmedberg? (Or does anyone really care?)
(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?
(Assignee)

Comment 16

9 years ago
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.
(Assignee)

Comment 17

9 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
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.
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).
(Assignee)

Comment 20

9 years ago
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.
I should have raised the severity when I realized what it was causing, too.  A post to dev.planning would be good.
(Assignee)

Comment 22

9 years ago
http://groups.google.com/group/mozilla.dev.planning/browse_thread/thread/1b2fc2eacb619630#
(Assignee)

Updated

9 years ago
Duplicate of this bug: 437446
Product: mozilla.org → Release Engineering
https://hg.mozilla.org/hgcustom/version-control-tools/rev/cb3ffb82c6df
https://hg.mozilla.org/hgcustom/version-control-tools/rev/66240ecdf62e
You need to log in before you can comment on or make changes to this bug.