Since installing and processing XPIDLSRCS doesn't depend on other build parts of the build, they make a good candidate for being the first to move to moz.build after DIRS. This will involve moving all logic that sets XPIDLSRCS and SDK_XPIDLSRCS from Makefile.in into moz.build. Marked as blocking 698251, since the SDK_XPIDLSRCS/XPIDLSRCS merge can be done during this conversion so that only XPIDLSRCS exists in moz.build.

Possible crazy idea: currently, we stage header and IDL files into corresponding directories in the object directory and then we copy/symlink them into /dist. We blow away these parts of /dist at the top of builds so we don't have orphaned files in /dist. What if we use the packager code for copying/installing files to maintain these parts of /dist? e.g. the build system could produce a packager manifest of all the known IDLs, etc. We could then call this code as part of the build and orphaned files would automagically be removed since they aren't in the packager manifest! Note that this is very similar to how Tup works. If the plan is to switch to Tup, maybe this isn't worth pursuing.

This is not a crazy idea. In fact i was planning to do it once variables like XPIDLSRCS or EXPORTS move to moz.build. Not with a package manifest, though. Just with an ad-hoc script using the packager library.

rules.mk assigns $(MODULE) to XPIDL_MODULE if XPIDL_MODULE isn't defined. This patch removes unnecessary XPIDL_MODULE assignments from Makefile.in.

Preliminary patch for moz.build support for this. Focusing on straight port right now. Parallel optimizations will come later (possibly in a different bug). We also have bug 844654 to consider how we want MODULE and friends to work long term. Not asking for review yet because I don't have tests. Drive-by's welcome.

I wrote a script that moves variables from Makefile.in to moz.build files. This is what it produced. I confess to not having looked at the output in much detail. A cursory glace reveals it seems OK!

The script I used in part 3 did not touch occurrences protected by conditionals. This patch was a manual conversion of the bits the script didn't touch.

Automatic conversion of XPIDLSRCS and SDK_IDLSRCS from Makefile.in to moz.build. As part of the conversion, these two variables were merged together and the resulting file list was sorted. I haven't looked at the result in much detail but it seems to be pretty good.

As with part 2, things happening in conditionals weren't touched by my script. I performed manual conversion of the remaining items.

Some Makefile.in were empty after moving IDLSRCS and MODULE foo out. This patch gets rid of empty files.

Why does MOZ_SYSTEM_PLY exist? Why can't we just use whatever ply we have in tree? It seems risky/silly for us to use a system/unknown ply when we have a known good version in the tree. What context am I missing?

(In reply to Gregory Szorc [:gps] from comment #10) > Why does MOZ_SYSTEM_PLY exist? Why can't we just use whatever ply we have in > tree? It seems risky/silly for us to use a system/unknown ply when we have a > known good version in the tree. What context am I missing? The scripts using ply are shipped with the sdk, and when i ship the sdk on debian i don't want it to ship its own ply when there is a system one.

Comment on attachment 717669 [details] [diff] [review] Part 1: Remove unnecessary XPIDL_MODULE definitions, v1 Review of attachment 717669 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/sample/Makefile.in @@ -18,2 @@ > # i.e. dist/bin/components/xpcomsample.xpt > -XPIDL_MODULE = xpcomsample Please don't remove it here, it's a sample Makefile. Instead add a comment that it is only needed when different from MODULE.

We should probably just ditch XPIDL_MODULE. I don't think it really serves a purpose at this point, given that we link all XPT files together at packaging time anyway.

We'll just do a mechanical conversion first. Figuring out data modeling can come later.

Per our meeting last week, it was decided the general strategy for moving things to moz.build files will be to perform a largely mechanical conversion of variables to moz.build files and then figure out moz.build-enabled enhancements (like parallel building) later. In this patch, I add generic support for proxying moz.build variables into backend.mk. Tests will come in a later patch once there is something to actually test! (Currently this patch has no impact on behavior other than the creation of a local VariablePassthru instance in the emitter.) I will rebase other patches accordingly.

Comment on attachment 721384 [details] [diff] [review] Part 2: Support variable passthru, v1 Review of attachment 721384 [details] [diff] [review]: ----------------------------------------------------------------- ::: python/mozbuild/mozbuild/backend/recursivemake.py @@ +180,5 @@ > backend_file.environment.create_config_file(obj.output_path)) > self.summary.managed_count += 1 > + elif isinstance(obj, VariablePassthru): > + # Sorted so output is consistent and we don't bump mtimes. > + for k in sorted(obj.variables.keys()): for k, v in sorted(obj.variables.items()) ?

https://hg.mozilla.org/integration/mozilla-inbound/rev/b04032e5289e https://hg.mozilla.org/integration/mozilla-inbound/rev/c22a331c755f

https://hg.mozilla.org/mozilla-central/rev/b04032e5289e https://hg.mozilla.org/mozilla-central/rev/c22a331c755f

Add XPIDLSRCS and SDK_XPIDLSRCS to deprecated list in rules.mk. Remove SDK_XPIDLSRCS foo from rules.mk (combining into single variable). Define XPIDL_SOURCES variable in moz.build sandbox. Add variable passthru for XPIDL_SOURCES -> XPIDLSRCS. Add tests for variable passthru behavior. Future conversions shouldn't be as time intensive because we can probably skimp on the test writing. I'd rather save it for later when emitter.py and recursivemake.py are doing something more involved than passing variables from moz.build to backend.mk.

Comment on attachment 722570 [details] [diff] [review] Part 3: Support XPIDLSRCS in moz.build, v2 Review of attachment 722570 [details] [diff] [review]: ----------------------------------------------------------------- ::: python/mozbuild/mozbuild/frontend/sandbox_symbols.py @@ +141,5 @@ > variables declared during configure. > """), > + > + # IDL Generation. > + 'XPIDL_SOURCES': (list, [], 'XPIDL_SOURCES' seems to sound a lot longer than 'XPIDLSRCS'. @@ +142,5 @@ > """), > + > + # IDL Generation. > + 'XPIDL_SOURCES': (list, [], > + """IDL files. s/IDL files/XPIDL files/ (we've also got WebIDL files in the tree).

Comment on attachment 722570 [details] [diff] [review] Part 3: Support XPIDLSRCS in moz.build, v2 Review of attachment 722570 [details] [diff] [review]: ----------------------------------------------------------------- ::: config/rules.mk @@ +32,5 @@ > $(error Variable $(var) is defined in Makefile. It should only be defined in moz.build files),\ > )) > > +$(foreach var,$(_LEGACY_UNSUPPORTED_VARIABLES),$(if $($(var)),\ > + $(error Legacy variable $(var) is defined in Makefile. It has no effect and must be deleted),\ It would be better to indicate that they should be moved in XPIDL_SOURCES. Which brings us to the other problem: since the name in moz.build and the old name in Makefile.in are different, the message for _MOZBUILD_EXTERNAL_VARIABLES is not entirely helpful. It would be better to give an explicit message that $var should be defined in moz.build as $new_var. That's probably a pita to do with make, so an explicit message from the moz.build parser that XPIDLSRCS is XPIDL_SOURCES would probably work. It will however make people do the change in two times and be annoyed. ::: python/mozbuild/mozbuild/frontend/sandbox_symbols.py @@ +142,5 @@ > """), > + > + # IDL Generation. > + 'XPIDL_SOURCES': (list, [], > + """IDL files. "XPCOM Interface Definition files (xpidl)" ?

I changed the description in sandbox_symbols.py and the error code in rules.mk. While I was here, I also added a "current makefile" variable to the error message. I also changed the existing error message to include the make file.

Fully automatic conversion.

And the manual part (mostly stuff inside conditionals). This warrants more review than the auto patch. Try at https://tbpl.mozilla.org/?tree=Try&rev=482a56ccb869.

Looks like B2G is busted. I must have copied a conditional incorrectly :/

Auto-generated from mozbuild-migrate, aside from a single if-condition in dom/bluetooth, and the rules.mk change.

Auto-generated from mozbuild-migrate, aside from the config.mk change. The config.mk file had to be modified since backend.mk is included before config.mk, and config.mk overwrites XPIDL_FLAGS. Also, rules.mk cannot check for an empty XPIDL_FLAGS since config.mk is always setting it to a default value. We could possibly make config.mk set DEFAULT_XPIDL_FLAGS or something instead, so that XPIDL_FLAGS will be empty if not defined by moz.build.

Comment on attachment 723558 [details] [diff] [review] Part 3: Support XPIDLSRCS in moz.build, v3 Review of attachment 723558 [details] [diff] [review]: ----------------------------------------------------------------- ::: config/rules.mk @@ +30,2 @@ > $(foreach var,$(_MOZBUILD_EXTERNAL_VARIABLES),$(if $($(var)),\ > + $(error Variable $(var) is defined in Makefile $(_current_makefile). It should only be defined in moz.build files),\ -Makefile

Comment on attachment 723805 [details] [diff] [review] Part 6: Support XPIDL_MODULE in moz.build Review of attachment 723805 [details] [diff] [review]: ----------------------------------------------------------------- I actually would have granted r+ without the tests. IMO if we are just doing variable pass-thru, we don't need tests for each individual variable: we're going to rip out the pass-thru later and the impact of failed pass-thru is obvious, so I wouldn't waste time on the tests. ::: python/mozbuild/mozbuild/frontend/sandbox_symbols.py @@ +152,5 @@ > + > + 'XPIDL_MODULE': (unicode, "", > + """XPCOM Interface Definition Module Name. > + > + This is the name of the .xpt file that is created by linking XPIDL_SOURCES together. If unspecified, it defaults to be the same as MODULE. You should wrap this long line. See rest of file.

Comment on attachment 723806 [details] [diff] [review] Part 7: Move XPIDL_MODULE to moz.build Review of attachment 723806 [details] [diff] [review]: ----------------------------------------------------------------- rs=me. I assume you used the conversion tool. I only glanced at these and trust the tool.

Comment on attachment 723807 [details] [diff] [review] Part 8: Support XPIDL_FLAGS in moz.build Review of attachment 723807 [details] [diff] [review]: ----------------------------------------------------------------- Maybe one day we can get rid of the -I arguments because we'll have a whole-world view and will just know where things are or could perform all the IDL foo in one directory.

(In reply to Gregory Szorc [:gps] from comment #32) > I assume you used the conversion tool. I only glanced at these and trust the > tool. Correct - I mentioned in the comments which parts were not from the conversion tool (dom/bluetooh, plus config.mk / rules.mk). I was using a patched conversion tool as described here: https://bugzilla.mozilla.org/show_bug.cgi?id=847066#c7 Otherwise, XPIDL_FLAGS would not convert properly since it was usually the last variable in Makefile.in

Comment on attachment 723809 [details] [diff] [review] Part 9: Move XPIDL_FLAGS to moz.build Review of attachment 723809 [details] [diff] [review]: ----------------------------------------------------------------- rs=me ::: browser/components/places/moz.build @@ +6,5 @@ > DIRS += ['src'] > TEST_DIRS += ['tests'] > + > +XPIDL_FLAGS += [ > + '-I$(topsrcdir)/browser/components/', Hmmm. I'm not totally thrilled with the idea of having $(topsrcdir) in moz.build files. But, if the goal is to effectively transplant strings from Makefile.in to moz.build, then I suppose we'll carry this over now and fix later. ::: config/config.mk @@ +525,5 @@ > > # Default location of include files > IDL_DIR = $(DIST)/idl > > +XPIDL_FLAGS += -I$(srcdir) -I$(IDL_DIR) Boo. Sadly, we need to include backend.mk before config.mk because config.mk can introduce variables that would fail the "is mozbuild variable defined" check.

Parts 3-5 (XPIDLSRCS move): https://hg.mozilla.org/projects/build-system/rev/b7be91238e89 https://hg.mozilla.org/projects/build-system/rev/d54481a205e3 https://hg.mozilla.org/projects/build-system/rev/b5dc4f0d5a32 mshal: If you have push privileges to b-s, feel free to push your patches! Perhaps we can merge this into m-c by EOD.

And parts 6-9: https://hg.mozilla.org/projects/build-system/rev/b8f89bbfb7f1 https://hg.mozilla.org/projects/build-system/rev/57a08895cacc https://hg.mozilla.org/projects/build-system/rev/570563fe3824 https://hg.mozilla.org/projects/build-system/rev/e105eedc18c9

https://hg.mozilla.org/mozilla-central/rev/b7be91238e89 https://hg.mozilla.org/mozilla-central/rev/d54481a205e3 https://hg.mozilla.org/mozilla-central/rev/b5dc4f0d5a32 https://hg.mozilla.org/mozilla-central/rev/b8f89bbfb7f1 https://hg.mozilla.org/mozilla-central/rev/57a08895cacc https://hg.mozilla.org/mozilla-central/rev/570563fe3824 https://hg.mozilla.org/mozilla-central/rev/e105eedc18c9 w00t.

Pushed by frgrahl@gmx.net: https://hg.mozilla.org/comm-central/rev/a7df814385d4 Part 4: Move XPIDLSRCS into moz.build (auto); rs=glandium https://hg.mozilla.org/comm-central/rev/9b4c645eca90 Part 7: Move XPIDL_MODULE to moz.build; rs=gps