Closed Bug 784812 Opened 12 years ago Closed 11 years ago

Real build dependencies for the WebIDL parser

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: khuey, Assigned: khuey)

References

Details

(Whiteboard: [buildfaster])

Attachments

(1 file, 5 obsolete files)

The WebIDL bindings build stuff is totally FUBARed.  Let's fix that.
Attached patch Part 1: Parser bits (obsolete) — Splinter Review
Should be pretty self-explanatory.  Handling cycles was fun.
Attachment #654368 - Flags: review?(justin.lebar+bug)
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
erm, this should be

    deps |= getDependencies(d, visited)

of course.
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 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-
Attached patch Part 1: Parser bits (obsolete) — Splinter Review
Attachment #654368 - Attachment is obsolete: true
Attachment #705979 - Flags: review?(justin.lebar+bug)
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)
Attached patch Part 1: Parser bits (obsolete) — Splinter Review
You're right on all counts.
Attachment #705979 - Attachment is obsolete: true
Attachment #707150 - Flags: review?(justin.lebar+bug)
Attachment #707150 - Flags: review?(justin.lebar+bug) → review+
Attached patch Patch (obsolete) — Splinter Review
Attachment #707150 - Attachment is obsolete: true
Attachment #710094 - Flags: review?(justin.lebar+bug)
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+
Or you could say

  return set([x._getDependentObjects() for x in self._overloads])
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?
> 2)  Why do you need the square brackets around the argument to join()?

It's a list comprehension: [foo(x) for x in bar]
Oh wow...I had no idea, but apparently generator comprehensions are a thing.
> Oh wow...I had no idea, but apparently generator comprehensions are a thing.

Yes, exactly.  ;)
(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.
Attachment #710094 - Flags: review?(bzbarsky) → review-
Attached patch Patch (obsolete) — Splinter Review
(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)
Attachment #710094 - Attachment is obsolete: true
Attachment #710094 - Flags: review?(ted)
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+
Comment on attachment 711267 [details] [diff] [review]
Patch

Ted can you review the build system horror?
Attachment #711267 - Flags: review?(ted)
Whiteboard: [buildfaster]
Attached patch PatchSplinter Review
Attachment #711267 - Attachment is obsolete: true
Attachment #711267 - Flags: review?(ted)
Attachment #716640 - Flags: review?(ted)
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+
https://hg.mozilla.org/mozilla-central/rev/10d6868530d7
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.