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)
Release Engineering
General
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: emorley, Assigned: aleth)
References
Details
Attachments
(3 files)
(deleted),
patch
|
clokep
:
review+
Fallen
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
nthomas
:
review+
catlee
:
feedback+
nthomas
:
checked-in+
|
Details | Diff | Splinter Review |
(deleted),
text/plain
|
Details |
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)
Comment 1•8 years ago
|
||
CC aleth and ewong (What type of SHA does SeaMonkey use?)
Flags: needinfo?(philip.chee)
Reporter | ||
Comment 2•8 years ago
|
||
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.
Assignee | ||
Comment 3•8 years ago
|
||
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•8 years ago
|
||
(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.
Assignee | ||
Comment 5•8 years ago
|
||
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)
Assignee | ||
Comment 6•8 years ago
|
||
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 | ||
Updated•8 years ago
|
Assignee: nobody → aleth
Status: NEW → ASSIGNED
Comment 7•8 years ago
|
||
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+
Assignee | ||
Comment 8•8 years ago
|
||
Updated•8 years ago
|
Attachment #8799147 -
Flags: review?(clokep) → review+
Assignee | ||
Comment 9•8 years ago
|
||
https://hg.mozilla.org/comm-central/rev/e2aac779fb822973a141a0f2f2d42840ee563bae
Bug 1306723 - Use 40 character SHA revisions everywhere. r=clokep,philipp
Assignee | ||
Comment 10•8 years ago
|
||
Leaving this open until we can check the issue in comment 0 has gone.
Assignee | ||
Comment 11•8 years ago
|
||
jorgk: This needs to be uplifted to c-a and c-b, but I can't request approval flags here.
Flags: needinfo?(jorgk)
Assignee | ||
Updated•8 years ago
|
Keywords: leave-open
Reporter | ||
Comment 12•8 years ago
|
||
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)
Comment 13•8 years ago
|
||
Aleth, can you please follow-up on comment #12 and let me know when it's ready for uplift.
Flags: needinfo?(jorgk)
Assignee | ||
Comment 14•8 years ago
|
||
(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)
Assignee | ||
Comment 15•8 years ago
|
||
(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)
Updated•8 years ago
|
Component: General Automation → Build Config
Flags: needinfo?(jorgk)
Product: Release Engineering → Thunderbird
QA Contact: catlee
Version: unspecified → Trunk
Assignee | ||
Comment 16•8 years ago
|
||
There are quite a few hardcoded references to 12-character revisions in buildbotcustom. I'm surprised these only affect TB though.
Comment 17•8 years ago
|
||
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+
Assignee | ||
Comment 18•8 years ago
|
||
(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•8 years ago
|
||
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 ;-)
Assignee | ||
Comment 20•8 years ago
|
||
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
Reporter | ||
Comment 21•8 years ago
|
||
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)
Assignee | ||
Comment 22•8 years ago
|
||
(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.
Reporter | ||
Comment 23•8 years ago
|
||
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)
Assignee | ||
Comment 24•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
Attachment #8799147 -
Attachment is obsolete: true
Assignee | ||
Comment 25•8 years ago
|
||
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.
Assignee | ||
Updated•8 years ago
|
Attachment #8799147 -
Attachment is obsolete: false
Comment 26•8 years ago
|
||
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+
Assignee | ||
Comment 27•8 years ago
|
||
It's still not clear to me what needs to be done to actually address this problem. (See comment 25)
Flags: needinfo?(nthomas)
Comment 28•8 years ago
|
||
(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)
Updated•8 years ago
|
Updated•8 years ago
|
Updated•8 years ago
|
Summary: Make Thunderbird jobs in builds-4hr use 40 character SHA revision → Make non-mozharness jobs in builds-4hr use 40 character SHA revision
Assignee | ||
Comment 29•8 years ago
|
||
(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)
Reporter | ||
Comment 30•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
Attachment #8799868 -
Flags: review?(nthomas)
Updated•8 years ago
|
Flags: needinfo?(catlee)
Comment 31•8 years ago
|
||
(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•8 years ago
|
||
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 33•8 years ago
|
||
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+
Comment 34•8 years ago
|
||
https://hg.mozilla.org/build/buildbotcustom/rev/cbe5e57f6cad9ba5febfa24637e6217f953d64f6 for prod, will go live in 25 minutes.
Comment 35•8 years ago
|
||
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.
Assignee | ||
Comment 36•8 years ago
|
||
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
Comment 38•7 years ago
|
||
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
Updated•7 years ago
|
Component: General Automation → General
You need to log in
before you can comment on or make changes to this bug.
Description
•