Closed Bug 911375 Opened 11 years ago Closed 11 years ago

Merge purge manifests into install manifests


(Firefox Build System :: General, defect)

Not set


(Not tracked)



(Reporter: gps, Assigned: gps)




(3 files, 3 obsolete files)

mozpack has a simple file purger (and corresponding manifest) and an installer (and corresponding manifest) mainly due to history. Purger was implemented first to solve a problem. Install manifests came shortly thereafter.

An install manifest can be made to work like a purge manifest if we add a "optional existing file" type. I plan to do that in this bug. Then I'll kill purge manifests and the FilePurger. I may hold off on the later because I only need the former for moving dist/include to install manifests (bug 896797) and don't want to hold up progress on that bug.
This should unblock work on moving dist/include to install manifests.
Attachment #798061 - Flags: review?(mh+mozilla)
Assignee: nobody → gps
Comment on attachment 798061 [details] [diff] [review]
Part 1: Add support for optional existing files

Review of attachment 798061 [details] [diff] [review]:

::: python/mozbuild/mozpack/
@@ +280,5 @@
>              errors.fatal("Required existing file doesn't exist: %s" %
>                  dest.path)
> +class OptionalExistingFile(BaseFile):

Why not merge OptionalExistingFile and RequiredExistingFile in a single class whose __init__ takes an "optional" bool argument?
Attachment #798061 - Flags: review?(mh+mozilla) → review+
Merged classes into ExistingFile class, per review request.
Attachment #799232 - Flags: review?(mh+mozilla)
Attachment #798061 - Attachment is obsolete: true
Comment on attachment 799232 [details] [diff] [review]
Part 1: Add support for optional existing files

Review of attachment 799232 [details] [diff] [review]:

::: python/mozbuild/mozpack/
@@ +297,5 @@
>                  registry.add(dest, File(entry[1]))
>                  continue
>              if install_type == self.REQUIRED_EXISTS:
> +                registry.add(dest, ExistingFile(True))

required=True, maybe (likewise for other locations)
Attachment #799232 - Flags: review?(mh+mozilla) → review+
Flags: in-testsuite+
Whiteboard: [leave open]
The rest of this bug is suitable for someone wanting to get their hands dirty in the build system.

We have some code in /python/mozbuild/mozpack that is redundant.

In mozpack.copier we have FilePurger. It is a special case of mozpack.copier.FileRegistry whose entries are mozpack.file.ExistingFile instances.

In mozpack.manifest we have PurgeManifest. It is a special case of mozpack.manifests.InstallManifest whose entries, like FilePurger, can be ExistingFile instances.

You need to remove references to PurgeManifest and FilePurger from the tree. Most of this code is in /python/mozbuild. There is a reference to the purge_manifests script in / However, one of the build peers can worry about that if you don't want to touch a make file :)
Whiteboard: [leave open] → [leave open][mentor=gps][lang=python]
No longer blocks: 896797
Arbitrary blocker.
Blocks: 915804
Whiteboard: [leave open][mentor=gps][lang=python] → [leave open]
This patch converts uses of PurgeManifest to InstallManifest. The build
rules in / were updated to facilitate multiple install
manifests being processed concurrently. Before, the purge manifest
Python script launched separate threads to perform concurrent purging.
But with the GIL, it likely wasn't as concurrent as we would like.

This *may* experience bustage during PGO builds. I'm try to remember the
weirdness with MOZ_PROFILE_USE.
Attachment #804022 - Flags: review?(mh+mozilla)
And here we delete all remaining references to PurgeManifest from the
tree. I still retained mozpack.copier.FilePurger because I think that
class is useful because it offers a simpler API over using FileCopier
with optional existing files.
Attachment #804027 - Flags: review?(mh+mozilla)
Comment on attachment 804022 [details] [diff] [review]
Part 2: Convert uses of PurgeManifest to InstallManifest

Review of attachment 804022 [details] [diff] [review]:

@@ +70,5 @@
> +$(foreach m,bin idl include public private sdk,\
> +  $(eval $(call create_install_manifest_rule,dist-$(m),$(DIST)/$(m),dist_$(m))) \
> +)
> +
> +$(eval $(call create_install_manifest_rule,tests,_tests,tests))

I think it would be better without an $(eval). Like (untested):

INSTALL_DIST = bin idl include public private sdk
.PHONY: $(addprefix install-dist-,$(INSTALL_DIST))
$(addprefix install-dist-,$(INSTALL_DIST)): install-dist-%: CLOBBER $(topsrcdir)/configure config.status backend.RecursiveMakeBackend.built
        $(call py_action,process_install_manifest,$(DIST)/$* _build_manifests/install/dist_$* js/src/_build_manifests/install/dist_$*

install-tests: CLOBBER $(topsrcdir)/configure config.status backend.RecursiveMakeBackend.built
        $(call py_action,process_install_manifest,_tests _build_manifests/install/dist_tests js/src/_build_manifests/install/dist_tests

::: python/mozbuild/mozbuild/backend/
@@ +158,1 @@
>          )

Maybe it's time to write this with a list comprehension?
dict((d, InstallManifest()) for d in ['dist_bin', ...])
Attachment #804022 - Flags: review?(mh+mozilla) → review+
Attachment #804027 - Flags: review?(mh+mozilla) → review+
Incorporated both points of feedback and landed:
Whiteboard: [leave open]
Turns out the removed export:: rules were necessary for PGO builds. I guess my try push lied to me. Bustage followup simply removes some lines deleted by part 2.
Backed out last 3 changesets for breaking the build. More science is needed. Will try to get this fixed today.
Whiteboard: [leave open]
I modified the dependencies in I /think/ this is the magic

Attachment #806403 - Flags: review?(mh+mozilla)
Attachment #804022 - Attachment is obsolete: true
Comment on attachment 806403 [details] [diff] [review]
Part 2: Convert uses of PurgeManifest to InstallManifest;

Review of attachment 806403 [details] [diff] [review]:

@@ +84,5 @@
> +# Windows PGO builds don't perform a clean before the 2nd pass. So, we want
> +# to preserve content for the 2nd pass on Windows. Everywhere else, we always
> +# process the install manifests as part of export.
> +export:: install-manifests

That doesn't match the content, in that this doesn't make the 2nd pass do install-manifests on linux, where we *do* maybe_clean_blahblah

@@ +88,5 @@
> +export:: install-manifests
> +else
> +ifneq ($(OS_ARCH)_$(GNU_CC), WINNT_)
> +export:: install-manifests

There are directories where NO_PROFILE_GUIDED_OPTIMIZE is set, independently of whether a pgo build is undergoing or not. You probably still want the dependency there.
Attachment #806403 - Flags: review?(mh+mozilla) → review-
Blocks: 901990
(In reply to Mike Hommey [:glandium] from comment #16)
> > +export:: install-manifests
> That doesn't match the content

the comment, not the content.
I'm not sure what you are looking for here. Maybe you read the patch
wrong? I rewrote the ifdef logic to possibly make it easier to read.

AFAICT this logic is similar to what we had recently. I think I
accidentally changed the logic slightly as part of bug 896797. This
patch attempts to honor the original logic.
Attachment #807531 - Flags: review?(mh+mozilla)
Attachment #806403 - Attachment is obsolete: true
Oh, I also added a "backdoor" to pass --no-remove to the manifest processor. So, now people can type `NO_REMOVE=1 mach build install-manifests` to process installs. This is suitable for partial tree builds.
Comment on attachment 806403 [details] [diff] [review]
Part 2: Convert uses of PurgeManifest to InstallManifest;

Review of attachment 806403 [details] [diff] [review]:

Oh man, i so misread this patch.
Attachment #806403 - Flags: review- → review+
Attachment #807531 - Flags: review?(mh+mozilla) → review+
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Depends on: 921816
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.