813809 - Implement an IDL checking hook on mozilla-beta and mozilla-release repos to refuse binary changes without specific approval

Status: RESOLVED FIXED

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

Description

[Lukas Blakk [:lsblakk] use ?needinfo]

Attachment: prevent uuid changes without approval hook & tests v1

The attached hook looks at any files ending in .idl in a push and scans the changes for 'uuid(' - if found, the user is warned and their push is rejected if they do not have binary approval (ba=...) in the tip commit comment.

The tests pass except checking that if deleting a file with a uuid in it is found for approval. I would love a second pair of eyes on that one (on top of the whole deal).

Once the hook is reviewed and approved it will need to be installed on our hg server as a pretxnchangegroup hook (same as treeclosure) on mozilla-beta and mozilla-release repos only.

Comment 1

[Lukas Blakk [:lsblakk] use ?needinfo]

Assigning to me for now, during review process, and will flip back to ServerOps when it's ready to go live.

Comment 2

[(not currently active) Ted Mielczarek]

We normally use separate bugs for developing hooks and deploying them, FWIW. Keeps things simpler.

Comment 3

[Scott Johnson (:jwir3)]

As a side note, should we maybe implement a hook that disallows changes to IDL files that don't have a UUID change? 

This could be complicated, as if only the comments change, the UUID doesn't need to change, but perhaps something that simply reminded someone "hey, did you forget to rev the uuid?"

Comment 4

[Scott Johnson (:jwir3)]

(In reply to Scott Johnson (:jwir3) from comment #3)
> As a side note, should we maybe implement a hook that disallows changes to
> IDL files that don't have a UUID change? 
> 
> This could be complicated, as if only the comments change, the UUID doesn't
> need to change, but perhaps something that simply reminded someone "hey, did
> you forget to rev the uuid?"

Also, if there is interest, I'd be happy to write a patch for this. I already have something cooking on my local machine to implement this kind of functionality, as a safeguard for myself, since I've been caught with this in the past.

Comment 5

[Johnathan Nightingale [:johnath]]

(In reply to Scott Johnson (:jwir3) from comment #4)
> (In reply to Scott Johnson (:jwir3) from comment #3)
> > As a side note, should we maybe implement a hook that disallows changes to
> > IDL files that don't have a UUID change? 
> > 
> > This could be complicated, as if only the comments change, the UUID doesn't
> > need to change, but perhaps something that simply reminded someone "hey, did
> > you forget to rev the uuid?"
> 
> Also, if there is interest, I'd be happy to write a patch for this. I
> already have something cooking on my local machine to implement this kind of
> functionality, as a safeguard for myself, since I've been caught with this
> in the past.

+Mossop, who had code a while ago that scanned IDL changes looking for UUID revs, iinm. Sounds quite lovely to me, though probably as a follow up bug so as not to derail this one?

Comment 6

[Dave Townsend [:mossop]]

(In reply to Johnathan Nightingale [:johnath] from comment #5)
> (In reply to Scott Johnson (:jwir3) from comment #4)
> > (In reply to Scott Johnson (:jwir3) from comment #3)
> > > As a side note, should we maybe implement a hook that disallows changes to
> > > IDL files that don't have a UUID change? 
> > > 
> > > This could be complicated, as if only the comments change, the UUID doesn't
> > > need to change, but perhaps something that simply reminded someone "hey, did
> > > you forget to rev the uuid?"
> > 
> > Also, if there is interest, I'd be happy to write a patch for this. I
> > already have something cooking on my local machine to implement this kind of
> > functionality, as a safeguard for myself, since I've been caught with this
> > in the past.
> 
> +Mossop, who had code a while ago that scanned IDL changes looking for UUID
> revs, iinm. Sounds quite lovely to me, though probably as a follow up bug so
> as not to derail this one?

Bug 477166 has the WIP that I got to, it all got bitrotted when we switched to using python to parse idl files though.

Comment 7

[Alex Keybl [:akeybl]]

(In reply to Scott Johnson (:jwir3) from comment #3)
> As a side note, should we maybe implement a hook that disallows changes to
> IDL files that don't have a UUID change? 
> 
> This could be complicated, as if only the comments change, the UUID doesn't
> need to change, but perhaps something that simply reminded someone "hey, did
> you forget to rev the uuid?"

I'm not sure whether these changes commonly have binary add-on compatibility impact (I'm assuming so). Hopefully bsmedberg can clarify. If yes, it'd be great to get your help Scott.

Comment 8

[Benjamin Smedberg]

Yes, it would be equally bad to add or modify a method and *not* change the UUID as it would be to change the UUID. I suspect that the "good enough" solution here is just to disallow *any* change to an .idl file unless it has a special ba= flag.

Comment 9

[(not currently active) Ted Mielczarek]

Comment on attachment 683810 [details] [diff] [review]
prevent uuid changes without approval hook & tests v1

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

Sorry for the review delay, I needed to commit a little time to actually testing some parts of this patch.

::: mozhghooks/prevent_uuid_changes.py
@@ +27,5 @@
> +    for change_id in xrange(repo[node].rev(), len(repo)):
> +        # Loop through each file for the current changeset
> +        for file in repo[change_id].files():
> +            # Only Check IDL Files
> +            if re.match('.*\.(idl)',file):

if file.endswith(".idl"):

@@ +30,5 @@
> +            # Only Check IDL Files
> +            if re.match('.*\.(idl)',file):
> +                ctx = repo.changectx(change_id)
> +                for f in ctx:
> +                    fctx = ctx.filectx(f)

This is kind of crazy. You're looping over every single file in the repo at this revision here and reading all of their data. This is going to be super-slow. I think what you really wanted to do was:
if re.match('.*\.(idl)',file):
  fctx = repo[change_id].filectx(file)
  if re.search('uuid\(', fctx.data()):
    ..

Also, just to note, repo[foo] == repo.changectx(foo).

@@ +36,5 @@
> +                        if re.search('ba\S*=', repo.changectx('tip').description().lower()) :
> +                            print "You've got approval for the binary change in your push, thanks for the extra caution."
> +                        else:
> +                            error += "UUID change in file: %s - please get binary change approval for this patch." % file
> +                            print "Pushing a UUID change to tree requires your top changeset comment to include: ba=... (or, more accurately, ba\\S*=...)"

You're going to print this text once per-changeset that fails the hook. Do you really want that?

@@ +37,5 @@
> +                            print "You've got approval for the binary change in your push, thanks for the extra caution."
> +                        else:
> +                            error += "UUID change in file: %s - please get binary change approval for this patch." % file
> +                            print "Pushing a UUID change to tree requires your top changeset comment to include: ba=... (or, more accurately, ba\\S*=...)"
> +                

You've got extraneous whitespace here.

::: runtests.py
@@ +41,5 @@
> +        if line != content:
> +            newlines.append(line)
> +  with open(filename, 'a') as f:
> +    f.write(''.join(newlines))
> +    

Thanks for writing a whole bunch of tests! My only complaint here is that there's a ton of extra spaces on blank lines here (you can see them in the Splinter view of this patch). Please clean those up before landing.

Comment 10

[Lukas Blakk [:lsblakk] use ?needinfo]

Attachment: prevent uuid changes without approval hook & tests v2

This patch addresses feedback from v1 and also now successfully tracks if an .idl file has been removed in a changeset being pushed without "ba=" in it.

Comment 11

[Masatoshi Kimura [:emk]]

Is this patch block .idl changes without uuid bump? (see comment 8)

Comment 12

[Lukas Blakk [:lsblakk] use ?needinfo]

Attachment: prevent uuid changes without approval hook & tests v3

Thanks for calling that out, Masatoshi, I've simplified this hook to just catch IDL changes across the board - edits & removals.

Comment 13

[Robert Kaiser]

BTW, even though the summary says beta and release, I guess we probably want this on the ESR repos as well. And what about b2g18?

Comment 14

[(not currently active) Ted Mielczarek]

Comment on attachment 691157 [details] [diff] [review]
prevent uuid changes without approval hook & tests v3

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

Looks good.

Comment 15

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to Robert Kaiser (:[e-mail]) from comment #13)
> BTW, even though the summary says beta and release, I guess we probably want
> this on the ESR repos as well. And what about b2g18?

Good idea, ESR repos would be useful.  For b2g18 I do not know if binary compatibility is as much of an issue - isn't this primarily for addons?  If so then I wouldn't think b2g18 would need such precautions.  Someone please correct me if I'm missing something here, enabling the hook on any particular repo is cheap.

Comment 16

[Lukas Blakk [:lsblakk] use ?needinfo]

Landed, will file Server Ops bug for deployment to repos.

http://hg.mozilla.org/hgcustom/hghooks/rev/98849c35e27f

Comment 17

[(no longer active)]

(In reply to comment #15)
> (In reply to Robert Kaiser (:[e-mail]) from comment #13)
> > BTW, even though the summary says beta and release, I guess we probably want
> > this on the ESR repos as well. And what about b2g18?
> 
> Good idea, ESR repos would be useful.  For b2g18 I do not know if binary
> compatibility is as much of an issue - isn't this primarily for addons?  If so
> then I wouldn't think b2g18 would need such precautions.  Someone please
> correct me if I'm missing something here, enabling the hook on any particular
> repo is cheap.

Right, binary compat is not a concern there.

Comment 18

[Ed Morley [:emorley]]

v3 of this patch lost the |bc = True| so the variable is always defined as false, and we never print the "thank you" message. I don't suppose you could file a followup to either remove the message altogether or else set bc where appropriate?

Comment 19

[Ed Morley [:emorley]]

(or else indicate what your preference is and I'll file/fix :-))

Comment 20

[Lukas Blakk [:lsblakk] use ?needinfo]

Attachment: cleanup.diff

here we go - can you get this landed and installed?

Comment 21

[Ed Morley [:emorley]]

Comment on attachment 8338691 [details] [diff] [review]
cleanup.diff

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

r=me with the change below :-)

::: mozhghooks/prevent_uuid_changes.py
@@ +31,5 @@
>              if file.endswith('.idl'):
>                  if not re.search('ba\S*=', repo.changectx('tip').description().lower()):
>                          error += "IDL file %s altered in this changeset" % file
> +                else:
> +                    bc = True

The |bc = True| can be put the line after the file.endswith() (ie avoiding the else), since the "thank you" message isn't printed in the case of errors.

Comment 22

[Ed Morley [:emorley]]

Comment on attachment 8338691 [details] [diff] [review]
cleanup.diff

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

(Forgot to set the flag)

Comment 23

[Lukas Blakk [:lsblakk] use ?needinfo]

Attachment: cleanup take 2

here we go, thanks!

Comment 24

[Ed Morley [:emorley]]

Thank you :-)

https://hg.mozilla.org/hgcustom/hghooks/rev/b6c4f0008071

(Bug 943486 will need a hghooks prod push, so I'll save this until then)

Comment 25

[[e-mail] (:bhearsum)]

(In reply to Ed Morley [:edmorley UTC+0] from comment #24)
> Thank you :-)
> 
> https://hg.mozilla.org/hgcustom/hghooks/rev/b6c4f0008071
> 
> (Bug 943486 will need a hghooks prod push, so I'll save this until then)

FYI: When IT was deploying bug 944423 they told me that this wasn't rolled out yet.

Comment 26

[Ed Morley [:emorley]]

(In reply to Ben Hearsum [:bhearsum] from comment #25)
> (In reply to Ed Morley [:edmorley UTC+0] from comment #24)
> > (Bug 943486 will need a hghooks prod push, so I'll save this until then)
> 
> FYI: When IT was deploying bug 944423 they told me that this wasn't rolled
> out yet.

Yeah bug 943486 hasn't landed yet; thank you for deploying this :-)