1345111 - Make the IPDL compiler error out when it finds sync-messages.ini entries without corresponding IPDL sync messages

Status: RESOLVED FIXED

(Core :: IPC, enhancement)

Description

[(no longer active)]

This ensures that our sync-messages.ini correctly reflects the existing set of
sync IPC messages we have.

Comment 1

[(no longer active)]

Attachment: Make the IPDL compiler error out when it finds sync-messages.ini entries without corresponding IPDL sync messages

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8844446 [details] [diff] [review]
Make the IPDL compiler error out when it finds sync-messages.ini entries without corresponding IPDL sync messages

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

::: ipc/ipdl/ipdl/checker.py
@@ +8,5 @@
>  
>  class SyncMessageChecker(Visitor):
>      def __init__(self, syncMsgList):
>          self.syncMsgList = syncMsgList
> +        self.seenProtocols = []

It might be nice to make some of these fields static. That is, define them as SyncMessageChecker.seenProtocols, for example. Then you don't need to use the same instance every time.

@@ +39,3 @@
>              self.errorAsyncMessageCanRemove(md.loc, pn)
>  
> +    def getFixedSyncMessages(self):

This could be an @static_method then.

@@ +39,4 @@
>              self.errorAsyncMessageCanRemove(md.loc, pn)
>  
> +    def getFixedSyncMessages(self):
> +        return list(set(self.syncMsgList) - set(self.seenSyncMessages))

Doesn't seem like you need this list coercion here.

Comment 3

[(no longer active)]

Thanks for the suggestions.  I didn't know python supports static data/methods.  :-)

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2288120b007e
Make the IPDL compiler error out when it finds sync-messages.ini entries without corresponding IPDL sync messages; r=billm

Comment 5

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

I had to back this out for build failures like https://treeherder.mozilla.org/logviewer.html#?job_id=82303633&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/c16695dbf5f0

Comment 6

[(no longer active)]

PDocAccessible::GetWindowedPluginIAccessible is a Windows only sync IPC message unfortunately, so I'll have to do something more sophisticated.

I think we can use a platform annotation in the ini file to note platform specific sync IPC messages, and ignore those in this check.  I will post a patch that does this in a second.

Comment 7

[(no longer active)]

Attachment: Make the IPDL compiler error out when it finds sync-messages.ini entries without corresponding IPDL sync messages

This ensures that our sync-messages.ini correctly reflects the existing set of
sync IPC messages we have.

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8844710 [details] [diff] [review]
Make the IPDL compiler error out when it finds sync-messages.ini entries without corresponding IPDL sync messages

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

::: ipc/ipdl/ipdl/checker.py
@@ +11,3 @@
>          self.errors = []
> +        SyncMessageChecker.syncMsgList = syncMsgList
> +        SyncMessageChecker.seenProtocols = []

You can actually define these initially in the class itself:

class SyncMessageChecker:
  seenProtocols = []
  seenSyncMessages = []

syncMessageList you'll of course need to keep here.

@@ +58,5 @@
> +        protocol = item.split('::')[0]
> +        # Ignore things like sync messages in test protocols we didn't compile.
> +        # Also, ignore platform-specific IPC messages.
> +        if protocol in SyncMessageChecker.seenProtocols and \
> +           'platform' not in config.options(item):

Might be nice to check platform.system() or something here, but I can imagine why you didn't want to.

Comment 9

[(no longer active)]

(In reply to Bill McCloskey (:billm) from comment #8)
> @@ +58,5 @@
> > +        protocol = item.split('::')[0]
> > +        # Ignore things like sync messages in test protocols we didn't compile.
> > +        # Also, ignore platform-specific IPC messages.
> > +        if protocol in SyncMessageChecker.seenProtocols and \
> > +           'platform' not in config.options(item):
> 
> Might be nice to check platform.system() or something here, but I can
> imagine why you didn't want to.

Yeah... I mean, honestly for this purpose all we care about is whether we have to keep them listed in this file, so as long as we need them on any platform, we don't care exactly which one.  ;-)

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e77b218d6da5
Make the IPDL compiler error out when it finds sync-messages.ini entries without corresponding IPDL sync messages; r=billm

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/db91cf168102
follow-up: Add a few platform-specific sync IPC messages back; fake-r=billm

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e77b218d6da5
https://hg.mozilla.org/mozilla-central/rev/db91cf168102