Last Comment Bug 818246 - Move XPIDLSRCS out of Makefile.in, into moz build
: Move XPIDLSRCS out of Makefile.in, into moz build
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla22
Assigned To: Gregory Szorc [:gps]
:
Mentors:
: 698251 (view as bug list)
Depends on: 784841
Blocks: nomakefiles 698251 850380 850389
  Show dependency treegraph
 
Reported: 2012-12-04 13:15 PST by Michael Shal [:mshal]
Modified: 2013-05-14 16:49 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1: Remove unnecessary XPIDL_MODULE definitions, v1 (16.84 KB, patch)
2013-02-24 13:45 PST, Gregory Szorc [:gps]
mh+mozilla: review+
Details | Diff | Splinter Review
Part 2: Support MODULE, XPIDL_MODULE, and IDLSRCS in moz.build, v1 (10.43 KB, patch)
2013-02-24 17:39 PST, Gregory Szorc [:gps]
no flags Details | Diff | Splinter Review
Part 3: Move MODULE and XPIDL_MODULE to moz.build (auto), v1 (691.59 KB, patch)
2013-02-24 17:40 PST, Gregory Szorc [:gps]
no flags Details | Diff | Splinter Review
Part 4: Move MODULE and XPIDL_MODULE to moz.build (manual), v1 (4.02 KB, patch)
2013-02-24 17:41 PST, Gregory Szorc [:gps]
no flags Details | Diff | Splinter Review
Part 5: Move IDLSRCS and SDK_ILDSRCS to moz.build (auto), v1 (247.14 KB, patch)
2013-02-24 17:43 PST, Gregory Szorc [:gps]
no flags Details | Diff | Splinter Review
Part 6: Move IDLSRCS and SDK_IDLSRCS to moz.build (manual), v1 (28.58 KB, patch)
2013-02-24 17:44 PST, Gregory Szorc [:gps]
no flags Details | Diff | Splinter Review
Part 7: Remove empty Makefile.in, v1 (53.94 KB, patch)
2013-02-24 17:45 PST, Gregory Szorc [:gps]
no flags Details | Diff | Splinter Review
Part 2: Support variable passthru, v1 (4.69 KB, patch)
2013-03-05 11:52 PST, Gregory Szorc [:gps]
mh+mozilla: review+
Details | Diff | Splinter Review
Part 3: Support XPIDLSRCS in moz.build, v2 (9.67 KB, patch)
2013-03-07 16:51 PST, Gregory Szorc [:gps]
mh+mozilla: review+
Details | Diff | Splinter Review
Part 3: Support XPIDLSRCS in moz.build, v3 (10.04 KB, patch)
2013-03-11 11:08 PDT, Gregory Szorc [:gps]
mh+mozilla: review+
Details | Diff | Splinter Review
Part 4: Move XPIDLSRCS to moz.build (auto), v1 (238.05 KB, patch)
2013-03-11 11:22 PDT, Gregory Szorc [:gps]
mh+mozilla: review+
Details | Diff | Splinter Review
Part 5: Move IDLSRCS and SDK_ILDSRCS to moz.build (manual), v1 (29.12 KB, patch)
2013-03-11 12:12 PDT, Gregory Szorc [:gps]
mh+mozilla: review+
Details | Diff | Splinter Review
Part 6: Support XPIDL_MODULE in moz.build (5.59 KB, patch)
2013-03-11 21:58 PDT, Michael Shal [:mshal]
gps: review+
Details | Diff | Splinter Review
Part 7: Move XPIDL_MODULE to moz.build (117.33 KB, patch)
2013-03-11 22:00 PDT, Michael Shal [:mshal]
gps: review+
Details | Diff | Splinter Review
Part 8: Support XPIDL_FLAGS in moz.build (6.16 KB, patch)
2013-03-11 22:02 PDT, Michael Shal [:mshal]
gps: review+
Details | Diff | Splinter Review
Part 9: Move XPIDL_FLAGS to moz.build (52.21 KB, patch)
2013-03-11 22:05 PDT, Michael Shal [:mshal]
gps: review+
Details | Diff | Splinter Review
Part 6: Support XPIDL_MODULE in moz.build v2 (5.60 KB, patch)
2013-03-12 10:08 PDT, Michael Shal [:mshal]
gps: review+
Details | Diff | Splinter Review
Part 8: Support XPIDL_FLAGS in moz.build v2 (6.16 KB, patch)
2013-03-12 10:09 PDT, Michael Shal [:mshal]
gps: review+
Details | Diff | Splinter Review

Description Michael Shal [:mshal] 2012-12-04 13:15:48 PST
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.
Comment 1 Gregory Szorc [:gps] 2013-02-23 14:10:36 PST
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.
Comment 2 Mike Hommey [:glandium] 2013-02-24 04:42:52 PST
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.
Comment 3 Gregory Szorc [:gps] 2013-02-24 13:45:22 PST
Created attachment 717669 [details] [diff] [review]
Part 1: Remove unnecessary XPIDL_MODULE definitions, v1

rules.mk assigns $(MODULE) to XPIDL_MODULE if XPIDL_MODULE isn't defined. This patch removes unnecessary XPIDL_MODULE assignments from Makefile.in.
Comment 4 Gregory Szorc [:gps] 2013-02-24 17:39:10 PST
Created attachment 717681 [details] [diff] [review]
Part 2: Support MODULE, XPIDL_MODULE, and IDLSRCS in moz.build, v1

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.
Comment 5 Gregory Szorc [:gps] 2013-02-24 17:40:33 PST
Created attachment 717683 [details] [diff] [review]
Part 3: Move MODULE and XPIDL_MODULE to moz.build (auto), v1

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!
Comment 6 Gregory Szorc [:gps] 2013-02-24 17:41:35 PST
Created attachment 717684 [details] [diff] [review]
Part 4: Move MODULE and XPIDL_MODULE to moz.build (manual), v1

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.
Comment 7 Gregory Szorc [:gps] 2013-02-24 17:43:07 PST
Created attachment 717685 [details] [diff] [review]
Part 5: Move IDLSRCS and SDK_ILDSRCS to moz.build (auto), v1

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.
Comment 8 Gregory Szorc [:gps] 2013-02-24 17:44:04 PST
Created attachment 717686 [details] [diff] [review]
Part 6: Move IDLSRCS and SDK_IDLSRCS to moz.build (manual), v1

As with part 2, things happening in conditionals weren't touched by my script. I performed manual conversion of the remaining items.
Comment 9 Gregory Szorc [:gps] 2013-02-24 17:45:00 PST
Created attachment 717687 [details] [diff] [review]
Part 7: Remove empty Makefile.in, v1

Some Makefile.in were empty after moving IDLSRCS and MODULE foo out. This patch gets rid of empty files.
Comment 10 Gregory Szorc [:gps] 2013-02-24 19:07:00 PST
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?
Comment 11 Mike Hommey [:glandium] 2013-02-25 01:47:05 PST
(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 12 Mike Hommey [:glandium] 2013-02-25 04:28:32 PST
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.
Comment 13 Ted Mielczarek [:ted.mielczarek] 2013-02-25 05:55:53 PST
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.
Comment 14 Gregory Szorc [:gps] 2013-03-01 14:55:51 PST
We'll just do a mechanical conversion first. Figuring out data modeling can come later.
Comment 15 Gregory Szorc [:gps] 2013-03-05 11:52:48 PST
Created attachment 721384 [details] [diff] [review]
Part 2: Support variable passthru, v1

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 16 Mike Hommey [:glandium] 2013-03-05 15:50:41 PST
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()) ?
Comment 19 Gregory Szorc [:gps] 2013-03-07 16:51:25 PST
Created attachment 722570 [details] [diff] [review]
Part 3: Support XPIDLSRCS in moz.build, v2

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 20 :Ms2ger (⌚ UTC+1/+2) 2013-03-07 23:19:06 PST
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 21 Mike Hommey [:glandium] 2013-03-08 05:51:55 PST
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)" ?
Comment 22 Gregory Szorc [:gps] 2013-03-11 11:08:22 PDT
Created attachment 723558 [details] [diff] [review]
Part 3: Support XPIDLSRCS in moz.build, v3

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.
Comment 23 Gregory Szorc [:gps] 2013-03-11 11:22:35 PDT
Created attachment 723566 [details] [diff] [review]
Part 4: Move XPIDLSRCS to moz.build (auto), v1

Fully automatic conversion.
Comment 24 Gregory Szorc [:gps] 2013-03-11 12:12:48 PDT
Created attachment 723591 [details] [diff] [review]
Part 5: Move IDLSRCS and SDK_ILDSRCS to moz.build (manual), v1

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.
Comment 25 Gregory Szorc [:gps] 2013-03-11 14:22:34 PDT
Looks like B2G is busted. I must have copied a conditional incorrectly :/
Comment 26 Michael Shal [:mshal] 2013-03-11 21:58:24 PDT
Created attachment 723805 [details] [diff] [review]
Part 6: Support XPIDL_MODULE in moz.build
Comment 27 Michael Shal [:mshal] 2013-03-11 22:00:50 PDT
Created attachment 723806 [details] [diff] [review]
Part 7: Move XPIDL_MODULE to moz.build

Auto-generated from mozbuild-migrate, aside from a single if-condition in dom/bluetooth, and the rules.mk change.
Comment 28 Michael Shal [:mshal] 2013-03-11 22:02:54 PDT
Created attachment 723807 [details] [diff] [review]
Part 8: Support XPIDL_FLAGS in moz.build
Comment 29 Michael Shal [:mshal] 2013-03-11 22:05:27 PDT
Created attachment 723809 [details] [diff] [review]
Part 9: Move XPIDL_FLAGS to moz.build

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 30 Mike Hommey [:glandium] 2013-03-12 02:07:12 PDT
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 31 Gregory Szorc [:gps] 2013-03-12 09:53:42 PDT
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 32 Gregory Szorc [:gps] 2013-03-12 09:56:24 PDT
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 33 Gregory Szorc [:gps] 2013-03-12 09:58:23 PDT
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.
Comment 34 Michael Shal [:mshal] 2013-03-12 10:08:46 PDT
Created attachment 724003 [details] [diff] [review]
Part 6: Support XPIDL_MODULE in moz.build v2
Comment 35 Michael Shal [:mshal] 2013-03-12 10:09:49 PDT
Created attachment 724005 [details] [diff] [review]
Part 8: Support XPIDL_FLAGS in moz.build v2
Comment 36 Michael Shal [:mshal] 2013-03-12 10:11:38 PDT
(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 37 Gregory Szorc [:gps] 2013-03-12 10:11:48 PDT
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.
Comment 38 Gregory Szorc [:gps] 2013-03-12 10:23:34 PDT
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.
Comment 40 Gregory Szorc [:gps] 2013-03-12 12:51:02 PDT
*** Bug 698251 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.