911375 - Merge purge manifests into install manifests

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Gregory Szorc [:gps]]

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.

Comment 1

[Gregory Szorc [:gps]]

Attachment: Part 1: Add support for optional existing files

This should unblock work on moving dist/include to install manifests.

Comment 2

[Mike Hommey [:glandium]]

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?

Comment 3

[Gregory Szorc [:gps]]

Attachment: Part 1: Add support for optional existing files

Merged classes into ExistingFile class, per review request.

Comment 4

[Mike Hommey [:glandium]]

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)

Comment 5

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5298dd852af0

Comment 6

[Gregory Szorc [:gps]]

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 :)

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5298dd852af0

Comment 8

[Gregory Szorc [:gps]]

Arbitrary blocker.

Comment 9

[Gregory Szorc [:gps]]

Attachment: Part 2: Convert uses of PurgeManifest to InstallManifest

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.

Comment 10

[Gregory Szorc [:gps]]

Attachment: Part 3: Remove PurgeManifest

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

Comment 11

[Mike Hommey [:glandium]]

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', ...])

Comment 12

[Gregory Szorc [:gps]]

Incorporated both points of feedback and landed:

https://hg.mozilla.org/projects/build-system/rev/312925464acb
https://hg.mozilla.org/projects/build-system/rev/0608e7abe181

Comment 13

[Gregory Szorc [:gps]]

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

Comment 14

[Gregory Szorc [:gps]]

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

Comment 15

[Gregory Szorc [:gps]]

Attachment: Part 2: Convert uses of PurgeManifest to InstallManifest;

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

Comment 16

[Mike Hommey [:glandium]]

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.

Comment 17

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #16)
> > +export:: install-manifests
> 
> That doesn't match the content

the comment, not the content.

Comment 18

[Gregory Szorc [:gps]]

Attachment: Part 2: Convert uses of PurgeManifest to InstallManifest;

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.

Comment 19

[Gregory Szorc [:gps]]

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 20

[Mike Hommey [:glandium]]

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.

Comment 21

[Gregory Szorc [:gps]]

Parts 2 and 3.

https://hg.mozilla.org/integration/mozilla-inbound/rev/43b40211d0da
https://hg.mozilla.org/integration/mozilla-inbound/rev/b748f8af804c

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/43b40211d0da
https://hg.mozilla.org/mozilla-central/rev/b748f8af804c