Merge purge manifests into install manifests

RESOLVED FIXED in mozilla27

Status

defect
RESOLVED FIXED
6 years ago
Last year

People

(Reporter: gps, Assigned: gps)

Tracking

Trunk
mozilla27
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

Assignee

Description

6 years ago
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.
Assignee

Comment 1

6 years ago
This should unblock work on moving dist/include to install manifests.
Attachment #798061 - Flags: review?(mh+mozilla)
Assignee

Updated

6 years ago
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/files.py
@@ +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+
Assignee

Comment 3

6 years ago
Merged classes into ExistingFile class, per review request.
Attachment #799232 - Flags: review?(mh+mozilla)
Assignee

Updated

6 years ago
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/manifests.py
@@ +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+
Assignee

Comment 5

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/5298dd852af0
Status: NEW → ASSIGNED
Flags: in-testsuite+
Assignee

Updated

6 years ago
Whiteboard: [leave open]
Assignee

Comment 6

6 years ago
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 /Makefile.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]
Assignee

Updated

6 years ago
No longer blocks: 896797
Assignee

Comment 8

6 years ago
Arbitrary blocker.
Blocks: 915804
Assignee

Updated

6 years ago
Whiteboard: [leave open][mentor=gps][lang=python] → [leave open]
Assignee

Comment 9

6 years ago
This patch converts uses of PurgeManifest to InstallManifest. The build
rules in /Makefile.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)
Assignee

Comment 10

6 years ago
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.

https://tbpl.mozilla.org/?tree=Try&rev=d7e5d8c1d606
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]:
-----------------------------------------------------------------

::: Makefile.in
@@ +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/recursivemake.py
@@ +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+
Assignee

Comment 13

6 years ago
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.

https://hg.mozilla.org/projects/build-system/rev/f24d5b2801f8
Assignee

Comment 14

6 years ago
Backed out last 3 changesets for breaking the build. More science is needed. Will try to get this fixed today.

https://hg.mozilla.org/projects/build-system/rev/00a1d4006f2b
Whiteboard: [leave open]
Assignee

Comment 15

6 years ago
I modified the dependencies in Makefile.in. I /think/ this is the magic
combination.

Normal: https://tbpl.mozilla.org/?tree=Try&rev=151979dc0620
PGO: https://tbpl.mozilla.org/?tree=Try&rev=73de99d3e694
Attachment #806403 - Flags: review?(mh+mozilla)
Assignee

Updated

6 years ago
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]:
-----------------------------------------------------------------

::: Makefile.in
@@ +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.
> +ifndef MOZ_PROFILE_USE
> +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_)
> +ifndef NO_PROFILE_GUIDED_OPTIMIZE
> +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-
Assignee

Updated

6 years ago
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.
Assignee

Comment 18

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

Updated

6 years ago
Attachment #806403 - Attachment is obsolete: true
Assignee

Comment 19

6 years ago
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+
https://hg.mozilla.org/mozilla-central/rev/43b40211d0da
https://hg.mozilla.org/mozilla-central/rev/b748f8af804c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Depends on: 921816

Updated

Last year
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.