Real build dependencies for the WebIDL parser

RESOLVED FIXED in mozilla22

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: khuey, Assigned: khuey)

Tracking

unspecified
mozilla22
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [buildfaster])

Attachments

(1 attachment, 5 obsolete attachments)

The WebIDL bindings build stuff is totally FUBARed.  Let's fix that.
Created attachment 654368 [details] [diff] [review]
Part 1: Parser bits

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-
Created attachment 705979 [details] [diff] [review]
Part 1: Parser bits
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)
Created attachment 707150 [details] [diff] [review]
Part 1: Parser bits

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+
Created attachment 710094 [details] [diff] [review]
Patch
Attachment #707150 - Attachment is obsolete: true
Attachment #710094 - Flags: review?(justin.lebar+bug)
Attachment #710094 - Flags: review?(bzbarsky)
Attachment #710094 - Flags: review?(ted)
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-
Created attachment 711267 [details] [diff] [review]
Patch

(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)
Duplicate of this bug: 774550

Updated

5 years ago
Whiteboard: [buildfaster]
Created attachment 716640 [details] [diff] [review]
Patch
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/integration/mozilla-inbound/rev/a71766c2c85d
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.
Relanded with a small test tweak.

https://hg.mozilla.org/integration/mozilla-inbound/rev/10d6868530d7
https://hg.mozilla.org/mozilla-central/rev/10d6868530d7
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.