1454296 - prevent_webidl_changes shouldn't generate a warning when unrelated servo files are modified

Status: RESOLVED FIXED

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

Description

[:glob ✱]

prevent_webidl_changes has:

> # Loop through each file for the current changeset
> for file in c.files():
>     if file.startswith('servo/'):
>         ui.write('(%s modifies %s from Servo; not enforcing peer '
>                  'review)\n' % (short(c.node()), file))
>         continue

this results in it always issuing a warning on _any_ servo change.

this warning should only be generated when .webidl files are changed.

Comment 1

[Phabricator Automation]

Attachment: move webidl and ipcsync hooks to the modern hg hook framework, fixup messages

Comment 2

[:glob ✱]

Attachment: hghooks: Bump Mercurial version to 4.4.2 (bug 1454296)

Bump Mercurial version of the hgdev environment to 4.4.2 to match production,
fix tests.

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

Comment 3

[:glob ✱]

Attachment: hghooks: pass 'node' to pre() hook methods (bug 1454296)

Pass 'node' to pre() to allow hooks to inspect the whole commit.

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

Comment 4

[:glob ✱]

Attachment: hghooks: move webidl hook to modern hook class (bug 1454296)

move webidl hook to the modern hg hook framework, and remove the extraneous
servo message.

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

Comment 5

[:glob ✱]

Attachment: hghooks: move ipcsync hook to modern hook class (bug 1454296)

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

Comment 6

[Gregory Szorc [:gps]]

Comment on attachment 8968804 [details]
hghooks: pass 'node' to pre() hook methods (bug 1454296)

https://reviewboard.mozilla.org/r/237524/#review244366

Comment 7

[Gregory Szorc [:gps]]

Comment on attachment 8968805 [details]
hghooks: move webidl hook to modern hook class (bug 1454296)

https://reviewboard.mozilla.org/r/237526/#review244368

Aside from a minor nit, this is mostly good. Thanks for porting the hooks.

::: hghooks/mozhghooks/check/prevent_webidl_changes.py:70
(Diff revision 1)
> +    def relevant(self):
> +        return self.repo_metadata['firefox_releasing']
> +
> +    def pre(self, node):
> +        # Accept the entire push for code uplifts
> +        changesets = list(self.repo.changelog.revs(self.repo[node].rev()))

`node` here is guaranteed to be the latest revision in the push. So this whole calling into `repo.changelog.revs()` should not be necessary. Just do `repo[node].description()`.

Comment 8

[Gregory Szorc [:gps]]

Comment on attachment 8968806 [details]
hghooks: move ipcsync hook to modern hook class (bug 1454296)

https://reviewboard.mozilla.org/r/237528/#review244372

This is mostly good.

When we deploy this, we'll need to manually remove the hook from all hgrc files where it is currently explicitly listed.

::: hghooks/mozhghooks/check/prevent_sync_ipc_changes.py:48
(Diff revision 1)
> +    def relevant(self):
> +        return self.repo_metadata['firefox_releasing']
> +
> +    def pre(self, node):
> +        # Accept the entire push for code uplifts
> +        changesets = list(self.repo.changelog.revs(self.repo[node].rev()))

Same comment as previous patch.

::: hghooks/mozhghooks/check/prevent_sync_ipc_changes.py:66
(Diff revision 1)
> +        if is_backout(ctx.description()):
> +            return True
> +
> +        # Ignore changes that don't touch sync-messages.ini
> +        ipc_files = [f for f in ctx.files()
> +                     if f.endswith('ipc/ipdl/sync-messages.ini')]

There's only one file in the repo with path fragment `ipc/ipdl/sync-messages.ini`. I think we want this to be an exact path compare instead of `endswith()`.

Comment 9

[Gregory Szorc [:gps]]

Comment on attachment 8968803 [details]
hghooks: Bump Mercurial version to 4.4.2 (bug 1454296)

https://reviewboard.mozilla.org/r/237522/#review244374

We're currently running Mercurial 4.2 on hgssh. It is OK to bump the Mercurial version in the hgdev virtualenv. But we need to ensure that hooks and other hgssh activities still work with Mercurial 4.2. You can use `run-tests --all-hg-versions` to test with all supported versions.

Comment 10

[:glob ✱]

Comment on attachment 8968805 [details]
hghooks: move webidl hook to modern hook class (bug 1454296)

https://reviewboard.mozilla.org/r/237526/#review244368

> `node` here is guaranteed to be the latest revision in the push. So this whole calling into `repo.changelog.revs()` should not be necessary. Just do `repo[node].description()`.

this isn't correct - pretxnchangegroup sets node to the first changeset in the group that was added, not the latest

Comment 11

[:glob ✱]

(In reply to Gregory Szorc [:gps] from comment #9)
> Comment on attachment 8968803 [details]
> hghooks: Bump Mercurial version to 4.4.2 (bug 1454296)
> 
> https://reviewboard.mozilla.org/r/237522/#review244374
> 
> We're currently running Mercurial 4.2 on hgssh. It is OK to bump the
> Mercurial version in the hgdev virtualenv. But we need to ensure that hooks
> and other hgssh activities still work with Mercurial 4.2. You can use
> `run-tests --all-hg-versions` to test with all supported versions.

as per our discussion, i'll change instead of pinning the version of mercurial in hghook tests to match the version we run on the server, and fixup tests that fail under 4.2 only.

Comment 12

[:glob ✱]

Attachment: hghooks: pin mercurial version to 4.2 in tests (bug 1454296)

pin mercurial version to 4.2 in tests, and fixes some tests.
of note the ducktyping in mozhg/util to detect 4.3+ was broken.

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

Comment 13

[:glob ✱]

Comment on attachment 8968804 [details]
hghooks: pass 'node' to pre() hook methods (bug 1454296)

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

Comment 14

[:glob ✱]

Comment on attachment 8968805 [details]
hghooks: move webidl hook to modern hook class (bug 1454296)

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

Comment 15

[:glob ✱]

Comment on attachment 8968806 [details]
hghooks: move ipcsync hook to modern hook class (bug 1454296)

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

Comment 16

[Gregory Szorc [:gps]]

Comment on attachment 8970447 [details]
hghooks: pin mercurial version to 4.2 in tests (bug 1454296)

https://reviewboard.mozilla.org/r/239218/#review245188

::: hghooks/tests/test-advertise-upgrade.t:3
(Diff revision 1)
> +# pin to the version of mercurial running on hg.m.o
> +#require hg42
> +#require no-hg43

So basically these changes mean I can't run tests against a modern Mercurial version without mucking up every test file. That's ugly.

We need to support running tests against any Mercurial you point the test harness at (`run-tests --with-hg ...`).

I think the patch you want to write is one that changes `run-tests` to invoke the hook tests with a specific Mercurial version by default. But such functionality is scope bloat for this series and I wouldn't fault you for ignoring this problem so you can land the hooks changes.

::: pylib/mozhg/mozhg/util.py:65
(Diff revision 1)
>      repo_root = repo.ui.config('mozilla', 'repo_root', '/repo/hg/mozilla')
>      if not repo_root.endswith('/'):
>          repo_root += '/'
>  
>      # TRACKING hg43
> -    if configitems:
> +    if util.versiontuple(n=2) >= (4, 3):

Version sniffing isn't as robust as feature sniffing. For the same reasons that web sites should sniff features instead of keying off the User-Agent string.

Please restore the original code. But put it in a `with mercurial.demandimport.deactivated()` block to force the import. Without this context manager, the import always works when Mercurial's on-demand importer is enabled because the import isn't attempted until the module is accessed. You've likely stumbled upon a systemic problem in our extensions :/

Comment 17

[Gregory Szorc [:gps]]

Comment on attachment 8968804 [details]
hghooks: pass 'node' to pre() hook methods (bug 1454296)

https://reviewboard.mozilla.org/r/237524/#review245194

::: hghooks/mozhghooks/checks.py:65
(Diff revision 2)
>          The return value of this method is tightly validated to help prevent
>          logic bugs.
>          """
>  
>      @abc.abstractmethod
> -    def pre(self):
> +    def pre(self, node):

Since you will be submitting a revised series, would you mind documenting this argument in the docstring?

Comment 18

[Gregory Szorc [:gps]]

Comment on attachment 8968805 [details]
hghooks: move webidl hook to modern hook class (bug 1454296)

https://reviewboard.mozilla.org/r/237526/#review245196

::: hghooks/mozhghooks/check/prevent_webidl_changes.py:96
(Diff revision 2)
> +        # Allow patches authored by peers
> +        if self._is_peer_email(util.email(ctx.user())):
> +            return True
> +
> +        # Categorise files
> +        file_counts = dict(total=0, chrome=0, servo=0)

Pro tip: `collections.Counter` is a useful data structure. It has the property that keys automatically assume the value of `0` when accessed. So you can do `file_counts = collections.Counter(); file_counts['total'] += 1` and it "just works."

::: hghooks/mozhghooks/check/prevent_webidl_changes.py:124
(Diff revision 2)
> +        return False
> +
> +    def post_check(self):
> +        return True
> +
> +    @staticmethod

Nit: I'm not a fan of `@staticmethod` because it is too object oriented for Python. This isn't Java and not everything needs to be attached to a class. A `@staticmethod` can by definition be a regular file-level function.

Comment 19

[Gregory Szorc [:gps]]

Comment on attachment 8968805 [details]
hghooks: move webidl hook to modern hook class (bug 1454296)

https://reviewboard.mozilla.org/r/237526/#review245202

::: hghooks/tests/test-prevent-webidl-changes.t
(Diff revision 2)
> +    - Peter Van der Beken (:peterv)
> +  *********************************************************
>    
> -  ************************** ERROR ****************************
> -  
> -  WebIDL file original.webidl altered in changeset 743ef64f8a38 without DOM peer review

Losing the changeset triggering the violation is not great. It means whoever sees this warning has to query VCS to find the offending commit.

Let's print the changeset causing problems so people know where to start looking.

Comment 20

[Gregory Szorc [:gps]]

Comment on attachment 8968806 [details]
hghooks: move ipcsync hook to modern hook class (bug 1454296)

https://reviewboard.mozilla.org/r/237528/#review245198

Looks good aside from missing the changeset from the error message.

::: hghooks/mozhghooks/check/prevent_sync_ipc_changes.py:48
(Diff revision 2)
> +        changesets = list(self.repo.changelog.revs(self.repo[node].rev()))
> +        self.is_uplift = 'a=release' in self.repo.changectx(
> +            changesets[-1]).description().lower()

That's the second hook where we iterate through contexts and access the commit message. This would be more efficient if we only accessed the context once. But this is definitely a micro-optimization and probably isn't worth caring about at this time.

::: hghooks/tests/test-prevent-sync-ipc-changes.t
(Diff revision 2)
> +    - Nathan Froyd (:froydnj)
> +  *****************************************************************
>    
> -  ************************** ERROR ****************************
> -  
> -  sync-messages.ini altered in changeset 8fb3e82ba334 without IPC peer review

Same issue as previous commit: let's print the changeset so people know what commit to fix.

Comment 21

[:glob ✱]

Comment on attachment 8970447 [details]
hghooks: pin mercurial version to 4.2 in tests (bug 1454296)

https://reviewboard.mozilla.org/r/239218/#review245188

> Version sniffing isn't as robust as feature sniffing. For the same reasons that web sites should sniff features instead of keying off the User-Agent string.
> 
> Please restore the original code. But put it in a `with mercurial.demandimport.deactivated()` block to force the import. Without this context manager, the import always works when Mercurial's on-demand importer is enabled because the import isn't attempted until the module is accessed. You've likely stumbled upon a systemic problem in our extensions :/

i'll make this change but i disagree with it.

the browser landscape is not in any way comparable here; we have a single target product, with clearly defined features scoped by version numbers; there's a direct immutable map between a mercurial release version and the features it supports.

Comment 22

[:glob ✱]

Attachment: testing: default to hg42 for tests run under the hgdev environment (bug 1454296)

Put in place infrastructure to set the default hg version for hooks to match
that which is running on production.  This allows the default version of
mercurial in the dev environment to be increased without impacting testing
outcomes.

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

Comment 23

[:glob ✱]

Attachment: pylib: add mozhg.util.import_module() and use it to detect mercurial.configitems (bug 1454296)

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

Comment 24

[:glob ✱]

Comment on attachment 8968804 [details]
hghooks: pass 'node' to pre() hook methods (bug 1454296)

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

Comment 25

[:glob ✱]

Comment on attachment 8968805 [details]
hghooks: move webidl hook to modern hook class (bug 1454296)

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

Comment 26

[:glob ✱]

Comment on attachment 8968806 [details]
hghooks: move ipcsync hook to modern hook class (bug 1454296)

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

Comment 27

[Gregory Szorc [:gps]]

Comment on attachment 8972499 [details]
testing: default to hg42 for tests run under the hgdev environment (bug 1454296)

https://reviewboard.mozilla.org/r/241084/#review247068

I don't like this approach because we already have a mechanism to associate tests with compatible Mercurial versions and the hooks tests are already using it.

Tests associated with extensions (which hghooks are) parse the `testedwith` line looking for Mercurial version compatibility. When you run `run-tests --all-hg-versions`, it only runs with Mercurial versions listed in `testedwith` plus `@` (`@` is special).

If there's anything to be done here, it would be to have `run-tests` without `--all-hg-versions` examine the virtualenv's default `hg` version and ensure that tests are compatible with that version. We may also want a mechanism (in `testing.py`) to pin certain tests to certain versions by default. I'm OK with that functionality - but only if it lives in `testing.py`: `run-tests` feels like the wrong layer for it.

Since hooks are supposed to be compatible with every version of Mercurial listed in `testedwith`, running tests with a pinned Mercurial version by default doesn't really accomplish anything because the hooks are supposed to be compatible with the versions they list! i.e. `run-tests --all-hg-versions` should pass.

Comment 28

[Gregory Szorc [:gps]]

Comment on attachment 8972500 [details]
pylib: add mozhg.util.import_module() and use it to detect mercurial.configitems (bug 1454296)

https://reviewboard.mozilla.org/r/241086/#review247076

::: hgext/configwizard/__init__.py:33
(Diff revision 1)
>  
>  OUR_DIR = os.path.dirname(__file__)
>  execfile(os.path.join(OUR_DIR, '..', 'bootstrap.py'))
>  
>  from configobj import ConfigObj
> +from mozhg.util import import_module

We have to be a bit careful with imports in configwizard because the extension needs to load on downright ancient versions of Mercurial.

I'd feel better if this extension used the legacy mechanism.

Comment 29

[Gregory Szorc [:gps]]

Comment on attachment 8972500 [details]
pylib: add mozhg.util.import_module() and use it to detect mercurial.configitems (bug 1454296)

https://reviewboard.mozilla.org/r/241086/#review247080

I dropped the configwizard changes and landed this as https://hg.mozilla.org/hgcustom/version-control-tools/rev/22cebb54cab1b63898ea653baad2dcf8ff16207f. Phabricator would have closed just this review. But we never made MozReview that intelligent. So you'll need to resubmit the series.

Comment 30

[Gregory Szorc [:gps]]

Comment on attachment 8968805 [details]
hghooks: move webidl hook to modern hook class (bug 1454296)

https://reviewboard.mozilla.org/r/237526/#review247084

Comment 31

[Gregory Szorc [:gps]]

Comment on attachment 8968806 [details]
hghooks: move ipcsync hook to modern hook class (bug 1454296)

https://reviewboard.mozilla.org/r/237528/#review247086

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/17c536046e5b
hghooks: pass 'node' to pre() hook methods ; r=gps
https://hg.mozilla.org/hgcustom/version-control-tools/rev/bea8bcaf5b67
hghooks: move webidl hook to modern hook class ; r=gps
https://hg.mozilla.org/hgcustom/version-control-tools/rev/a14dc1982344
hghooks: move ipcsync hook to modern hook class ; r=gps

Comment 33

[Gregory Szorc [:gps]]

Comment on attachment 8968806 [details]
hghooks: move ipcsync hook to modern hook class (bug 1454296)

https://reviewboard.mozilla.org/r/237528/#review247088

I landed the 3 hooks commits manually. You might as well close the series and not worry about the first patch to run-tests.