Closed Bug 813809 Opened 12 years ago Closed 12 years ago

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

Categories

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

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: lsblakk, Assigned: lsblakk)

References

Details

Attachments

(2 files, 3 obsolete files)

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.
Attachment #683810 - Flags: review?(ted)
Assigning to me for now, during review process, and will flip back to ServerOps when it's ready to go live.
Assignee: server-ops-devservices → lsblakk
We normally use separate bugs for developing hooks and deploying them, FWIW. Keeps things simpler.
Component: Server Operations: Developer Services → Hg: Customizations
QA Contact: shyam
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?"
(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.
(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?
(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.
(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.
Flags: needinfo?(benjamin)
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.
Flags: needinfo?(benjamin)
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.
Attachment #683810 - Flags: review?(ted) → review-
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.
Attachment #683810 - Attachment is obsolete: true
Attachment #691146 - Flags: review?(ted)
Is this patch block .idl changes without uuid bump? (see comment 8)
Thanks for calling that out, Masatoshi, I've simplified this hook to just catch IDL changes across the board - edits & removals.
Attachment #691146 - Attachment is obsolete: true
Attachment #691146 - Flags: review?(ted)
Attachment #691157 - Flags: review?(ted)
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 on attachment 691157 [details] [diff] [review]
prevent uuid changes without approval hook & tests v3

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

Looks good.
Attachment #691157 - Flags: review?(ted) → review+
(In reply to Robert Kaiser (:kairo@mozilla.com) 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.
Landed, will file Server Ops bug for deployment to repos.

http://hg.mozilla.org/hgcustom/hghooks/rev/98849c35e27f
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Blocks: 820910
(In reply to comment #15)
> (In reply to Robert Kaiser (:kairo@mozilla.com) 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.
Depends on: 826469
Blocks: 862411
Depends on: 879086
Product: mozilla.org → Release Engineering
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?
Flags: needinfo?(lsblakk)
(or else indicate what your preference is and I'll file/fix :-))
Attached patch cleanup.diff (obsolete) — Splinter Review
here we go - can you get this landed and installed?
Attachment #8338691 - Flags: review?(emorley)
Flags: needinfo?(lsblakk)
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 on attachment 8338691 [details] [diff] [review]
cleanup.diff

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

(Forgot to set the flag)
Attachment #8338691 - Flags: review?(emorley) → review+
Attached patch cleanup take 2Splinter Review
here we go, thanks!
Attachment #8338691 - Attachment is obsolete: true
Attachment #8338968 - Flags: review+
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)
(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.
(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 :-)
Product: Release Engineering → Developer Services
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: