1348852 - Add a commit hook that requires a special keyword to override changes to NSPR and NSS

Status: RESOLVED FIXED

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

Description

[Kai Engert (:KaiE:)]

In the recent past, it has repeatedly happened, that people broke the rule that Firefox shouldn't use any local patches on top of NSPR (except in emergencies).

Could we use the following approach?

"Require that any commit to directory /nsprpub
 contains the string OVERRIDE_NSPR_RELEASE in the commit message."

Comment 1

[Kai Engert (:KaiE:)]

I've filed bug 1348853 to request a similar hook for NSS.

Comment 2

[Kai Engert (:KaiE:)]

Masatoshi, thanks a lot for suggesting a commit hook in bug 1347932.

Do you know who could help us with getting that implemented?
Does this bug have the correct component?

Comment 3

[Kai Engert (:KaiE:)]

I've been pointed to 
  https://hg.mozilla.org/hgcustom/version-control-tools/file/tip/hghooks/mozhghooks

We should work on a python script to implement the hook.

Comment 5

[Kai Engert (:KaiE:)]

Attachment: 1348852-v1.patch

Comment 6

[Kai Engert (:KaiE:)]

Comment on attachment 8849544 [details] [diff] [review]
1348852-v1.patch

What do you think?

I suggest to add this hook to
  mozilla-central, 
  mozilla-inbound, 
  autoland, 
  mozilla-aurora, 
  mozilla-beta, 
  mozilla-release, and 
  mozilla-esr52.

Comment 7

[(not currently active) Ted Mielczarek]

Comment on attachment 8849544 [details] [diff] [review]
1348852-v1.patch

The concept seems fine. I'm going to let gps do the actual review here since he's more in-tune with our Mercurial customizations.

Comment 8

[Gregory Szorc [:gps]]

Comment on attachment 8849544 [details] [diff] [review]
1348852-v1.patch

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

This is mostly good. Should get r+ on next round of review.

::: hghooks/mozhghooks/prevent_nspr_nss_changes.py
@@ +1,4 @@
> +# This software may be used and distributed according to the terms of the
> +# GNU General Public License version 2 or any later version.
> +
> +import grp

Unused import.

@@ +18,5 @@
> +        ctx = repo[rev]
> +        have_nspr = any(f.startswith('nsprpub/') for f in ctx.files())
> +        have_nss = any(f.startswith('security/nss/') for f in ctx.files())
> +
> +        if have_nspr:

You can just inline this:

if any(f.startswith...:

@@ +31,5 @@
> +        if nspr_nodes:
> +            ui.write('(%d changesets contain changes to protected nsprpub/ '
> +                     'directory: %s)\n' % (len(nspr_nodes), ', '.join(nspr_nodes)))
> +
> +            if repo.changectx('tip').description().find("UPGRADE_NSPR_RELEASE") == -1:

if 'UPGRADE_NSPR_RELEASE' not in repo['tip'].description():

@@ +41,5 @@
> +        if nss_nodes:
> +            ui.write('(%d changesets contain changes to protected security/nss/ '
> +                     'directory: %s)\n' % (len(nss_nodes), ', '.join(nss_nodes)))
> +
> +            if repo.changectx('tip').description().find("UPGRADE_NSS_RELEASE") == -1:

Please also fix this revision access and text test.

@@ +47,5 @@
> +                res = 1
> +            else:
> +                ui.write('good, you said you are following the rules and are upgrading to an NSS release snapshot, permission granted.\n')
> +        
> +        if res == 1:

if res:

@@ +56,5 @@
> +            ui.write('\n')
> +            ui.write('these directories are kept in sync with the canonical '
> +                     'upstream repositories at\n'
> +                     'https://hg.mozilla.org/projects/nspr and '
> +                     'https://hg.mozilla.org/projects/nspr\n')

You said nspr twice.

::: hghooks/tests/test-prevent-nspr-nss-changes.t
@@ +11,5 @@
> +
> +  $ touch file0
> +  $ hg -q commit -A -m "initial"
> +
> +  $ USER=[e-mail] hg push

USER here should be irrelevant. Please remove it here and in the rest of the file.

@@ +39,5 @@
> +  
> +  these directories are kept in sync with the canonical upstream repositories at
> +  https://hg.mozilla.org/projects/nspr and https://hg.mozilla.org/projects/nspr
> +  
> +  please contact the NSPR/NSS developers to get your changes merged, released and uplifted.

Reading the test and relating to the experience of seeing this message, my next question is "how do I contact the NSPR or NSS developers?" Can you add some minimal instructions here?

@@ +92,5 @@
> +  [255]
> +
> +With magic word, can change security/nss/
> +
> +  $ USER=[e-mail] hg rollback

`hg rollback` is deprecated and should not be used by users or by tests. Please add "[extensions]\nstrip=" to the hgrc in the test and use `hg strip -r .` instead.

Comment 9

[Kai Engert (:KaiE:)]

Attachment: 1348852-v1.patch

Thanks a lot for your review.

This updated patch should address all of your requests.

Comment 10

[Kai Engert (:KaiE:)]

Attachment: 1348852-v2.patch

Correct new patch.

Comment 11

[Gregory Szorc [:gps]]

Oh, also since I reviewed this, we now have a much saner development/test experience for hg hooks and extensions in version-control-tools. It is documented at https://mozilla-version-control-tools.readthedocs.io/en/latest/hgmods/index.html#creating-and-maintaining-a-development-environment. tl;dr

  $ ./create-environment hgdev
  $ source venv/hgdev/bin/activate
  $ ./run-tests -j8 hghooks

There is no Docker and the above commands should take like 2 minutes.

Comment 12

[Gregory Szorc [:gps]]

Comment on attachment 8865855 [details] [diff] [review]
1348852-v2.patch

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

I said you'd likely get r+ after the 2nd review. I apologize, but I forgot to point out a critical issue: consideration for uplifts and merges.

Our hooks typically short circuit for uplifts:

    if 'a=release' in repo['tip'].description().lower():
        return 0

And for merge commits (since ctx.files() contains a union of files modified in both sides of a merge):

    if len(ctx.parents()) > 1:
        continue

See prevent_webidl_changes.py for an example.

This change should be pretty straightfoward and I'm fine not writing test coverage for it (since if we fail to handle these cases sheriffs tend to get quite vocal about it).

Sorry for the added review cycle: I should have caught this during the first review.

::: hghooks/mozhghooks/prevent_nspr_nss_changes.py
@@ +43,5 @@
> +                #ui.write("=== NSS change, but couldn't find NSS magic word===\n")
> +                res = 1
> +            else:
> +                ui.write('good, you said you are following the rules and are upgrading to an NSS release snapshot, permission granted.\n')
> +        

Nit: trailing whitespace

Comment 13

[Kai Engert (:KaiE:)]

Attachment: 1348852-v3.patch

No problem, thanks again for reviewing.

Comment 14

[Kai Engert (:KaiE:)]

Attachment: 1348852-v4.patch

Just a minor update to v3, I've added comments for the "skip uplifts" and "skip merges" actions.

Comment 15

[Gregory Szorc [:gps]]

Comment on attachment 8867116 [details] [diff] [review]
1348852-v4.patch

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

Looks good to me. I'll land this for you. I may deploy today if we don't have any other major changes to hg.mo infra in version-control-tools. But I won't activate the hook until next week because I don't like fire drills over the weekend.

Comment 16

[Gregory Szorc [:gps]]

I just landed this. Not sure where pulsebot is at.

Anyway, there are just enough changes in version-control-tools that I don't feel comfortable deploying this today. So needinfo me to remind myself to deploy and configure this next week.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/55a931c19a1d
hghooks: hook to verify changes to NSS and NSPR are approved ; r=gps

Comment 18

[Gregory Szorc [:gps]]

This is deployed. Hook activated on:

mozilla-central
integration/autoland
integration/mozilla-inbound
releases/mozilla-aurora
releases/mozilla-beta
releases/mozilla-release
releases/mozilla-esr52

If it issues false positives, ping someone in #vcs. If a normal VCS admin isn't around, make noise in #moc or #sysadmins.

To disable the hook:

1. ssh into hgssh4.dmz.scl3.mozilla.com
2. Comment out (with #) the line referencing "prevent_nspr_nss_changes" in /repo/hg/mozilla/<repo>/.hg/hgrc
3. Save file (changes take effect immediately)

Then needinfo [e-mail] to dig into the aftermath.

Comment 19

[Kai Engert (:KaiE:)]

(In reply to Gregory Szorc [:gps] from comment #18)
> This is deployed. Hook activated on:

Thanks a lot!

For future reference:
- upgrading to a newer snapshot of NSS requires the following keyword in the commit message:
  UPGRADE_NSS_RELEASE
- upgrading to a newer snapshot of NSPR requires the following keyword in the commit message:
  UPGRADE_NSPR_RELEASE

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I was blocked on merging mozilla-inbound to mozilla-central because of this hook. A commit on inbound updated NSS to a new version. That commit had its own "UPGRADE_NSS_RELEASE" in the message, but the merge wouldn't go through until I added that to the merge commit message.

The code is supposed to skip the hook on merges, but that doesn't seem to be working. gps says that's a bug in the hook

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I don't think it's critical enough to back out the hook (I can just add the keyword), but it would be nice to get this fixed sometime soon.

Comment 22

[Kai Engert (:KaiE:)]

(In reply to Gregory Szorc [:gps] from comment #12)
> 
> And for merge commits (since ctx.files() contains a union of files modified
> in both sides of a merge):
> 
>     if len(ctx.parents()) > 1:
>         continue
> 
> See prevent_webidl_changes.py for an example.


I might not have used the correct code when implementing this.

What I have:

    for rev in repo.changelog.revs(repo[node].rev()):
        ctx = repo[rev]
	# Skip merge changesets.
        if len(ctx.parents()) > 1:
            continue

What prevent_webidl_changes.py has:

    for i in reversed(changesets):
        c = repo.changectx(i)
        if len(c.parents()) > 1:
            # Skip merge changesets
            continue

Are these equivalent?

If they are equivalent, should we skip on "a=merge" ?

Comment 23

[Gregory Szorc [:gps]]

repo.changectx(i) and repo[rev] are basically the same.

The issue is we do a first pass to see if *any* changesets touch NSS/NSPR then only examine repo['tip'] for the special syntax. On mozilla-central, repo['tip'] could be a merge, a backout, etc. Most likely not the change touching NSS/NSPR.

We'll want to change the hook to look for the special syntax in the changeset actually touching the files, excluding merges.

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I'm again hitting a merge that's getting rejected because the merge commit doesn't have the magic words (this time, for both the NSS *and* NSPR bits of the hook) while the actual commits changing things do. 

Any progress in updating the hook?

Comment 25

[Kai Engert (:KaiE:)]

Could you help me construct a sample series of commits (locally), that I could use to test that the updated hook will handle your merges correctly?

Comment 26

[Kai Engert (:KaiE:)]

(In reply to Gregory Szorc [:gps] from comment #23)
> 
> We'll want to change the hook to look for the special syntax in the
> changeset actually touching the files, excluding merges.

Ok, got it. Will attach new patch.

Comment 27

[Kai Engert (:KaiE:)]

Attachment: 1348852-v5.patch

Comment 28

[Kai Engert (:KaiE:)]

Comment on attachment 8875267 [details] [diff] [review]
1348852-v5.patch

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

Some comments on the changes, to make reviewing easier.

::: hghooks/mozhghooks/prevent_nspr_nss_changes.py
@@ +9,4 @@
>      if source in ('pull', 'strip'):
>          return 0
>  
> +	# Leave uplifts alone.

whitespace only

@@ +33,1 @@
>  

These are the important changes. If the respective keyword is present for an individual commit, don't remember the node, just skip it.

@@ +37,4 @@
>          if nspr_nodes:
>              ui.write('(%d changesets contain changes to protected nsprpub/ '
>                       'directory: %s)\n' % (len(nspr_nodes), ', '.join(nspr_nodes)))
> +            res = 1

We don't need to check for the keyword here. We know there wasn't a keyword, because we added the node to the list. Just report which commit we're rejecting.

@@ -44,1 @@
>  

Skipping the "good" message, it seems unnecessary.

@@ +41,5 @@
>  
>          if nss_nodes:
>              ui.write('(%d changesets contain changes to protected security/nss/ '
>                       'directory: %s)\n' % (len(nss_nodes), ', '.join(nss_nodes)))
> +            res = 1

same for nss

@@ -54,1 @@
>  

same for nss

@@ +55,5 @@
>                       'https://hg.mozilla.org/projects/nspr and '
>                       'https://hg.mozilla.org/projects/nss\n')
>              ui.write('\n')
> +            ui.write('Please contact the NSPR/NSS maintainers at [e-mail] or on IRC\n'
> +                     'channel #nss to request that your changes are merged, released and uplifted.\n')

Better wrapping, to avoid very long lines.

::: hghooks/tests/test-prevent-nspr-nss-changes.t
@@ +110,3 @@
>  
> +Multiple changesets handled properly, each changeset touching protected
> +files must contain the correct corresponding keyword

clarify that we changed the rules

@@ +139,5 @@
>    rollback completed
>    abort: pretxnchangegroup.prevent_nspr_nss hook failed
>    [255]
>  
> +  $ hg -q commit --amend -m 'add security/nss/file3, UPGRADE_NSS_RELEASE'

only add the nss keyword, because the latest commit touches nss only.

The nspr keyword contained in the previous commit is sufficient to approve the nspr change from that previous commit.

Comment 29

[Kai Engert (:KaiE:)]

Please click the "Splinter Review" link next to attachment v5, to view my comments at the correct places.

Comment 30

[Gregory Szorc [:gps]]

Comment on attachment 8875267 [details] [diff] [review]
1348852-v5.patch

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

I'll land this for you so I can normalize some tabs to spaces.

Comment 31

[Gregory Szorc [:gps]]

I forgot to reference the bug number in the commit message. Derp.

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