Move XPIDLSRCS out of Makefile.in, into moz build

RESOLVED FIXED in mozilla22

Status

()

Core
Build Config
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: mshal, Assigned: gps)

Tracking

(Blocks: 1 bug)

Trunk
mozilla22
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(9 attachments, 9 obsolete attachments)

16.84 KB, patch
glandium
: review+
Details | Diff | Splinter Review
4.69 KB, patch
glandium
: review+
Details | Diff | Splinter Review
10.04 KB, patch
glandium
: review+
Details | Diff | Splinter Review
238.05 KB, patch
glandium
: review+
Details | Diff | Splinter Review
29.12 KB, patch
glandium
: review+
Details | Diff | Splinter Review
117.33 KB, patch
gps
: review+
Details | Diff | Splinter Review
52.21 KB, patch
gps
: review+
Details | Diff | Splinter Review
5.60 KB, patch
gps
: review+
Details | Diff | Splinter Review
6.16 KB, patch
gps
: review+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
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.
(Assignee)

Comment 1

4 years ago
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.
(Assignee)

Updated

4 years ago
Depends on: 844654
(Assignee)

Comment 3

4 years ago
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.
Attachment #717669 - Flags: review?(mh+mozilla)
(Assignee)

Comment 4

4 years ago
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.
Assignee: nobody → gps
Status: NEW → ASSIGNED
(Assignee)

Comment 5

4 years ago
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!
(Assignee)

Comment 6

4 years ago
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.
(Assignee)

Comment 7

4 years ago
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.
(Assignee)

Comment 8

4 years ago
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.
(Assignee)

Comment 9

4 years ago
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.
(Assignee)

Comment 10

4 years ago
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.
Attachment #717669 - Flags: review?(mh+mozilla) → review+
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.
(Assignee)

Comment 14

4 years ago
We'll just do a mechanical conversion first. Figuring out data modeling can come later.
No longer depends on: 844654

Updated

4 years ago
Blocks: 847009
(Assignee)

Comment 15

4 years ago
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.
Attachment #717681 - Attachment is obsolete: true
Attachment #717683 - Attachment is obsolete: true
Attachment #717684 - Attachment is obsolete: true
Attachment #717685 - Attachment is obsolete: true
Attachment #717686 - Attachment is obsolete: true
Attachment #717687 - Attachment is obsolete: true
Attachment #721384 - Flags: review?(mh+mozilla)
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()) ?
Attachment #721384 - Flags: review?(mh+mozilla) → review+
(Assignee)

Comment 17

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b04032e5289e
https://hg.mozilla.org/integration/mozilla-inbound/rev/c22a331c755f
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/b04032e5289e
https://hg.mozilla.org/mozilla-central/rev/c22a331c755f
(Assignee)

Comment 19

4 years ago
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.
Attachment #722570 - Flags: review?(mh+mozilla)
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)" ?
Attachment #722570 - Flags: review?(mh+mozilla) → review+
(Assignee)

Comment 22

4 years ago
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.
Attachment #722570 - Attachment is obsolete: true
Attachment #723558 - Flags: review?(mh+mozilla)
(Assignee)

Comment 23

4 years ago
Created attachment 723566 [details] [diff] [review]
Part 4: Move XPIDLSRCS to moz.build (auto), v1

Fully automatic conversion.
Attachment #723566 - Flags: review?(mh+mozilla)
(Assignee)

Comment 24

4 years ago
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.
Attachment #723591 - Flags: review?(mh+mozilla)
(Assignee)

Comment 25

4 years ago
Looks like B2G is busted. I must have copied a conditional incorrectly :/
(Reporter)

Comment 26

4 years ago
Created attachment 723805 [details] [diff] [review]
Part 6: Support XPIDL_MODULE in moz.build
(Reporter)

Comment 27

4 years ago
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.
Attachment #723806 - Flags: review?
(Reporter)

Comment 28

4 years ago
Created attachment 723807 [details] [diff] [review]
Part 8: Support XPIDL_FLAGS in moz.build
Attachment #723807 - Flags: review?(gps)
(Reporter)

Comment 29

4 years ago
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.
Attachment #723809 - Flags: review?(gps)
(Reporter)

Updated

4 years ago
Attachment #723805 - Flags: review?(gps)
(Reporter)

Updated

4 years ago
Attachment #723806 - Flags: review? → review?(gps)
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
Attachment #723558 - Flags: review?(mh+mozilla) → review+
Attachment #723566 - Flags: review?(mh+mozilla) → review+
Attachment #723591 - Flags: review?(mh+mozilla) → review+
(Assignee)

Comment 31

4 years ago
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.
Attachment #723805 - Flags: review?(gps) → review+
(Assignee)

Comment 32

4 years ago
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.
Attachment #723806 - Flags: review?(gps) → review+
(Assignee)

Comment 33

4 years ago
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.
Attachment #723807 - Flags: review?(gps) → review+
(Reporter)

Comment 34

4 years ago
Created attachment 724003 [details] [diff] [review]
Part 6: Support XPIDL_MODULE in moz.build v2
Attachment #723805 - Attachment is obsolete: true
(Reporter)

Comment 35

4 years ago
Created attachment 724005 [details] [diff] [review]
Part 8: Support XPIDL_FLAGS in moz.build v2
Attachment #723807 - Attachment is obsolete: true
(Reporter)

Comment 36

4 years ago
(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
(Assignee)

Comment 37

4 years ago
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.
Attachment #723809 - Flags: review?(gps) → review+
(Assignee)

Comment 38

4 years ago
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.
(Reporter)

Updated

4 years ago
Attachment #724003 - Flags: review?(gps)
(Reporter)

Updated

4 years ago
Attachment #724005 - Flags: review?(gps)
(Assignee)

Updated

4 years ago
Attachment #724003 - Flags: review?(gps) → review+
(Assignee)

Updated

4 years ago
Attachment #724005 - Flags: review?(gps) → review+
(Assignee)

Comment 39

4 years ago
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
Whiteboard: [leave open]
(Assignee)

Updated

4 years ago
Duplicate of this bug: 698251
(Assignee)

Updated

4 years ago
Blocks: 850380
(Assignee)

Updated

4 years ago
Blocks: 850389
(Assignee)

Comment 41

4 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.