Open Bug 1270229 Opened 4 years ago Updated 2 years ago

Refactor xpidl directory traversal

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(Not tracked)

People

(Reporter: gps, Unassigned)

References

(Blocks 2 open bugs)

Details

Attachments

(3 files)

Everything related to XPIDL directory traversal is horrible. As part of working on bug 1253110 XPIDL make foo is causing me excessive pain. So I'm just going to inject a little more sanity into the chaos.
SUBMAKE is somewhat evil. Eliminate the SUBMAKE for generating
idl-parser files by properly adding a directory traversal dependency
to ensure the idl-parser files are generated before processing XPIDL
files.

This possibly also eliminates a race condition where
xpcom/idl-parser/xpidl:export could get processes at the same time.

Review commit: https://reviewboard.mozilla.org/r/50537/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50537/
Attachment #8748809 - Flags: review?(mh+mozilla)
Attachment #8748810 - Flags: review?(mh+mozilla)
Attachment #8748811 - Flags: review?(mh+mozilla)
Currently, we process the dist/idl install manifest both in the
top-level Makefile.in and inside xpcom/xpidl/Makefile.in:export.

I'm intent on removing the redundancy and on removing
xpcom/xpidl/Makefile. In preparation for this, move the install manifest
processing invocation to config/makefiles/xpidl/Makefile.in.

This required a new make target and a make reinvocation because make
sucks. This is the same as before, just all inside the same Makefile.in
now.

Review commit: https://reviewboard.mozilla.org/r/50539/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50539/
Before this commit, both the root Makefile and
config/makefiles/xpidl/Makefile were processing the dist/idl install
manifest. This was redundant.

This patches eliminates the redundancy by removing the dist/idl install
manifest processing in the root Makefile. A nice benefit of this is some
one-off hacks in the faster backend have been eliminated.

Review commit: https://reviewboard.mozilla.org/r/50541/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50541/
Assignee: nobody → gps
Status: NEW → ASSIGNED
Comment on attachment 8748809 [details]
MozReview Request: Bug 1270229 - Add directory dependency on idl-parser; r?glandium

https://reviewboard.mozilla.org/r/50537/#review48339
Attachment #8748809 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8748810 [details]
MozReview Request: Bug 1270229 - Process dist/idl manifest in actual xpidl Makefile; r?glandium

https://reviewboard.mozilla.org/r/50539/#review48347

::: config/makefiles/xpidl/Makefile.in:79
(Diff revision 1)
>  
> -xpidl:: $(xpt_files) $(chrome_manifests) $(interfaces_manifests)
> +# We can't have the install manifest processing as an actual dependency of
> +# the xpts because it is .PHONY and would cause the target to always be out
> +# of date. So we have make reinvoke itself with another rule.
> +.PHONY: xpidl
> +xpidl::

I'd rather you didn't touch top-level Makefile, and "simply" remove the manual install manifest processing from xpcom/xpidl.

Why? Because:
- your change is going to break mach build faster
- on a recursive build, the dist_idl install manifest processing happens during pre-export, so everything in the export tier is guaranteed to happen after
- on a mach build faster build, we have the right dependencies making the dist_idl install manifest processed before xpidl (and xpcom/xpidl is happily ignored)
- the only thing that actually needs this manual dist_idl install manifest processing is the NONRECURSIVE_TARGETS processing. And the dependency rules for that could be changed so that the rules from the top-level Makefile are used instead of those from xpcom/xpidl:

NONRECURSIVE_TARGETS_export_xpidl_DIRECTORY = $(DEPTH)
NONRECURSIVE_TARGETS_export_xpidl_TARGETS = install-dist_idl
Attachment #8748810 - Flags: review?(mh+mozilla)
Comment on attachment 8748811 [details]
MozReview Request: Bug 1270229 - Remove dist/idl install manifest processing from root Makefile; r?glandium

https://reviewboard.mozilla.org/r/50541/#review48349

::: config/faster/rules.mk:101
(Diff revision 1)
> -# The xpidl target in config/makefiles/xpidl requires the install manifest for
> -# dist/idl to have been processed. When using the hybrid
> -# FasterMake/RecursiveMake backend, this dependency is handled in the top-level
> +# The xpidl target in config/makefiles/xpidl all the install manifests for
> +# dist/bin to have been processed because it adds interfaces.manifest
> +# references with buildlist.py.

This breaks running mach build faster when it's not part of artifact builds.
Attachment #8748811 - Flags: review?(mh+mozilla)
I'm not actively working on this.
Assignee: gps → nobody
Status: ASSIGNED → NEW
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.