1270317 - Replace HgtoolVCS with raw and modern Mercurial commands

Status: RESOLVED FIXED

(Release Engineering :: Applications: MozharnessCore, defect)

Description

[Gregory Szorc [:gps]]

Many of the things that hgtool does can now be performed by Mercurial itself.

In this bug, I'll be overhauling the MercurialVCS class to make it conform the the VCSMixin interface and will make it use modern and efficient hg commands without going through an additional abstraction layer. I'll also start removing usage of HgtoolVCS from mozharness, as we'll be able to use a modern MercurialVCS.

If I'm ambitious, I may remove all uses of hgtool from mozharness.

Comment 1

[Gregory Szorc [:gps]]

Attachment: MozReview Request: Bug 1270317 - Upgrade to Mercurial 3.7.3 in the mozharness test environment; r=jlund

We had a test environment running on Python 2.6 and an ancient version
of Mercurial. AFAICT we run Python 2.7 everywhere, so this environment
can be dropped.

We also upgrade to Mercurial 3.7.3, as that is what automation now runs.

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

Comment 2

[Gregory Szorc [:gps]]

Attachment: MozReview Request: Bug 1270317 - Define a clone_upstream_url property; r=jlund

We currently have a "clone_by_revision" property that indicates to
perform a `hg clone -r`. We use it for cloning from Try.

Cloning single revisions undermines the benefits of clone bundles. So,
I'll be replacing "clone_by_revision" with a feature that clones from
another "upstream" repo then does a `hg pull -r` on the wanted revision.

This commit starts that work by introducing a "clone_upstream_url"
property. We define it on Try. It is currently unused.

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

Comment 3

[Gregory Szorc [:gps]]

Attachment: MozReview Request: Bug 1270317 - Add query_pushinfo to MercurialVCS; r=jlund

MercurialVCS doesn't currently implement the VCSMixin interface.

This commit copies the implementation of query_pushinfo() from
HgtoolVCS to MercurialVCS so it implements the interface.

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

Comment 4

[Gregory Szorc [:gps]]

Attachment: MozReview Request: Bug 1270317 - Use modern, optimal cloning in MercurialVCS.ensure_repo_and_revision(); r=jlund

The code for ensure_repo_and_revision() has been completely overhauled.

We now require shared repos using auto pooled storage via the share
extension. This ensures that only a single copy of a logical
repository's history is stored on disk. e.g. if you clone
mozilla-central, inbound, and try, they'll all automatically share the
same storage.

The new code ensures the destination repo is using modern conventions
and will delete the destination repo if it isn't. So once this gets
deployed to production, machines will slowly start using optimal
storage. This should make VCS operations significantly faster.

Another optimization that is now in here is we check for presence of the
wanted revision before doing `hg pull`. This saves some communication
with the server if the revision is already present locally.

This change effectively requires a modern version of Mercurial to be
installed to run ensure_repo_and_revision(). Since Mercurial <3.7.3 has
security vulnerabilities, we shouldn't be running <3.7.3 in automation.
So I think this will be OK. If not, it will certainly be easy to
identify which machines aren't updated!

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

Comment 5

[Gregory Szorc [:gps]]

Attachment: MozReview Request: Bug 1270317 - Stop using hgtool for Firefox builds; r=jlund

Now that the MercurialVCS VCS tool does things optimally, we no longer
need to use hgtool!

Again, this will effectively require a modern Mercurial version or
things will fail.

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

Comment 6

[Gregory Szorc [:gps]]

Comment on attachment 8748947 [details]
MozReview Request: Bug 1270317 - Upgrade to Mercurial 3.7.3 in the mozharness test environment; r=jlund

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

Comment 7

[Gregory Szorc [:gps]]

Comment on attachment 8748948 [details]
MozReview Request: Bug 1270317 - Define a clone_upstream_url property; r=jlund

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

Comment 8

[Gregory Szorc [:gps]]

Comment on attachment 8748949 [details]
MozReview Request: Bug 1270317 - Add query_pushinfo to MercurialVCS; r=jlund

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

Comment 9

[Gregory Szorc [:gps]]

Comment on attachment 8748950 [details]
MozReview Request: Bug 1270317 - Use modern, optimal cloning in MercurialVCS.ensure_repo_and_revision(); r=jlund

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

Comment 10

[Gregory Szorc [:gps]]

Comment on attachment 8748951 [details]
MozReview Request: Bug 1270317 - Stop using hgtool for Firefox builds; r=jlund

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

Comment 11

[Gregory Szorc [:gps]]

CC glandium and smacleod in case you want to do a drive-by.

Comment 12

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/50639/#review47395

This commit breaks some tests by introducing the requirement that the VCS share base directory be defined (it isn't defined in some tests, it appears).

Also, there's a Try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f6e42077b55.

Comment 13

[Gregory Szorc [:gps]]

Comment on attachment 8748947 [details]
MozReview Request: Bug 1270317 - Upgrade to Mercurial 3.7.3 in the mozharness test environment; r=jlund

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

Comment 14

[Gregory Szorc [:gps]]

Comment on attachment 8748948 [details]
MozReview Request: Bug 1270317 - Define a clone_upstream_url property; r=jlund

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

Comment 15

[Gregory Szorc [:gps]]

Comment on attachment 8748949 [details]
MozReview Request: Bug 1270317 - Add query_pushinfo to MercurialVCS; r=jlund

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

Comment 16

[Gregory Szorc [:gps]]

Comment on attachment 8748950 [details]
MozReview Request: Bug 1270317 - Use modern, optimal cloning in MercurialVCS.ensure_repo_and_revision(); r=jlund

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

Comment 17

[Gregory Szorc [:gps]]

Comment on attachment 8748951 [details]
MozReview Request: Bug 1270317 - Stop using hgtool for Firefox builds; r=jlund

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

Comment 18

[Gregory Szorc [:gps]]

Comment on attachment 8748950 [details]
MozReview Request: Bug 1270317 - Use modern, optimal cloning in MercurialVCS.ensure_repo_and_revision(); r=jlund

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

Comment 19

[Gregory Szorc [:gps]]

Comment on attachment 8748951 [details]
MozReview Request: Bug 1270317 - Stop using hgtool for Firefox builds; r=jlund

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

Comment 20

[Gregory Szorc [:gps]]

Comment on attachment 8748950 [details]
MozReview Request: Bug 1270317 - Use modern, optimal cloning in MercurialVCS.ensure_repo_and_revision(); r=jlund

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

Comment 21

[Gregory Szorc [:gps]]

Comment on attachment 8748951 [details]
MozReview Request: Bug 1270317 - Stop using hgtool for Firefox builds; r=jlund

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

Comment 22

[Gregory Szorc [:gps]]

Comment on attachment 8748950 [details]
MozReview Request: Bug 1270317 - Use modern, optimal cloning in MercurialVCS.ensure_repo_and_revision(); r=jlund

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50639/diff/5-6/

Comment 23

[Gregory Szorc [:gps]]

Comment on attachment 8748951 [details]
MozReview Request: Bug 1270317 - Stop using hgtool for Firefox builds; r=jlund

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50641/diff/5-6/

Comment 24

[Gregory Szorc [:gps]]

Comment on attachment 8748950 [details]
MozReview Request: Bug 1270317 - Use modern, optimal cloning in MercurialVCS.ensure_repo_and_revision(); r=jlund

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50639/diff/6-7/

Comment 25

[Gregory Szorc [:gps]]

Comment on attachment 8748951 [details]
MozReview Request: Bug 1270317 - Stop using hgtool for Firefox builds; r=jlund

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50641/diff/6-7/

Comment 26

[Gregory Szorc [:gps]]

Comment on attachment 8748950 [details]
MozReview Request: Bug 1270317 - Use modern, optimal cloning in MercurialVCS.ensure_repo_and_revision(); r=jlund

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50639/diff/7-8/

Comment 27

[Gregory Szorc [:gps]]

Comment on attachment 8748951 [details]
MozReview Request: Bug 1270317 - Stop using hgtool for Firefox builds; r=jlund

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50641/diff/7-8/

Comment 28

[Gregory Szorc [:gps]]

Comment on attachment 8748950 [details]
MozReview Request: Bug 1270317 - Use modern, optimal cloning in MercurialVCS.ensure_repo_and_revision(); r=jlund

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50639/diff/8-9/

Comment 29

[Gregory Szorc [:gps]]

Comment on attachment 8748951 [details]
MozReview Request: Bug 1270317 - Stop using hgtool for Firefox builds; r=jlund

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50641/diff/8-9/

Comment 30

[Chris AtLee [:catlee]]

Comment on attachment 8748950 [details]
MozReview Request: Bug 1270317 - Use modern, optimal cloning in MercurialVCS.ensure_repo_and_revision(); r=jlund

https://reviewboard.mozilla.org/r/50639/#review47501

::: testing/mozharness/mozharness/base/vcs/mercurial.py:10
(Diff revision 9)
>  # You can obtain one at http://mozilla.org/MPL/2.0/.
>  # ***** END LICENSE BLOCK *****
>  """Mercurial VCS support.
>  
>  Largely copied/ported from
>  https://hg.mozilla.org/build/tools/file/cf265ea8fb5e/lib/python/util/hg.py .

this probably isn't true any more

::: testing/mozharness/mozharness/base/vcs/mercurial.py:336
(Diff revision 9)
> -            # Need better fallback
> -            self.error("Error updating %s from shared_repo (%s): " % (dest, shared_repo))
> -            self.exception(level='error')
> -            self.rmtree(dest)
> -
>      def share(self, source, dest, branch=None, revision=None):

is this method used any more?

::: testing/mozharness/mozharness/base/vcs/mercurial.py:423
(Diff revision 9)
> +        # well-formed shared storage or we have nothing.
> +        created = False
> +
> +        # Create the destination if necessary.
> +        if not os.path.exists(dest):
> +            # We do a full `hg clone` (no `hg clone -r`) because it will use

is there a way to guarantee this happens? if we try and clone try without a bundle, things will fail.

::: testing/mozharness/mozharness/base/vcs/mercurial.py:424
(Diff revision 9)
> +        created = False
> +
> +        # Create the destination if necessary.
> +        if not os.path.exists(dest):
> +            # We do a full `hg clone` (no `hg clone -r`) because it will use
> +            # clone bundles, which are the most effecient way to transfer data.

s/effecient/efficient/

::: testing/mozharness/mozharness/base/vcs/mercurial.py:460
(Diff revision 9)
> +                have_wanted_rev = wanted_rev in output
> +
> +        if not have_wanted_rev:
> +            self.info('pulling to obtain %s' % wanted_rev)
> +            args = self.hg + ['pull', '-r', wanted_rev, repo_url]
> +            # TODO we may want to wrap this in a retry().

We should leave it up to the caller to retry this operation.

::: testing/mozharness/mozharness/base/vcs/mercurial.py:469
(Diff revision 9)
> +        # Now we should have the wanted revision in the store. Perform
> +        # working directory manipulation.
> +
> +        # Do a purge first, if requested. We purge first because this way we're
> +        # guaranteed to not have conflicts on `hg update`.
> +        if purge and not created:

I think mozharness will need to be taught how to purge properly on windows. See https://bugzilla.mozilla.org/show_bug.cgi?id=1157704 for how we did it in hgtool

Comment 31

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/50639/#review47501

> is this method used any more?

Nope. I'll nuke it in the next submission.

> is there a way to guarantee this happens? if we try and clone try without a bundle, things will fail.

I'm fine with failing fast in this scenario, as it means there is a configuration problem.

> I think mozharness will need to be taught how to purge properly on windows. See https://bugzilla.mozilla.org/show_bug.cgi?id=1157704 for how we did it in hgtool

I'll add the purgelong extension to mozharness.

Comment 32

[Gregory Szorc [:gps]]

Attachment: MozReview Request: Bug 1270317 - Add the purgelong Mercurial extension; r=jlund

The build/tools repo has a "purgelong" extension that is used to delete
long filenames on Windows. Without this extension, the APIs Mercurial
uses may run into path length issues and `hg purge` will fail.

This commit is a straight import of the purgelong extension
from https://hg.mozilla.org/build/tools minus the shebang line, which
isn't needed.

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

Comment 33

[Gregory Szorc [:gps]]

Attachment: MozReview Request: Bug 1270317 - Use modern exception syntax in purgelong; r=jlund

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

Comment 34

[Gregory Szorc [:gps]]

Attachment: MozReview Request: Bug 1270317 - Remove purgelong compatibility with ancient Mercurial versions; r=jlund

We no longer use <3.7 in automation. So drop support for <3.2. While I
was here, I also added magic variables to the extension so Mercurial
can react intelligently to version compatibility issues.

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

Comment 35

[Gregory Szorc [:gps]]

Comment on attachment 8748947 [details]
MozReview Request: Bug 1270317 - Upgrade to Mercurial 3.7.3 in the mozharness test environment; r=jlund

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

Comment 36

[Gregory Szorc [:gps]]

Comment on attachment 8748948 [details]
MozReview Request: Bug 1270317 - Define a clone_upstream_url property; r=jlund

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

Comment 37

[Gregory Szorc [:gps]]

Comment on attachment 8748949 [details]
MozReview Request: Bug 1270317 - Add query_pushinfo to MercurialVCS; r=jlund

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

Comment 38

[Gregory Szorc [:gps]]

Comment on attachment 8748950 [details]
MozReview Request: Bug 1270317 - Use modern, optimal cloning in MercurialVCS.ensure_repo_and_revision(); r=jlund

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50639/diff/9-10/

Comment 39

[Gregory Szorc [:gps]]

Comment on attachment 8748951 [details]
MozReview Request: Bug 1270317 - Stop using hgtool for Firefox builds; r=jlund

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50641/diff/9-10/

Comment 40

[Gregory Szorc [:gps]]

Comment on attachment 8748947 [details]
MozReview Request: Bug 1270317 - Upgrade to Mercurial 3.7.3 in the mozharness test environment; r=jlund

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

Comment 41

[Gregory Szorc [:gps]]

Comment on attachment 8749323 [details]
MozReview Request: Bug 1270317 - Add the purgelong Mercurial extension; r=jlund

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

Comment 42

[Gregory Szorc [:gps]]

Comment on attachment 8749324 [details]
MozReview Request: Bug 1270317 - Use modern exception syntax in purgelong; r=jlund

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

Comment 43

[Gregory Szorc [:gps]]

Comment on attachment 8749325 [details]
MozReview Request: Bug 1270317 - Remove purgelong compatibility with ancient Mercurial versions; r=jlund

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

Comment 44

[Gregory Szorc [:gps]]

Comment on attachment 8748948 [details]
MozReview Request: Bug 1270317 - Define a clone_upstream_url property; r=jlund

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

Comment 45

[Gregory Szorc [:gps]]

Comment on attachment 8748949 [details]
MozReview Request: Bug 1270317 - Add query_pushinfo to MercurialVCS; r=jlund

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

Comment 46

[Gregory Szorc [:gps]]

Comment on attachment 8748950 [details]
MozReview Request: Bug 1270317 - Use modern, optimal cloning in MercurialVCS.ensure_repo_and_revision(); r=jlund

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50639/diff/10-11/

Comment 47

[Gregory Szorc [:gps]]

Comment on attachment 8748951 [details]
MozReview Request: Bug 1270317 - Stop using hgtool for Firefox builds; r=jlund

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50641/diff/10-11/

Comment 48

[Jordan Lund (:jlund)]

https://reviewboard.mozilla.org/r/50631/#review47587

so much cleaner! I'm not up to date with current hg versions across all our infra slaves. If we are up to date with the version expected/defined in requirements.txt, ++ to this patch. tyvm for this

::: testing/mozharness/configs/builds/branch_specifics.py:187
(Diff revision 10)
>      },
>      'try': {
>          'repo_path': 'try',
>          'clone_by_revision': True,
>          'clone_with_purge': True,
> +        # FUTURE this should be a unified repo because Try pushes may e.g.for

looks like typo: `may e.g.for be`

::: testing/mozharness/mozharness/base/vcs/mercurial.py:364
(Diff revision 10)
> +        share_base = c.get('vcs_share_base', os.environ.get('HG_SHARE_BASE_DIR', None))
> +
> +        # We require shared storage is configured because it guarantees we
> +        # only have 1 local copy of logical repo stores.
> +        if not share_base:
> +            self.fatal('vcs share base not defined; refusing to operate sub-optimally',

hmm, I wonder how this will affect Taskcluster tasks and developer local runs..

::: testing/mozharness/mozharness/base/vcs/mercurial.py:397
(Diff revision 10)
> -            if not c.get('allow_unshared_local_clones'):
> -                # Re-raise the exception so it gets caught below.
> -                # We'll then clobber dest, and clone from original
> -                # repo
> +
> +            # FUTURE when we require generaldelta, this is where we can check
> +            # for that.
> +        except IOError as e:
> +            if e.errno != errno.ENOENT:
>                  raise

wondering if we could hit a situation where dest_hg_sharedpath raises ENOENT and we silently continue without running self.rmtree(dest)

... might be wrong or could be overkill

::: testing/mozharness/mozharness/base/vcs/mercurial.py:408
(Diff revision 10)
> -        # OS supports it
> -        try:
> -            self.clone(shared_repo, dest, update_dest=False)
> -            return self.update(dest, branch=branch, revision=revision)
> -        except VCSException:
> -            # Need better fallback
> +        # Create the destination if necessary.
> +        if not os.path.exists(dest):
> +            # We do a full `hg clone` (no `hg clone -r`) because it will use
> +            # clone bundles, which are the most efficient way to transfer data.
> +            # `hg clone` with pooled storage automatically performs a
> +            # share under the hood. So no additional share magic is necessary.

\o/

::: testing/mozharness/mozharness/base/vcs/mercurial.py:409
(Diff revision 10)
> -        try:
> -            self.clone(shared_repo, dest, update_dest=False)
> -            return self.update(dest, branch=branch, revision=revision)
> -        except VCSException:
> -            # Need better fallback
> -            self.error("Error updating %s from shared_repo (%s): " % (dest, shared_repo))
> +        if not os.path.exists(dest):
> +            # We do a full `hg clone` (no `hg clone -r`) because it will use
> +            # clone bundles, which are the most efficient way to transfer data.
> +            # `hg clone` with pooled storage automatically performs a
> +            # share under the hood. So no additional share magic is necessary.
> +            clone_url = upstream if upstream else repo_url

not even a nit: I tend to `a = b or c` rather than `a = b if b else c`. ooc, why do you prefer the latter? readibility?

::: testing/mozharness/mozharness/base/vcs/purgelong.py:2
(Diff revision 10)
> +'''
> +Mercurial extension to enable purging long filenames on Windows

being independant of mozharness, I feel like this file should be in http://hg.mozilla.org/mozilla-central/file/e5a10bc7dac4/testing/mozharness/external_tools

Comment 49

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/50631/#review47587

> hmm, I wonder how this will affect Taskcluster tasks and developer local runs..

TC uses its own version control management (tc-vcs). Many (all?) of the TC tasks skip the checkout-sources mozharness step. TC is a separate beast (which I may tackle after this lands).

Good point regarding developer local runs. I'll have to think about this a bit more...

Comment 50

[Gregory Szorc [:gps]]

Comment on attachment 8749323 [details]
MozReview Request: Bug 1270317 - Add the purgelong Mercurial extension; r=jlund

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

Comment 51

[Gregory Szorc [:gps]]

Comment on attachment 8749324 [details]
MozReview Request: Bug 1270317 - Use modern exception syntax in purgelong; r=jlund

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

Comment 52

[Gregory Szorc [:gps]]

Comment on attachment 8749325 [details]
MozReview Request: Bug 1270317 - Remove purgelong compatibility with ancient Mercurial versions; r=jlund

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

Comment 53

[Gregory Szorc [:gps]]

Comment on attachment 8748948 [details]
MozReview Request: Bug 1270317 - Define a clone_upstream_url property; r=jlund

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50635/diff/5-6/

Comment 54

[Gregory Szorc [:gps]]

Comment on attachment 8748949 [details]
MozReview Request: Bug 1270317 - Add query_pushinfo to MercurialVCS; r=jlund

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50637/diff/5-6/

Comment 55

[Gregory Szorc [:gps]]

Comment on attachment 8748950 [details]
MozReview Request: Bug 1270317 - Use modern, optimal cloning in MercurialVCS.ensure_repo_and_revision(); r=jlund

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50639/diff/11-12/

Comment 56

[Gregory Szorc [:gps]]

Comment on attachment 8748951 [details]
MozReview Request: Bug 1270317 - Stop using hgtool for Firefox builds; r=jlund

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50641/diff/11-12/

Comment 57

[Gregory Szorc [:gps]]

Attachment: MozReview Request: Bug 1270317 - Record hg version and install info; r=jlund

hgtool printed the hg version info when running. This is useful data
when debugging Mercurial failures. Add it back in.

We also add `hg debuginstall`, which prints useful bits about the
install, including the Python path and version.

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

Comment 58

[Jordan Lund (:jlund)]

Comment on attachment 8748947 [details]
MozReview Request: Bug 1270317 - Upgrade to Mercurial 3.7.3 in the mozharness test environment; r=jlund

https://reviewboard.mozilla.org/r/50633/#review47787

Comment 59

[Jordan Lund (:jlund)]

Comment on attachment 8749323 [details]
MozReview Request: Bug 1270317 - Add the purgelong Mercurial extension; r=jlund

https://reviewboard.mozilla.org/r/50851/#review47789

Comment 60

[Jordan Lund (:jlund)]

Comment on attachment 8749324 [details]
MozReview Request: Bug 1270317 - Use modern exception syntax in purgelong; r=jlund

https://reviewboard.mozilla.org/r/50853/#review47793

Comment 61

[Jordan Lund (:jlund)]

Comment on attachment 8749325 [details]
MozReview Request: Bug 1270317 - Remove purgelong compatibility with ancient Mercurial versions; r=jlund

https://reviewboard.mozilla.org/r/50855/#review47795

looks like we probably still use purgelong from its original location (tools repo) https://dxr.mozilla.org/build-central/source/tools/lib/python/util/hg.py#180

I suppose we should apply the same patch in the tools repo?

Comment 62

[Jordan Lund (:jlund)]

Comment on attachment 8748948 [details]
MozReview Request: Bug 1270317 - Define a clone_upstream_url property; r=jlund

https://reviewboard.mozilla.org/r/50635/#review47797

Comment 63

[Jordan Lund (:jlund)]

Comment on attachment 8748949 [details]
MozReview Request: Bug 1270317 - Add query_pushinfo to MercurialVCS; r=jlund

https://reviewboard.mozilla.org/r/50637/#review47803

it would be great to kill hgtool.py (from base/vcs/ and external_tools/) and replace usage soley with mercurial.py :)

Comment 64

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/50637/#review47803

That is my eventual goal. I'm inclined to defer to a follow-up bug: there is a long tail of mozharness scripts and configs still using hgtool and I want to get the big fish (Firefox builds) out of the way first.

Comment 65

[Jordan Lund (:jlund)]

Comment on attachment 8748950 [details]
MozReview Request: Bug 1270317 - Use modern, optimal cloning in MercurialVCS.ensure_repo_and_revision(); r=jlund

https://reviewboard.mozilla.org/r/50639/#review47805

this looks good to me. one small nit

::: testing/mozharness/mozharness/base/vcs/mercurial.py:323
(Diff revisions 10 - 12)
>  
>      @property
>      def purgelong_path(self):
>          """Path to the purgelong extension."""
>          here = os.path.abspath(os.path.dirname(__file__))
> -        ext = os.path.join(here, 'purgelong.py')
> +        ext = os.path.join(here, '..', '..', '..', 'external_tools',

I prefer this abspath style of finding external_tools to .. rel path.

https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/mozharness/mozilla/purge.py#14

Comment 66

[Jordan Lund (:jlund)]

Comment on attachment 8748951 [details]
MozReview Request: Bug 1270317 - Stop using hgtool for Firefox builds; r=jlund

https://reviewboard.mozilla.org/r/50641/#review47809

\o/

Comment 67

[Jordan Lund (:jlund)]

Comment on attachment 8749466 [details]
MozReview Request: Bug 1270317 - Record hg version and install info; r=jlund

https://reviewboard.mozilla.org/r/50971/#review47811

Comment 68

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/50855/#review47795

If I have my way, we'll soon kill hgtool, which is the only place purgelong is used. So I'm inclined to not do this.

Comment 69

[Gregory Szorc [:gps]]

Comment on attachment 8748947 [details]
MozReview Request: Bug 1270317 - Upgrade to Mercurial 3.7.3 in the mozharness test environment; r=jlund

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50633/diff/5-6/

Comment 70

[Gregory Szorc [:gps]]

Comment on attachment 8749323 [details]
MozReview Request: Bug 1270317 - Add the purgelong Mercurial extension; r=jlund

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

Comment 71

[Gregory Szorc [:gps]]

Comment on attachment 8749324 [details]
MozReview Request: Bug 1270317 - Use modern exception syntax in purgelong; r=jlund

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

Comment 72

[Gregory Szorc [:gps]]

Comment on attachment 8749325 [details]
MozReview Request: Bug 1270317 - Remove purgelong compatibility with ancient Mercurial versions; r=jlund

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

Comment 73

[Gregory Szorc [:gps]]

Comment on attachment 8748948 [details]
MozReview Request: Bug 1270317 - Define a clone_upstream_url property; r=jlund

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50635/diff/6-7/

Comment 74

[Gregory Szorc [:gps]]

Comment on attachment 8748949 [details]
MozReview Request: Bug 1270317 - Add query_pushinfo to MercurialVCS; r=jlund

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50637/diff/6-7/

Comment 75

[Gregory Szorc [:gps]]

Comment on attachment 8748950 [details]
MozReview Request: Bug 1270317 - Use modern, optimal cloning in MercurialVCS.ensure_repo_and_revision(); r=jlund

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50639/diff/12-13/

Comment 76

[Gregory Szorc [:gps]]

Comment on attachment 8748951 [details]
MozReview Request: Bug 1270317 - Stop using hgtool for Firefox builds; r=jlund

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50641/diff/12-13/

Comment 77

[Gregory Szorc [:gps]]

Comment on attachment 8749466 [details]
MozReview Request: Bug 1270317 - Record hg version and install info; r=jlund

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

Comment 78

[Jordan Lund (:jlund)]

Comment on attachment 8748950 [details]
MozReview Request: Bug 1270317 - Use modern, optimal cloning in MercurialVCS.ensure_repo_and_revision(); r=jlund

https://reviewboard.mozilla.org/r/50639/#review47817

Comment 79

[Gregory Szorc [:gps]]

Comment on attachment 8748947 [details]
MozReview Request: Bug 1270317 - Upgrade to Mercurial 3.7.3 in the mozharness test environment; r=jlund

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50633/diff/6-7/

Comment 80

[Gregory Szorc [:gps]]

Comment on attachment 8749323 [details]
MozReview Request: Bug 1270317 - Add the purgelong Mercurial extension; r=jlund

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

Comment 81

[Gregory Szorc [:gps]]

Comment on attachment 8749324 [details]
MozReview Request: Bug 1270317 - Use modern exception syntax in purgelong; r=jlund

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

Comment 82

[Gregory Szorc [:gps]]

Comment on attachment 8749325 [details]
MozReview Request: Bug 1270317 - Remove purgelong compatibility with ancient Mercurial versions; r=jlund

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

Comment 83

[Gregory Szorc [:gps]]

Comment on attachment 8748948 [details]
MozReview Request: Bug 1270317 - Define a clone_upstream_url property; r=jlund

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50635/diff/7-8/

Comment 84

[Gregory Szorc [:gps]]

Comment on attachment 8748949 [details]
MozReview Request: Bug 1270317 - Add query_pushinfo to MercurialVCS; r=jlund

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50637/diff/7-8/

Comment 85

[Gregory Szorc [:gps]]

Comment on attachment 8748950 [details]
MozReview Request: Bug 1270317 - Use modern, optimal cloning in MercurialVCS.ensure_repo_and_revision(); r=jlund

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50639/diff/13-14/

Comment 86

[Gregory Szorc [:gps]]

Comment on attachment 8748951 [details]
MozReview Request: Bug 1270317 - Stop using hgtool for Firefox builds; r=jlund

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50641/diff/13-14/

Comment 87

[Gregory Szorc [:gps]]

Comment on attachment 8749466 [details]
MozReview Request: Bug 1270317 - Record hg version and install info; r=jlund

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

Comment 88

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b893641e2d9f
https://hg.mozilla.org/integration/mozilla-inbound/rev/b01744f2d97d
https://hg.mozilla.org/integration/mozilla-inbound/rev/e679c2e0b1b5
https://hg.mozilla.org/integration/mozilla-inbound/rev/e97f355e24c9
https://hg.mozilla.org/integration/mozilla-inbound/rev/214b6958340a
https://hg.mozilla.org/integration/mozilla-inbound/rev/20cb1be5da65
https://hg.mozilla.org/integration/mozilla-inbound/rev/86fccb7da8ac
https://hg.mozilla.org/integration/mozilla-inbound/rev/beded14a92c5
https://hg.mozilla.org/integration/mozilla-inbound/rev/273d52aff2ad

Comment 89

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b893641e2d9f
https://hg.mozilla.org/mozilla-central/rev/b01744f2d97d
https://hg.mozilla.org/mozilla-central/rev/e679c2e0b1b5
https://hg.mozilla.org/mozilla-central/rev/e97f355e24c9
https://hg.mozilla.org/mozilla-central/rev/214b6958340a
https://hg.mozilla.org/mozilla-central/rev/20cb1be5da65
https://hg.mozilla.org/mozilla-central/rev/86fccb7da8ac
https://hg.mozilla.org/mozilla-central/rev/beded14a92c5
https://hg.mozilla.org/mozilla-central/rev/273d52aff2ad

Comment 90

[Pulsebot]

Backout:
https://hg.mozilla.org/mozilla-central/rev/e181d853889e
https://hg.mozilla.org/mozilla-central/rev/57805a2e6627
https://hg.mozilla.org/mozilla-central/rev/40e92de8aff7
https://hg.mozilla.org/mozilla-central/rev/fb78e9b459c4
https://hg.mozilla.org/mozilla-central/rev/2f71ed818fd8
https://hg.mozilla.org/mozilla-central/rev/00f66c012ddc
https://hg.mozilla.org/mozilla-central/rev/2fbd433ca1a1
https://hg.mozilla.org/mozilla-central/rev/97ea348edccf
https://hg.mozilla.org/mozilla-central/rev/043082cb7bd8

Comment 91

[Carsten Book [:Tomcat]]

seems some builders were not happy about this, after landing and periodic clobber this broke it seems nightly builds like https://treeherder.mozilla.org/logviewer.html#?job_id=3847233&repo=mozilla-central

with errors like:

Copy/paste: hg --config ui.merge=internal:merge --config extensions.share= --config share.pool=C:/builds/hg-shared clone -U https://hg.mozilla.org/build/tools c:\builds\moz2_slave\m-cen-w32-ntly-000000000000000\build\tools

 03:07:15     INFO -  (sharing from new pooled repository 7ae7fb134bf7aec6ec96a062ff47a69053dd2973)
 03:07:15     INFO -  applying clone bundle from https://s3-external-1.amazonaws.com/moz-hg-bundles-us-east-1/build/tools/b030af490d9395958c3dc17443204037faf6a991.packed1.hg
 03:07:15     INFO -  2005 files to transfer, 26.7 MB of data
 03:07:15     INFO -  transferred 26.7 MB in 5.5 seconds (4.88 MB/sec)
 03:07:15     INFO -  finished applying clone bundle
 03:07:15     INFO -  searching for changes
 03:07:15     INFO -  no changes found
 03:07:15     INFO -  Traceback (most recent call last):
 03:07:15     INFO -    File "mercurial\dispatch.pyc", line 191, in _runcatch
 03:07:15     INFO -    File "mercurial\dispatch.pyc", line 924, in _dispatch
 03:07:15     INFO -    File "mercurial\dispatch.pyc", line 681, in runcommand
 03:07:15     INFO -    File "mercurial\extensions.pyc", line 195, in closure
 03:07:15     INFO -    File "hgext\color.pyc", line 518, in colorcmd
 03:07:15     INFO -    File "mercurial\dispatch.pyc", line 1055, in _runcommand
 03:07:15     INFO -    File "mercurial\dispatch.pyc", line 1015, in checkargs
 03:07:15     INFO -    File "mercurial\dispatch.pyc", line 921, in <lambda>
 03:07:15     INFO -    File "mercurial\util.pyc", line 991, in check
 03:07:15     INFO -    File "mercurial\extensions.pyc", line 195, in closure
 03:07:15     INFO -    File "mercurial\util.pyc", line 991, in check
 03:07:15     INFO -    File "hgext\share.pyc", line 122, in clone
 03:07:15     INFO -    File "mercurial\util.pyc", line 991, in check
 03:07:15     INFO -    File "mercurial\commands.pyc", line 1563, in clone
 03:07:15     INFO -    File "mercurial\hg.pyc", line 489, in clone
 03:07:15     INFO -    File "mercurial\hg.pyc", line 372, in clonewithshare
 03:07:15     INFO -    File "mercurial\hg.pyc", line 223, in share
 03:07:15     INFO -    File "mercurial\scmutil.pyc", line 364, in mkdir
 03:07:15     INFO -  WindowsError: [Error 3] The system cannot find the path specified: 'c:\\builds\\moz2_slave\\m-cen-w32-ntly-000000000000000\\build\\tools'
 03:07:15     INFO -  abort: The system cannot find the path specified: 'c:\builds\moz2_slave\m-cen-w32-ntly-000000000000000\build\tools'
03:07:15 ERROR - Return code: 255 

so backedout this out to hopefully unbreak the builds :)

Comment 92

[Gregory Szorc [:gps]]

Looks like clobberer removes the c:\builds\moz2_slave\m-cen-w32-ntly-000000000000000\build parent directory. We just need to ensure the parent directory exists before doing the clone. Should be a trivial fix-up.

Comment 94

[Gregory Szorc [:gps]]

Comment on attachment 8748947 [details]
MozReview Request: Bug 1270317 - Upgrade to Mercurial 3.7.3 in the mozharness test environment; r=jlund

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50633/diff/7-8/

Comment 95

[Gregory Szorc [:gps]]

Comment on attachment 8749323 [details]
MozReview Request: Bug 1270317 - Add the purgelong Mercurial extension; r=jlund

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50851/diff/5-6/

Comment 96

[Gregory Szorc [:gps]]

Comment on attachment 8749324 [details]
MozReview Request: Bug 1270317 - Use modern exception syntax in purgelong; r=jlund

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50853/diff/5-6/

Comment 97

[Gregory Szorc [:gps]]

Comment on attachment 8749325 [details]
MozReview Request: Bug 1270317 - Remove purgelong compatibility with ancient Mercurial versions; r=jlund

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50855/diff/5-6/

Comment 98

[Gregory Szorc [:gps]]

Comment on attachment 8748948 [details]
MozReview Request: Bug 1270317 - Define a clone_upstream_url property; r=jlund

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50635/diff/8-9/

Comment 99

[Gregory Szorc [:gps]]

Comment on attachment 8748949 [details]
MozReview Request: Bug 1270317 - Add query_pushinfo to MercurialVCS; r=jlund

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50637/diff/8-9/

Comment 100

[Gregory Szorc [:gps]]

Comment on attachment 8748950 [details]
MozReview Request: Bug 1270317 - Use modern, optimal cloning in MercurialVCS.ensure_repo_and_revision(); r=jlund

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50639/diff/14-15/

Comment 101

[Gregory Szorc [:gps]]

Comment on attachment 8748951 [details]
MozReview Request: Bug 1270317 - Stop using hgtool for Firefox builds; r=jlund

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50641/diff/14-15/

Comment 102

[Gregory Szorc [:gps]]

Comment on attachment 8749466 [details]
MozReview Request: Bug 1270317 - Record hg version and install info; r=jlund

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

Comment 103

[Gregory Szorc [:gps]]

Comment on attachment 8748950 [details]
MozReview Request: Bug 1270317 - Use modern, optimal cloning in MercurialVCS.ensure_repo_and_revision(); r=jlund

Please look at the interdiff of just this one to see the fixes for missing parent directory and abandoned transaction issues.

Comment 104

[Jordan Lund (:jlund)]

Comment on attachment 8748950 [details]
MozReview Request: Bug 1270317 - Use modern, optimal cloning in MercurialVCS.ensure_repo_and_revision(); r=jlund

https://reviewboard.mozilla.org/r/50639/#review48541

looks good. couple questions below

::: testing/mozharness/mozharness/base/vcs/mercurial.py:40
(Diff revisions 13 - 15)
>  # build/tools/scripts to MercurialVCS -- generic tagging logic belongs here.
>  REVISION, BRANCH = 0, 1
>  
>  
> -class RepositoryUnrelatedParser(OutputParser):
> -    """Parse `hg pull` output for "repository unrelated" errors."""
> +class MercurialErrorParser(OutputParser):
> +    """Parse cloen and pull output for various errors."""

s/cloen/clone

::: testing/mozharness/mozharness/base/vcs/mercurial.py:399
(Diff revisions 13 - 15)
>                  self.rmtree(dest)
>  
>              # FUTURE when we require generaldelta, this is where we can check
>              # for that.
>  
> +        def recover_from_abandoned_transaction(parser):

wondering if this should be defined outside of ensure_repo_and_revision impl

::: testing/mozharness/mozharness/base/vcs/mercurial.py:405
(Diff revisions 13 - 15)
> +            # We can run `hg recover` in the destination repo because it is
> +            # shared and `hg recover` always operates on the shared store.
> +            if not self.run_command(self.hg + ['recover'], cwd=dest):
> +                return
> +
> +            with open(os.path.join(dest, '.hg', 'sharedpath'), 'rb') as fh:

now that we are opening and reading from this twice, maybe we should have a helper method for this. Or better yet, use this one each time we read sharedpath:

self.read_from_file()

https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/mozharness/base/script.py#773

::: testing/mozharness/mozharness/base/vcs/mercurial.py:421
(Diff revisions 13 - 15)
>  
>          # Create the destination if necessary.
>          if not os.path.exists(dest):
> +            # Ensure parent directory exists.
> +            try:
> +                os.makedirs(os.path.dirname(dest))

fyi for next time, you should be able to safely call: self.mkdir_p(os.path.dirname(dest))

https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/mozharness/base/script.py#146

::: testing/mozharness/test/test_base_vcs_mercurial.py:375
(Diff revisions 13 - 15)
> +            'branch': 'default',
> +            'vcs_share_base': os.path.join(self.tmpdir, 'share'),
> +        }
> +        m.ensure_repo_and_revision()
> +
> +        with open(os.path.join(self.wc, '.hg', 'sharedpath'), 'rb') as fh:

in addition to whereever we read sharedpath in the mercurial.py script, we can use m.read_from_file() anywhere in these tests too

::: testing/mozharness/test/test_base_vcs_mercurial.py:386
(Diff revisions 13 - 15)
> +
> +        # Touching the journal file simulates an abondoned transaction.
> +        with open(os.path.join(shared_path, 'store', 'journal'), 'a'):
> +            pass
> +
> +        m.vcs_config['dest'] = os.path.join(self.tmpdir, 'dest2')

sorry, could you explain what's happening here and how this test works?

Comment 105

[Gregory Szorc [:gps]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3dff69b69da4

Comment 106

[Gregory Szorc [:gps]]

Attachment: MozReview Request: Bug 1270317 - Add robustcheckout.py Mercurial extension; r?jlund

Copy of https://hg.mozilla.org/hgcustom/version-control-tools:hgext/robustcheckout/__init__.py
at revision XXX.

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

Comment 107

[Gregory Szorc [:gps]]

Attachment: MozReview Request: Bug 1270317 - Use robustcheckout extension for checking out Mercurial repos; r?jlund

Functionality for doing an optimal clone/pull+share+purge+update is now
implemented in the robustcheckout extension so it can be implemented in one
place and used by all the various tools needing to perform a "robust"
checkout using optimal practices.

This commit switches the MercurialVCS to use it.

Functionality for interfacing with shared repos and associated tests have
been removed because this is all implemented and tested in robustcheckout.

Various other tests have also been removed because they are redundant with
tests in the robustcheckout extension.

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

Comment 108

[Gregory Szorc [:gps]]

Comment on attachment 8748947 [details]
MozReview Request: Bug 1270317 - Upgrade to Mercurial 3.7.3 in the mozharness test environment; r=jlund

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50633/diff/8-9/

Comment 109

[Gregory Szorc [:gps]]

Comment on attachment 8748948 [details]
MozReview Request: Bug 1270317 - Define a clone_upstream_url property; r=jlund

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50635/diff/9-10/

Comment 110

[Gregory Szorc [:gps]]

Comment on attachment 8748949 [details]
MozReview Request: Bug 1270317 - Add query_pushinfo to MercurialVCS; r=jlund

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50637/diff/9-10/

Comment 111

[Gregory Szorc [:gps]]

Comment on attachment 8748951 [details]
MozReview Request: Bug 1270317 - Stop using hgtool for Firefox builds; r=jlund

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50641/diff/15-16/

Comment 112

[Gregory Szorc [:gps]]

Comment on attachment 8749466 [details]
MozReview Request: Bug 1270317 - Record hg version and install info; r=jlund

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

Comment 113

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/53254/#review50048

New plan here.

Instead of implementing the "do a checkout" logic in multiple places (namely mozharness, hgtool, and [eventually] tc-vcs), I've created a Mercurial extension that performs a clone+share+checkout using best practices. This is a standalone file that provides a command that we can call into. This makes the code in mozharness and hgtool simpler and mostly minimal.

Comment 114

[Gregory Szorc [:gps]]

Comment on attachment 8748947 [details]
MozReview Request: Bug 1270317 - Upgrade to Mercurial 3.7.3 in the mozharness test environment; r=jlund

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50633/diff/9-10/

Comment 115

[Gregory Szorc [:gps]]

Comment on attachment 8748948 [details]
MozReview Request: Bug 1270317 - Define a clone_upstream_url property; r=jlund

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50635/diff/10-11/

Comment 116

[Gregory Szorc [:gps]]

Comment on attachment 8748949 [details]
MozReview Request: Bug 1270317 - Add query_pushinfo to MercurialVCS; r=jlund

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50637/diff/10-11/

Comment 117

[Gregory Szorc [:gps]]

Comment on attachment 8753414 [details]
MozReview Request: Bug 1270317 - Add robustcheckout.py Mercurial extension; r?jlund

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

Comment 118

[Gregory Szorc [:gps]]

Comment on attachment 8753415 [details]
MozReview Request: Bug 1270317 - Use robustcheckout extension for checking out Mercurial repos; r?jlund

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

Comment 119

[Gregory Szorc [:gps]]

Comment on attachment 8748951 [details]
MozReview Request: Bug 1270317 - Stop using hgtool for Firefox builds; r=jlund

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50641/diff/16-17/

Comment 120

[Gregory Szorc [:gps]]

Comment on attachment 8749466 [details]
MozReview Request: Bug 1270317 - Record hg version and install info; r=jlund

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50971/diff/5-6/

Comment 121

[Jordan Lund (:jlund)]

ran out of time to review this today. will look tomorrow

Comment 122

[Jordan Lund (:jlund)]

Comment on attachment 8753414 [details]
MozReview Request: Bug 1270317 - Add robustcheckout.py Mercurial extension; r?jlund

https://reviewboard.mozilla.org/r/53252/#review50430

stamping purely as support to adding this extension, not a review of the extention itself as it has been reviewed elsewhere.

Comment 123

[Jordan Lund (:jlund)]

Comment on attachment 8753415 [details]
MozReview Request: Bug 1270317 - Use robustcheckout extension for checking out Mercurial repos; r?jlund

https://reviewboard.mozilla.org/r/53254/#review50426

before I stamp:

iiuc,

in mozharness: we use and enforce the use of robustcheckout everywhere that currently calls MercurialVCS.ensure_repo_and_revision()?

in tools repo: we use but not enforce robustcheckout from anywhere that calls lib/python/hg.py's mecurial() which include hgtool.py release-runner, l10n.py, etc. we don't enforce because of the variety of ways each of these call mercurial()?

is that correct?

this leaves me wondering what this means for mozharness's copy of tools' hgtool.py and mozharness's own version of hgtool, HgtoolVCS
  * https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/mozharness/base/vcs/hgtool.py
      * used:
          * https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/mozharness/base/vcs/vcsbase.py#27
          * https://dxr.mozilla.org/mozilla-central/search?q=path%3Amozharnes+path%3Aconfigs+hgtool&redirect=true&case=false
  * https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/external_tools/hgtool.py
      * used:
          *  https://dxr.mozilla.org/mozilla-central/search?q=path%3Amozharnes+external_tools+hgtool.py&redirect=false&case=false

we have too many hg "tools" :(

::: testing/mozharness/mozharness/base/vcs/mercurial.py:347
(Diff revision 2)
> +        # The API here is kind of bad because we're relying on state in
> +        # self.vcs_config instead of passing arguments. This confuses
> +        # scripts that have multiple repos. This includes the clone_tools()
> +        # step :(

hm, can't each repo have its own vcs_config defined in self.config["repos"] so that we can isolate prefs in vcs_checkout_repos()[1] or even add custom configs (kwargs) to vcs_checkout() call directly?

Or are you saying that scripts are not using that correctly?

[1] https://dxr.mozilla.org/mozilla-central/search?q=vcs_checkout_repos&=mozilla-central&redirect=true

::: testing/mozharness/mozharness/base/vcs/mercurial.py:362
(Diff revision 2)
> +
> +        # We require shared storage is configured because it guarantees we
> +        # only have 1 local copy of logical repo stores.
> +        if not share_base:
> +            raise VCSException('vcs share base not defined; '
> +                               'refusing to operate sub-optimally')

lolz. I suppose its not too bad to enforce this even for devs

::: testing/mozharness/mozharness/base/vcs/mercurial.py:384
(Diff revision 2)
> -            share_base = None
>  
> -        if share_base:
> -            return self._ensure_shared_repo_and_revision(share_base)
> +        parser = RepositoryUpdateRevisionParser(config=self.config,
> +                                                log_obj=self.log_obj)
> +        if self.run_command(args, output_parser=parser):
> +            raise VCSException('repo checkout failed!')

++ to using extension/external_tool to control the complexity

Comment 124

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/53254/#review50426

That assessment is correct.

I wasn't willing to change all consumers to use robustcheckout yet. The main reason is I'm a bit scared of random processes breaking. I'd rather take baby steps.

I'd like to eventually switch everything in mozharness to use MercurialVCS and get rid of HgtoolVCS. I'd also like all consumers of hgtool (if there are any remaining) to use robustcheckout. These can be follow-up bugs.

I also plan to land the hgtool changes a day or two after these mozharness changes. I feel like hgtool changes are higher risk and want the dust to settle before I land them.

> hm, can't each repo have its own vcs_config defined in self.config["repos"] so that we can isolate prefs in vcs_checkout_repos()[1] or even add custom configs (kwargs) to vcs_checkout() call directly?
> 
> Or are you saying that scripts are not using that correctly?
> 
> [1] https://dxr.mozilla.org/mozilla-central/search?q=vcs_checkout_repos&=mozilla-central&redirect=true

I don't think scripts are doing that properly. I'm basically cargo culting behavior from before the patch. I put the comment in because I figured this seemingly incorrect behavior should be documented. I'm inclined to address things as a follow-up.

> lolz. I suppose its not too bad to enforce this even for devs

We had landed this before and nobody complained. I'm inclined to land it again and see if anyone notices. I /think/ most local users of mozharness already have checkouts so they never encounter this. I could be wrong. I'm inclined to cross the bridge if we come to it.

Comment 125

[Jordan Lund (:jlund)]

Comment on attachment 8753415 [details]
MozReview Request: Bug 1270317 - Use robustcheckout extension for checking out Mercurial repos; r?jlund

https://reviewboard.mozilla.org/r/53254/#review50450

Comment 126

[Jordan Lund (:jlund)]

https://reviewboard.mozilla.org/r/53254/#review50426

okay, this works for me too. getting follow up bugs on file and making comments under HgtoolVCS and external_tools/hgtool.py of their deprecation. :D

I'm excited for this patch's clean up and the thought of dealing with all edge cases in the future :)

> I don't think scripts are doing that properly. I'm basically cargo culting behavior from before the patch. I put the comment in because I figured this seemingly incorrect behavior should be documented. I'm inclined to address things as a follow-up.

okay, would you mind getting a bug on file to ensure proper vcs_config kwargs being passed everywhere and add the bug number in this comment?

> We had landed this before and nobody complained. I'm inclined to land it again and see if anyone notices. I /think/ most local users of mozharness already have checkouts so they never encounter this. I could be wrong. I'm inclined to cross the bridge if we come to it.

cool. wfm

Comment 127

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/fx-team/rev/0e3c3535be0264138fb4fd3d0e85a6b42661e472
Bug 1270317 - Upgrade to Mercurial 3.7.3 in the mozharness test environment; r=jlund

https://hg.mozilla.org/integration/fx-team/rev/318f1f07c33a30204927d106b190f6bbba15391f
Bug 1270317 - Define a clone_upstream_url property; r=jlund

https://hg.mozilla.org/integration/fx-team/rev/dabc54198823a3b4bb43e4c6a70e724ff1d37bee
Bug 1270317 - Add query_pushinfo to MercurialVCS; r=jlund

https://hg.mozilla.org/integration/fx-team/rev/d3a21a577df1f78346c6fbe568d732b0b59a5f9d
Bug 1270317 - Add robustcheckout.py Mercurial extension; r=jlund

https://hg.mozilla.org/integration/fx-team/rev/7ca64e0cf6f1250637f659781a782fa9bedc8224
Bug 1270317 - Use robustcheckout extension for checking out Mercurial repos; r=jlund

https://hg.mozilla.org/integration/fx-team/rev/1d374d3a6191c2241a2c9577aff06b0d0b58bc84
Bug 1270317 - Stop using hgtool for Firefox builds; r=jlund

https://hg.mozilla.org/integration/fx-team/rev/71d0b557c99cbf8c124a3034600e905a50bdf6c2
Bug 1270317 - Record hg version and install info; r=jlund

Comment 128

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/53254/#review50426

> okay, would you mind getting a bug on file to ensure proper vcs_config kwargs being passed everywhere and add the bug number in this comment?

Bug 1274059 filed to track removing usage of hgtool from mozharness.

Comment 129

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0e3c3535be02
https://hg.mozilla.org/mozilla-central/rev/318f1f07c33a
https://hg.mozilla.org/mozilla-central/rev/dabc54198823
https://hg.mozilla.org/mozilla-central/rev/d3a21a577df1
https://hg.mozilla.org/mozilla-central/rev/7ca64e0cf6f1
https://hg.mozilla.org/mozilla-central/rev/1d374d3a6191
https://hg.mozilla.org/mozilla-central/rev/71d0b557c99c

Comment 130

[Wander Lairson Costa]

Taskcluster docker images install mercurial from http://mockbuild-repos.pub.build.mozilla.org/releng/public/CentOS/6/x86_64/, is it possible to upgrade it? Or at least push a package with the newer version?

Comment 131

[Jordan Lund (:jlund)]

(In reply to Wander Lairson Costa [:wcosta] from comment #130)
> Taskcluster docker images install mercurial from
> http://mockbuild-repos.pub.build.mozilla.org/releng/public/CentOS/6/x86_64/,
> is it possible to upgrade it? Or at least push a package with the newer
> version?

wcosta switched to installing hg via pip.