979886 - moving definition from one WEBIDL_FILE to another needs clobber to compile c++

Status: NEW

(Firefox Build System :: General, defect)

Description

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: PTR (patch to reproduce) - moves dictionary from one webidl file to another

STR:
1. make -f client.mk
2. Apply attached PTR
3. make -f client.mk

Here's how it fails. Looks like the generated include dependency tree becomes outdated.

...
UnifiedBindings14.o
UnifiedBindings15.o
UnifiedBindings17.o
Unified_cpp_layout_build0.o
UnifiedBindings31.o
webapprt.o
nsBrowserApp.o
Unified_cpp_media_webspeech_recognition0.o
In file included from /Users/Jan/moz/mozilla-central/dom/camera/DOMCameraControl.cpp:14:
../../dist/include/mozilla/MediaManager.h:500:16: error: no type named 'MediaStreamConstraintsInternal' in namespace 'mozilla::dom'; did you mean 'MediaTrackConstraintsInternal'?
    const dom::MediaStreamConstraintsInternal& aConstraints,
          ~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
               MediaTrackConstraintsInternal
../../dist/include/mozilla/dom/MediaStreamTrackBinding.h:138:8: note: 'MediaTrackConstraintsInternal' declared here
struct MediaTrackConstraintsInternal : public DictionaryBase
       ^
1 error generated.

A clobber fixes it. Worried about landing bug 916012.

Comment 1

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Uhm, clobber didn't fix it. I may have reported to fast. Hold on...

Comment 2

[Boris Zbarsky [:bzbarsky]]

There are files in the tree that include MediaStreamTrackBinding.h to get the dictionary types that now need to start including MediaStreamBinding.h.

But once that's fixed, there is in fact a clobber issue here.  It arises because we in fact have no provisions for moving stuff between webidl files like this; our dependency tracking doesn't handle it.  Even touching "dom/bindings/mozwebidlcodegen/__init__.py" is not good enough here.

The right long-term plan here, probably, is to generate separate headers for each dictionary, so the header to include for a dictionary won't depend on the webidl file the dictionary is declared in...  But in the meantime, do you really need to move this dictionary across files?

Comment 3

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Apply first and build before STR

Ok, verified. I was right, there is a problem here, I just got thrown in comment 1 by the output of different runs.

Apply this patch first and build everything before any of the STRs to make the results starker (makes the STR not lie when it says it compiles with clobber, not without). Here's the right output of the failure that clobber fixes:

...
UnifiedBindings15.o
UnifiedBindings17.o
UnifiedBindings31.o
webapprt.o
nsBrowserApp.o
Unified_cpp_media_webspeech_recognition0.o
In file included from /Users/Jan/moz/mozilla-central/obj-x86_64-apple-darwin12.2.1-debug/dom/bindings/UnifiedBindings17.cpp:162:
./NavigatorBinding.cpp:1183:36: error: no member named 'FastMediaStreamConstraints' in namespace 'mozilla::dom::binding_detail'; did you mean 'MediaStreamConstraints'?
  RootedDictionary<binding_detail::FastMediaStreamConstraints > arg0(cx);
                   ~~~~~~~~~~~~~~~~^
./NavigatorBinding.cpp:1184:8: error: use of undeclared identifier 'arg0'; did you mean 'args'?
  if (!arg0.Init(cx, args[0], "Argument 1 of Navigator.mozGetUserMedia")) {
       ^~~~
       args
./NavigatorBinding.cpp:1177:117: note: 'args' declared here
mozGetUserMedia(JSContext* cx, JS::Handle<JSObject*> obj, mozilla::dom::Navigator* self, const JSJitMethodCallArgs& args)
                                                                                                                    ^
FWIW, the error happens the other way too: when I revert the patch after a clobbered build.

Comment 4

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Boris Zbarsky [:bz] (reviews will be slow; ask someone else) from comment #2)
> But once that's fixed, there is in fact a clobber issue here.

Right, we mid-aired.

> But in the meantime, do you really need to move this dictionary across files?

Uhm, yes according to you. ;-)

-    (boolean or object) audio = false; // TODO: Once Bug 767924 fixed change to
-    (boolean or object) video = false; // (boolean or MediaTrackConstraints)
+    (boolean or MediaTrackConstraints) audio = false;
+    (boolean or MediaTrackConstraints) video = false;


TypeError: Dictionary contains a union that contains a dictionary in the same WebIDL file.  This won't compile.  Move the inner dictionary to a different file.
/Users/Jan/moz/mozilla-central/dom/webidl/MediaStreamTrack.webidl line 33:16
    (boolean or MediaTrackConstraints) audio = false; // TODO: Once Bug 767924 fixed change to
                ^
/Users/Jan/moz/mozilla-central/dom/webidl/MediaStreamTrack.webidl line 52:0
dictionary MediaTrackConstraints {
^
make[5]: *** [codegen.pp] Error 255

Comment 5

[Boris Zbarsky [:bzbarsky]]

Yeah, you'll just want to CLOBBER for now on that move.  :(

Comment 6

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

OK.

Comment 7

[Gregory Szorc [:gps]]

It's not clear to me who owns fixing this.

My reading of the comments is that this is a deficiency in the core WebIDL parser and not in how mozwebidlcodegen (the part I consider straddling the build system) operates.

That being said, one (hacky) solution is to abuse the global dependencies of code generation. There is a set of well-defined files whose change will trigger code generation for every .webidl file. We could add a CLOBBER-like file to this global dependencies list that when updated will rebuild every .webidl file. Is that worth pursuing?

Comment 8

[Boris Zbarsky [:bzbarsky]]

There are a few things going on here.

One issue is that bindings do not depend on the webidl files their arguments/return values are defined in, because if we did that then transitively everything would depend on everything else.  So for example as far as the build system is concerned NavigatorBinding only have Navigator.webidl and LegacyQueryInterface.webidl as inputs, not MediaStreamTrack.webidl.  See the big comment in IDLWrapperType._getDependentObjects.  We could try somehow fixing this by doing some sort of non-deep dependency here, perhaps...  So change the return value of _getDependentObjects from a set of objects to a set of (object, boolean) pairs, where the boolean indicates whether to recursively look at the deps of that object.  That would still mean that changes to all sorts of files would cause random other files to be rebuilt when not needed, probably way too often.

Another issue is that IDLMethod._getDependentObjects is buggy and fails to assign deps.union(overload._getDependentObjects()) to anything.  This doesn't affect anything in practice because of the first issue, but we should probably still fix.

I could probably live with a CLOBBER-like file for webidl to avoid doing whole-tree clobbers.

For this bug, the most-targeted fix would be for the code generator to keep track, for each binding file, of which objects it used getDeclarationFilename on and what the return value was and tell the build system about it.  The build system would then save that information and next time verify that for all those objects the getDeclarationFilename has not changed or rebuild the relevant binding file.  Or heck, all of them if that's simpler; moving things across webidl files should be pretty rare.

And again, codegen changes that change which headers we put dictionaries in would also fix this bug, because then getDeclarationFilename() on a dictionary would become stable as long as the name of the dictionary is stable (and if the name changes, the consumer webidl would also change, unless you go out of your way to confuse the issue with typedefs).

Comment 9

[Boris Zbarsky [:bzbarsky]]

gps, how viable is the "most-targeted" fix idea?  I can probably output whatever information the build system wants there; I just need to know what format the build system would like it in.

Comment 10

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Not really a blocker of Bug 916012 since there's an obvious workaround (CLOBBER)