1274059 - Remove HgtoolVCS

Status: RESOLVED FIXED

(Release Engineering :: Applications: MozharnessCore, defect)

Description

[Gregory Szorc [:gps]]

With bug 1270317, MercurialVCS should now be a suitable substitute for HgtoolVCS. We should remove HgtoolVCS from mozharness.

Comment 1

[Justin Wood (:Callek)]

Attachment: Bug 1274059 - Remove HgtoolVCS - Part 1, remove users of hgtool and fixups.

[mq]: l10n-checkout-fixes, and remove hgtool config

Review commit: https://reviewboard.mozilla.org/r/71460/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/71460/

Comment 2

[Justin Wood (:Callek)]

Attachment: Bug 1274059 - Remove HgtoolVCS - Part 2, remove actual classes and docs for hgtool.

Review commit: https://reviewboard.mozilla.org/r/71462/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/71462/

Comment 3

[Justin Wood (:Callek)]

The above patches were indeed based off some initial work by nick, so if you feel another reviewer is appropriate I can swap it.

There was some try jobs kicked off: https://treeherder.mozilla.org/#/jobs?repo=try&revision=454900d62fc5 for l10n and other jobs.

And an additional one (which I *think* kicked multi-locale android right): https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f4bf4097745

Comment 4

[Justin Wood (:Callek)]

Comment on attachment 8780921 [details]
Bug 1274059 - Remove HgtoolVCS - Part 1, remove users of hgtool and fixups.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71460/diff/1-2/

Comment 5

[Justin Wood (:Callek)]

Comment on attachment 8780922 [details]
Bug 1274059 - Remove HgtoolVCS - Part 2, remove actual classes and docs for hgtool.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71462/diff/1-2/

Comment 6

[Justin Wood (:Callek)]

(In reply to Justin Wood (:Callek) from comment #3)
> And an additional one (which I *think* kicked multi-locale android right):
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f4bf4097745

That actually failed to kick multi-locale (to the point where it'd succeed)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e4ae263d896 actually worked (for non-taskcluster, TC had other unrelated-to-patchset issues)

Comment 7

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

Comment on attachment 8780921 [details]
Bug 1274059 - Remove HgtoolVCS - Part 1, remove users of hgtool and fixups.

https://reviewboard.mozilla.org/r/71460/#review70494

Broadly looks fine, just the issue around 'default' in the release configs.

::: testing/mozharness/configs/multi_locale/release_mozilla-beta_android.json:11
(Diff revision 2)
>      "locales_platform": "android-multilocale",
>      "locales_dir": "mobile/android/locales",
>      "ignore_locales": ["en-US", "multi"],
>      "repos": [{
>          "repo": "https://hg.mozilla.org/releases/mozilla-beta",
> +        "branch": "default",

This pattern of adding "revision": "default" is fairly common in this patch, is it just a safety or required ? Presumably we actually update to a release tag in the code when using this release config. Could you verify/explain what's happening ? A comment in the config would be useful. Repeat as necessary for other lines & configs.

Comment 8

[Justin Wood (:Callek)]

(In reply to Nick Thomas [:nthomas] from comment #7)
> Comment on attachment 8780921 [details]
> Bug 1274059 - Remove HgtoolVCS - Part 1, remove users of hgtool and fixups.
> 
> https://reviewboard.mozilla.org/r/71460/#review70494
> 
> Broadly looks fine, just the issue around 'default' in the release configs.
> 
> :::
> testing/mozharness/configs/multi_locale/release_mozilla-beta_android.json:11
> (Diff revision 2)
> >      "locales_platform": "android-multilocale",
> >      "locales_dir": "mobile/android/locales",
> >      "ignore_locales": ["en-US", "multi"],
> >      "repos": [{
> >          "repo": "https://hg.mozilla.org/releases/mozilla-beta",
> > +        "branch": "default",
> 
> This pattern of adding "revision": "default" is fairly common in this patch,
> is it just a safety or required ? Presumably we actually update to a release
> tag in the code when using this release config. Could you verify/explain
> what's happening ? A comment in the config would be useful. Repeat as
> necessary for other lines & configs.

This file in particular, I did s/tag/branch/ which is more accurate for how robustcheckout performs (afaict), for the 'default' in general...

With robustcheckout any clone/checkout that doesn't specify a revision (or branch/tag) fails entirely.

With robustcheckout any clone/checkout that specifies a revision that isn't a changeset hash fails as unexpected.

With robustcheckout if you specify a branch: "something" it resolves that branch remotely before updating the local repo to that resolved changeset (or pulls in that resolved changeset first).

I opted to "in the case of no rev/branch specified" to mark 'default'; and normalize on "branch" instead of "tag" here.  Without simultaneously trying to evaluate if another rev is appropriate.

As to your your comment on beta/code in a release config, these jobs have (so far) cloned from default, did their stuff, then on pull of the en-US build, update to the en-US revision, which is *not* ideal but is how it works at present. -- Fixing that in l10n at the least is one of my alternate-bug priorities.

Comment 9

[Justin Wood (:Callek)]

(In reply to Justin Wood (:Callek) from comment #8)
> (In reply to Nick Thomas [:nthomas] from comment #7)
> > Comment on attachment 8780921 [details]
> > Bug 1274059 - Remove HgtoolVCS - Part 1, remove users of hgtool and fixups.
> > 
> > https://reviewboard.mozilla.org/r/71460/#review70494
> > 
> > Broadly looks fine, just the issue around 'default' in the release configs.
> > 
> > :::
> > testing/mozharness/configs/multi_locale/release_mozilla-beta_android.json:11
...
> This file in particular, ....

Err actually I looked at "mozilla-beta_android" not "release_mozilla-beta_android" when writing that. My prose still holds though, since the case without branch/revision would fail with robustcheckout in use.

Comment 10

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

Comment on attachment 8780921 [details]
Bug 1274059 - Remove HgtoolVCS - Part 1, remove users of hgtool and fixups.

https://reviewboard.mozilla.org/r/71460/#review70540

I see now that branch or revision is required at 
  https://dxr.mozilla.org/mozilla-beta/source/testing/mozharness/external_tools/robustcheckout.py#125
And robustcheckout doesn't support tag, so swapping to branch is required. So far so good.

These repo dicts are only used when the pull_build_source action is used though. Normally we don't run that (already have en-US code & build), and use tag_override (beta/release) or hg_l10n_tag (nightly/aurora) to set the branch for robustcheckout.

eg these logs without it which use robustcheckout and work OK:
  https://archive.mozilla.org/pub/mobile/candidates/49.0b4-candidates/build1/logs/release-mozilla-beta-android-api-15_build-bm72-build1-build7.txt.gz
  https://archive.mozilla.org/pub/mobile/nightly/2016/08/2016-08-18-03-02-26-mozilla-central-android-api-15/mozilla-central-android-api-15-nightly-bm71-build1-build3.txt.gz

So that leaves running the script standalone, and I'm finding it hard to worry about that. So please either undo the changes the multilocale configs, or change the branch for buildbot-configs to 'production', and we'll be done with this.

r+ with that.

Comment 11

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

Comment on attachment 8780922 [details]
Bug 1274059 - Remove HgtoolVCS - Part 2, remove actual classes and docs for hgtool.

https://reviewboard.mozilla.org/r/71462/#review70496

::: testing/mozharness/mozharness/base/vcs/mercurial.py:75
(Diff revision 2)
>          return '/'.join([p.strip('/') for p in [repo, 'raw-file', revision, filename]])
>  
>  
>  class MercurialVCS(ScriptMixin, LogMixin, TransferMixin):
>      # For the most part, scripts import mercurial, update,
> -    # hgtool uses mercurial, share, out
> +    # uses mercurial, share, out

Failing to parse this, zap the whole line ?

Comment 12

[Justin Wood (:Callek)]

Comment on attachment 8780921 [details]
Bug 1274059 - Remove HgtoolVCS - Part 1, remove users of hgtool and fixups.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71460/diff/2-3/

Comment 13

[Justin Wood (:Callek)]

Comment on attachment 8780922 [details]
Bug 1274059 - Remove HgtoolVCS - Part 2, remove actual classes and docs for hgtool.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71462/diff/2-3/

Comment 14

[Justin Wood (:Callek)]

Comment on attachment 8780921 [details]
Bug 1274059 - Remove HgtoolVCS - Part 1, remove users of hgtool and fixups.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71460/diff/3-4/

Comment 15

[Justin Wood (:Callek)]

Comment on attachment 8780922 [details]
Bug 1274059 - Remove HgtoolVCS - Part 2, remove actual classes and docs for hgtool.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71462/diff/3-4/

Comment 16

[Justin Wood (:Callek)]

Adding checkin-needed since I'll be away next week and this has a higher-than-normal chance to break jobs, especially periodic/nightly jobs.

I don't actually mind whenever this gets checked in as long as someone is watching trees and willing to either correct or backout should stuff break.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/953617804cb1
Remove HgtoolVCS - Part 1, remove users of hgtool and fixups. r=nthomas
https://hg.mozilla.org/integration/autoland/rev/bfd601e3587c
Remove HgtoolVCS - Part 2, remove actual classes and docs for hgtool. r=nthomas

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/953617804cb1
https://hg.mozilla.org/mozilla-central/rev/bfd601e3587c