1128518 - Add commit hook on m-i/m-c/... for detecting missing IDL UUID revs

Status: RESOLVED FIXED

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

Description

[Georg Fritzsche [:gfritzsche]]

It's too easy to forget to update the UUIDs when changing IDL interfaces, leading to expensive-to-trace-back issues for sheriffs.

We already have hooks flagging this on mozilla-beta apparently; i would love to see this on our m-c and our integration repositories as well.

While this might lead to false-positives, we could always allow an override in the commit message for those that are sure about it.

Comment 1

[Georg Fritzsche [:gfritzsche]]

gps, is that related to your current work?

Comment 2

[Ryan VanderMeulen [:RyanVM]]

I could see RelMan appreciating this too as one of their current merge-day responsibilities involves finding changed interfaces missing a UUID rev.

My understanding is that the concern is over false-positives (like a comment change), but I don't see why we can't work around that by adding a commit message flag like "IGNORE IDL" or something. However, I think the value of having a hook outweighs the occasional inconvenience such a situation would cause.

Comment 3

[Sylvestre Ledru [:Sylvestre]]

That would be indeed awesome. lmandel_afk (ni) has plan to work on it.

For now, release management is working with this tool: https://github.com/jwir3/checkiid
However, if a comment-only change has been done, a false positive will show. 

Here is the documentation used by release management in order to prepare the merge day:
https://wiki.mozilla.org/Release_Management/Merge_Documentation#Merge_Day_.28Central.2C_Aurora.2C_Beta.29

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Also, bonus points if we can detect the interface name within the changed IDL and output a customized message when the push is rejected along the lines of "Changes were made to the XXX interface, but the UUID wasn't changed. Please run |./mach update-uuids XXX| and re-push. If this was intentional, please amend the commit message to include 'IGNORE IDL'"

Comment 5

[Ryan VanderMeulen [:RyanVM]]

This bit us again on a push to fx-team last night. It's a major footgun to have an issue where a push can cause mass bustage that Try can't detect ahead of time.

I'm not sure what system Dev Services uses for prioritizing bugs, but can this be higher up the list?

Comment 6

[Biru [:poiru]]

I'll take this. (I talked with lmandel on IRC so clearing ni.)

Comment 7

[Sylvestre Ledru [:Sylvestre]]

If I can give a piece of advice, I did the check using  https://github.com/jwir3/checkiid for the 36 m-c => aurora merge.
You can double check if your tool is able to catch what I found in bug 1119818 (maybe I missed some). There was a few false positives.
If your tool is able to detect these missing uuid bump without the false positive, you rock! :)

Comment 8

[Biru [:poiru]]

Attachment: Add hook to reject non-comment IDL interface changes without corresponding UUID bump

Ryan, Sylvestre, are you happy with this logic:

For each interface block (think '[...] interface ... { ... }') in the modified IDL files, we find the matching block in the previous file revision. If the blocks differ after stripping comments away and if the UUID has remained the same, the hook will trigger an error similar to:

  ************************** ERROR ****************************
  Push rejected because the following IDL interfaces were
  modified without changing the UUID:
    - nsIBar
    - nsIFoo

  To update the UUID for all of the above interfaces and their
  descendants, run:
    ./mach update-uuids nsIBar nsIFoo

  If you intentionally want to keep the current UUID, include
  "IGNORE IDL" in the commit message.
  *************************************************************

Although comment-only changes and changes outside of interface blocks are ignored, the hook will still be triggered by e.g. changing indentation in the interface blocks. That said, we really can't achieve 0% false positives unless we import the XPIDL parser (which I think would be overkill).

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8560067 [details] [diff] [review]
Add hook to reject non-comment IDL interface changes without corresponding UUID bump

Sounds fantastic to me if it works!

Comment 10

[Gregory Szorc [:gps]]

Comment on attachment 8560067 [details] [diff] [review]
Add hook to reject non-comment IDL interface changes without corresponding UUID bump

Review of attachment 8560067 [details] [diff] [review]:
-----------------------------------------------------------------

This looks mostly good. Should be smooth sailing on next iteration.

::: hghooks/mozhghooks/prevent_idl_change_without_uuid_bump.py
@@ +12,5 @@
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write to the Free Software
> +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.

Let's go with the shorter GPL notice:

# This software may be used and distributed according to the terms of the
# GNU General Public License version 2 or any later version.

@@ +74,5 @@
> +                if (x in old_interfaces and
> +                    new_interfaces[x]['uuid'] == old_interfaces[x]['uuid'] and
> +                    new_interfaces[x]['body'] != old_interfaces[x]['body'])]
> +
> +def hook(ui, repo, hooktype, node, **kwargs):

This will need to follow the pattern in bug 1128698:

def hook(ui, repo, hooktype, node, source=None, **kwargs):
    if source == 'strip':
        return 0

@@ +85,5 @@
> +
> +    unbumped_interfaces = []
> +
> +    for changeset in changesets:
> +        ctx = repo.changectx(changeset)

for rev in changesets:
    ctx = repo[rev]

@@ +93,5 @@
> +        for path in ctx.files():
> +            if not path.endswith('.idl'):
> +                continue
> +
> +            if not path in ctx:  # Deleted file

if path not in ctx:

@@ +100,5 @@
> +            fctx = ctx[path]
> +            if fctx.filerev() == 0:  # New file
> +                continue
> +
> +            prev_fctx = fctx.filectx(fctx.filerev() - 1)

Instead of iterating ctx.files() and doing manual deletion and add detection, consider instead:

  status = ctx.status(ctx.p1())

This will return a named tuple like class (http://selenic.com/repo/hg/file/1265a3a71d75/mercurial/scmutil.py#l23) containing everything you should need. Iterating over status.modified should be all you need to do.

@@ +106,5 @@
> +                check_unbumped_idl_interfaces(prev_fctx.data(), fctx.data()))
> +
> +    if unbumped_interfaces:
> +        unbumped_interfaces = sorted(set(unbumped_interfaces))
> +        print REJECT_MESSAGE % ('\n  - '.join(unbumped_interfaces),

ui.warn(msg)

(be sure to include the trailing \n)

::: hghooks/tests/test-prevent-idl-change-without-uuid-bump.t
@@ +92,5 @@
> +
> +  $ >> a.idl echo "test"
> +  $ hg commit -A -m "a.idl"
> +  $ >> a.idl echo "test"
> +  $ hg commit -A -m "a.idl a=release"

I'd feel much better about this test if you attempted the push first before showing that "a=release" changes behavior.

You can use `hg commit --amend` to amend the commit message if needed.

Comment 11

[Biru [:poiru]]

Attachment: Add hook to reject non-comment IDL interface changes without corresponding UUID bump

Comment 10 addressed.

Comment 12

[Biru [:poiru]]

Attachment: Add hook to reject non-comment IDL interface changes without corresponding UUID bump

Missed ui.warn in previous patch, fixed now.

Comment 13

[Gregory Szorc [:gps]]

Comment on attachment 8560243 [details] [diff] [review]
Add hook to reject non-comment IDL interface changes without corresponding UUID bump

Review of attachment 8560243 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM from the VCS side.

I can't speak for the actual behavior it is enforcing. I'd feel much better if you got review from someone this will affect before this lands.

Comment 14

[Biru [:poiru]]

Comment on attachment 8560243 [details] [diff] [review]
Add hook to reject non-comment IDL interface changes without corresponding UUID bump

Ms2ger, as per gps's request in comment 13, does the behaviour enforced by this patch seem reasonable? See comment 8 for a description.

Comment 15

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8560243 [details] [diff] [review]
Add hook to reject non-comment IDL interface changes without corresponding UUID bump

Review of attachment 8560243 [details] [diff] [review]:
-----------------------------------------------------------------

This seems okay, but note that there can also be problems if we forget to update the uuid of a descendant interface, which this patch doesn't check.

::: hghooks/mozhghooks/prevent_idl_change_without_uuid_bump.py
@@ +93,5 @@
> +            unbumped_interfaces.extend(
> +                check_unbumped_idl_interfaces(prev_fctx.data(), fctx.data()))
> +
> +    if unbumped_interfaces:
> +        unbumped_interfaces = sorted(set(unbumped_interfaces))

What's the set() for?

Comment 16

[Biru [:poiru]]

Landed this:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/09a443476930

(In reply to :Ms2ger from comment #15)
> This seems okay, but note that there can also be problems if we forget to
> update the uuid of a descendant interface, which this patch doesn't check.

Yep, hopefully this is good enough for now.

> ::: hghooks/mozhghooks/prevent_idl_change_without_uuid_bump.py
> @@ +93,5 @@
> > +            unbumped_interfaces.extend(
> > +                check_unbumped_idl_interfaces(prev_fctx.data(), fctx.data()))
> > +
> > +    if unbumped_interfaces:
> > +        unbumped_interfaces = sorted(set(unbumped_interfaces))
> 
> What's the set() for?

To remove duplicates in unbumped_interfaces. If the same IDL is modified in multiple changesets, it will be in unbumped_interfaces multiple times.

Comment 17

[Biru [:poiru]]

Attachment: Show changesets in IDL hook error message

Comment 18

[Gregory Szorc [:gps]]

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

Comment 19

[Gregory Szorc [:gps]]

Comment on attachment 8561052 [details] [diff] [review]
Show changesets in IDL hook error message

Review of attachment 8561052 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM. I trust you can make the minor edits without re-review.

::: hghooks/mozhghooks/prevent_idl_change_without_uuid_bump.py
@@ +90,5 @@
>                  continue
>  
>              fctx = ctx[path]
>              prev_fctx = fctx.filectx(fctx.filerev() - 1)
> +            unbumped_interfaces.extend([(x, short(ctx.node())) for x in

short(ctx.node()) == ctx.short()

@@ +96,5 @@
>  
>      if unbumped_interfaces:
> +        names_and_revs = sorted([name + ' in changeset ' + rev
> +                                     for name, rev in unbumped_interfaces])
> +        names = sorted(set([name for name, rev in unbumped_interfaces]))

You don't need the [] in here. "x for x in foo" is a generator expression, which will happily be accepted by set() or sorted() without the additional intermediate list instance (created via []).

Comment 20

[Biru [:poiru]]

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

(In reply to Gregory Szorc [:gps] from comment #19)
> ::: hghooks/mozhghooks/prevent_idl_change_without_uuid_bump.py
> @@ +90,5 @@
> >                  continue
> >  
> >              fctx = ctx[path]
> >              prev_fctx = fctx.filectx(fctx.filerev() - 1)
> > +            unbumped_interfaces.extend([(x, short(ctx.node())) for x in
> 
> short(ctx.node()) == ctx.short()

ctx.short() doesn't exist so left this as is.

Comment 21

[Gregory Szorc [:gps]]

Do you want this deployed now?

Comment 22

[Biru [:poiru]]

(In reply to Gregory Szorc [:gps] from comment #21)
> Do you want this deployed now?

Yep. RyanVM says to enable it on all the usual branches except Try (see below). I'll post about this on dev.platform.

21:01     poiru: RyanVM|sheriffduty: On which branches do we want to enable the UUID bump hook?
21:01  RyanVM|s: everything but Try sounds good to me :)
21:02  RyanVM|s: poiru: though I guess the correct answer is "Whatever other branches we run all the usual hooks on" i.e. bug number in the commit message, no Try syntax, etc

Comment 23

[Lawrence Mandel [:lmandel] (use needinfo)]

Greg - Are you responsible for deployment? If so, can you get this deployed before 38 merges to Aurora on Mon, Feb, 23?

Comment 24

[Biru [:poiru]]

(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #23)
> Greg - Are you responsible for deployment? If so, can you get this deployed
> before 38 merges to Aurora on Mon, Feb, 23?

Note that the hook checks UUID bumps at a commit level. I suspect merging central to Aurora will result in a lot of rejected commits (even for changes with follow-up UUID bumps). The Aurora merge commit should probably include 'IGNORE IDL' or 'a=release' to avoid this. Perhaps we should allow 'a=merge', too?

(In the future, after this hook has been deployed for a while, this will no longer be an issue as all IDL changes will either bump the UUID or have 'IGNORE IDL' in the commit message.)

Comment 25

[Biru [:poiru]]

(In reply to Birunthan Mohanathas [:poiru] from comment #24)
> Note that the hook checks UUID bumps at a commit level. I suspect merging
> central to Aurora will result in a lot of rejected commits (even for changes
> with follow-up UUID bumps). The Aurora merge commit should probably include
> 'IGNORE IDL' or 'a=release' to avoid this. Perhaps we should allow
> 'a=merge', too?

The WebIDL hook seems to ignore merge commits[0]. Perhaps this hook should as well?

[0]: https://hg.mozilla.org/hgcustom/version-control-tools/file/e4b216aa7132/hghooks/mozhghooks/prevent_webidl_changes.py#l86

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Sounds reasonable.

Also, Release Engineering sets "IGNORE BAD CHANGESETS" when they do branch merges, which I would assume takes priority over this hook.

Comment 27

[Gregory Szorc [:gps]]

I have deployed the hook on the following repositories:

mozilla-central
integration/b2g-inbound
integration/fx-team
integration/mozilla-inbound
releases/mozilla-aurora
releases/mozilla-beta
releases/mozilla-release
releases/mozilla-esr31

Please reopen if I should hit more repos. (We sadly don't have an easy way to define "groups" in the server to make these deployments more consistent.)

Comment 28

[Lawrence Mandel [:lmandel] (use needinfo)]

I don't know that we care about UUID bumps on B2G (we may) as our primary concern on desktop is not breaking add-ons. If someone knows that we do care about UUID bumps on B2G, we should reopen and add the hook the the various B2G repos (b2g37, b2g34, b2g32,...) as well.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

I'm not terribly concerned about the B2G release repos since the vast majority of what's landing on them will have landed on trunk first (and therefore be subject to the hook) anyway.

Comment 30

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Could you also enable this for comm-central?

Comment 31

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

or rather, comm-* (sorry for the extra bugmail)

Comment 32

[Gregory Szorc [:gps]]

Enabled on:

comm-central
releases/comm-aurora
releases/comm-beta
releases/comm-release
releases/comm-esr24
releases/comm-esr31