Closed Bug 896797 Opened 7 years ago Closed 7 years ago

Use install manifests for EXPORTS

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla26

People

(Reporter: gps, Assigned: gps)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 5 obsolete files)

+++ This bug was initially created as a clone of Bug #890097 +++

In bug 890097 we are adding install manifests to the build system. We should hook EXPORTS up to them.
Depends on: 899241
Depends on: 900569
I've had a patch for this bug sitting in my patch queue for a while. However, there are some dependency issues.

Install manifests prune unknown files when applied. That's a core feature of install manifests and is necessary behavior.

To make install manifests play well with dist/include, we need to process the manifest early in the build. This is because "external" build systems install files into dist/include which moz.build files have no knowledge of. Since we don't have knowledge of these files, we can't exclude them from deletion. If we delay manifest processing, external build systems may have already installed files and manifest processing will prune these files.

A further complication is js/src. js/src has its own moz.build tree and set of installed files. We can merge the js manifest with the main manifest to create a union of installed files. All is well. However, the js/src manifest could be regenerated if a moz.build in js/src changes. This may result in files in dist/include not being pruned if they lost their reference in the new manifest.

Yet another complication is that processing install manifests on leaf directory builds will not be safe. If we process the install manifest from a leaf Makefile, we'll prune 3rd party headers since we won't invoke their Makefiles. Derp.

I'm not sure how we want to tackle this. We could certainly hook up the root Makefile to ensure js/src/RecursiveMakeBackend.built is up to date before processing install manifests. However, we still have the issue of untracked files installed from external build systems, especially their interaction with leaf Makefile builds.

Anyone have any suggestions?
Random idea: dist/include-tracked and dist/include-untracked?
As mentioned on irc, things would just be simpler if external build systems installed in a subdir of dist/include (or to some completely different directory, for that matter)
Depends on: 905691
We need to ensure js/src's moz.build backend is up to date before
processing install manifests because we merge the install manifests
produced by the main moz.build with js/src's manifests.
Attachment #794301 - Flags: review?(mh+mozilla)
Assignee: nobody → gps
Latest version of patch. Appears to work for me just fine! Not ready for
review because I got lazy with Makefile.in changes and started using
$(INSTALL). I'm also pretty sure I broke some tests.
Comment on attachment 794301 [details] [diff] [review]
Part 1: Ensure js/src moz.build is up to date before building

Review of attachment 794301 [details] [diff] [review]:
-----------------------------------------------------------------

::: Makefile.in
@@ +63,5 @@
>  	$(call py_action,purge_manifests,-d _build_manifests/purge .)
>  endif
>  
> +default alldep all:: CLOBBER $(topsrcdir)/configure config.status backend.RecursiveMakeBackend.built
> +	$(call SUBMAKE,backend.RecursiveMakeBackend.built,js/src,1)

Looks like this could just go in the rule above.
Attachment #794301 - Flags: review?(mh+mozilla) → review+
Whiteboard: [leave open]
This patch is a bit hacky and makes me feel a bit dirty. But, it will
enable dist/include to be managed by install manifests with minimal
effort.

Since in our brave new install manifest world files managed by the
install manifest need to be present when the install manifest executes,
this means auto-generated .h files can't be present in the main install
manifest. This means they can't be in EXPORTS.

This patch removes all auto-generated .h files from EXPORTS and forbids
non-existing .h files to be defined in EXPORTS.

In the long run, this patch is "reverted" by employing one or more of
the following:

a) Having CONFIGURE_SUBST_FILES define an explicit output directory in
$(DIST) and having the install manifest be aware of these files.

b) Having a generic "codegen" rule with output files that are
automatically registered in the install manifest.

c) Having a more sane way to copy auto-generated files into $(DIST) and
making the install manifests aware of these.

The solution for install manifests in this bug will be far from perfect
(it won't know about "external" includes for one). So, I'm content to
unhack this patch through a series of followups. The overall good of
managing $(DIST)/include with an install manifest should be worth the
temporary setback in "purity."

This patch currently fails for Windows, Android, and possibly B2G due to
not converting all EXPORTS to the new strictness. I'll post a r2 patch
shortly. Just figured I'd put this up for comment.
Attachment #797404 - Flags: review?(mh+mozilla)
This version will hopefully unbust Windows and Android. I feel even
dirtier for writing this since it reverts more conversion work :/

https://tbpl.mozilla.org/?tree=Try&rev=87ab03f740be
Attachment #797413 - Flags: review?(mh+mozilla)
Attachment #797404 - Attachment is obsolete: true
Attachment #797404 - Flags: review?(mh+mozilla)
Depends on: 911362
Depends on: 911375
This puts dist/include under install manifest control. Previously, it
was under the control of a purge manifest.

I still need to remove the old rules from rules.mk. I'm still playing
with that locally to ensure nothing blows up. I'll submit that as an
additional patch. But this one should be ready for review.
Attachment #798068 - Flags: review?(mh+mozilla)
Attachment #794302 - Attachment is obsolete: true
Hopefully fixed all the issues causing try bustage.

https://tbpl.mozilla.org/?tree=Try&rev=c6ea91cceedc

(includes part 3)
Attachment #798071 - Flags: review?(mh+mozilla)
Attachment #797413 - Attachment is obsolete: true
Attachment #797413 - Flags: review?(mh+mozilla)
Oh, with this patch applied, we remove 600-700 files from dist_include. We're missing nspr, nss, webidl, and a handful of autogenerated files. The majority are webidl header files. Will need to convert dom/webidl/WebIDL.mk to moz.build or process that file as part of moz.build. But that's for another bug.
Part 3 broke standalone SM builds, presumably because SM doesn't know it needs to process an install manifest.
We can't remove EXPORTS from rules.mk until mfbt/exported_headers.mk is moved to moz.build (bug 870401) I'm tempted to punt to a followup.
Hopefully fixed SM builds.

https://tbpl.mozilla.org/?tree=Try&rev=38d068fb2791
Attachment #798082 - Flags: review?(mh+mozilla)
Attachment #798068 - Attachment is obsolete: true
Attachment #798068 - Flags: review?(mh+mozilla)
Comment on attachment 798071 [details] [diff] [review]
Part 2: Don't list autogenerated files in EXPORTS

Review of attachment 798071 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/public/ia2/Makefile.in
@@ +67,5 @@
>  
>  EMBED_MANIFEST_AT = 2
>  
> +INSTALL_TARGETS += midl
> +midl_FILES := $(filter %.h,$(MIDL_GENERATED_FILES)) $(filter %._i.c,$(MIDL_GENERATED_FILES))

And now comes the ¢2 question: why the hell are we installing .c files in dist/include?

::: gfx/layers/moz.build
@@ +143,5 @@
>      'ipc/TaskThrottler.h',
>      'opengl/CompositingRenderTargetOGL.h',
>      'opengl/CompositorOGL.h',
> +    'opengl/GrallocTextureClient.h',
> +    'opengl/GrallocTextureHost.h',

heh, VPATH. I wish we were done with that already.

::: layout/style/Makefile.in
@@ +32,5 @@
>  		-I$(srcdir)/../../content/xbl/src \
>  		-I$(srcdir)/../../content/xul/document/src \
>  		$(NULL)
>  
> +nsStyleStructList.h: $(srcdir)/generate-stylestructlist.py

I'd rather not touch this. This makes things harder to hg blame.
Attachment #798071 - Flags: review?(mh+mozilla) → review+
Comment on attachment 798082 [details] [diff] [review]
Part 3: Use install manifests for managing dist/include

Review of attachment 798082 [details] [diff] [review]:
-----------------------------------------------------------------

::: Makefile.in
@@ +214,5 @@
>  endif
> +
> +# Convenience target to process dist_include installation manifest.
> +# This was added to make partial tree builds less painful. When partial tree
> +# builds are no longer supported, this can be removed.

FWIW, I don't think we should remove partial tree builds, *but* i agree we can, in the long run, unsupport the expectation that a partial tree build does the "right" thing about idls, headers, etc.

@@ +217,5 @@
> +# This was added to make partial tree builds less painful. When partial tree
> +# builds are no longer supported, this can be removed.
> +.PHONY: dist_include
> +dist_include:
> +	$(call py_action,process_install_manifest,--no-remove $(DIST)/include _build_manifests/install/dist_include js/src/_build_manifests/install/dist_include)

What is triggering this rule?

::: js/src/Makefile.in
@@ +176,5 @@
>  MOZILLA_DTRACE_SRC = $(srcdir)/devtools/javascript-trace.d
>  endif
>  
>  default::
> +	$(call py_action,process_install_manifest,--no-remove $(DIST)/include _build_manifests/install/dist_include)

That's going to leave cruft in incremental standalone js builds.

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +150,5 @@
>              'config', 'autoconf.mk'))
>  
> +        self._install_manifests = dict(
> +            dist_include=InstallManifest(),
> +        )

self._install_manifests is recreated from scratch a few lines below.

::: python/mozbuild/mozbuild/frontend/emitter.py
@@ +70,5 @@
>  
>          This is a generator of mozbuild.frontend.data.SandboxDerived instances.
>          """
>  
> +        is_distribution = not sandbox.get('NO_DIST_INCLUDE')

i don't have a better suggestion, but "is_distribution" doesn't really ring a bell to me. Also, what is setting NO_DIST_INCLUDE?
Attachment #798082 - Flags: review?(mh+mozilla) → review-
(In reply to Mike Hommey [:glandium] from comment #19)
> Comment on attachment 798082 [details] [diff] [review]
> Part 3: Use install manifests for managing dist/include
> 
> Review of attachment 798082 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: Makefile.in
> @@ +214,5 @@
> >  endif
> > +
> > +# Convenience target to process dist_include installation manifest.
> > +# This was added to make partial tree builds less painful. When partial tree
> > +# builds are no longer supported, this can be removed.
> 
> FWIW, I don't think we should remove partial tree builds, *but* i agree we
> can, in the long run, unsupport the expectation that a partial tree build
> does the "right" thing about idls, headers, etc.
> 
> @@ +217,5 @@
> > +# This was added to make partial tree builds less painful. When partial tree
> > +# builds are no longer supported, this can be removed.
> > +.PHONY: dist_include
> > +dist_include:
> > +	$(call py_action,process_install_manifest,--no-remove $(DIST)/include _build_manifests/install/dist_include js/src/_build_manifests/install/dist_include)
> 
> What is triggering this rule?

Nothing. I initially tried to get the NONRECURSIVE rules to work. This didn't work due to inability to implement a "run once" functionality because we process child dir traversal before our own targets. I'll just remove this rule. Less bloat.

> ::: python/mozbuild/mozbuild/backend/recursivemake.py
> @@ +150,5 @@
> >              'config', 'autoconf.mk'))
> >  
> > +        self._install_manifests = dict(
> > +            dist_include=InstallManifest(),
> > +        )
> 
> self._install_manifests is recreated from scratch a few lines below.

Gah - too much rebasing and patch conflicts.

> ::: python/mozbuild/mozbuild/frontend/emitter.py
> @@ +70,5 @@
> >  
> >          This is a generator of mozbuild.frontend.data.SandboxDerived instances.
> >          """
> >  
> > +        is_distribution = not sandbox.get('NO_DIST_INCLUDE')
> 
> i don't have a better suggestion, but "is_distribution" doesn't really ring
> a bell to me. Also, what is setting NO_DIST_INCLUDE?

Nothing in my OS X build is setting NO_DIST_INCLUDE and EXPORTS. But I'm not sure this is globally true. I figure we can leave it in (with a comment saying it may not be necessary) and can remove it later once we've validated it isn't necessary.
Incorporated review feedback.
Attachment #799250 - Flags: review?(mh+mozilla)
Attachment #798082 - Attachment is obsolete: true
Comment on attachment 799250 [details] [diff] [review]
Part 3: Use install manifests for managing dist/include

Review of attachment 799250 [details] [diff] [review]:
-----------------------------------------------------------------

::: Makefile.in
@@ +217,5 @@
> +# This was added to make partial tree builds less painful. When partial tree
> +# builds are no longer supported, this can be removed.
> +.PHONY: dist_include
> +dist_include:
> +	$(call py_action,process_install_manifest,--no-remove $(DIST)/include _build_manifests/install/dist_include js/src/_build_manifests/install/dist_include)

You didn't remove this rule as per comment 21.
Attachment #799250 - Flags: review?(mh+mozilla) → review+
(In reply to Phil Ringnalda (:philor) from comment #24)
> Backed out part 2 in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/49c8cb0df337 for the
> Windows a11y bustage in
> https://tbpl.mozilla.org/php/getParsedLog.php?id=27350392&tree=Mozilla-
> Inbound

Hmmm. Not sure what's going on there. I had good try runs with this code before. And my local Windows build is working. I'm going to push parts 2 and 3 together once my local build completes and the try shows signs of life. If we still go red and try is happy, then it's clobber time :/
Oh, my local build was able to repro. Hmmm. What changed...
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae9f95fc1136
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce03cc2994aa

There was a dumb typo in part 2: %._i.c instead of %_i.c.
Status: NEW → ASSIGNED
Whiteboard: [leave open]
Some days, it just doesn't pay to push until after I've gone to bed.

Backed out part 2 and part 3 in https://hg.mozilla.org/integration/mozilla-inbound/rev/28c308fbc854 for SpiderMonkey shell build bustage like https://tbpl.mozilla.org/php/getParsedLog.php?id=27354345&tree=Mozilla-Inbound
Part 3 caused the bustage, so relanded part 2.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3cf5058f6296
Whiteboard: [leave open]
Depends on: 912359
https://hg.mozilla.org/integration/mozilla-inbound/rev/8090e534656a

With the purging logic not conditionally removing files based on standalone build flag. glandium wanted this in because it's better. However, since it broke some tbpl builders and since it isn't a regression in build system behavior (SM's standalone build doesn't currently handle removed header files), it was decided this was OK to land. Bug 912359 tracks moving the build logic for standalone builds into the tree so we can have more control over this.
Whiteboard: [leave open]
No longer depends on: 912359
Backed out part 3 (only), on suspicion of causing bug 912451:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1faf7273eaae
Whiteboard: [leave open]
Depends on: 912451
(In reply to Ed Morley [:edmorley UTC+1] from comment #31)
> on suspicion of causing bug 912451:

confirmed
I, uh, have no clue what's going on here.

Build works fine in the first pass. mozmemory_wrap.c compiles fine, which presumably means jemalloc_types.h is getting installed. On the 2nd pass, we don't invoke the install manifest. However, we shouldn't need to: I see no record of dist/include being pruned.

I'll keep looking.
I'm able to reproduce this locally on OS X. dist/include is getting cleaned as part of the |make clean|. We're not restoring files because the install manifest processing is inside an ifndef MOZ_PROFILE_USE. Will need to rejiggle /Makefile a bit.
Status: ASSIGNED → NEW
https://hg.mozilla.org/integration/mozilla-inbound/rev/743278b7340f

Moved the processing of the install manifest to outside the MOZ_PROFILE_USE. This was a one line change that caused my local PGO build on OS X to start working and was IMO trivial enough to not require a review.
Whiteboard: [leave open]
No longer depends on: 905691, 911375
https://hg.mozilla.org/mozilla-central/rev/3cf5058f6296
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Depends on: 912795
Depends on: 921770
Depends on: 921822
Duplicate of this bug: 874711
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.