983185 - Adding a WebIDL may not incur codegen

Status: RESOLVED FIXED

(Firefox Build System :: General, defect, P2)

Description

[David Teller [:Yoric] - still alive but not very active]

While attempting to land bug 961665, we ended up with a set of patches that build nicely on all platforms except ASAN. At first sight, this looks like a build system bug: https://tbpl.mozilla.org/php/getParsedLog.php?id=36067100&tree=Fx-Team&full=1

Comment 1

[Mike Hommey [:glandium]]

This looks like a(nother) clobber issue with webidl.

Comment 2

[Gregory Szorc [:gps]]

STR:

1) mach clobber
2) hg up 720962c9f993
2) mach build (ASAN not required)
3) hg up 1fd26e822e73 (the child of 720962c9f993)
4) mach build

 0:18.53 UnifiedBindings17.o
 0:18.53 UnifiedBindings18.o
 0:18.55 UnifiedBindings19.o
 0:18.57 UnifiedBindings20.o
 0:18.60 UnifiedBindings21.o
 0:18.61 UnifiedBindings22.o
 0:18.63 UnifiedBindings23.o
 0:18.63 UnifiedBindings24.o
 0:18.81 UnifiedBindings25.o
 0:21.90 /Users/gps/src/firefox/obj-firefox.noindex/dom/bindings/UnifiedBindings17.cpp:194:10: fatal error: 'NativeOSFileInternalsBinding.cpp' file not found
 0:21.90 #include "NativeOSFileInternalsBinding.cpp"
 0:21.90          ^
 0:21.99 UnifiedBindings26.o
 0:22.15 UnifiedBindings27.o
 0:22.79 1 error generated.
 0:22.82 In the directory  /Users/gps/src/firefox/obj-firefox.noindex/dom/bindings
 0:22.82 The following command failed to execute properly:
 0:22.82 /usr/local/bin/ccache /usr/bin/clang++ -o UnifiedBindings17.o -c -fvisibility=hidden -DOS_POSIX=1 -DOS_MACOSX=1 -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -DSTATIC_EXPORTABLE_JS_API -DNO_NSPR_10_SUPPORT -I/Users/gps/src/firefox/dom/bindings -I. -I../../dist/include/mozilla/dom -I/Users/gps/src/firefox/content/base/src -I/Users/gps/src/firefox/content/canvas/src -I/Users/gps/src/firefox/content/html/content/src -I/Users/gps/src/firefox/content/html/document/src -I/Users/gps/src/firefox/content/media/webaudio -I/Users/gps/src/firefox/content/media/webspeech/recognition -I/Users/gps/src/firefox/content/svg/content/src -I/Users/gps/src/firefox/content/xml/content/src -I/Users/gps/src/firefox/content/xul/content/src -I/Users/gps/src/firefox/content/xul/document/src -I/Users/gps/src/firefox/dom/base -I/Users/gps/src/firefox/dom/battery -I/Users/gps/src/firefox/dom/bluetooth -I/Users/gps/src/firefox/dom/camera -I/Users/gps/src/firefox/dom/events -I/Users/gps/src/firefox/dom/file -I/Users/gps/src/firefox/dom/indexedDB -I/Users/gps/src/firefox/dom/src/geolocation -I/Users/gps/src/firefox/dom/workers -I/Users/gps/src/firefox/dom/xbl -I/Users/gps/src/firefox/dom/xslt/base -I/Users/gps/src/firefox/dom/xslt/xpath -I/Users/gps/src/firefox/js/ipc -I/Users/gps/src/firefox/js/xpconnect/src -I/Users/gps/src/firefox/js/xpconnect/wrappers -I/Users/gps/src/firefox/layout/style -I/Users/gps/src/firefox/layout/xul/tree -I/Users/gps/src/firefox/media/mtransport -I/Users/gps/src/firefox/media/webrtc/signaling/src/common/time_profiling -I/Users/gps/src/firefox/media/webrtc/signaling/src/peerconnection -I/Users/gps/src/firefox/ipc/chromium/src -I/Users/gps/src/firefox/ipc/glue -I/Users/gps/src/firefox/obj-firefox.noindex/ipc/ipdl/_ipdlheaders -I../../dist/include -I/Users/gps/src/firefox/obj-firefox.noindex/dist/include/nspr -I/Users/gps/src/firefox/obj-firefox.noindex/dist/include/nss -I/Users/gps/src/firefox/obj-firefox.noindex/dist/include -fPIC -Qunused-arguments -DMOZILLA_CLIENT -include ../../mozilla-config.h -MD -MP -MF .deps/UnifiedBindings17.o.pp -Qunused-arguments -Qunused-arguments -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Werror=int-to-pointer-cast -Wtype-limits -Wempty-body -Wsign-compare -Wno-invalid-offsetof -Wno-inline-new-delete -Wno-c++0x-extensions -Wno-extended-offsetof -Wno-unknown-warning-option -Wno-return-type-c-linkage -Wno-mismatched-tags -fcolor-diagnostics -fno-exceptions -fno-strict-aliasing -fno-rtti -fno-exceptions -fno-math-errno -std=gnu++0x -pthread -DNO_X11 -pipe -Wno-uninitialized -DDEBUG -D_DEBUG -DTRACING -g -O3 -fno-omit-frame-pointer /Users/gps/src/firefox/obj-firefox.noindex/dom/bindings/UnifiedBindings17.cpp
 0:22.82 make[5]: *** [UnifiedBindings17.o] Error 1
 0:22.82 make[5]: *** Waiting for unfinished jobs....

Digging a little deeper, the config.status updates file-lists.json:

--- /Users/gps/src/firefox/obj-firefox.noindex/dom/bindings/file-lists.json
+++ /Users/gps/src/firefox/obj-firefox.noindex/dom/bindings/file-lists.json
@@ -268,8 +268,9 @@
     "MozTimeManager",
     "MozWakeLock",
     "MutationEvent",
     "MutationObserver",
+    "NativeOSFileInternals",
     "Navigator",
     "NetDashboard",
     "NetworkOptions",
     "Node",
@@ -807,8 +808,9 @@
     "/Users/gps/src/firefox/dom/webidl/MozTimeManager.webidl",
     "/Users/gps/src/firefox/dom/webidl/MozWakeLock.webidl",
     "/Users/gps/src/firefox/dom/webidl/MutationEvent.webidl",
     "/Users/gps/src/firefox/dom/webidl/MutationObserver.webidl",
+    "/Users/gps/src/firefox/dom/webidl/NativeOSFileInternals.webidl",
     "/Users/gps/src/firefox/dom/webidl/NetDashboard.webidl",
     "/Users/gps/src/firefox/dom/webidl/NetworkOptions.webidl",
     "/Users/gps/src/firefox/dom/webidl/Node.webidl",
     "/Users/gps/src/firefox/dom/webidl/NodeFilter.webidl",

But we don't perform codegen:

 mach build -v dom/bindings
 0:00.09 /usr/bin/make -C /Users/gps/src/firefox/obj-firefox.noindex -j8 -s backend.RecursiveMakeBackend
 0:01.32 /usr/bin/make -C dom/bindings -j8
 0:01.33 /Applications/Xcode.app/Contents/Developer/usr/bin/make export
 0:01.33 if test -d ../../dist/bin/browser ; then touch ../../dist/bin/browser/.purgecaches ; fi
 0:01.33 if test -d ../../dist/bin/webapprt ; then touch ../../dist/bin/webapprt/.purgecaches ; fi
 0:01.34 /Applications/Xcode.app/Contents/Developer/usr/bin/make -C test export
 0:01.35 make[2]: Nothing to be done for `export'.
 0:01.35 /Applications/Xcode.app/Contents/Developer/usr/bin/make compile
 0:01.47 UnifiedBindings17.o

It seems that we are missing file-lists.json from the codegen make dependencies. IIRC I thought I had a good reason for not doing that. Did I miss the file add use case or refactor the patch to break that?

Comment 4

[Gregory Szorc [:gps]]

Attachment: Ensure adding a WebIDL file incurs code generation

This seems like proper behavior.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Huh.  I wonder how this ever worked for adding new WebIDL files!  It totally makes sense to me to redo codegen if the file lists change.

Will that also happen to catch the cases of files moving to/from preprocessed that we ran into recently?

Comment 6

[Gregory Szorc [:gps]]

(In reply to Boris Zbarsky [:bz] from comment #5)
> Huh.  I wonder how this ever worked for adding new WebIDL files!  It totally
> makes sense to me to redo codegen if the file lists change.

Yeah, that surprises me too. Did most WebIDL additions involve refactoring of existing .webidl files? That would trigger codegen.

> Will that also happen to catch the cases of files moving to/from
> preprocessed that we ran into recently?

That was a separate issue related to dependencies *inside* codegen. (bug 979665 for reference)

Comment 7

[Boris Zbarsky [:bzbarsky]]

> Did most WebIDL additions involve refactoring of existing .webidl files?

Quite possible, yes.

Comment 8

[Mike Hommey [:glandium]]

Comment on attachment 8392392 [details] [diff] [review]
Ensure adding a WebIDL file incurs code generation

Review of attachment 8392392 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/Makefile.in
@@ +56,2 @@
>  codegen_dependencies := \
> +  file-lists.json \

Does codegen avoid reprocessing everything when the file list only change is a file addition?

Comment 10

[Gregory Szorc [:gps]]

(In reply to Mike Hommey [:glandium] from comment #8)
> Does codegen avoid reprocessing everything when the file list only change is
> a file addition?

Sadly, no. That's what bug 940469 is for. But that's preferable to clobbers, IMO.

Comment 11

[Gregory Szorc [:gps]]

All the trees are closed. Falling back to manual autoland.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5d322ef4bb93

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5d322ef4bb93