See https://tbpl.mozilla.org/php/getParsedLog.php?id=29282321&tree=Mozilla-Inbound from checkin https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=2a6d4fa91d5b Those methods were added to dom/webidl/PeerConnectionObserver.webidl and should have been available to all platforms.

Another one?! How many times are we going to fix bugs in WebIDL dependencies :/

Yeah, enough is enough. Let's kill dom/bindings/Makefile.in.

Apparently the patch in bug 926091 tickled this as well, since philor landed a clobber on my behalf.

This is burning many many hours of compute time, since the sheriffs have to resort to mass-clobbering multiple trunk trees on a regular basis. Anyone touching the CLOBBER file (as recommended by us) will cause a clobber across all platforms, not just Windows.

Some men just want to watch the world burn. I am not one of those men.

I've been working on this a bit over the weekend. Hopefully I'll have something up for review by California Monday's EOD.

WebIDL build system integration has been rewritten from the ground up. Changes: * Generated events <stem>.cpp files are now compiled into the unified sources. Previously, only the <stem>Binding.cpp files were compiled into unified sources. * Exported .h files are now generated directly into their final location. Previously, they were generated into the local directory then installed in their final location. * The list of globalgen-generated files now lives in mozbuild and isn't duplicated in 3 places. * GlobalGen.py, BindingGen.py, and ExampleGen.py have been largely rewritten. They have been upgraded to modern Python. Unused command arguments have been removed. Methods of passing file lists have been changed.

Comment on attachment 823594 [details] [diff] [review] Rewrite WebIDL build system integration I forgot to comment with bzexport. This is getting very close. I'm still tracking down a few dependency issues around partial rebuild and will need to perform verification on Windows/pymake. I wanted to post this here in case anyone had comments before formal review. It's probably best to just look at the new code rather than try to grok a diff.

>* Exported .h files are now generated directly into their final > location. Previously, they were generated into the local directory > then installed in their final location. Presumably that's the functional change that actually fixes the bug?

(In reply to Boris Zbarsky [:bz] from comment #11) > >* Exported .h files are now generated directly into their final > > location. Previously, they were generated into the local directory > > then installed in their final location. > > Presumably that's the functional change that actually fixes the bug? That and the fact we're using install manifests to manage staging the .webidl files into the objdir.

I'm still verifying this on Windows. But I think it is close enough to commence strict scrutiny. Commit messages folows. --- WebIDL build system integration has been rewritten from the ground up. Changes: * .webidl files are now installed into the common objdir location via install manifests instead of INSTALL_TARGETS. * Generated events <stem>.cpp files are now compiled into the unified sources. Previously, only the <stem>Binding.cpp files were compiled into unified sources. * Exported .h files are now generated directly into their final location. Previously, they were generated into the local directory then installed in their final location. * The list of globalgen-generated files now lives in mozbuild and isn't duplicated in 3 places. * GlobalGen.py, BindingGen.py, and ExampleGen.py have been largely rewritten. They have been upgraded to modern Python. Unused command arguments have been removed. Methods of passing file lists have been changed. These programs now output summary and progress messages to help with debugging. * The binding generation rules for test-only bindings have been consolidated to the main makefile.

I forgot to finish writing the new docs. Will upload a new version of the patch sometime.

In the new world, what is the equivalent of "make -C $objdir/dom/bindings/test" of the old world? That is, something that does codegen and only compiles the test bindings?

(In reply to Boris Zbarsky [:bz] from comment #15) > In the new world, what is the equivalent of "make -C > $objdir/dom/bindings/test" of the old world? That is, something that does > codegen and only compiles the test bindings? I don't believe I changed this. Please apply the patch and let me know otherwise.

Comment on attachment 823677 [details] [diff] [review] Rewrite WebIDL build system integration Review of attachment 823677 [details] [diff] [review]: ----------------------------------------------------------------- r=me, though the Makefile.in changes could definitely stand a second look. Mostly just small changes/comments. A patch series would have been helpful, but perhaps the Makefile.in/moz.build changes were so intertwined that it wouldn't have helped a lot. ::: build/docs/webidl.rst @@ +60,5 @@ > +location. For static files, this involves symlinking. However, > +preprocessed and externally-generated ``.webidl`` have special actions. > + > +Producing C++ code from ``.webidl`` consists of 2 steps: global > +generation and bindings generation. ENOBINDINGSGENERATIONDOCUMENTATION @@ +69,5 @@ > +Global generation reads in the entire set of ``.webidl`` files and > +produces some well-defined output files. These include the pickled > +representation of the parsed ``.webidl`` files > +(``ParserResults.pickle``), a series of header files > +(``GeneratedAtomList.h``, ``PrototypeList.h``, ``UnionConversions``) UnionConversions.h ::: dom/bindings/BindingGen.py @@ +102,5 @@ > + # > + # #3 should never be the only file to change: if a .webidl or a support > + # file changed, the parser output should change. If this actually occurs, > + # the build system is confused. Bug 874923 tracks one issue we had with > + # this. But the specific order of invents tickling it should be mitigated "of events tickling it" ::: dom/bindings/GlobalGen.py @@ +76,2 @@ > > # Generate the prototype list. For similarity with "# Atom list." this should probably be "# Prototype list." @@ +79,2 @@ > > # Generate the common code. Likewise, "# Common code." ::: dom/bindings/Makefile.in @@ -22,5 @@ > - > -generated_header_files := $(subst .webidl,Binding.h,$(all_webidl_files)) $(exported_generated_events_headers) > -generated_cpp_files := $(subst .webidl,Binding.cpp,$(all_webidl_files)) $(linked_generated_events_cpp_files) > - > -# We want to be able to only regenerate the .cpp and .h files that really need This comment still seems valuable. binding_dependency_trackers could stand to be moved closer to its uses, too. @@ +20,5 @@ > binding_dependency_trackers := $(subst .webidl,Binding,$(all_webidl_files)) > > +ifdef GNU_CC > +CXXFLAGS += -Wno-uninitialized > +endif This is not going to be effective unless you include config.mk before it. @@ +65,5 @@ > $(srcdir)/GenerateCSS2PropertiesWebIDL.py $(webidl_base)/CSS2Properties.webidl.in > CSS2Properties.webidl > > +.PHONY: prepare-directory > +prepare-directory: > + $(call py_action,process_install_manifest,--no-remove . $(DEPTH)/_build_manifests/install/webidl) I don't know how hard it would be to make this go away, but not having to copy all these files over would be a nice win. @@ +87,4 @@ > $(GLOBAL_DEPS) \ > $(NULL) > > +ParserResults.pickle: $(globalgen_dependencies) ParserResults.pickle might deserve its own variable. @@ +145,5 @@ > + > +# End binding generation logic. > + > +# We can't have the install manifest processing as a dependency of .BindingGen > +# because it'sa phony target and .BindingGen would always be out of date. "it's a" @@ +177,5 @@ > GARBAGE += \ > webidlyacc.py \ > parser.out \ > + $(wildcard *.h) \ > + $(wildcard *.cpp) \ Why get rid of everything and not just *-example*? @@ +178,5 @@ > webidlyacc.py \ > parser.out \ > + $(wildcard *.h) \ > + $(wildcard *.cpp) \ > + $(wildcard *Binding) \ Why not $(binding_dependency_trackers)? ::: dom/bindings/test/Makefile.in @@ -46,5 @@ > - $(NULL) > - > -ifdef GNU_CC > -CXXFLAGS += -Wno-uninitialized > -endif Bonus points for not deleting this and just including config.mk before it, to make it actually work. ::: python/mozbuild/mozbuild/backend/recursivemake.py @@ +1004,5 @@ > + > + # Non-test headers are part of the public API. Account for them. > + for stem in manager.all_regular_bindinggen_stems(): > + header = 'mozilla/dom/%s.h' % stem > + self._install_manifests['dist_include'].add_optional_exists(header) You might as well pull out self._install_manifests['dist_include'] into a separate variable for this loop and the one below. @@ +1018,5 @@ > + > + mk = Makefile() > + > + mk.add_statement('all_webidl_files := %s' % ' '.join( > + sorted(manager.all_basenames()))) Maybe factor this as: def add_variable(mk, name, values): mk.add_statement('%s := %s' % (name, ' '.join(sorted(values)))) add_variable(mk, 'all_webidl_files', manager.all_basenames()) and so on for the bits below. @@ +1024,5 @@ > + sorted(manager.generated_events_basenames()))) > + mk.add_statement('test_stems := %s' % ' '.join( > + sorted(manager.all_test_stems()))) > + mk.add_statement('exported_bindingen_headers := %s' % ' '.join( > + sorted('%.h' for b in manager.all_regular_bindinggen_stems()))) Is this supposed to be |sorted('%.h' % b for b in ...)| ? @@ +1040,5 @@ > + # Remove the file before writing so bindings that go from > + # static to preprocessed don't end up writing to a symlink, > + # which would modify content in the source directory. > + '$(RM) $@', > + '$(PYTHON) $(topsrcdir)/config/Preprocessor.py $(DEFINES) ' You want a PYTHONDONTWRITEBYTECODE=1 here.

Comment on attachment 823677 [details] [diff] [review] Rewrite WebIDL build system integration Review of attachment 823677 [details] [diff] [review]: ----------------------------------------------------------------- ::: build/docs/webidl.rst @@ +11,5 @@ > + > +Overview > +======== > + > +``.webidl`` files throughout the tree define interfaces the browser Throughout dom/webidl, perhaps @@ +30,5 @@ > +WEBIDL_FILES > + Refers to regular/static ``.webidl`` files. Most WebIDL interfaces > + are defined this way. > + > +GENERATED_EVENTS_WEBIDL_FILES Spookily empty definition

So I don't forget, the patch as-is requires a CLOBBER touch for Windows due to -I ordering. With the current patch, .h files in objdir/dom/bindings are getting picked up before the regenerated .h files in $(DIST)/include/mozilla/dom. I'm optimistic I can fix this before the final patch.

I /think/ this patch is functionally correct. Just need to finish docs and address review comments. Big change since last patch is ExampleGen.py has been merged into BindingGen.py.

The ExampleGen changes are wrong. The intent of ExampleGen.py was to be able to do: make Foo-example for any interface Foo the developer wants and get output that shows what the C++ signatures and whatnot for that interface should be. This is completely broken with the attached patch: mozilla% make -C ../obj-firefox/dom/bindings/ Node-example make: *** No rule to make target `Node-example'. Stop. TestExampleGen.webidl is meant to test that the example codegen works, but it needs to work for all interfaces, not just the ones in that file. The ones in this file do involve dependency tracking, of course, which is not needed for the normal (manual) case of example generation. It would be good to split this patch up into pieces, and especially splitting apart supposed-to-be-cleanup pieces that are not meant to change functionality and functional changes, to avoid issues like that sneaking in. Also, I'm not sure I follow the changes to the .BindingGen dependencies in dom/bindings/test. After the change, it doesn't seem to depend on non-test webidl, does it? But it should, shouldn't it? Worse yet, with this patch test codegen is completely broken in general, since the .BindingGen in there no longer depends on $(bindinggen_dependencies). So the simple case of editing Codegen.py and making in the test dir to see how the change affects the output doesn't work, because changing Codegen.py no longer triggers anything to happen in the test dir. Which I guess answers your question from comment 16: the new setup is broken.

To be clear, I _really_ appreciate you working on this stuff. It definitely needed cleanup. But I'd also appreciate it if we didn't throw all the babies out with the bathwater here. ;)

One other thing. It would be really nice if we didn't spew all the files for which we're regenerating bindings by default. It makes working on codegen a huge pain, since you end up with hundreds of lines of scrollback all the time which hides the things you actually care about...

Considering the mess that this Makefile is, it would probably be clearer if there was a list of all the rules and results you expect to be there.

Are you talking about the makefile in dom/bindings, or the one in dom/bindings/test? I'm happy to put together whatever information Gregory needs here, of course. Just let me know.

I wasn't aware of the requirement to generate arbitrary -example files! It doesn't seem to be documented in the make file and I was simply trying to port what the build system did to new code. But, that wildcard %-example rule does make a lot more sense now! bz: Please document exactly what your workflow requirements are so I can provide mechanisms for them in the new world.

OK. I think the following should cover them all (and most of these are already fine in the attached patch): dom/bindings/test: 1) Unit tests (mochitests and WebIDL parser "make check" tests) continue to work as part of a normal toplevel tinderbox build/test cycle. 2) The "check-interactive" target or some equivalent continues to work for manually running the WebIDL parser unit tests and having them output human-parseable failure info. 3) Manual running of the mochitests here continues to be possible in the usual way. 4) Manual make with no explicit target in this directory will rerun the codegen as needed (e.g. if any .webidl files changed or if any of the codegen/parser .py files changed, etc; the usual conditions) and then compile the the .cpp files for the test bindings only. This needs to regenerate TestInterface-example.h and TestExampleProxyInterface-example.h as needed, because TestExampleGenBinding.cpp includes those headers and calls methods on those objects, so if TestExampleGen.webidl has changed those files need to be updated. 5) #4 is not subject to the .pp problem documented in the big comment before .DEFAULT_GOAL. ;) 6) During a toplevel build, some equivalent of #4 happens so that we actually build the test codegen .cpp files and make sure they compile. dom/bindings: 7) Toplevel build rebuilds the minimal amount of stuff correctly. 8) Manual "make export" in this directory does the minimal amount of binding regeneration and creates new *Binding.h/cpp files. This can use a different target if needed for the manual make case, of course; there just needs to be _some_ target that does this. This needs to also regenerate the test *Binding.h/cpp files. 9) The "make export" or whatever target from #8 picks up the .pp files for the binding generation itself but not the ones for all the binding .o files, because parsing/loading all those takes forever. 10) Manual make with no target in this directory does all the stuff from step 8 plus compiles the resulting binding .cpp files. 11) The test binding headers are not exported. 12) Manual "make Foo-example" for some interface Foo works. 13) Manual "make FooBinding.o" ideally works, triggering the stuff in #8 as needed. Though this is less important now that #10 is much faster than it used to be. Plus the obvious bits: the files that should be preprocessed should get preprocessed, changing global defines should rerun the preprocessing stuff, the generated webidl files (at the moment, just CSS2Properties.webidl) need to be properly regenerated if their dependencies change. I'm not quite sure what the additional requirements, if any, are for the generated events stuff; I haven't been involved in that. And again, thank you for working on this!

Having thought about this some more, make sucks and we're going to need to support multiple build backends in the near future. So, I decided to kill as much make gunk as possible and reimplement logic in Python. The result is drastically simpler make logic. The hacks are all gone. Now, you can argue some "complicated" Python replaced it. But I think the new code in mozwebidl.py isn't too bad. I still need to test some scenarios, especially on Windows. I'd also like to write some tests. But that will likely be very involved. Commit message is as follows. --- WebIDL build system integration has been rewritten from the ground up. Changes: * GlobalGen.py, BindingGen.py, and ExampleGen.py have been combined into mozwebidl.py. That importable module contains a class for managing WebIDL code generation in the context of Mozilla's build system. * mach commands for generating *-example files from interfaces and for running the WebIDL tests have been provided. Old make targets to perform these actions have been removed. * Static .webidl files are now processed directly in their original location and aren't copied to the object directory. * Generated events <stem>.cpp files are now compiled into the unified sources. Previously, only the <stem>Binding.cpp files were compiled into unified sources. * Exported .h files are now generated directly into their final location. Previously, they were generated into the local directory then installed in their final location. * The list of globalgen-generated files now lives in Python and isn't duplicated in 3 places. * The make dependencies are much simpler as a result of using a single command to perform all code generation. The auto-generated .pp file from code generation sets up all dependencies necessary to reinvoke code generation and Python takes care of dependency management.

Comment on attachment 828829 [details] [diff] [review] Rewrite WebIDL build system integration bzexport gives me an error when requesting feedback. Derp.

(In reply to Gregory Szorc [:gps] from comment #29) > bzexport gives me an error when requesting feedback. Derp. Could you file a bug blocking bug 728778 please? :-)

And apparently I lost my preamble to the commit message. It's important. Thinking about this some more, make sucks and we'll soon need to support multiple build backends (like Tup). So, I set about moving as much logic out of make and into Python as possible. The result is drastically simpler make files. All of the hacks are gone. Python code replaces them, but I think the Python isn't as complicated as you would expect. Following the theme of "don't use make for developer tasks," I moved the make targets for performing developer tasks into mach commands. I believe I captured all of bz's requirements from comment #27. I still need to test some scenarios, especially on Windows. I'd also like to write tests to ensure common refactor scenarios are properly detected by the build system. But that test writing will be very involved and I wonder if the time investment will be worth it. Followup bug?

> Old make targets to perform these actions have been removed. What does that mean in practice? Does "make check" still run the parser tests (and in particular, are they still run on tinderbox)? Which other make targets went away and what are the replacements?

make check is still there and runs as part of automation. make check-interactive -> mach webidl-test make Foo-example -> mach webidl-example Foo

Comment on attachment 828829 [details] [diff] [review] Rewrite WebIDL build system integration Review of attachment 828829 [details] [diff] [review]: ----------------------------------------------------------------- Seriously, this would be waaaay easier to look at if you split it up. Each of your bullet points could easily be a separate patch, maybe multiple patches. Especially with the scenarios bz provided, it's hard for me--and probably even hard for bz, who works with the scenarios all day--to tell whether this still works with those scenarios.

So mach webidl-test just runs the parser unit tests, right? Or does it also run the codegen compilation tests? If it doesn't run the codegen compilation tests, are those still run by a make export or make (no target) in dom/bindings/test or via some other means?

What are the codegen compilation tests? I only see references to dom/bindings/parser/runtests.py (which I assume are the parser unit tests) in existing central.

That's the test webidl in dom/bindings/test, which is codegenned and compiled, but not run or linked.

We still codegen and compile TestExampleGenBinding, if that's what you are asking.

I'm talking about TestCodeGenBinding/TestExampleGenBinding/TestJSImplGenBinding. As long as we codegen and compile those three during a normal --enable-tests build, life is good. ;)

(I rebased this over my LOCAL_INCLUDES changes and the preprocessor changes, and split a few smaller changes off into their own patches.)

(I forgot a hunk.)

I'm attempting to write some unit tests. Is there a minimal subset of static .webidl files in the tree I can feed into a parser so I can test things? I'm currently chasing a dependency tail and that seems fragile. Perhaps I should just create some static .webidl files in the test directory? Not sure how much pain will be involved with all the possibilities...

Hmmm. I just created enough empty interfaces for things that are hardcoded in the Python (e.g. DummyInterface) and I've got a parser working. Let's see what happens at review time...

I've split the patch into 5 parts. The first 2 are the parts Ms2ger created. Part 3 (this part) contains new Python code to do codegen foo. Since the last upload, I've started to work on unit tests.

> Is there a minimal subset of static .webidl files in the tree I can feed into a parser so > I can test things? Yes. EventTarget + EventListener + Event should compile on its own for the moment. If you then need to add some stuff, you can add XMLHttpRequestEventTarget + EventHandler, I think.

This patch adds a new Python API for managing WebIDL files in the context of the build system. It effectively replaces the *Gen.py files. It will form the base that replaces the complex make rules with something much, much simpler. You'll notice that I moved dependency calculation from make to Python. This should invite concerns about reinventing wheels. However, WebIDLs are special. And, if we write this logic in Python once, all future build backends should be able to reuse it. *That* is the big win from doing it this way. I jumped through some extra hoops to make the new Python code importable as a module and made the class generic so it can be instantiated multiple times. This follows my best practice of "no unpackaged new Python in the tree." More importantly, it facilitates testing. You'll see we now have unit test coverage for binding generation. It doesn't cover every scenario. But it's better than nothing. If we find future clobber scenarios, we can write tests for them. I'm optimistic this patch in isolation won't be too contentious as you can view this patch as a large refactor (consolidation) + moving dependency logic into Python.

Trivial patch.

Comment on attachment 831187 [details] [diff] [review] Part 3: Consolidate all WebIDL Python logic into mozwebidl module Time to file some bugs against bzexport not working well when you don't use mq.

(In reply to Gregory Szorc [:gps] from comment #51) > Comment on attachment 831187 [details] [diff] [review] > Part 3: Consolidate all WebIDL Python logic into mozwebidl module I will review this tomorrow, but I just wanted to say that this looks fabulously self-contained. :) Thank you!

I made some minor changes as I was tidying up the build system patch.

Comment on attachment 831189 [details] [diff] [review] Part 4: mach command for generating WebIDL example files Ugh, bzexport.

Someone can review this if they want.

Normalizing the patch series.

Normalizing patch series.

WebIDL build system integration has been rewritten from the ground up. Changes: * GlobalGen.py, BindingGen.py, and ExampleGen.py have been removed in favor of mozwebidl.py. * Static .webidl files are now processed directly in their original location and aren't copied to the object directory. * Generated events <stem>.cpp files are now compiled into the unified sources. Previously, only the <stem>Binding.cpp files were compiled into unified sources. * Exported .h files are now generated directly into their final location. Previously, they were generated into the local directory then installed in their final location. * The list of globalgen-generated files now lives in Python and isn't duplicated in 3 places. * The make dependencies are much simpler as a result of using a single command to perform all code generation. The auto-generated .pp file from code generation sets up all dependencies necessary to reinvoke code generation and Python takes care of dependency management.

Comment on attachment 832269 [details] [diff] [review] Part 3: Consolidate all WebIDL Python logic into mozwebidl module Review of attachment 832269 [details] [diff] [review]: ----------------------------------------------------------------- I think this all looks good, modulo some refactoring comments. Feedback welcome on whether the suggested refactorings are valuable or not. ::: dom/bindings/Makefile.in @@ +89,5 @@ > globalgen_headers_DEST = $(ABS_DIST)/include/mozilla/dom > globalgen_headers_TARGET := export > INSTALL_TARGETS += globalgen_headers > > +PYTHON_UNIT_TESTS := $(wildcard $(srcdir)/mozwebidl/test/test_*.py) Weren't we trying to get rid of $(wildcard) at one point? ::: dom/bindings/mozwebidl/__init__.py @@ +78,5 @@ > + config_path refers to a WebIDL config file (e.g. Bindings.conf). > + inputs is a 3-tuple describing the input .webidl files and how to > + process them. Members are: > + (set(.webidl files), set(basenames of exported files), > + set(basenames of generated events files)) This seems like sort of a weird convention. I guess you did this to avoid even more arguments to this function? @@ +130,5 @@ > + webidls={}, > + global_depends={}, > + ) > + > + self._state = state I think the computation of self._state is complicated enough to warrant its own method. Maybe even an actual class for state with a bit of documentation. @@ +166,5 @@ > + result = BuildResult() > + > + # If we parse, we always update globals - they are cheap and it is > + # easier that way. > + created, updated, unchanged = self._write_global_derived() AFAICS, you never use |unchanged| here, so you could probably just ditch computing it in _write_global_derived. @@ +171,5 @@ > + result.created |= created > + result.updated |= updated > + > + # Now comes the fun part where we compute the work we need to perform. > + changed_inputs = set() I think computing changed_inputs warrants its own method. I'm not quite sure where to break off, but about everything from here to just after inheriting dependencies from previous runs could legitimately be a separate function. Maybe the cutoff point is "# We've now populated the base set of inputs that have changed." @@ +185,5 @@ > + # a lot of extra work and most build systems don't do that anyway. > + > + # Now we move on to the input files. > + old_hashes = {v['filename']: v['sha1'] > + for v in self._state['webidls'].values()} TIL about hash comprehensions. Nifty. @@ +249,5 @@ > + > + # Generate a make dependency file. > + if self._make_deps_path: > + mk = Makefile() > + codegen_rule = mk.create_rule(['codegen.pp']) I assume codegen.pp gets used in nifty ways in the new Makefile/buildfile. @@ +267,5 @@ > + root = CGExampleRoot(self.config, interface) > + > + out_base = os.path.join(self._codegen_dir, '%s-example' % interface) > + > + result = (set(), set(), set()) This stuff should be refactored to something like: def _write_codegen_object(self, cgobj, stem, header_dir, cpp_dir, result=None): if result is None: result = (set(), set(), set()) self._maybe_write_file(os.path.join(header_dir, '%s.h' % stem), root.declare(), result) self._maybe_write_file(os.path.join(cpp_dir, '%s.cpp' % stem), root.define(), result) return result and called here and below in _generate_build_files_for_webidl. @@ +338,5 @@ > + basename = os.path.basename(p) > + stem = os.path.splitext(basename)[0] > + binding_stem = '%sBinding' % stem > + > + # Header file may or may not be exported. This header file logic could maybe be its own function and used in a couple different places, including the example file codegen. @@ +345,5 @@ > + else: > + header_dir = self._codegen_dir > + > + paths.add(os.path.join(header_dir, '%s.h' % binding_stem)) > + paths.add(os.path.join(self._codegen_dir, '%s.cpp' % binding_stem)) Bonus points for factoring out the path stuff here and using it here, below, and in _write_codegen_object. @@ +412,5 @@ > + return True, current_hashes > + > + previous_files = {f for f in self._state['global_depends'].keys() > + if os.path.exists(f)} > + if current_files ^ previous_files: The new previous_files can only be >= the size of the old previous_files. Therefore, if there's something that gets returned here and not by the previous check, then it must have been a file in current_files. Said file must therefore have been present in the old previous_files, which implies that it *doesn't* exist on disk. But that means something is very wrong: current_files should all exist on disk, as the comment several lines up indicates. So I think this check is unnecessary. I'm pretty sure that's right...welcome a double-check on that. @@ +450,5 @@ > + """Create a WebIDLManager for use by the build system.""" > + src_dir = os.path.join(topsrcdir, 'dom', 'bindings') > + obj_dir = os.path.join(topobjdir, 'dom', 'bindings') > + > + with open(os.path.join(obj_dir, 'file-lists.json'), 'rb') as fh: Who's responsible for creating file-lists.json? moz.build, I think, IIRC?

I was looking at this to see if it would cause any problems with tup, and I noticed this in the make build log: Makefile:69: codegen.pp: No such file or directory I guess you want -include codegen.pp in dom/bindings/Makefile.in ?

(In reply to Nathan Froyd (:froydnj) from comment #59) > ::: dom/bindings/Makefile.in > @@ +89,5 @@ > > globalgen_headers_DEST = $(ABS_DIST)/include/mozilla/dom > > globalgen_headers_TARGET := export > > INSTALL_TARGETS += globalgen_headers > > > > +PYTHON_UNIT_TESTS := $(wildcard $(srcdir)/mozwebidl/test/test_*.py) > > Weren't we trying to get rid of $(wildcard) at one point? Yes. I'm being lazy. > ::: dom/bindings/mozwebidl/__init__.py > @@ +78,5 @@ > > + config_path refers to a WebIDL config file (e.g. Bindings.conf). > > + inputs is a 3-tuple describing the input .webidl files and how to > > + process them. Members are: > > + (set(.webidl files), set(basenames of exported files), > > + set(basenames of generated events files)) > > This seems like sort of a weird convention. I guess you did this to avoid > even more arguments to this function? Yes. > @@ +130,5 @@ > > + webidls={}, > > + global_depends={}, > > + ) > > + > > + self._state = state > > I think the computation of self._state is complicated enough to warrant its > own method. Maybe even an actual class for state with a bit of > documentation. This is reasonable. > > @@ +171,5 @@ > > + result.created |= created > > + result.updated |= updated > > + > > + # Now comes the fun part where we compute the work we need to perform. > > + changed_inputs = set() > > I think computing changed_inputs warrants its own method. I'm not quite > sure where to break off, but about everything from here to just after > inheriting dependencies from previous runs could legitimately be a separate > function. Maybe the cutoff point is "# We've now populated the base set of > inputs that have changed." Yeah, this was on my mind too. Glad to see we both have the same alarm bell in our heads go off when functions get too long. Having already refactored this, the new code is much easier to read! > @@ +249,5 @@ > > + > > + # Generate a make dependency file. > > + if self._make_deps_path: > > + mk = Makefile() > > + codegen_rule = mk.create_rule(['codegen.pp']) > > I assume codegen.pp gets used in nifty ways in the new Makefile/buildfile. Yes. And this should be an argument. I'll change it. > @@ +412,5 @@ > > + return True, current_hashes > > + > > + previous_files = {f for f in self._state['global_depends'].keys() > > + if os.path.exists(f)} > > + if current_files ^ previous_files: > > The new previous_files can only be >= the size of the old previous_files. > Therefore, if there's something that gets returned here and not by the > previous check, then it must have been a file in current_files. Said file > must therefore have been present in the old previous_files, which implies > that it *doesn't* exist on disk. But that means something is very wrong: > current_files should all exist on disk, as the comment several lines up > indicates. So I think this check is unnecessary. I agree. > @@ +450,5 @@ > > + """Create a WebIDLManager for use by the build system.""" > > + src_dir = os.path.join(topsrcdir, 'dom', 'bindings') > > + obj_dir = os.path.join(topobjdir, 'dom', 'bindings') > > + > > + with open(os.path.join(obj_dir, 'file-lists.json'), 'rb') as fh: > > Who's responsible for creating file-lists.json? moz.build, I think, IIRC? Yes. Part 6.

(In reply to Michael Shal [:mshal] from comment #60) > I was looking at this to see if it would cause any problems with tup, and I > noticed this in the make build log: > > Makefile:69: codegen.pp: No such file or directory > > I guess you want -include codegen.pp in dom/bindings/Makefile.in ? When make encounters an "include" or "-include," it looks for a rule to build the argument to the include. If it's there, it executes it immediately (I think). I'm torn between using include or -include. On one hand, the file isn't optional because it's part of the dependency graph, so we shouldn't need the -. On the other, that (harmless) "No such file or directory" message is annoying. I suppose I could introduce another intermediate target to represent whether codegen has been performed.

Comment on attachment 832352 [details] [diff] [review] Part 1: Remove trailing whitespace from Codegen.py r=me

Comment on attachment 832354 [details] [diff] [review] Part 2: Provide a mach command to run WebIDL parser tests to replace a make target So this is not quite equivalent to the check-interactive target, because it also does codegen on all the binding stuff, correct? Please drop that part, and maybe rename the command to "webidl-parser-test" or something, and r=me

Incorporated review feedback. Mostly a lot of minor refactoring.

Comment on attachment 832269 [details] [diff] [review] Part 3: Consolidate all WebIDL Python logic into mozwebidl module Thank you very much for breaking this up! Makes review much more sane. >+++ b/dom/bindings/mozwebidl/__init__.py Not sure about the directory naming, but I'm not sure I have a better proposal... webidlbuild? webidlgen? >+class BuildResult(object): I wish this had a bit more documentation about what its members actually mean. It's not clear to me whether the "result" in this case means the .cpps or the .os or something else. >+class WebIDLManager(LoggingMixin): Is this more of a WebIDLBuildManager? >+ (set(.webidl files), set(basenames of exported files), So exported_stems is not actually set by anything in this patch (in that nothing actually generates the json file this code ends up reading), right? Or did I miss it? It's fine if it's set in later patches, but I want to make sure I'm not missing something. >+ for k, v in state['webidls'].items(): Can you document why this is needed? That is are state['webidls'][k]['inputs'] not already sets (e.g. they're lists) or is there something else going on here? >+ def generate_build_files(self): >+ """Generate files required for the build. So what does this generate in practice? The .h/.cpp files? The post-preprocessing webidl for preprocessed cases? The generated webidl files? All of the above? It would be good to make that a little clearer. In particular, I'm not clear about the difference between "build" and "when this function runs", though clearly there is some difference. Documenting _that_ would be good too. >+ Because reading and regenerating every .webidl on every invocation >+ is expensive, we only regenerate the minimal set of files on every Missing end of sentence? In any case, we pretty much always _read_ all webidl files, and very few webidl files are generated in any sense. What we skip is regenerating the *Binding.cpp/.h files. >Only regenerate output files impacted >+ by the modified .webidl. By "output" this means "*Binding.{h,cpp}" files, right? Might be worth just saying that. >+ # Should we only regenerate impacted files? I think so, yes, if it's not too complicated. Consider the case of someone adding a new .webidl file. If I read this right, this will go ahead and treat all .webidl files as changed if that happens, because the Binding.h/Binding.cpp for that binding are missing, right? That seems a bit suboptimal. A followup to fix this is probably the way to go here; I'm fine with the initial landing keeping this as-is. >+ # If any of the extra dependencies changed, regenerate the world. Is it worth doing all the dependency-checking work in an else for this guy? Or is that usually fast enough to not matter whether we skip it? >+ codegen_rule = mk.create_rule(['codegen.pp']) I'm not sure I follow this bit. This is listing all the input paths and all the global_hashes files as dependencies for the codegen.pp file, right? Why do we need that, exactly? It's worth documenting that here. >+ def _global_dependencies_changed(self): It's not obvious to me how this function gets the right set of global dependencies. It's also not obvious to me what exactly it means by "global dependencies"... Clearly the .conf file is included, but what else is? How does this global dependency set include WebIDL.py, for example? What about Codegen.py and Configuration.py? Those aren't under dom/bindings/mozwebidl, right? >+ # The set of files has changed. We have a few repetitions of this logic; I wonder whether it's worth factoring out. >+++ b/dom/bindings/mozwebidl/test/test_mozwebidl.py What actually calls this, if anything so far? Or is that coming later? r=me with the above issues addressed, and sorry for any questions explained by my poor python knowledge.

Argh. There was an updated version of that but both had review requests? :(

Comment on attachment 832534 [details] [diff] [review] Part 3: Consolidate all WebIDL Python logic into mozwebidl module Actually, looks like most of my comments still apply, if Bugzilla's interdiff is not lying to me. Additional ones: >+ def _compute_changed_inputs(self): >+ """Compute the set of input files that need regenerated.""" "need to be regenerated" In this new setup, what exactly happens when a .webidl file is removed? r=me modulo those.

We'll need to document the changed method of generating example files in https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Adding_WebIDL_bindings_to_a_class step 7.

(In reply to Boris Zbarsky [:bz] from comment #66) > Not sure about the directory naming, but I'm not sure I have a better > proposal... webidlbuild? webidlgen? There are only two hard things in computer science... > >+ # If any of the extra dependencies changed, regenerate the world. > > Is it worth doing all the dependency-checking work in an else for this guy? > Or is that usually fast enough to not matter whether we skip it? Performance wise, it likely doesn't matter. But I think the if..else makes the invalidation logic easier to follow. > >+ def _global_dependencies_changed(self): > > It's not obvious to me how this function gets the right set of global > dependencies. It's also not obvious to me what exactly it means by "global > dependencies"... Clearly the .conf file is included, but what else is? How > does this global dependency set include WebIDL.py, for example? What about > Codegen.py and Configuration.py? Those aren't under dom/bindings/mozwebidl, > right? Oh, this is a bug. I didn't update __file__ after moving this file under mozwebidl/. > What actually calls this, if anything so far? Or is that coming later? Actual users are in a later patch.

Rebased on top of reworked bug 937803 (very minimal test-only change). Incorporated review comments. I could land this with just bz review. But I'd still like Nathan's blessing because this code has been the source of many clobbers and I want to get it right.

Comment on attachment 8334069 [details] [diff] [review] Part 3: Consolidate all WebIDL Python logic into mozwebidl module; Review of attachment 8334069 [details] [diff] [review]: ----------------------------------------------------------------- Just a couple of changes here. r=me with those addressed. ::: dom/bindings/Makefile.in @@ +89,5 @@ > globalgen_headers_DEST = $(ABS_DIST)/include/mozilla/dom > globalgen_headers_TARGET := export > INSTALL_TARGETS += globalgen_headers > > +PYTHON_UNIT_TESTS += $(wildcard $(srcdir)/mozwebidl/test/test_*.py) Please stop being lazy and drop the $(wildcard). :) ::: dom/bindings/mozwebidl/__init__.py @@ +185,5 @@ > + self._make_deps_target = make_deps_target > + > + if (make_deps_path and not make_deps_target) or (not make_deps_path and > + make_deps_target): > + raise Exception('Must define both make_deps_path and make_deps_target.') The error message here doesn't match the condition. ::: dom/bindings/mozwebidl/test/test_mozwebidl.py @@ +85,5 @@ > + > + with open(p, 'wb') as fh: > + json.dump({'version': 520000, 'foobar': '1'}, fh) > + > + manager = WebIDLManager(**args) You may want to export WebIDLManager.STATE_VERSION and test both a too new (STATE_VERSION+1) and too old (STATE_VERSION-1) version here. Not going to push hard on this though, your call.

Comment on attachment 8334069 [details] [diff] [review] Part 3: Consolidate all WebIDL Python logic into mozwebidl module; >+class WebIDLManager(LoggingMixin): Still think WebIDLBuildManager or WebIDLCodegenManager would be clearer.... This is not managing WebIDL, but code generation from it. > # FUTURE Only regenerate minimum set. File a followup bug depending on this one and put that bug# in the comment? It shouldn't be too far in the future, ideally. > We need to catch other .py files from /dom/bindings That makes the world depend on GenerateCSS2PropertiesWebIDL.py. I'd rather we didn't do that. Comments that are still unaddressed: 1) Directory name. I vote for "webidlbuild". The current name makes it unclear how the dir differs from dom/webidl, for example. 2) Documentation about what codegen.pp is for. 3) The comments from comment 68.

(In reply to Boris Zbarsky [:bz] from comment #73) > > We need to catch other .py files from /dom/bindings > > That makes the world depend on GenerateCSS2PropertiesWebIDL.py. I'd rather > we didn't do that. It only returns /loaded/ modules under the specified path. Also, everything through part 5 should be safe to land incrementally. I'm very tempted to land those as soon as I finish addressing nits and inbound reopens.

> It only returns /loaded/ modules under the specified path. Ah, excellent. :) Agreed on landing stuff through part 5 (though the docs for "webidl-test" in part 5 might need changing based on the review comments above about not doing codegen just to run the parser unit tests).

(In reply to Boris Zbarsky [:bz] from comment #73) > 1) Directory name. I vote for "webidlbuild". The current name makes it > unclear how the dir differs from dom/webidl, for example. So, I want to get the tree to a state where all Python (well, at least as most as possible) is inside the virtualenv. This means all importable .py files are inside packages. This means those existing .py files currently in dom/bindings need to get moved somewhere. Bug 936086 tracks. I chose the name "mozwebidl" over something with "build" in it because I thought that name would be more appropriate in the future. If you are fine moving everything under "webidlbuild," it's your directory.

Hmm. So in the new world, can the files just live in dom/bindings and have that be the package? If not, what files would be in the package dir (or dirs? will they all be in one dir?), exactly once we've reached the desired end state?

Parts 1 and 2: https://hg.mozilla.org/integration/mozilla-inbound/rev/d51357993d25 https://hg.mozilla.org/integration/mozilla-inbound/rev/c386d6f16bd5 I accidentally pushed the other changesets and subsequently backed them out in https://hg.mozilla.org/integration/mozilla-inbound/rev/644408afbf21. Derp.

We /could/ make dom/bindings the package. However, it's a best practice for Python package directories to only contain Pythonish things (.py files, setup.py, maybe a readme, test files, etc). See any directory under python/. Long term, we'll likely copy Python package directories to the object directory (or perform the equivalent Python packaging) as part of installing them into the virtualenv. This is all necessary to achieve optimal building (with Python bytecaching enabled) on read-only filesystems. The first step is consolidating all the Python into packages.

OK. What about the second part of comment 77? Will this new dir contain the new stuff you're adding plus the current functionality of Codegen.py/Configuration.py, basically?

I imagine the structure will look like: mozwebidl/ __init__.py (always present - denotes a Python module directory) codegen.py configuration.py parser/ __init__.py webidl.py test/ test_foo.py Technically speaking, you should have another intermediate directory in there. e.g. mozwebidl setup.py README etc mozwebidl/ __init__.py ... This is what we've done in python/mozbuild. We can also move files later or hack around it in build system land.

So what I think would make the most sense is two packages, actually: "codegen" (containing your new stuff and codegen.py/configuration.py) and "parser" (containing the standalone webidl parser). Or are those package names too generic in the sense that they need to not collide with other package names elsewhere?

"codegen" is definitely too generic considering it will sit alongside the standard library and other build system foo. We typically prefix packages with "moz" to avoid this problem. "mozbuild" is everything related to the build system, for example.

Alright. In that case, mozwebidlcodegen and mozwebidlparser, I guess. :(

https://hg.mozilla.org/mozilla-central/rev/d51357993d25 https://hg.mozilla.org/mozilla-central/rev/c386d6f16bd5 https://hg.mozilla.org/mozilla-central/rev/afc54efcf0fd https://hg.mozilla.org/mozilla-central/rev/b6f5dfa97f98

Comment on attachment 832355 [details] [diff] [review] Part 6: Rewrite WebIDL build system integration Review of attachment 832355 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bindings/Makefile.in @@ +8,5 @@ > # Generated by moz.build > include webidlsrcs.mk > > +ifdef GNU_CC > +CXXFLAGS += -Wno-uninitialized Please include config.mk before this, so that this statement actually does something, instead of getting overwritten by the later (implicit) inclusion of config.mk in rules.mk. @@ +69,4 @@ > > GARBAGE += \ > + codegen.pp \ > + codegen.json \ file-lists.json should probably live here too. ::: dom/bindings/test/Makefile.in @@ -40,5 @@ > - $(NULL) > - > -ifdef GNU_CC > -CXXFLAGS += -Wno-uninitialized > -endif You lost this hunk. Same comment from dom/bindings/Makefile.in applies here. ::: python/mozbuild/mozbuild/backend/recursivemake.py @@ +546,5 @@ > poison_windows_h=False): > files_per_unified_file = 16 > > + # In case it's a generator. > + files = sorted(files) Thank you for clarifying that. @@ +1054,5 @@ > + 'include') > + for f in manager.expected_build_output_files(): > + if f.startswith(include_dir): > + self._install_manifests['dist_include'].add_optional_exists( > + f[len(include_dir)+1:]) I think this is just os.path.relpath(f, include_dir), isn't it? @@ +1086,5 @@ > + self._add_unified_build_rules(mk, > + webidls.all_regular_cpp_basenames(), > + bindings_dir, > + unified_prefix='UnifiedBindings', > + unified_files_makefile_variable='unified_binding_cpp_files') You lost the bit about poisoning windows.h here. Please put it back.

glandium, can you please prioritize this review? Thanks!

Comment on attachment 832355 [details] [diff] [review] Part 6: Rewrite WebIDL build system integration Review of attachment 832355 [details] [diff] [review]: ----------------------------------------------------------------- This is a first pass. The LOCAL_INCLUDES comment is important enough to leave this message before going further with the review. ::: CLOBBER @@ +18,5 @@ > # Modifying this file will now automatically clobber the buildbot machines \o/ > # > > +Bug 928195 rewrote WebIDL building from the ground up, hopefully eliminating > +needs for future clobbers ... because of it :) ::: dom/bindings/Makefile.in @@ +8,5 @@ > # Generated by moz.build > include webidlsrcs.mk > > +ifdef GNU_CC > +CXXFLAGS += -Wno-uninitialized Use OS_CXXFLAGS or do as Nathan said. @@ +25,5 @@ > +# is touched. > +# > +# Ideally, binding generation uses the prefixed header file names. > +# Bug 932092 tracks. > +LOCAL_INCLUDES += -I$(DIST)/include/mozilla/dom LOCAL_INCLUDES are now after '.', so the whole comment blows up. Why not just make the generated bindings include mozilla/dom/*Binding.h?

(In reply to Mike Hommey [:glandium] from comment #88) > LOCAL_INCLUDES are now after '.', so the whole comment blows up. Why not > just make the generated bindings include mozilla/dom/*Binding.h? Bug 932082. I think this is pretty straightforward, but there's some trickery around test files: http://mxr.mozilla.org/mozilla-central/source/dom/bindings/Configuration.py#242

(In reply to Nathan Froyd (:froydnj) from comment #89) > (In reply to Mike Hommey [:glandium] from comment #88) > > LOCAL_INCLUDES are now after '.', so the whole comment blows up. Why not > > just make the generated bindings include mozilla/dom/*Binding.h? > > Bug 932082. I think this is pretty straightforward, but there's some > trickery around test files: > > http://mxr.mozilla.org/mozilla-central/source/dom/bindings/Configuration. > py#242 ...and http://mxr.mozilla.org/mozilla-central/source/dom/bindings/Codegen.py#791 ...and probably other things I don't yet understand about the bindings code.

So this really needs to land to reduce this clobber hell that's going on right now. I'm getting on a 15 hour flight to Hong Kong tomorrow and will probably be caught up in jet lag for a few days. If someone wants to pick this up and carry it over the finish line in the next 72 hours, you won't be stepping on my toes. If not, I should be able to finish it early next week.

Comment on attachment 832355 [details] [diff] [review] Part 6: Rewrite WebIDL build system integration In addtion to Nathan's comment 86 (which covered all the substantive things I found), I have two general questions: 1) it's not clear to me what makes sure bindings are regenerated if someone edits TestCodeGen.webidl or Codegen.py and then builds or exports in dom/bindings/test (the idea being to only regenerate bindings and maybe build the test ones without rebuilding all the other stuff). What makes that work in the new world, or what's the new way of achieving that effect? 2) What avoids the long pause to load .pp files for all the .cpp bits when doing just a "make export" to trigger codegen in dom/bindings? I checked, and even with unified builds all that .pp gunk still takes 1.5s or so... Apart from not quite understanding how those two things work, this looks pretty awesome! r=me Some nits, but this isn't my module, so feel free to ignore as needed: >+++ b/python/mozbuild/mozbuild/backend/common.py >+ def all_regular_basenames(self): >+ return [os.path.basename(source) for source in self.all_regular_sources()] Here and similar, is there a reason to return a list instead of a generator? I'm not sure whether we want all the copypasted os.path.splitext(b)[0] or whether it's worth mapping a getstem function on things instead. Similar for os.path.basename(s).

I /think/ I addressed all the review points. But my brain reset as I crossed the Pacific Ocean, so I'm asking for a final r+ before landing.

Comment on attachment 8339820 [details] [diff] [review] Part 3: Consolidate all WebIDL Python logic into mozwebidl module No, I didn't apply all the review comments. FWIW, ReviewBoard has a checkbox next to each item of feedback so you can track whether it has been incorporated. I can't wait to start using it.

OK, now I think I addressed all review comments.

Incorporated review feedback. This is part 6, updated to part 4 because other parts referenced code in it, so it needs to logically come first. There is one outstanding review feedback: perf of including .pp files during export. That is one optimization I dropped as part of the refactor. For simplicitly reasons, I'd like to keep it killed. But if it's really a burden on developers, we can add it back in as a follow-up.

This already has r+. It just requires part 4 to work.

Reordering as part 6. I haven't audited the docs since a few refactors ago. Probably need to do that. https://tbpl.mozilla.org/?tree=Try&rev=8875e1974411 contains everything up through part 4.

(In reply to Boris Zbarsky [:bz] from comment #92) > Comment on attachment 832355 [details] [diff] [review] > Part 6: Rewrite WebIDL build system integration > > In addtion to Nathan's comment 86 (which covered all the substantive things > I found), I have two general questions: > > 1) it's not clear to me what makes sure bindings are regenerated if someone > edits TestCodeGen.webidl or Codegen.py and then builds or exports in > dom/bindings/test (the idea being to only regenerate bindings and maybe > build the test ones without rebuilding all the other stuff). What makes > that work in the new world, or what's the new way of achieving that effect? The python dependencies are handled by the iter_modules_in_path call in WebIDLCodegenManager._global_dependencies_changed. Not sure about the building in the test directory part. Will have to look closer when I review these next week. > 2) What avoids the long pause to load .pp files for all the .cpp bits when > doing just a "make export" to trigger codegen in dom/bindings? I checked, > and even with unified builds all that .pp gunk still takes 1.5s or so... Comment 96 suggests the current patch series punts on that.

> But if it's really a burden on developers, we can add it back in as a > follow-up. OK, I can live with that. It adds 1-1.5s (which is better than the 3s it used to be) to what looks like a 2-3s operation normally at this point, so it's annoying but not fatal.

Comment on attachment 8339830 [details] [diff] [review] Part 3: Consolidate all WebIDL Python logic into mozwebidl module Review of attachment 8339830 [details] [diff] [review]: ----------------------------------------------------------------- r=me with some final small fixes. ::: dom/bindings/mozwebidlcodegen/__init__.py @@ +214,5 @@ > + """Generate files required for the build. > + > + This function is in charge of generating all the .h/.cpp files derived > + from input .webidl files. Please note that there are build actions > + required to produce .webidl files and these build actions are Nit: "produce some .webidl files" is a little more accurate. (The current way reads to me as though there are build actions for all .webidl files.) @@ +398,5 @@ > + """Compute binding metadata for an input path. > + > + Returns a tuple of: > + > + (stem, binding_stem, is_event, output_files) This documentation does not match the actual return values; we are now returning header_dir between is_event and output_files. ...or were we once returning header_dir because it was useful in some places and no longer is (it is unused at all the callsites)? Either way, fix, please.

> what makes sure bindings are regenerated if someone edits TestCodeGen.webidl or > Codegen.py and then builds or exports in dom/bindings/test The answer based on applying the patches and testing seems to be: absolutely nothing. If I change either one and then "make -C ../obj-firefox/dom/bindings/test/" or "env MOZCONFIG=`pwd`/.mozconfig-fox ./mach build dom/bindings/test", there is no regeneration or rebuilding of anything. As in, all the test stuff is totally broken.

Comment on attachment 8339847 [details] [diff] [review] Part 4: Rewrite WebIDL build system integration Review of attachment 8339847 [details] [diff] [review]: ----------------------------------------------------------------- Given comment 102, I'm going to cancel this review. A few comments below. Please make sure to do a PGO try run to ensure that GARBAGE is correctly set. I am ignorant of the precise steps we take during a PGO build and it's not obvious to me that all those files would get regenerated. (I realize that I was the one who told you to put file-lists.json in GARBAGE...) ::: dom/bindings/Makefile.in @@ +71,2 @@ > parser.out \ > + WebIDLGrammar.pkl \ A comment here on where this comes from would be helpful for the casual reader. ::: dom/bindings/mozwebidlcodegen/__init__.py @@ +522,5 @@ > result[2].add(path) > + > + > +def create_build_system_manager(topsrcdir, topobjdir, dist_dir): > + """Create a WebIDLManager for use by the build system.""" This should be WebIDLCodegenManager.

gps, would you mind posting an interdiff from the old part 6 (what I reviewed in comment 92) to the new part 4? Bugzilla interdiff fails as usual. :(

Comment on attachment 8339847 [details] [diff] [review] Part 4: Rewrite WebIDL build system integration Would really like that interdiff, plus fixes to make the test stuff actually work.

Interdiffs can be seen at https://reviewboard.allizom.org/r/15/diff/. I'm not sure exactly which revisions correspond to what - I just brute force went through my repo history and exported old revisions.

That's.... not very helpful. I mean, I can try to take the various changes in this bug, apply them locally, and produce interdiffs myself. That would probably be less work than trying to sort through the link at comment 106, esp since a bunch of those interdiffs are actually filled with "Diff currently unavailable. Error: The patch to '$filename' didn't apply cleanly. ...". But I was somewhat hoping that you might have that sort of thing on hand, since that's the normal way to work on large patches with multiple patch revisions, or failing that would be willing to generate the interdiffs if you didn't keep them to start with. I guess I'm also willing to trust Nathan on this review, so if he doesn't need an interdiff, fine. But please do fix the functional bug.

(In reply to Boris Zbarsky [:bz] from comment #102) > > what makes sure bindings are regenerated if someone edits TestCodeGen.webidl or > > Codegen.py and then builds or exports in dom/bindings/test > > The answer based on applying the patches and testing seems to be: absolutely > nothing. > > If I change either one and then "make -C ../obj-firefox/dom/bindings/test/" > or "env MOZCONFIG=`pwd`/.mozconfig-fox ./mach build dom/bindings/test", > there is no regeneration or rebuilding of anything. As in, all the test > stuff is totally broken. This is not how things work any more. dom/bindings/test does pretty much nothing now. All codegen logic lives in dom/bindings or is available via a mach command. The proper way to do development is: 1) Touch your WebIDL files 2) mach build dom/bindings

Minor changes to incorporate review feedback.

Comment on attachment 8344518 [details] [diff] [review] Part 4: Rewrite WebIDL build system integration > The proper way to do development is: No. Not having to rebuild all binding cpp files simply to rebuild the test codegen so the impact of a codegen change can be examined was one of the explicit requirements in comment 27. Item 4, to be exact. It doesn't have to be "make" in dom/bindings/test, but the functionality needs to be provided, and afaict it's not.

And in particular, note your misguided assumption that I'm touching WebIDL files. I'm not.

Comment on attachment 8344518 [details] [diff] [review] Part 4: Rewrite WebIDL build system integration $ make -C dom/bindings export That will ensure all codegen is up to date without recompiling C++.

Comment on attachment 8344518 [details] [diff] [review] Part 4: Rewrite WebIDL build system integration Please read the requirements again. It needs to be possible to do three separate things: 1) Do the codegen, no compilation. 2) Do the codegen, test cpp compilation, no other compilation. 3) Compile everything. With your patch we have #1 and #3, but not #2.

Added a 'compiletests' make target in dom/bindings to satisfy bz's development requirement. Also fixed a bug in test compilation not working.

Comment on attachment 8344992 [details] [diff] [review] Part 4: Rewrite WebIDL build system integration Thank you, that's much better. I wish I could tab-complete targets like I can directories, though... Maybe name this target testlibs? r=me, I think. I mostly skimmed the diff (see lack of interdiffs), but did verify that the compiletests target seems to do what I want.

Removed bit rot.

Comment on attachment 8339853 [details] [diff] [review] Part 5: mach command for generating WebIDL example files; Carry over r+ from froydnj.

I'd like one of the WebIDL people to review the docs. This review won't hold up landing. Once bug 939367 lands, we can move this file to be under dom/bindings and you can make it part of your module and you can update it without involving us build peeps.

Comment on attachment 8345849 [details] [diff] [review] Part 4: Rewrite WebIDL build system integration Review of attachment 8345849 [details] [diff] [review]: ----------------------------------------------------------------- Mostly followup fodder. A file should probably be filed for all of them (most are related). I'd like to see the PP_TARGETS change before landing, though. ::: dom/bindings/Makefile.in @@ +13,4 @@ > endif > > +# These come from webidlsrcs.mk. > +CPPSRCS += $(globalgen_sources) $(unified_binding_cpp_files) Followup fodder: the list could go directly in backend.mk. @@ +32,5 @@ > + $(topsrcdir)/layout/style/nsCSSPropList.h \ > + $(topsrcdir)/layout/style/nsCSSPropAliasList.h \ > + $(webidl_base)/CSS2Properties.webidl.in \ > + $(webidl_base)/CSS2PropertiesProps.h \ > + $(srcdir)/GenerateCSS2PropertiesWebIDL.py \ Followup fodder: have these dependencies emitted by GenerateCSS2PropertiesWebIDL.py as a .pp file. @@ +56,4 @@ > $(GLOBAL_DEPS) \ > $(NULL) > > +include codegen.pp $(call include_deps,codegen.pp) @@ +63,3 @@ > @$(TOUCH) $@ > > +export:: codegen.pp Technically, the simple fact that codegen.pp is included guarantees that the rule is executed before anything happens in this directory, and that it's refreshed when there are changes (since it contains dependencies for itself) IOW, you can remove this line. @@ +66,3 @@ > > +.PHONY: compiletests > +compiletests: export That dependency on export seems unnecessary. @@ +74,5 @@ > parser.out \ > + WebIDLGrammar.pkl \ > + $(wildcard *.h) \ > + $(wildcard *.cpp) \ > + $(wildcard *.webidl) \ This should be derived from variables but meh. ::: dom/bindings/mozwebidlcodegen/__init__.py @@ +540,5 @@ > + if e.errno != errno.EEXIST: > + raise > + > + return WebIDLCodegenManager( > + os.path.join(src_dir, 'Bindings.conf'), mozpath? ::: dom/bindings/test/Makefile.in @@ +6,2 @@ > > +# $(test_stems) comes from webidlsrcs.mk. test_sources @@ +6,3 @@ > > +# $(test_stems) comes from webidlsrcs.mk. > +CPPSRCS += $(addprefix ../,$(test_sources)) Followup fodder: the list could go to backend.mk directly. @@ +7,5 @@ > +# $(test_stems) comes from webidlsrcs.mk. > +CPPSRCS += $(addprefix ../,$(test_sources)) > + > +ifdef GNU_CC > +OS_CXXFLAGS += -Wno-uninitialized Followup fodder: this could probably be replaced by the emission of the following in the generated code: #pragma GCC diagnostic ignored "-Wuninitialized" ::: python/mozbuild/mozbuild/backend/common.py @@ +214,5 @@ > + obj.basename)) > + > + elif isinstance(obj, PreprocessedWebIDLFile): > + self._webidls.preprocessed_sources.add(mozpath.join( > + obj.srcdir, obj.basename)) Followup fodder: It feels like the WebIDLCollection could be collected at the emitter level, and emitted from there. ::: python/mozbuild/mozbuild/backend/recursivemake.py @@ +713,5 @@ > > + # These contain autogenerated sources that the build config doesn't > + # yet know about. > + self._may_skip['compile'] -= {'ipc/ipdl'} > + self._may_skip['compile'] -= {'dom/bindings', 'dom/bindings/test'} Followup fodder: have the ipdl and webidl stuff emit their generated sources as if they were GENERATED_SOURCES in moz.build (that would get rid of this may_skip hack) @@ +1164,5 @@ > + # static to preprocessed don't end up writing to a symlink, > + # which would modify content in the source directory. > + '$(RM) $@', > + '$(call py_action,preprocessor,$(DEFINES) $(ACDEFINES) ' > + '$(XULPPFLAGS) $< -o $@)' PP_TARGETS? @@ +1175,5 @@ > + webidls.all_regular_cpp_basenames(), > + bindings_dir, > + unified_prefix='UnifiedBindings', > + unified_files_makefile_variable='unified_binding_cpp_files', > + poison_windows_h=True) Followup fodder: I think the $(CURDIR)/%: % dance generated by _add_unified_build_rules is not necessary anymore.

The docs should probably document the compiletests target or whatever its name ends up being?

(In reply to Mike Hommey [:glandium] from comment #121) > @@ +63,3 @@ > > @$(TOUCH) $@ > > > > +export:: codegen.pp > > Technically, the simple fact that codegen.pp is included guarantees that the > rule is executed before anything happens in this directory, and that it's > refreshed when there are changes (since it contains dependencies for itself) > > IOW, you can remove this line. You are correct. > @@ +66,3 @@ > > > > +.PHONY: compiletests > > +compiletests: export > > That dependency on export seems unnecessary. Also correct. > ::: dom/bindings/mozwebidlcodegen/__init__.py > @@ +540,5 @@ > > + if e.errno != errno.EEXIST: > > + raise > > + > > + return WebIDLCodegenManager( > > + os.path.join(src_dir, 'Bindings.conf'), > > mozpath? Yes. I'll update part 3 as well to use mozpath everywhere.

Addressed review comments. Biggest change is that now since we are using PP_TARGETS, the build rules require the input files to have a separate name from the output files or else make gets confused. It would take considerable effort to fix make. Long term, bug 935987 should help us here. This means I had to add ".in" to the .webidl files that are preprocessed. This shouldn't come as a surprise - this convention is used throughout the tree and .webidls were an exception. The build system now enforces that entries in the moz.build PREPROCESSED* variables end with .in. This is the part I'd like sign-off from a DOM peer on. Should be rubber stamp worthy.

Properly rebased try. https://tbpl.mozilla.org/?tree=Try&rev=3c76120ad54b

Now that bug 939367 landed, Sphinx docs can be anywhere in the source tree. So I updated this patch to move things into dom/bindings/docs. It's part of your module now :)

Sorry for the review spam. But this version adds the mozwebidlcodegen Python package to Sphinx's auto API docs.

Comment on attachment 8346392 [details] [diff] [review] Part 4: Rewrite WebIDL build system integration; That's actually pretty annoying because it means that you have to rename the file to go to/from preprocessing. And preprocessing is likely to be a temporary state in many cases as we add experimental APIs, so I do expect files to be moving in/out of preprocessing a lot. Can we instead change the name of the _output_ file in this case? Alternately, could we have a rule that creates .webidl.in files in the objdir by copying/linking the .webidl from the srcdir and then another rule that preprocesses those? If we _do_ have to do this renaming, which I would very much like to avoid, please do it with hg mv, not like it's done in this patch.

I did type `hg mv`. I think I encountered a bug in the experimental Mercurial extension I'm using. Not too surprising. I guess I'll revert to not use PP_TARGETS. This will preserve filenames at the expense of making build system integration not ideal. Followup fodder.

Comment on attachment 8346405 [details] [diff] [review] Part 6: Add docs for WebIDL and the build system Review of attachment 8346405 [details] [diff] [review]: ----------------------------------------------------------------- I am going to assume that you got the moz.build changes correct. ::: dom/bindings/docs/index.rst @@ +32,5 @@ > + are defined this way. > + > +GENERATED_EVENTS_WEBIDL_FILES > + In addition to generating a binding, these ``.webidl`` files also > + generate an event source file. How about "...generate a source file implementing the event object in C++"? @@ +36,5 @@ > + generate an event source file. > + > +PREPROCESSED_WEBIDL_FILES > + The ``.webidl`` files are generated by preprocessing an input file. > + They otherwise behave like **WEBIDL_FILES**. Depending on what bz/glandium say about preprocessed files, it is worth expanding this section just a smidge explaining the naming conventions here. @@ +47,5 @@ > + Like **TEST_WEBIDL_FILES** except the ``.webidl`` is obtained via > + preprocessing, much like **PREPROCESSED_WEBIDL_FILES**. > + > +GENERATED_WEBIDL_FILES > + The ``.webidl`` for these is obtained through an *external* Nit: the *emphasis* here seems unnecessary. @@ +72,5 @@ > +Parser unit tests > + There are parser tests provided by ``dom/bindings/parser/runtests.py`` > + that should run as part of ``make check``. There must be a mechanism > + to run the tests in *human* mode so they output friendly error > + messages. Since you're documenting how other things are run below, can you document what bit runs this? @@ +81,5 @@ > + > +Test interfaces are generated as part of the build > + ``TestExampleGenBinding.cpp`` calls into methods from the > + ``TestExampleInterface`` and ``TestExampleProxyInterface`` interfaces. > + These interfaces need to be generated as part of the build. Probably want to make explicit that test interface headers should not be exported. @@ +84,5 @@ > + ``TestExampleInterface`` and ``TestExampleProxyInterface`` interfaces. > + These interfaces need to be generated as part of the build. > + > +Running tests automatically rebuilds > + When a developer runs the WebIDL tests, they expect any necessary These are the parser tests, right? So at least... @@ +87,5 @@ > +Running tests automatically rebuilds > + When a developer runs the WebIDL tests, they expect any necessary > + rebuilds to occur. > + > + This is faciliated through ``mach webidl-parser-test``. ...through here should probably be combined with the "Parser unit tests", above. And then this section doesn't really have anything to do with "running" tests, per se, so the section should be renamed. Maybe it should be combined with the "Test interfaces are generated..." section...and then that section just renamed "Working with test interfaces" or somesuch. @@ +95,5 @@ > + > +Minimal rebuilds > + Reprocessing every output for every change is expensive. So we don't > + inconvenience people changing ``.webidl`` files, the build system > + should only perform a minimal rebuild when sources change. Worth adding a note about how we don't do this yet, e.g. bug 940469's requirement? I was going to say that defining "minimal rebuild" would be good, but perhaps it is sufficient that the tests for the codegen manager have decent documentation on this? Can we link to those tests? @@ +98,5 @@ > + inconvenience people changing ``.webidl`` files, the build system > + should only perform a minimal rebuild when sources change. > + > +Explicit method for performing codegen > + There needs to be an explicit method for incurring code generation. Nit: "incurring" reads oddly to me; "invoking" seems more natural. @@ +107,5 @@ > +No-op binding generation should be fast > + So developers touching ``.webidl`` files are not inconvenienced, > + no-op binding generation should be fast. Watch out for the build system > + processing large dependency files it doesn't need in order to perform > + code generation. Since the current patch series is totally punting on this, we should add noises to that effect in the documentation. @@ +113,5 @@ > +Ability to generate example files > + *Any* interface can have example ``.h``/``.cpp`` files generated. > + There must be a mechanism to facilitate this. > + > + This is currently facilitated through ``mach webidl-example``. Example invocation?

Reverted to the old preprocessor way. No file renaming involved. This is effectively the patch that got r+ from bz, so carrying review forward. Only changes are to address glandium's review comments. I anticipate a landing in the morning. https://tbpl.mozilla.org/?tree=Try&rev=d75c7113b2cc Try is with PGO just in case.

> I guess I'll revert to not use PP_TARGETS. Thank you. I really appreciate that!

Comment on attachment 8346578 [details] [diff] [review] Part 4: Rewrite WebIDL build system integration; Review of attachment 8346578 [details] [diff] [review]: ----------------------------------------------------------------- The f+ was because i wanted to take a look at the changes for PP_TARGETS, but since we agreed not to do that, the f+ morphs into an r+.

Part 3-5 \o/ https://hg.mozilla.org/integration/mozilla-inbound/rev/f9614eb176ad https://hg.mozilla.org/integration/mozilla-inbound/rev/67fa1478308e https://hg.mozilla.org/integration/mozilla-inbound/rev/0d9f8da0806e Will apply Nathan's docs feedback and get that landed sometime when I'm not heads down on build system foo with glandium.

And part 6. https://hg.mozilla.org/integration/mozilla-inbound/rev/54c6e55f60af https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Adding_WebIDL_bindings_to_a_class still needs updating, so keeping dev-doc-needed. Perhaps (some of) that content should be rolled into the tree so it is versioned with the source code? I think that's up to others.

https://hg.mozilla.org/mozilla-central/rev/f9614eb176ad https://hg.mozilla.org/mozilla-central/rev/67fa1478308e https://hg.mozilla.org/mozilla-central/rev/0d9f8da0806e https://hg.mozilla.org/mozilla-central/rev/54c6e55f60af

I've updated the documentation.

When I invoke |make client.mk| in a recently refreshed ( a few hours ago) comm-central tree, (I removed mozobj directory just in case and re-creating the binary) I get the following error: ... Making all in include Making all in testsuite Making all in man Makefile:69: codegen.pp: No such file or directory Traceback (most recent call last): File "/usr/lib/python2.7/runpy.py", line 162, in _run_module_as_main "__main__", fname, loader, pkg_name) File "/usr/lib/python2.7/runpy.py", line 72, in _run_code exec code in run_globals File "/new-hd1/extra/ishikawa/TB-3HG/NEW-COMMSRC/mozilla/python/mozbuild/mozbuild/action/webidl.py", line 17, in <module> sys.exit(main(sys.argv[1:])) File "/new-hd1/extra/ishikawa/TB-3HG/NEW-COMMSRC/mozilla/python/mozbuild/mozbuild/action/webidl.py", line 12, in main manager = BuildSystemWebIDL.from_environment().manager File "/new-hd1/extra/ishikawa/TB-3HG/NEW-COMMSRC/mozilla/python/mozbuild/mozbuild/base.py", line 175, in from_environment if not samepath(topobjdir, config_topobjdir) \ File "/new-hd1/extra/ishikawa/TB-3HG/NEW-COMMSRC/mozilla/python/mozbuild/mozbuild/base.py", line 41, in samepath return os.path.samefile(path1, path2) File "/new-hd1/extra/ishikawa/TB-3HG/objdir-tb3/mozilla/_virtualenv/lib/python2.7/posixpath.py", line 163, in samefile s2 = os.stat(f2) OSError: [Errno 2] No such file or directory: '/new-hd1/extra/ishikawa/TB-3HG/NEW-COMMSRC/objdir-tb3' make[6]: *** [codegen.pp] Error 1 make[5]: *** [bindings_export] Error 2 make[5]: *** Waiting for unfinished jobs.... make[4]: *** [export] Error 2 make[3]: *** [export] Error 2 make[2]: *** [default] Error 2 make[1]: *** [default] Error 2 make: *** [build] Error 2 real 0m21.898s user 0m4.000s sys 0m18.513s I searched bugzilla and ended up with this bugzilla entry. Can it be that comm-central is lagging behind in incorporating the latest patches? TIA

(In reply to ISHIKAWA, Chiaki from comment #140) > When I invoke |make client.mk| in a recently refreshed ( a few hours ago) > comm-central tree, Pretty sure this is bug 950332.

(In reply to Gregory Szorc [:gps] from comment #141) > (In reply to ISHIKAWA, Chiaki from comment #140) > > When I invoke |make client.mk| in a recently refreshed ( a few hours ago) > > comm-central tree, > > Pretty sure this is bug 950332. Thanks. This was it. Once I removed the use of relative path in mozconfig to specify MOZ_OBJDIR, things are working again.