Closed Bug 1170718 Opened 5 years ago Closed 2 years ago

Remove hg hook that complains about not reving UUIDs

Categories

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

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jrmuizel, Unassigned)

Details

Attachments

(1 file)

UUIDs don't really serve any purpose anymore. We shouldn't complain about not changing them.
This seems like a big change and it's news to me! I'd like a second opinion.

Benjamin: do you support this change?
Flags: needinfo?(benjamin)
Aren't UUIDs still used by the build system to do correct incremental rebuilds?

I feel like if we aren't going to care about this we ought to get rid of IDL UUIDs altogether and generate random UUIDs at build time.
Flags: needinfo?(benjamin)
The build system gets its dependencies from the IDL parser (.deps attribute). And, from a brief reading of xpcom/idl-parser/xpidl.py. I /think/ UUIDs have nothing to do with dependency management.

Assuming the build system doesn't care about UUIDs, does that mean you approve removing the hook? Or do you want to block it on changing how UUIDs work?
Flags: needinfo?(benjamin)
We should not make changes to the hg hooks until we've changed the rule about revving IIDs. Since that hasn't happened, the hook should stay in place.
Flags: needinfo?(benjamin)
Closing this until a new IID policy is instituted.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → WONTFIX
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #4)
> We should not make changes to the hg hooks until we've changed the rule
> about revving IIDs. Since that hasn't happened, the hook should stay in
> place.

Revving IIDs was only required for binary extensions as far as I know, which we don't support any more.  Is there another reason to keep doing that?
Flags: needinfo?(nfroyd)
Flags: needinfo?(benjamin)
(In reply to :Ehsan Akhgari from comment #6)
> (In reply to Benjamin Smedberg  [:bsmedberg] from comment #4)
> > We should not make changes to the hg hooks until we've changed the rule
> > about revving IIDs. Since that hasn't happened, the hook should stay in
> > place.
> 
> Revving IIDs was only required for binary extensions as far as I know, which
> we don't support any more.  Is there another reason to keep doing that?

Bug 977464?  (Thank you gps for laying out the problem in that bug report!)
Flags: needinfo?(nfroyd)
(In reply to Nathan Froyd [:froydnj] from comment #7)
> (In reply to :Ehsan Akhgari from comment #6)
> > (In reply to Benjamin Smedberg  [:bsmedberg] from comment #4)
> > > We should not make changes to the hg hooks until we've changed the rule
> > > about revving IIDs. Since that hasn't happened, the hook should stay in
> > > place.
> > 
> > Revving IIDs was only required for binary extensions as far as I know, which
> > we don't support any more.  Is there another reason to keep doing that?
> 
> Bug 977464?  (Thank you gps for laying out the problem in that bug report!)

I submitted a patch to remove this requirement for incremental builds.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Greg, what's the correct way to disable a hook for central/inbound and the like but keeping it enabled on Aurora/Beta/Release etc?
Flags: needinfo?(benjamin) → needinfo?(gps)
You ask me or fubar to remove the hook and we SSH into the server and do it.
Flags: needinfo?(gps)
Thanks!  Will that mean that the hook will ride the trains to disappear from all branches?

If not, it may be better to modify the hook to check out the version number in browser/config/version.txt and auto-disable itself on 47 onward.

What do you think?
Flags: needinfo?(gps)
Hooks don't ride the trains since each repo is independent (sadly).

We could look at version.txt. But it's probably easier to just disable the hook every 6 weeks since the hook will disappear in a few months.
Flags: needinfo?(gps)
(In reply to Gregory Szorc [:gps] from comment #12)
> We could look at version.txt. But it's probably easier to just disable the
> hook every 6 weeks since the hook will disappear in a few months.

Even given things like ESR and b2g branches?

If that's fine with you, I'm happy to just have the hook be disabled manually, but I have no faith in my memory to be able to remind you guys at the right times...  :(
Greg, can you please remove this hook from central, inbound, fx-team and b2g-inbound (and other similar repos which I may be forgetting)?

(Or if you'd like me to do the version.txt alternative, please let me know.)
Flags: needinfo?(gps)
Ping?
Removed from central, inbound, fx-team, b2g-inbound, and comm-central (which I also thinks meets the requirements since it is a tip-like repo).
Flags: needinfo?(gps)
(In reply to Gregory Szorc [:gps] from comment #16)
> Removed from central, inbound, fx-team, b2g-inbound, and comm-central (which
> I also thinks meets the requirements since it is a tip-like repo).

comm-central might still want this, since Thunderbird still permits binary extensions...though I suppose if Gecko isn't updating its UUIDs, Thunderbird's binary extension story gets tricky anyway...
Do you want this hook on c-c?
Flags: needinfo?(Pidgeot18)
C-c binary components use components in m-c, they will have hard time with the unrev'ed components.
Reintroducing only to c-c will not solve them problem.
(In reply to Gregory Szorc [:gps] from comment #18)
> Do you want this hook on c-c?

If mozilla-central isn't revving its UUIDs, then there's little point in forcing comm-central to rev its UUIDs, so no.
Flags: needinfo?(Pidgeot18)
I just removed the UUID hook from mozilla-beta since it looks like we stopped requiring UUID bumps as part of the 47 release.
QA Contact: hwine → klibby
So that's everywhere we care about at this point, right? Can we resolve this bug?
Flags: needinfo?(gps)
hghooks/mozhghooks/prevent_idl_change_without_uuid_bump.py and hghooks/mozhghooks/prevent_uuid_changes.py (and associated tests) still exist. I assume we'll want to nuke these hooks and traces of them from repo configs.

I'll do that right now...
Flags: needinfo?(gps)
Comment on attachment 8911782 [details]
hghooks: remove legacy IDL/UUID change verification hooks (bug 1170718);

https://reviewboard.mozilla.org/r/183204/#review188360

Thanks for cleaning this up.

::: hgserver/tests/test-hooks-smoketest.t
(Diff revision 1)
>  Install hooks that mimic production
>  
>    $ hgmo exec hgssh /set-hgrc-option mozilla-central hooks pretxnchangegroup.b_singlehead python:mozhghooks.single_head_per_branch.hook
>    $ hgmo exec hgssh /set-hgrc-option mozilla-central hooks pretxnchangegroup.c_commitmessage python:mozhghooks.commit-message.hook
>    $ hgmo exec hgssh /set-hgrc-option mozilla-central hooks pretxnchangegroup.d_webidl python:mozhghooks.prevent_webidl_changes.hook
> -  $ hgmo exec hgssh /set-hgrc-option mozilla-central hooks pretxnchangegroup.e_idl_no_uuid python:mozhghooks.prevent_idl_change_without_uuid_bump.hook

I guess we never enabled the `prevent_uuid_changes` hook for smoketesting... =/
Attachment #8911782 - Flags: review?(nfroyd) → review+
Comment on attachment 8911782 [details]
hghooks: remove legacy IDL/UUID change verification hooks (bug 1170718);

https://reviewboard.mozilla.org/r/183204/#review188360

> I guess we never enabled the `prevent_uuid_changes` hook for smoketesting... =/

This test fail is relatively new compared to the hook. The test coverage wasn't terrible important, as the hook's .t tests were sufficient for most cases. So no frowny face is deserved :)
Pushed by gszorc@mozilla.com:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/6a20a18a2d58
hghooks: remove legacy IDL/UUID change verification hooks ; r=froydnj
Status: REOPENED → RESOLVED
Closed: 5 years ago2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.