721065 - pyxpt: Report Typelib name(s) when IIDs/names differ

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Serge Gautherie (:sgautherie)]

+++ This bug was initially created as a clone of Bug #720955 +++

Part 1 was fixed there.
This bug is to fix part 2, at least a minimum.

***

Code is
{
1210                         else:
1211                             # Same name, different IIDs, raise an exception
1212                             raise DataError, \
1213                                   "Typelibs contain definitions of interface %s"\
1214                                   " with different IIDs!" % i.name
1215                 elif i.iid == j.iid and i.iid != Interface.UNRESOLVED_IID:
1216                     # Same IID, different names, raise an exception
1217                     raise DataError, \
1218                           "Typelibs contain definitions of interface %s"\
1219                           " with different names (%s vs. %s)!" %  \
1220                           (i.iid, i.name, j.name)
}

1) Line 1213:
IIDs should be reported, as names are at line 1218.

2) Both:
Could it be possible to report the involved xpts too?
With their timestamp?

Comment 1

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) pyxpt: Report Typelib filename and mtime, when IIDs/names differ

Minimal fix.
(Untested.)

Should I "pollute" class Interface instead with the 2 Typelib data, so "self.*" could become more meaningful as "j.*" ?
Assuming that wouldn't break anything.

NB: There might be other places which could use these new data, but I am not looking for them ftb.

Comment 2

[(not currently active) Ted Mielczarek]

Comment on attachment 591510 [details] [diff] [review]
(Av1) pyxpt: Report Typelib filename and mtime, when IIDs/names differ

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

r=me with one change.

::: xpcom/typelib/xpt/tools/xpt.py
@@ +1227,5 @@
>                      raise DataError, \
>                            "Typelibs contain definitions of interface %s" \
> +                            " with different names (%s (%s, %s) vs %s (%s, %s))!" % \
> +                            (i.iid, i.name, other.filename, time.ctime(other.mtime), \
> +                                    j.name, self.filename, time.ctime(self.mtime))

I don't think the modification time provides enough usefulness here to justify the extra noise in the output. Just output the filenames.

Comment 3

[Serge Gautherie (:sgautherie)]

Comment on attachment 591510 [details] [diff] [review]
(Av1) pyxpt: Report Typelib filename and mtime, when IIDs/names differ

(In reply to Ted Mielczarek [:ted, :luser] from comment #2)
> I don't think the modification time provides enough usefulness here to
> justify the extra noise in the output. Just output the filenames.

My idea about mtime is to help notice an older/obsolete typelib.
Also what about my comment 1 question? (which, of the two, would be the most useful.)

I agree about some "noise", but these errors should happen rarely and if more data can help figure_out/confirm (quicker) what is going on...
(As in bug 720952, these errors can happen +/- "randomly", making it more difficult to trace back what/when happened.)

What do you think?

Comment 4

[Serge Gautherie (:sgautherie)]

Ping for feedback.

Comment 5

[(not currently active) Ted Mielczarek]

Comment on attachment 591510 [details] [diff] [review]
(Av1) pyxpt: Report Typelib filename and mtime, when IIDs/names differ

I already r+ed this with that change made. My comments still stand.

Comment 6

[Serge Gautherie (:sgautherie)]

Comment on attachment 591510 [details] [diff] [review]
(Av1) pyxpt: Report Typelib filename and mtime, when IIDs/names differ

(In reply to Ted Mielczarek [:ted, :luser] from comment #5)
> I already r+ed this with that change made. My comments still stand.

I understand that you confirm not wanting mtime at all: noted.

What about my other question:
{
Should I "pollute" class Interface instead with the 2 Typelib data, so "self.*" could become more meaningful as "j.*" ?
Assuming that wouldn't break anything.
}
NB: Sorry to insist, but I don't think you explicitly answered that one and I think that may be valuable.

Comment 7

[(not currently active) Ted Mielczarek]

Comment on attachment 591510 [details] [diff] [review]
(Av1) pyxpt: Report Typelib filename and mtime, when IIDs/names differ

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

::: xpcom/typelib/xpt/tools/xpt.py
@@ +1063,5 @@
>                  raise FileFormatError, "Bad magic: %s" % data[0]
>              xpt = Typelib((data[1], data[2]))
> +            # Remember filename and modification time.
> +            xpt.filename = filename
> +            xpt.mtime = os.path.getmtime(filename)

It seems fine to stick the filename on here, but please add a value for it to the initialized class up above. It's perfectly legal to create Typelib instances by instantiating them directly (and not via Typelib.read), and if you're going to rely on .filename, it should always be set (even if it's None for when it didn't come from a file).

Let's not bother with the mtime.

Comment 8

[Serge Gautherie (:sgautherie)]

Attachment: (Av1a) pyxpt: Report Typelib filename, when IIDs/names differ [Checked in: See comment 10]

Av1, with comment 7 suggestion(s).

NB: I think that still doesn't really answer my comment 1 question. But let's complete this (first) part at least.

Comment 9

[(not currently active) Ted Mielczarek]

Comment on attachment 597293 [details] [diff] [review]
(Av1a) pyxpt: Report Typelib filename, when IIDs/names differ
[Checked in: See comment 10]

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

::: xpcom/typelib/xpt/tools/xpt.py
@@ +1015,5 @@
>          """
>          self.version = version
>          self.interfaces = list(interfaces)
>          self.annotations = list(annotations)
> +        # (Optionnaly) Set by read().

This comment seems unnecessary.

Comment 10

[Serge Gautherie (:sgautherie)]

Comment on attachment 597293 [details] [diff] [review]
(Av1a) pyxpt: Report Typelib filename, when IIDs/names differ
[Checked in: See comment 10]

https://hg.mozilla.org/mozilla-central/rev/ae8cce613aa0

Comment 11

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) pyxpt: Report true Typelib filename for both files, when IIDs/names differ

What I was thinking about in comment 1: to report true/initial filenames, as much as possible.
(Untested.)

Comment 12

[(not currently active) Ted Mielczarek]

Comment on attachment 597553 [details] [diff] [review]
(Bv1) pyxpt: Report true Typelib filename for both files, when IIDs/names differ

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

::: xpcom/typelib/xpt/tools/xpt.py
@@ +869,5 @@
>          # These are only used for writing out the interface
>          self._descriptor_offset = 0
>          self._name_offset = 0
>          self._namespace_offset = 0
> +        self.xpt = None

I don't think I want this, because it causes cycles which are likely to lead to excessive memory usage. How about you just store xpt_filename instead?

Comment 13

[Serge Gautherie (:sgautherie)]

Attachment: (Bv2) pyxpt: Report true Typelib filename for both files, when IIDs/names differ [Checked in: See comment 17]

Bv1, with comment 13 suggestion(s).
(Untested.)

Comment 14

[Justin Wood (:Callek)]

(In reply to Ted Mielczarek [:ted, :luser] from comment #12)
> Comment on attachment 597553 [details] [diff] [review]
> (Bv1) pyxpt: Report true Typelib filename for both files, when IIDs/names
> differ
> 
> Review of attachment 597553 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: xpcom/typelib/xpt/tools/xpt.py
> @@ +869,5 @@
> >          # These are only used for writing out the interface
> >          self._descriptor_offset = 0
> >          self._name_offset = 0
> >          self._namespace_offset = 0
> > +        self.xpt = None
> 
> I don't think I want this, because it causes cycles which are likely to lead
> to excessive memory usage. How about you just store xpt_filename instead?

Possibly could use weakref [http://docs.python.org/library/weakref.html]

Comment 15

[Serge Gautherie (:sgautherie)]

(In reply to Justin Wood (:Callek) from comment #14)

> > I don't think I want this, because it causes cycles which are likely to lead
> > to excessive memory usage. How about you just store xpt_filename instead?

I thought about it when I wrote my patch, but I wanted to have a review on it.

> Possibly could use weakref [http://docs.python.org/library/weakref.html]

Interesting, but I think it doesn't apply in this case:
(xpt.)filename must not be lost (even if initial xpt is garbage collected, after being merged).

Comment 16

[(not currently active) Ted Mielczarek]

Comment on attachment 598044 [details] [diff] [review]
(Bv2) pyxpt: Report true Typelib filename for both files, when IIDs/names differ
[Checked in: See comment 17]

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

Thanks, that looks good.

Comment 17

[Serge Gautherie (:sgautherie)]

Comment on attachment 598044 [details] [diff] [review]
(Bv2) pyxpt: Report true Typelib filename for both files, when IIDs/names differ
[Checked in: See comment 17]

https://hg.mozilla.org/mozilla-central/rev/2e34d3d7071f
Bv2, with removed leftover [ij].xpt checks.


Example of (intentional) error:
https://tbpl.mozilla.org/?tree=Try&rev=11b2fc055cf6
{
__main__.DataError: Typelibs contain definitions of interface 41e88f87-42cb-4db1-8724-f5456a16c410 with different names (mozIStorageResultSet (../../dist/xpt/browser/components/storage.xpt) vs nsIDOMMozBatteryManager (../../dist/xpt/browser/components/dom_battery.xpt))!
}
:-)

Ftr, I noticed
http://mxr.mozilla.org/comm-central/source/mozilla/xpcom/typelib/xpt/tools/runtests.py#398
399     def test_mergeConflict(self):
which tests that this code triggers.