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

RESOLVED FIXED in mozilla13

Status

()

Core
Build Config
--
enhancement
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: sgautherie, Assigned: sgautherie)

Tracking

Trunk
mozilla13
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

6 years ago
+++ 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?
Flags: in-testsuite-
(Assignee)

Comment 1

6 years ago
Created attachment 591510 [details] [diff] [review]
(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.
Attachment #591510 - Flags: review?(ted.mielczarek)
(Assignee)

Updated

6 years ago
Attachment #591510 - Attachment description: (Av1) pyxpt: Report Typelib filename(s) when IIDs/names differ. → (Av1) pyxpt: Report Typelib filename and mtime, when IIDs/names differ
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.
Attachment #591510 - Flags: review?(ted.mielczarek) → review+
(Assignee)

Comment 3

6 years ago
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?
Attachment #591510 - Flags: feedback?(ted.mielczarek)
(Assignee)

Comment 4

6 years ago
Ping for feedback.
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.
Attachment #591510 - Flags: feedback?(ted.mielczarek)
(Assignee)

Comment 6

6 years ago
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.
Attachment #591510 - Flags: feedback?(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.
Attachment #591510 - Flags: feedback?(ted.mielczarek) → feedback+
(Assignee)

Comment 8

5 years ago
Created attachment 597293 [details] [diff] [review]
(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.
Attachment #591510 - Attachment is obsolete: true
Attachment #597293 - Flags: review?(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.
Attachment #597293 - Flags: review?(ted.mielczarek) → review+
(Assignee)

Comment 10

5 years ago
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
Attachment #597293 - Attachment description: (Av1a) pyxpt: Report Typelib filename, when IIDs/names differ → (Av1a) pyxpt: Report Typelib filename, when IIDs/names differ [Checked in: See comment 10]
(Assignee)

Updated

5 years ago
Target Milestone: --- → mozilla13
(Assignee)

Comment 11

5 years ago
Created attachment 597553 [details] [diff] [review]
(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.)
Attachment #597553 - Flags: review?(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?
Attachment #597553 - Flags: review?(ted.mielczarek) → review-
(Assignee)

Comment 13

5 years ago
Created attachment 598044 [details] [diff] [review]
(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.)
Attachment #597553 - Attachment is obsolete: true
Attachment #598044 - Flags: review?(ted.mielczarek)
(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]
(Assignee)

Comment 15

5 years ago
(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 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.
Attachment #598044 - Flags: review?(ted.mielczarek) → review+
(Assignee)

Comment 17

5 years ago
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.
Attachment #598044 - Attachment description: (Bv2) pyxpt: Report true Typelib filename for both files, when IIDs/names differ → (Bv2) pyxpt: Report true Typelib filename for both files, when IIDs/names differ [Checked in: See comment 17]
(Assignee)

Updated

5 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Depends on: 720955
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.