Closed Bug 1306723 Opened 8 years ago Closed 8 years ago

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

Categories

(Release Engineering :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: emorley, Assigned: aleth)

References

Details

Attachments

(3 files)

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 :-)
Flags: needinfo?(philip.chee)
CC aleth and ewong (What type of SHA does SeaMonkey use?)
Flags: needinfo?(philip.chee)
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.
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?
(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.
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
Flags: needinfo?(emorley)
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.
Attachment #8799147 - Flags: review?(philipp)
Attachment #8799147 - Flags: review?(clokep)
Assignee: nobody → aleth
Status: NEW → ASSIGNED
Comment on attachment 8799147 [details] [diff] [review] Use 40 character SHA revisions everywhere Review of attachment 8799147 [details] [diff] [review]: ----------------------------------------------------------------- r=philipp
Attachment #8799147 - Flags: review?(philipp) → review+
Attachment #8799147 - Flags: review?(clokep) → review+
https://hg.mozilla.org/comm-central/rev/e2aac779fb822973a141a0f2f2d42840ee563bae Bug 1306723 - Use 40 character SHA revisions everywhere. r=clokep,philipp
Leaving this open until we can check the issue in comment 0 has gone.
jorgk: This needs to be uplifted to c-a and c-b, but I can't request approval flags here.
Flags: needinfo?(jorgk)
Keywords: leave-open
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
Flags: needinfo?(emorley)
Aleth, can you please follow-up on comment #12 and let me know when it's ready for uplift.
Flags: needinfo?(jorgk)
(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).
Flags: needinfo?(jorgk)
(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).
Flags: needinfo?(emorley)
Component: General Automation → Build Config
Flags: needinfo?(jorgk)
Product: Release Engineering → Thunderbird
QA Contact: catlee
Version: unspecified → Trunk
There are quite a few hardcoded references to 12-character revisions in buildbotcustom. I'm surprised these only affect TB though.
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?
Attachment #8799147 - Flags: approval-comm-beta+
Attachment #8799147 - Flags: approval-comm-aurora+
(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.
emorley: see comment 15 and comment 16
Component: Build Config → General Automation
Flags: approval-comm-beta+
Flags: approval-comm-aurora+
Product: Thunderbird → Release Engineering
QA Contact: catlee
Version: Trunk → unspecified
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?
Flags: needinfo?(emorley)
(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.
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? :-)
Flags: needinfo?(catlee)
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.
Attachment #8799868 - Flags: feedback?(catlee)
Attachment #8799147 - Attachment is obsolete: true
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.
Attachment #8799147 - Attachment is obsolete: false
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
Attachment #8799868 - Flags: feedback?(catlee) → feedback+
It's still not clear to me what needs to be done to actually address this problem. (See comment 25)
Flags: needinfo?(nthomas)
(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.
Flags: needinfo?(nthomas)
Summary: Make Thunderbird jobs in builds-4hr use 40 character SHA revision → Make non-mozharness jobs in builds-4hr use 40 character SHA revision
(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?
Flags: needinfo?(emorley)
Yeah builds are fine (only tests are in the list in comment 12). I don't understand what "treeherder's comparechooser" is?
Flags: needinfo?(emorley)
Attachment #8799868 - Flags: review?(nthomas)
Flags: needinfo?(catlee)
(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 on attachment 8799868 [details] [diff] [review] Use 40 character SHA revisions in buildbotcustom Lets merge this when there are some releng around.
Attachment #8799868 - Flags: review?(nthomas) → review+
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
Attachment #8799868 - Flags: checked-in+
Attached file List of changed builders (deleted) —
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.
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.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
Component: General Automation → General
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: