1273305 - Create a Mercurial extension to perform clones/checkouts in automation

Status: RESOLVED FIXED

(Developer Services :: Mercurial: hg.mozilla.org, defect)

Description

[Gregory Szorc [:gps]]

I'll explain the reasons for this in a commit message shortly.

Comment 1

[Gregory Szorc [:gps]]

Attachment: MozReview Request: robustcheckout: add extension to perform clone+checkout robustly (bug 1273305); r?smacleod

Firefox automation has multiple implementations of code that ensures
a Mercurial repository is checked out at a specified path. None of
these implementations are optimal and there are numerous efficiency
gains to be realized.

This commit introduces the "robustcheckout" extension. It provides
a command (`hg robustcheckout`) which effectively performs a
clone/pull + purge + update robustly using optimal techniques for
automation. These include the required use of pooled shared stores.

The goal is to replace the custom code in automation with this
extension, starting with hgtool.

The code in this commit comes from the mozharness work I did in
bug 1270317. It also incorporates the content of the "purgelong"
extension, which was previously a standalone extension only used
as part of doing a clone/pull+checkout in automation.

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

Comment 2

[Gregory Szorc [:gps]]

Comment on attachment 8753094 [details]
MozReview Request: robustcheckout: add extension to perform clone+checkout robustly (bug 1273305); r?smacleod

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

Comment 3

[Steven MacLeod [:smacleod]]

Comment on attachment 8753094 [details]
MozReview Request: robustcheckout: add extension to perform clone+checkout robustly (bug 1273305); r?smacleod

https://reviewboard.mozilla.org/r/53000/#review50090

::: hgext/robustcheckout/__init__.py:66
(Diff revision 2)
> +    '''Calls the original unlink function, and if that fails, calls
> +    unlink_long'''

multi-line summary

::: hgext/robustcheckout/__init__.py:72
(Diff revision 2)
> +    unlink_long'''
> +    try:
> +        ui.debug('calling unlink_orig %s\n' % fn)
> +        return unlink_orig(fn)
> +    except WindowsError as e:
> +        # windows error 3 corresponds to ERROR_PATH_NOT_FOUND

"W"indows

::: hgext/robustcheckout/__init__.py:74
(Diff revision 2)
> +        ui.debug('calling unlink_orig %s\n' % fn)
> +        return unlink_orig(fn)
> +    except WindowsError as e:
> +        # windows error 3 corresponds to ERROR_PATH_NOT_FOUND
> +        # only handle this case; re-raise the exception for other kinds of
> +        # failures

period

::: hgext/robustcheckout/__init__.py:84
(Diff revision 2)
> +    '''Context manager that patches the required functions that are used by
> +    the purge extension to remove files. When exiting the context manager
> +    the original functions are restored.'''

multi-line summary

::: hgext/robustcheckout/__init__.py:90
(Diff revision 2)
> +    the purge extension to remove files. When exiting the context manager
> +    the original functions are restored.'''
> +    purgemod = extensions.find('purge')
> +    to_wrap = [(purgemod.util, 'unlink')]
> +
> +    # pass along the ui object to the unlink_wrapper so we can get logging out

"P"ass

::: hgext/robustcheckout/__init__.py:91
(Diff revision 2)
> +    the original functions are restored.'''
> +    purgemod = extensions.find('purge')
> +    to_wrap = [(purgemod.util, 'unlink')]
> +
> +    # pass along the ui object to the unlink_wrapper so we can get logging out
> +    # of it

period.

::: hgext/robustcheckout/__init__.py:94
(Diff revision 2)
> +
> +    # pass along the ui object to the unlink_wrapper so we can get logging out
> +    # of it
> +    wrapped = functools.partial(unlink_wrapper, ui=ui)
> +
> +    # Wrap the original function(s) with our unlink_wrapper

period.

::: hgext/robustcheckout/__init__.py:103
(Diff revision 2)
> +        originals[mod, func] = extensions.wrapfunction(mod, func, wrapped)
> +
> +    try:
> +        yield
> +    finally:
> +        # Restore the originals

period

::: hgext/robustcheckout/__init__.py:110
(Diff revision 2)
> +    '''Runs the original purge() command inside the wrap_unlink() context
> +    manager.'''

multi-line summary

::: hgext/robustcheckout/__init__.py:139
(Diff revision 2)
> +    sharebase = sharebase or ui.config('share', 'pool')
> +    if not sharebase:
> +        raise error.Abort('share base directory not defined; refusing to operate',
> +                          hint='define share.pool config option or pass --sharebase')
> +
> +    # worker.backgroundclose only makes things faster if running A/V, which

It's not clear to me what "A/V" means in this context... antivirus? Even if it's just ignorance on my part of "A/V" being super common, I think spelling it out would be better.

::: hgext/robustcheckout/__init__.py:145
(Diff revision 2)
> +    return _docheckout(ui, url, dest, upstream=upstream, revision=revision,
> +                       branch=branch, purge=purge, sharebase=sharebase)

None of these are keyword arguments, so just go with positioning (since the names all match anyway)

::: hgext/robustcheckout/__init__.py:158
(Diff revision 2)
> +    def deletesharedstore():
> +        storepath = destvfs.read('.hg/sharedpath').strip()
> +        if storepath.endswith('.hg'):
> +            storepath = os.path.dirname(storepath)
> +
> +        storevfs = scmutil.vfs(storepath, audit=False)
> +        storevfs.rmtree(forcibly=True)

Can you move this down to just before `def handlerepoerror(e):` since it's not used until then.

::: hgext/robustcheckout/__init__.py:166
(Diff revision 2)
> +    if destvfs.exists() and not destvfs.exists('.hg'):
> +        ui.warn('(destination exists but no .hg directory; deleting)\n')
> +        destvfs.rmtree(forcibly=True)

This is kind of scary... should we fail instead of deleting the directory? Makes me think of some sort of shell scripting error accidentally passing "/" as the destination...

This is unlikely to be a problem, but still makes me feel uneasy. Do you have a good reason for doing this?

Comment 4

[Gregory Szorc [:gps]]

Comment on attachment 8753094 [details]
MozReview Request: robustcheckout: add extension to perform clone+checkout robustly (bug 1273305); r?smacleod

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

Comment 5

[Steven MacLeod [:smacleod]]

Comment on attachment 8753094 [details]
MozReview Request: robustcheckout: add extension to perform clone+checkout robustly (bug 1273305); r?smacleod

https://reviewboard.mozilla.org/r/53000/#review50154

Comment 6

[Gregory Szorc [:gps]]

https://hg.mozilla.org/hgcustom/version-control-tools/rev/f6af4580fc88