Closed
Bug 784812
Opened 12 years ago
Closed 11 years ago
Real build dependencies for the WebIDL parser
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla22
People
(Reporter: khuey, Assigned: khuey)
References
Details
(Whiteboard: [buildfaster])
Attachments
(1 file, 5 obsolete files)
35.26 KB,
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
The WebIDL bindings build stuff is totally FUBARed. Let's fix that.
Assignee | ||
Comment 1•12 years ago
|
||
Should be pretty self-explanatory. Handling cycles was fun.
Attachment #654368 -
Flags: review?(justin.lebar+bug)
Comment 2•12 years ago
|
||
Comment on attachment 654368 [details] [diff] [review] Part 1: Parser bits This feels pretty convoluted to me. Can't you do a breadth-first instead of depth-first search, without this visitor pattern? Roughly: def getRelevantFilenames(object, visited=set()): if object in visited: return set() visited.add(object) deps = set([object.getContainingFilename()]) for d in object.getDependentObjects(): deps |= getDependencies(d) return deps
Comment 3•12 years ago
|
||
erm, this should be deps |= getDependencies(d, visited) of course.
Comment 4•12 years ago
|
||
You may need to do def getRelevantFilenames(object): visited = set() def visit(obj): ... Because I'm not sure that a default param = set() will work properly.
Comment 5•12 years ago
|
||
Comment on attachment 654368 [details] [diff] [review] Part 1: Parser bits r- because we agreed irl that the algorithm in comment 4 is probably cleaner.
Attachment #654368 -
Flags: review?(justin.lebar+bug) → review-
Assignee | ||
Comment 6•11 years ago
|
||
Attachment #654368 -
Attachment is obsolete: true
Attachment #705979 -
Flags: review?(justin.lebar+bug)
Comment 7•11 years ago
|
||
Comment on attachment 705979 [details] [diff] [review] Part 1: Parser bits I didn't review that the dependencies themselves were right (or that all necessary classes have getDeps() functions); let me know if you'd like me to do that. >diff --git a/dom/bindings/parser/WebIDL.py b/dom/bindings/parser/WebIDL.py >--- a/dom/bindings/parser/WebIDL.py >+++ b/dom/bindings/parser/WebIDL.py >@@ -169,16 +169,42 @@ class IDLObject(object): > self.userData[key] = value > > def addExtendedAttributes(self, attrs): > assert False # Override me! > > def handleExtendedAttribute(self, attr): > assert False # Override me! > >+ def _getDependentObjects(self): >+ assert False # Override me! >+ >+ def getDeps(self, visited=None): >+ """ Return a list of files that this object depends on. If any of >+ these files are changed the parser needs to be rerun to regenerate >+ a new IDLObject.""" s/list/set/ >@@ -855,16 +884,22 @@ class IDLInterface(IDLObjectWithScope): >+ def _getDependentObjects(self): >+ deps = set() >+ deps.add([self.parent]) Is this right? deps.add([self.parent]) of course isn't the same as deps = set([self.parent]). >+ deps.add(self.members) >+ return deps Maybe this would be cleaner as |return set([self.parent] + self.members)|, if that's what you mean. If it's not what you mean, I'm confused as to what's going on here. >@@ -925,16 +960,21 @@ class IDLDictionary(IDLObjectWithScope): >+ def _getDependentObjects(self): >+ deps = set() >+ deps.add([self.parent]) >+ deps.add(self.members) >+ return deps ibid. >@@ -2742,16 +2830,21 @@ class IDLMethod(IDLInterfaceMember, IDLS >+ def _getDependentObjects(self): >+ deps = set() >+ for overload in self.overloads: >+ deps.union(overload._getDependentObjects()) Missing a return statement?
Attachment #705979 -
Flags: review?(justin.lebar+bug)
Assignee | ||
Comment 8•11 years ago
|
||
You're right on all counts.
Attachment #705979 -
Attachment is obsolete: true
Attachment #707150 -
Flags: review?(justin.lebar+bug)
Updated•11 years ago
|
Attachment #707150 -
Flags: review?(justin.lebar+bug) → review+
Assignee | ||
Comment 9•11 years ago
|
||
Attachment #707150 -
Attachment is obsolete: true
Attachment #710094 -
Flags: review?(justin.lebar+bug)
Assignee | ||
Updated•11 years ago
|
Attachment #710094 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•11 years ago
|
Attachment #710094 -
Flags: review?(ted)
Comment 10•11 years ago
|
||
Comment on attachment 710094 [details] [diff] [review] Patch >+ def _getDependentObjects(self): >+ deps = set() >+ for overload in self._overloads: >+ deps.union(overload._getDependentObjects()) >+ return deps ITYM deps = deps.union
Attachment #710094 -
Flags: review?(justin.lebar+bug) → review+
Comment 11•11 years ago
|
||
Or you could say return set([x._getDependentObjects() for x in self._overloads])
Comment 12•11 years ago
|
||
Comment on attachment 710094 [details] [diff] [review] Patch >+++ b/dom/bindings/BindingGen.py >+def generate_binding_header(config, outputprefix, srcprefix, webidlfile): >+ f.write("\n".join([filename + ": " + os.path.join(srcprefix, x) for x in list(root.deps())])) Three questions here: 1) Why do you need the list() around root.deps()? 2) Why do you need the square brackets around the argument to join()? 3) Is this preferable to: f.write(filename + ": " + "\\\n".join(os.path.join(srcprefix, x) for x in root.deps())) ? I can see either way; the compiler-generated .pp files I see in my tree seem to look like the latter. >+def generate_binding_cpp(config, outputprefix, srcprefix, webidlfile): Same comments. > class CGDescriptor(CGThing): >+ self._deps = descriptor.interface.getDeps() Why the extra member here but not other places? >+++ b/dom/bindings/parser/WebIDL.py >+ def getDeps(self, visited=None): Please document the "visited" argument. >+ dep = d.getDeps(visited) >+ deps = deps.union(dep) You don't need the dep temporary. > class IDLInterface(IDLObjectWithScope): >+ def _getDependentObjects(self): You need to add the implemented interfaces for this interface. Otherwise if this interface is the LHS of "implements" and the RHS is empty we won't flag this interface as depending on the RHS. That said, I wonder what happens when an "implements" statement is added to some other file that has this interface as the LHS. I'm not sure how we pick that up... >@@ -1694,16 +1755,19 @@ class IDLWrapperType(IDLType): >+ def _getDependentObjects(self): >+ return set() Shouldn't this return self.inner._getDependentObjects()? > class IDLMethodOverload: >+ def _getDependentObjects(self): >+ return set() Shouldn't this depend on the return type and arguments?
Comment 13•11 years ago
|
||
> 2) Why do you need the square brackets around the argument to join()?
It's a list comprehension: [foo(x) for x in bar]
Comment 14•11 years ago
|
||
Oh wow...I had no idea, but apparently generator comprehensions are a thing.
Comment 15•11 years ago
|
||
> Oh wow...I had no idea, but apparently generator comprehensions are a thing.
Yes, exactly. ;)
Comment 16•11 years ago
|
||
(In reply to Justin Lebar [:jlebar] from comment #11) > Or you could say > > return set([x._getDependentObjects() for x in self._overloads]) So this can (apparently; I haven't checked) be > return set (x._getDependentObjects() for x in self._overloads) or, with set comprehensions (python 2.7) > return {x._getDependentObjects() for x in self._overloads} Good call, Boris.
Updated•11 years ago
|
Attachment #710094 -
Flags: review?(bzbarsky) → review-
Assignee | ||
Comment 17•11 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #12) > Comment on attachment 710094 [details] [diff] [review] > Patch > > >+++ b/dom/bindings/BindingGen.py > >+def generate_binding_header(config, outputprefix, srcprefix, webidlfile): > >+ f.write("\n".join([filename + ": " + os.path.join(srcprefix, x) for x in list(root.deps())])) > > Three questions here: > > 1) Why do you need the list() around root.deps()? I don't. > 2) Why do you need the square brackets around the argument to join()? I guess its not strictly necessary, but I don't see any reason to ditch it. > 3) Is this preferable to: > > f.write(filename + ": " + "\\\n".join(os.path.join(srcprefix, x) for x in > root.deps())) > > ? I can see either way; the compiler-generated .pp files I see in my tree > seem to look like the latter. It is interpreted by make the same way. > >+def generate_binding_cpp(config, outputprefix, srcprefix, webidlfile): > > Same comments. Fixed. > > class CGDescriptor(CGThing): > >+ self._deps = descriptor.interface.getDeps() > > Why the extra member here but not other places? Some of the other things store the WebIDL parser object. CGDescriptor doesn't (afaict). > >+++ b/dom/bindings/parser/WebIDL.py > >+ def getDeps(self, visited=None): > > Please document the "visited" argument. Done. > >+ dep = d.getDeps(visited) > >+ deps = deps.union(dep) > > You don't need the dep temporary. Fixed. > > class IDLInterface(IDLObjectWithScope): > >+ def _getDependentObjects(self): > > You need to add the implemented interfaces for this interface. Otherwise if > this interface is the LHS of "implements" and the RHS is empty we won't flag > this interface as depending on the RHS. Fixed. > That said, I wonder what happens when an "implements" statement is added to > some other file that has this interface as the LHS. I'm not sure how we > pick that up... We're going to address this in a followup. > >@@ -1694,16 +1755,19 @@ class IDLWrapperType(IDLType): > >+ def _getDependentObjects(self): > >+ return set() > > Shouldn't this return self.inner._getDependentObjects()? No, because that would result in us depending on everything the interface depends on. What we really want to do here is just depend on the fact that "X" is an interface. We talked about doing non-transitive dependencies for this, but since changing X between interface/enum/dictionary should be very rare, and changing X's concrete type will involve fiddling with Bindings.conf that causes a global rebuild, so I decided to punt. > > class IDLMethodOverload: > >+ def _getDependentObjects(self): > >+ return set() > > Shouldn't this depend on the return type and arguments? Yes. Fixed.
Attachment #711267 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•11 years ago
|
Attachment #710094 -
Attachment is obsolete: true
Attachment #710094 -
Flags: review?(ted)
Comment 18•11 years ago
|
||
Comment on attachment 711267 [details] [diff] [review] Patch > I guess its not strictly necessary, but I don't see any reason to ditch it. Well, it prevents having to create a temporary list... > No, because that would result in us depending on everything the interface depends on. OK, could you add a comment explaining the conversation we just had about this? r=me
Attachment #711267 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 19•11 years ago
|
||
Comment on attachment 711267 [details] [diff] [review] Patch Ted can you review the build system horror?
Attachment #711267 -
Flags: review?(ted)
Updated•11 years ago
|
Whiteboard: [buildfaster]
Assignee | ||
Comment 21•11 years ago
|
||
Attachment #711267 -
Attachment is obsolete: true
Attachment #711267 -
Flags: review?(ted)
Attachment #716640 -
Flags: review?(ted)
Comment 22•11 years ago
|
||
Comment on attachment 716640 [details] [diff] [review] Patch Review of attachment 716640 [details] [diff] [review]: ----------------------------------------------------------------- I'm holding my nose at the Makefile bits here. ::: dom/bindings/Makefile.in @@ +100,5 @@ > +# foo.o boo.o: FORCE > +# The script has an advantage over including the *.pp files directly > +# because it handles the case when header files are removed from the build. > +# 'make' would complain that there is no way to build missing headers. > +ALL_PP_RESULTS = $(shell cat pp.list | $(PERL) $(BUILD_TOOLS)/mddepend.pl) Do you want to just implement the trick from bug 462463 in BindingGen.py and skip this mddepend nonsense? @@ +103,5 @@ > +# 'make' would complain that there is no way to build missing headers. > +ALL_PP_RESULTS = $(shell cat pp.list | $(PERL) $(BUILD_TOOLS)/mddepend.pl) > +$(eval $(ALL_PP_RESULTS)) > + > +endif Man, this is just horrible. Can you at least file a followup on making this less horrible? Maybe we can just do that as part of a "create generic Makefile rules for generating code" effort, since this stuff has promulgated all over the tree.
Attachment #716640 -
Flags: review?(ted) → review+
Assignee | ||
Comment 23•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a71766c2c85d
Comment 24•11 years ago
|
||
And backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/f63aacdb780a and relanded in https://hg.mozilla.org/integration/mozilla-inbound/rev/cd8481cc4a32 and backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/e83b8bbf1d5e for make check failures in test_nullable_equivalency.py.
Assignee | ||
Comment 25•11 years ago
|
||
Relanded with a small test tweak. https://hg.mozilla.org/integration/mozilla-inbound/rev/10d6868530d7
Comment 26•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/10d6868530d7
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•