Closed Bug 578478 Opened 14 years ago Closed 13 years ago

xpidl.py can't parse [deprecated] attributes/methods

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla8

People

(Reporter: humph, Assigned: khuey)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: fixed-in-bs)

Attachments

(5 files, 3 obsolete files)

Attached file IDL Test Case with [deprecated] member (obsolete) —
Currently xpidl.py fails when parsing IDL members flagged [deprecated]:

Traceback (most recent call last):
  File "header.py", line 443, in <module>
    idl = p.parse(open(file).read(), filename=file)
  File "/home/dave/moz/mozilla-central/xpcom/idl-parser/xpidl.py", line 1321, in parse
    return self.parser.parse(lexer=self)
  File "/usr/lib/python2.6/site-packages/ply/yacc.py", line 265, in parse
    return self.parseopt_notrack(input,lexer,debug,tracking,tokenfunc)
  File "/usr/lib/python2.6/site-packages/ply/yacc.py", line 971, in parseopt_notrack
    p.callable(pslice)
  File "/home/dave/moz/mozilla-central/xpcom/idl-parser/xpidl.py", line 1231, in p_member_method
    raises=p[7])
  File "/home/dave/moz/mozilla-central/xpcom/idl-parser/xpidl.py", line 765, in __init__
    raise IDLError("Unexpected attribute '%s'", aloc)
xpidl.IDLError: error: Unexpected attribute '%s', /tmp/deprecated-test.idl line 4:3
  [deprecated] boolean bad();
   ^
We have a few places in the tree where we do this now:

other-licenses/ia2/Accessible2.idl
storage/public/mozIStorageStatementWrapper.idl
storage/public/mozIStorageBaseStatement.idl
storage/public/mozIStorageStatement.idl

I've attached a small test case.
Attachment #457143 - Attachment is obsolete: true
Attachment #457367 - Flags: review? → review?(jorendorff)
Tested this against trunk (Linux), works great!
Comment on attachment 457367 [details] [diff] [review]
PyXPIDL header changes, rev. 1

Turning on pyxpidl is a bold change. Sounds good to me though.
Attachment #457367 - Flags: review?(jorendorff) → review+
Comment on attachment 461347 [details] [diff] [review]
Create deps from the IDL parser, rev. 1

Shame optparse doesn't give you a way to enforce "these two options must come together" on its own.
Attachment #461347 - Flags: review?(me) → review+
Comment on attachment 457367 [details] [diff] [review]
PyXPIDL header changes, rev. 1

I'm going to take these now, so DXR can be happier.
Attachment #457367 - Flags: approval2.0+
shouldn't this raise the need for python > 2.4? I just got a build error with .rpartition not contained in str when building with python-2.4.4. Googling around I found that .rpartition was introduced with python-2.5.
Hg says yes.

http://hg.mozilla.org/mozilla-central/rev/81344fe5f7a0

Marking FIXED.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Ah, this got backed out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
To fix the race condition we just check in the generated files.

The fatvals stuff is slightly more interesting.  Right now "jsval foo(in jsval bar)" has the signature "NS_IMETHOD foo(const jsval& bar, jsval* foo)" which I think is a bug.  This changes it to be references across the board and fixes up the tree.
Attachment #478652 - Flags: review?(benjamin)
That patch causes some of the indexeddb tests to fail.
khuey, are you sure that "references across the board" is correct? jsval* foo sound like the correct outparam form of a jsval return value.

Of course, matching the existing behavior of binary-xpidl is probably more important.

Checking in the generated files won't actually work unless you also somehow teach ply not to attempt to regenerate them if the timestamps don't work. And it seems really icky to me anyway... thoughts?
Attachment #478652 - Attachment is obsolete: true
Attachment #478652 - Flags: review?(benjamin)
(In reply to comment #15)
> khuey, are you sure that "references across the board" is correct? jsval* foo
> sound like the correct outparam form of a jsval return value.

Why?  I think the code at http://mxr.mozilla.org/mozilla-central/source/xpcom/typelib/xpidl/xpidl_typelib.c#877 is bogus.  In particular, nsrootidl defines jsval as 

"[ref, jsval]  native jsval(jsval);"

The code in the binary xpidl ignores the ref modifier, which is a bug.

> Of course, matching the existing behavior of binary-xpidl is probably more
> important.

Except that it's wrong.  I think we either need to change the definition of jsval or change the xpidl compiler for 2.0.  Noming for blocking because of that.
 
> Checking in the generated files won't actually work unless you also somehow
> teach ply not to attempt to regenerate them if the timestamps don't work. And
> it seems really icky to me anyway... thoughts?

AFAICT ply doesn't rewrite the files to disk if it reads them in successfully, so I don't think that's an issue.
blocking2.0: --- → ?
I believe it deletes/rewrites them if the modification time of any of the input files is later than the cache file.

jsval is a [ref] inparam so that we know the size (pointer-size), since jsval is a 64-bit int or struct size depending on platform. I'll assert that the correct types for jsval are:

(const jsval& inParam, jsval* outParam);

Although I'd also accept:

(const jsval& inParam, jsval&* outParam);

Either way, for now we should just do whatever the binary xpidl does. jsval is a special type anyway.
(In reply to comment #17)
> I believe it deletes/rewrites them if the modification time of any of the input
> files is later than the cache file.

Right, so that just means that if the input files change we have to check in the updated generated files too, right?

> jsval is a [ref] inparam so that we know the size (pointer-size), since jsval
> is a 64-bit int or struct size depending on platform. I'll assert that the
> correct types for jsval are:
> 
> (const jsval& inParam, jsval* outParam);
> 
> Although I'd also accept:
> 
> (const jsval& inParam, jsval&* outParam);
> 
> Either way, for now we should just do whatever the binary xpidl does. jsval is
> a special type anyway.

OK.
blocking2.0: ? → ---
hg doesn't preserve file-modification times when you clone, so if the cache files are written by hg before xpidl.py, the very first build at least would cause PLY to remake the cache files.
Attached patch Final fixupsSplinter Review
Modify xpidl.py to handle jsvals the same way binary xpidl does and move the cache dir to the tree.  Also check in the generated files.

I've verified that if I touch xpidl.py and reexport the tree ply doesn't regenerate the generated files.  From poking around in ply's source, it looks like passing optimize=1 (which we do) shortcuts any "check if the generated files are valid" logic and uses them without question.
Attachment #485761 - Flags: review?(benjamin)
Comment on attachment 457367 [details] [diff] [review]
PyXPIDL header changes, rev. 1

I don't think we'd let this into the tree anymore; renominate if you disagree, and explain why!
Attachment #457367 - Flags: approval2.0+ → approval2.0-
Attachment #485761 - Flags: review?(benjamin) → review+
Assignee: benjamin → khuey
I've updated these patches, but they totally destroy our buildtime on Windows (25% or more in my test).
We should patch bug 585015 and test with it as a pymake native command.
Attached patch Modified version for checking (obsolete) — Splinter Review
This appears to compile everybody, at least by running with this as my myrules.mk:
ifneq ($(XPIDLSRCS),)
XPIDL_DEPS = \
             $(topsrcdir)/xpcom/idl-parser/header.py \
             $(topsrcdir)/xpcom/idl-parser/xpidl.py \
             $(NULL)
$(XPIDL_GEN_DIR)/%-py.h: %.idl $(XPIDL_GEN_DIR)/%.h $(XPIDL_DEPS)
  $(PYTHON) $(topsrcdir)/config/pythonpath.py \
    -I$(topsrcdir)/other-licenses/ply \
    -I$(topsrcdir)/xpcom/idl-parser \
    $(topsrcdir)/xpcom/idl-parser/header.py --cachedir=$(DEPTH)/xpcom/idl-parser -I $(DIST)/idl $(_VPATH_SRCS) > $@

check-%-idl: $(XPIDL_GEN_DIR)/%-py.h $(XPIDL_GEN_DIR)/%.h
  diff -Bw -I'/\*.*\*/' $^
export:: $(patsubst %.idl,check-%-idl, $(XPIDLSRCS))
endif

Note that I disabled document comments in xpidl_header (there's one document comment somewhere which is stashed at the end of the interface... trying to keep track of that just isn't worth it). I also ignored the fact that the attributes just constantly get wrongly ordered, and ignored the various whitespace changes. I'll see if I can get a profile to see where time improvements could be made.
Doing some benchmarking, this doesn't appear to have any perf impact on Linux.  It's solely limited to windows, which makes me think it might just be process overhead and that pymake can save us.
This patch gets us better parity with xpidl-header, and essentially prints out the same information [IIRC] if you exclude comments (as mentioned earlier, one document comment is off and trying to fudge the output attribute serialization order just isn't worth it).

I could add a few more changes:
1. Modifying the CDATA regex ('(?s)%{[^\n]*\n.*?\n%}[^\n]*$' is what I have in my dxr internal lexer)
2. Handle octal literals properly [I don't see any in anything pulled in by comm-central, so we probably don't care]
3. Add in the other expression operators (^,&,/,%, and unary +,-,~)
4. Initial `_' in identifier

What I don't aim to do in this patch is to replace the native compiler. I'm happy for now to just have a version which I trust to be able to parse the IDL files without errors.
Attachment #542353 - Attachment is obsolete: true
Attachment #546636 - Flags: review?(benjamin)
And I timed it on Windows again and it was at most a 5% slowdown.  Not sure what I did to get teh numbers in comment 22 :-/
http://hg.mozilla.org/mozilla-central/rev/85f7679fd5eb
http://hg.mozilla.org/mozilla-central/rev/e5a6f94154c7
Status: REOPENED → RESOLVED
Closed: 14 years ago13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
Comment on attachment 546636 [details] [diff] [review]
Synchronize xpidl.py with native xpidl

I'm not sure how much of this is still needed, bug if it is please file a new bug for it to avoid confusion.
Attachment #546636 - Flags: review?(benjamin)
Kyle, xpcom/tests still seems to use old xpidl.exe in http://mxr.mozilla.org/mozilla-central/source/xpcom/tests/Makefile.in#196.  Should we keep this or file a new bug?
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: