1306723 - Make non-mozharness jobs in builds-4hr use 40 character SHA revision

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[Ed Morley [:emorley]]

Searching builds-4hr for jobs using the legacy revision format found:
https://bug1306707.bmoattachments.org/attachment.cgi?id=8796694

Treeherder would like to soon drop support for 12 character SHAs, so it would be helpful if Thunderbird jobs were switched to them.

Philip, could you help suggest who might be best to look at this?

Thanks :-)

Comment 1

[Philip Chee]

CC aleth and ewong (What type of SHA does SeaMonkey use?)

Comment 2

[Ed Morley [:emorley]]

Just to double check we're on the same page - we're not talking about different types of SHA (eg SHA1 vs SHA2) but how many characters of the Mercurial SHA are used to identify the changeset.

Mercurial internally uses 40 characters, and whilst both on the command line and elsewhere you can often get away with using fewer, we've already seen multiple collisions when only using 12 character SHAs with Mozilla repos.

As such, there's been an infra-wide switch to using the full 40 character SHA to avoid collisions.

Comment 3

[aleth [:aleth]]

As far as application.ini goes, I switched TB to long revisions in this patch (which was then uplifted to TB 49) https://hg.mozilla.org/comm-central/rev/b2b090d8d57e7608c0b015ab4c55ab69dc53b3aa

I thought anything downstream read the revision from that file, but it seems from this there are other places that need to be changed.

Where do the jobs get their revision format from? Buildbot?

Comment 4

[Edmund Wong (:ewong)]

(In reply to Philip Chee from comment #1)
> CC aleth and ewong (What type of SHA does SeaMonkey use?)

we use the short version; but since treeherder changes don't affect
us (for now), there's nothing we need to do.

However if we were to use treeherder in the near/distant
future), we would need to make changes.

Comment 5

[aleth [:aleth]]

emorley: To be consistent, you may also wish to update the remaining two short revisions in m-c (even if not relevant for treeherder), http://searchfox.org/mozilla-central/search?q=node%7Cshort

Comment 6

[aleth [:aleth]]

Attachment: Use 40 character SHA revisions everywhere

These are the remaining short nodes used in c-c. There don't appear to be any in buildbot, so I hope changing these takes care of the problem.

Comment 7

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8799147 [details] [diff] [review]
Use 40 character SHA revisions everywhere

Review of attachment 8799147 [details] [diff] [review]:
-----------------------------------------------------------------

r=philipp

Comment 8

[aleth [:aleth]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=38c26829364f00be8bef084b9f4ca301010c2d00

Comment 9

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/e2aac779fb822973a141a0f2f2d42840ee563bae
Bug 1306723 - Use 40 character SHA revisions everywhere. r=clokep,philipp

Comment 10

[aleth [:aleth]]

Leaving this open until we can check the issue in comment 0 has gone.

Comment 11

[aleth [:aleth]]

jorgk: This needs to be uplifted to c-a and c-b, but I can't request approval flags here.

Comment 12

[Ed Morley [:emorley]]

Thank you all for looking into this :-)

Unfortunately builds-4hr still contains:

TB Rev7 MacOSX Yosemite 10.10.5 comm-central debug test mozmill
TB Rev7 MacOSX Yosemite 10.10.5 comm-central opt test mozmill
TB Rev7 MacOSX Yosemite 10.10.5 comm-central opt test xpcshell
TB Windows 7 32-bit comm-central debug test mozmill
TB Windows 7 32-bit comm-central debug test xpcshell
TB Windows 7 32-bit comm-central opt test mozmill
TB Windows 7 32-bit comm-central opt test xpcshell
TB Windows XP 32-bit comm-central debug test mozmill
TB Windows XP 32-bit comm-central debug test xpcshell
TB Windows XP 32-bit comm-central opt test mozmill
TB Windows XP 32-bit comm-central opt test xpcshell
Ubuntu VM 12.04 comm-central debug test mozmill
Ubuntu VM 12.04 comm-central debug test xpcshell-1
Ubuntu VM 12.04 comm-central debug test xpcshell-2
Ubuntu VM 12.04 comm-central opt test mozmill
Ubuntu VM 12.04 comm-central opt test xpcshell
Ubuntu VM 12.04 x64 comm-central debug test mozmill
Ubuntu VM 12.04 x64 comm-central debug test xpcshell-1
Ubuntu VM 12.04 x64 comm-central debug test xpcshell-2
Ubuntu VM 12.04 x64 comm-central opt test mozmill
Ubuntu VM 12.04 x64 comm-central opt test xpcshell

Comment 13

[Jorg K (CEST = GMT+2)]

Aleth, can you please follow-up on comment #12 and let me know when it's ready for uplift.

Comment 14

[aleth [:aleth]]

(In reply to Jorg K (GMT+2) from comment #13)
> Aleth, can you please follow-up on comment #12 and let me know when it's
> ready for uplift.

You can uplift it now (even though it doesn't resolve emorley's problem, let's be consistent).

Comment 15

[aleth [:aleth]]

(In reply to Ed Morley [:emorley] from comment #12)
> Thank you all for looking into this :-)
> 
> Unfortunately builds-4hr still contains:

Where is builds-4hr getting its data from? Afaik there are no remaining short revisions generated in-tree (apart from those mentioned in comment 6).

Comment 16

[aleth [:aleth]]

There are quite a few hardcoded references to 12-character revisions in buildbotcustom. I'm surprised these only affect TB though.

Comment 17

[Jorg K (CEST = GMT+2)]

Comment on attachment 8799147 [details] [diff] [review]
Use 40 character SHA revisions everywhere

I changed the product to Thunderbird since we're landing a patch on C-C.
I've approved it, so Aleth, you can land it. Or I can land. Would you like to build or land it with DONTBUILD?

Comment 18

[aleth [:aleth]]

(In reply to Jorg K (GMT+2) from comment #17)
> I changed the product to Thunderbird since we're landing a patch on C-C.
> I've approved it, so Aleth, you can land it. Or I can land. Would you like
> to build or land it with DONTBUILD?

Please land it, it doesn't need a build.

Comment 19

[Jorg K (CEST = GMT+2)]

C-A (TB 51): https://hg.mozilla.org/releases/comm-aurora/rev/5d350371407ed718ea1d831888af9d926d9f2985
C-B (TB 50): https://hg.mozilla.org/releases/comm-beta/rev/9a530e5b076f0245eb11a83b3d48e67b9006c3d5
If you want tracking flags, please set them yourself ;-)

Comment 20

[aleth [:aleth]]

emorley: see comment 15 and comment 16

Comment 21

[Ed Morley [:emorley]]

builds-4hr comes from buildapi, which comes from the buildbot DBs, whose job results are a result of code from the various release engineering repos.

I'd imagine the offending code is in one of:

https://hg.mozilla.org/build/buildapi/
https://hg.mozilla.org/build/buildbotcustom/
https://hg.mozilla.org/build/buildbot-configs/
https://hg.mozilla.org/build/tools/
(presuming there's not leftovers in comm-central, or in mozilla-central's mozharness)

DXR can search all of the build repos:
https://dxr.mozilla.org/build-central/source/

I'd search for both `|short` plus things like `[0:12]`.

Failing that, perhaps someone from release engineering will know where the short SHAs are coming from?

Comment 22

[aleth [:aleth]]

(In reply to Ed Morley [:emorley] from comment #21)
> I'd search for both `|short` plus things like `[0:12]`.
> 
> Failing that, perhaps someone from release engineering will know where the
> short SHAs are coming from?

It's no problem to find references to short SHAs, there are quite a few -- see comment 16. I'm not sure it's OK to just change *all* of them though.

Comment 23

[Ed Morley [:emorley]]

Some of the others will definitely need changing, if only to fix bug 1306720, bug 1306722, bug 1306718, however I don't know enough about the releng side to say which.

Chris, could you advise here, or find someone else to help out? :-)

Comment 24

[aleth [:aleth]]

Attachment: Use 40 character SHA revisions in buildbotcustom

This replaces some of the short hashes in buildbotcustom. I marked the one I was unsure about with a comment, as it seems some database would have to be migrated to make a change there.

Comment 25

[aleth [:aleth]]

There are also short hashes in 

mozharness: https://dxr.mozilla.org/build-central/search?q=%7Cshort+path%3Amozharness&redirect=false
buildapi: https://dxr.mozilla.org/build-central/search?q=%3A12+path%3Abuildapi&redirect=true

As for the buildbotcustom hits, it's hard to guess which ones are relevant here though.

Comment 26

[Chris AtLee [:catlee]]

Comment on attachment 8799868 [details] [diff] [review]
Use 40 character SHA revisions in buildbotcustom

Review of attachment 8799868 [details] [diff] [review]:
-----------------------------------------------------------------

These all look pretty safe

Comment 27

[aleth [:aleth]]

It's still not clear to me what needs to be done to actually address this problem. (See comment 25)

Comment 28

[Nick Thomas [:nthomas] (UTC+12)]

(In reply to aleth [:aleth] from comment #25)
> mozharness:
> https://dxr.mozilla.org/build-central/
> search?q=%7Cshort+path%3Amozharness&redirect=false

That is an unused copy of mozharness, look in mozilla-central these days.

I'm not going to be able to look at this in detail right now. The factory.py changes in attachment 8799868 [details] [diff] [review] will likely fix the Thunderbird tests (builds seem to be fine already), since the sendchange uses got_revision from the build. Regarding the changes to try_mailer.py and generators.py, we deliberately left those as short revs when we converted Firefox to 40char SHA's. I don't really mind either way, but you should check if treeherder's comparechooser is OK with long revs.

Comment 29

[aleth [:aleth]]

(In reply to Nick Thomas [:nthomas] from comment #28)
> I'm not going to be able to look at this in detail right now. The factory.py
> changes in attachment 8799868 [details] [diff] [review] will likely fix the
> Thunderbird tests (builds seem to be fine already), since the sendchange
> uses got_revision from the build. 

That was my impression too.
emorley: Could you confirm builds are OK from a treeherder point of view?

> Regarding the changes to try_mailer.py and
> generators.py, we deliberately left those as short revs when we converted
> Firefox to 40char SHA's. I don't really mind either way, but you should
> check if treeherder's comparechooser is OK with long revs.

emorley: Since we're doing this for treeherder, I assume it will be fine?

Comment 30

[Ed Morley [:emorley]]

Yeah builds are fine (only tests are in the list in comment 12).

I don't understand what "treeherder's comparechooser" is?

Comment 31

[Nick Thomas [:nthomas] (UTC+12)]

(In reply to Ed Morley (Away 28th Oct -> 6th Nov) [:emorley] from comment #30)
> I don't understand what "treeherder's comparechooser" is?

Sorry, I meant perfherder. eg in the 'try submission' email sent on try push, if the talos is enabled this link is included 
  https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=%(revision)s
It seems to work fine if I feed that two long revs.

Comment 32

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 8799868 [details] [diff] [review]
Use 40 character SHA revisions in buildbotcustom

Lets merge this when there are some releng around.

Comment 33

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 8799868 [details] [diff] [review]
Use 40 character SHA revisions in buildbotcustom

Checked in with a fix to add property='got_revision' when removing extract_fn=short_hash.

https://hg.mozilla.org/build/buildbotcustom/rev/46bf43f4514d07ff88071002debca576e2a34f08

Comment 34

[Nick Thomas [:nthomas] (UTC+12)]

https://hg.mozilla.org/build/buildbotcustom/rev/cbe5e57f6cad9ba5febfa24637e6217f953d64f6 for prod, will go live in 25 minutes.

Comment 35

[Nick Thomas [:nthomas] (UTC+12)]

Attachment: List of changed builders

FTR, here's the builders that are being modified. The property has been set correctly on the 4 Android jobs I've rebuilt, eg:

Branch          releases/mozilla-release
Revision        87f8b968e129c84115218f26f9b33a6e3c303320
Got Revision    87f8b968e129c84115218f26f9b33a6e3c303320

Revision comes from the scheduler via hg poller; Got Revision is from the property code being changed.

Comment 36

[aleth [:aleth]]

comm-central looks OK too judging by the logs I spot-checked.

l10n_revision is still 12 characters. There are also some intermediate steps using short revisions output by hg.py.
However, my guess would be that for Treeherder purposes, we are done here.

Comment 38

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.