884587 - Use packager's file purger for deleting files in dist/

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Gregory Szorc [:gps]]

We currently blow away large chunks of dist/ at the top of the build (https://hg.mozilla.org/mozilla-central/file/default/Makefile.in#l44).

Between moz.build files allowing us to get a whole world view and therefore accounting of every file in the tree and the work in bug 884569, we should be able to selectively delete files from dist/. This should reduce the amount of work for a supposed no-op build and thus make these builds faster.

I think a good place to start is dist/include. We should be able to construct a manifest of the XPIDL_SOURCES and EXPORTS files and purge all remaining. We can bloat scope for more files in dist/ or we can file follow-up bugs.

Comment 1

[Gregory Szorc [:gps]]

For reference, I performed some no-op builds with and without the rm -rf in the root Makefile to measure the impact of restoring dist/ during a no-op build.

With existing deletion:

11228 lines of output (not using make -s)

real	1m18.598s
user	1m50.726s
sys	0m42.256s

Removing the rm -r recipe:

7016 lines of output

real	1m5.299s
user	1m29.595s
sys	0m26.674s

Modest time and resource improvement. Very nice 4200 reduction in lines of output! I'm optimistic the difference will be even more pronounced on pymake and Windows.

Of course, we won't yet achieve the lower numbers because we won't be accounting for every file we currently delete in dist. But we'll get there...

Comment 2

[Mike Hommey [:glandium]]

How do you handle headers from e.g. nspr and nss, that are not listed in moz.build?

Comment 3

[Gregory Szorc [:gps]]

(In reply to Mike Hommey (high latency until June 25) [:glandium] from comment #2)
> How do you handle headers from e.g. nspr and nss, that are not listed in
> moz.build?

We'll have to figure that out. Until then, we'll just purge them (like we currently do).

Comment 4

[Gregory Szorc [:gps]]

Attachment: Part 1: Perform file removal with purge manifests;

In part 1, I simply replaced the rm -r logic in /Makefile.in with Python
that should perform the equivalent.

The build output now contains a summary of what was deleted. e.g.

---
0:13.18 Purging ./dist/bin.
0:13.18 Deleted 2671 files and 214 directories.
0:13.18 Purging ./dist/include.
0:13.18 Deleted 3640 files and 60 directories.
0:13.18 Purging ./dist/private.
0:13.18 Deleted 155 files and 2 directories.
0:13.18 Purging ./dist/public.
0:13.18 Deleted 0 files and 0 directories.
0:13.18 Purging ./dist/sdk.
0:13.18 Deleted 22 files and 2 directories.
0:13.18 Purging ./_tests.
0:13.18 Deleted 13780 files and 514 directories.
---

In part 2, I'll add some content to the manifests.

Comment 5

[Gregory Szorc [:gps]]

Attachment: Part 2: Add EXPORTS files to purge manifest;

I added content from EXPORTS to the dist/include purge manifest.

This doesn't account for NO_DIST_INSTALL being defined. Do we care? If
so, I guess we'll need to track that alongside each exports object.

Given that the most deleted files in dist/ are under _tests/, I think
I'll go there next.

Comment 6

[Gregory Szorc [:gps]]

11000+ of the files in _tests are mochitests. Bug 868158 can't come soon enough.

Comment 7

[Gregory Szorc [:gps]]

So with EXPORTS and xpcshell manifest parsing, I've got this down to:

9773 lines of output

real	1m16.063s
user	1m46.040s
sys	0m37.851s

I don't think CPU time is decreasing as much as I hoped because we're trading rm -r (fast) for single-threaded Python (likely slower). We could look into using multiple I/O threads to perform the unlink and rmdir operations...

It would be worth measuring things on windows/pymake...

Comment 8

[Gregory Szorc [:gps]]

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

Comment 9

[Gregory Szorc [:gps]]

Attachment: Part 1: Perform file removal with purge manifests;

Made some improvements to the implementation:

* backend class now has a dict of purge manifests that can easily be
  added to to define new purge manifests.
* added tests
* docs in Makefile.in on what's going on
* purge_manifests.py now creates separate threads for processing each
  manifest so operations can be performed in parallel. This is a very
  hacky way to accomplish parallel processing, but it does work.

Comment 10

[Gregory Szorc [:gps]]

Attachment: Part 2: Add EXPORTS files to purge manifest;

Rebased on top of updated part 1. And I added a crude test!

Comment 11

[Gregory Szorc [:gps]]

Random thought: we should duplicate the functionality in these patches. But instead of purging, it will manage file installation. The build backend will just write out a manifest (or set of manifests) detailing file copies/symlinks/preprocessing. At the top of a build, we process the purge manifests, followed by simple file "installs." This would replace the bulk of the "export" tier as well as large swaths of libs (JSM install, test install, etc).

Ted: What are your thoughts?

Comment 12

[Mike Hommey [:glandium]]

Comment on attachment 766085 [details] [diff] [review]
Part 1: Perform file removal with purge manifests;

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

Only looked quickly.

::: config/purge_directories.py
@@ +60,5 @@
> +
> +    print('Purging unaccounted files from object directory...')
> +
> +    # We perform purging using threads for performance reasons. Hopefully
> +    # multiple I/O operations will be faster than just 1.

Does the gil actually allow multiple I/O to happen at the same time? If not, that's pointless. The reliable way to do parallel stuff in python is multiprocess.

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +134,5 @@
> +            dist_include=('dist/include', set()),
> +            dist_private=('dist/private', set()),
> +            dist_public=('dist/public', set()),
> +            dist_sdk=('dist/sdk', set()),
> +            tests=('_tests', set()),

it doesn't sound great to hardcode this here.

@@ +370,5 @@
> +            fh = FileAvoidWrite(os.path.join(man_dir, k))
> +            fh.write('1\n')
> +            fh.write('%s\n' % relpath)
> +            for entry in sorted(entries):
> +                fh.write('%s\n' % entry)

It would be better to have a separate class for the manifest reading/writing. It would make this code clearer and would also make it clearer to handle format changes in one place.

Comment 13

[Gregory Szorc [:gps]]

(In reply to Mike Hommey (high latency until June 25) [:glandium] from comment #12)
> Does the gil actually allow multiple I/O to happen at the same time? If not,
> that's pointless. The reliable way to do parallel stuff in python is
> multiprocess.

I know the GIL is released with I/O in Python 3.x and I'm pretty sure it is released in 2.7. A few weeks ago we verified the GIL was released with print() in 2.7. I could go source diving...

Comment 14

[Gregory Szorc [:gps]]

Attachment: Part 1: Perform file removal with purge manifests;

I moved the serialization logic into its own class. I put this class in
mozpack (not mozbuild) so it can live closer to the FilePurger class. I
didn't add the functionality to FilePurger itself because the existing
API maintains separation between manifests and the FileRegistry family
of classes.

I also added -d to purge_directories.py to avoid a $(wildcard) in
Makefile.in. It's also a generally useful feature, IMO.

The one comment I didn't address was hardcoding the to-be-purged
directories in recursivemake.py. I'm not sure how else you'd have me do
this!

Comment 15

[Gregory Szorc [:gps]]

Attachment: Part 2: Add EXPORTS files to purge manifest;

Rebased on latest part 1. Code is much simpler now!

Comment 16

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 766330 [details] [diff] [review]
Part 1: Perform file removal with purge manifests;

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

::: Makefile.in
@@ +54,5 @@
> +# a common directory. Each manifest is responsible for defining files in
> +# a specific subdirectory of the object directory. The invoked Python
> +# script simply iterates over all the manifests, purging files as
> +# necessary. To manage new directories or add files to the manifests,
> +# since the backend generator.

Do what, now?

Comment 17

[Gregory Szorc [:gps]]

Attachment: Part 1: Perform file removal with purge manifests;

Fixed Ms2ger-identified typo in comments.

Comment 18

[Mike Hommey [:glandium]]

Comment on attachment 766352 [details] [diff] [review]
Part 1: Perform file removal with purge manifests;

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

The name "purge manifest" is awkward, as the content of the file is a list of files that are *not* going to be purged. I thought i had commented to that effect on the FilePurger class name too, but it looks like i didn't... (or I can't find my comment)

As noted in bug 850380 comment 37, this might not be the right solution anyways, depending on what functionality you really want to provide.

::: python/mozbuild/mozpack/manifests.py
@@ +77,5 @@
> +        """
> +        p = FilePurger()
> +        for entry in self.entries:
> +            if prepend_relpath:
> +                entry = '%s/%s' % (self.relpath, entry)

mozpack.path.join

Comment 19

[Gregory Szorc [:gps]]

I think long term we'll want to use FileCopier for everything and replace the up-front file purging in this patch with file copying. However, getting there will require a bit of work and is beyond the scope of this bug. I think this patch represents a step in the right direction.

Regarding the name "purge manifest," I can't come up with anything better. I added an additional comment in the class's docstring clarifying this. I also added a new tier to the output directory. Manifests are now stored in _build_manifests/purge.

Comment 20

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8d90527c22c6
https://hg.mozilla.org/integration/mozilla-inbound/rev/447ff64adbb1

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Backed out for checktest failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff579fc118dd

https://tbpl.mozilla.org/php/getParsedLog.php?id=24577647&tree=Mozilla-Inbound

Comment 22

[Gregory Szorc [:gps]]

Gah. This is due to some inconsistency with |make check|'s environment on automation differing from the local environment. This wasn't caught by the earlier try push because that push hadn't implemented tests. I've ran into this before and worked around it by disabling tests. Let me dig into things...

Comment 23

[Gregory Szorc [:gps]]

No, I'm wrong: this was just me being stupid (I should have checked this first). I never ran tests on part 2 before pushing. I was able to reproduce the test failure locally. I applied a trivial patch to fix it.

https://hg.mozilla.org/integration/mozilla-inbound/rev/796961a384b4
https://hg.mozilla.org/integration/mozilla-inbound/rev/0244a34bc419

Comment 24

[Gregory Szorc [:gps]]

https://tbpl.mozilla.org/php/getParsedLog.php?id=24581226&tree=Mozilla-Inbound&full=1

e:\builds\moz2_slave\m-in-w32-000000000000000000000\build\obj-firefox\Makefile:63:0$ e:/builds/moz2_slave/m-in-w32-000000000000000000000/build/obj-firefox/_virtualenv/Scripts/python.exe e:/builds/moz2_slave/m-in-w32-000000000000000000000/build/config/purge_directories.py -d _build_manifests/purge .
Purging unaccounted files from object directory...
Deleted 3350 files and 255 directories from dist/bin.
Exception in thread Thread-3:
Traceback (most recent call last):
  File "c:\mozilla-build\python27\Lib\threading.py", line 551, in __bootstrap_inner
    self.run()
  File "c:\mozilla-build\python27\Lib\threading.py", line 504, in run
    self.__target(*self.__args, **self.__kwargs)
  File "e:/builds/moz2_slave/m-in-w32-000000000000000000000/build/config/purge_directories.py", line 18, in do_purge
    state['result'] = purger.purge(dest)
  File "e:\builds\moz2_slave\m-in-w32-000000000000000000000\build\python\mozbuild\mozpack\copier.py", line 206, in purge
    return FileCopier.copy(self, dest)
  File "e:\builds\moz2_slave\m-in-w32-000000000000000000000\build\python\mozbuild\mozpack\copier.py", line 171, in copy
    os.remove(f)
WindowsError: [Error 5] Access is denied: 'dist\\private\\nss\\ocspi.h'

Exception in thread Thread-2:
Traceback (most recent call last):
  File "c:\mozilla-build\python27\Lib\threading.py", line 551, in __bootstrap_inner
    self.run()
  File "c:\mozilla-build\python27\Lib\threading.py", line 504, in run
    self.__target(*self.__args, **self.__kwargs)
  File "e:/builds/moz2_slave/m-in-w32-000000000000000000000/build/config/purge_directories.py", line 18, in do_purge
    state['result'] = purger.purge(dest)
  File "e:\builds\moz2_slave\m-in-w32-000000000000000000000\build\python\mozbuild\mozpack\copier.py", line 206, in purge
    return FileCopier.copy(self, dest)
  File "e:\builds\moz2_slave\m-in-w32-000000000000000000000\build\python\mozbuild\mozpack\copier.py", line 171, in copy
    os.remove(f)
WindowsError: [Error 5] Access is denied: 'dist\\include\\nss\\nsslocks.h'

Traceback (most recent call last):
  File "e:/builds/moz2_slave/m-in-w32-000000000000000000000/build/config/purge_directories.py", line 70, in <module>
    state['result'].removed_files_count,
AttributeError: 'NoneType' object has no attribute 'removed_files_count'
e:\builds\moz2_slave\m-in-w32-000000000000000000000\build\obj-firefox\Makefile:63:0: command 'e:/builds/moz2_slave/m-in-w32-000000000000000000000/build/obj-firefox/_virtualenv/Scripts/python.exe e:/builds/moz2_slave/m-in-w32-000000000000000000000/build/config/purge_directories.py -d _build_manifests/purge .' failed, return code 1
e:\builds\moz2_slave\m-in-w32-000000000000000000000\build\client.mk:372:0: command 'C:/mozilla-build/python27/python.exe e:/builds/moz2_slave/m-in-w32-000000000000000000000/build/build/pymake/pymake/../make.py -j4 -C obj-firefox' failed, return code 2
e:\builds\moz2_slave\m-in-w32-000000000000000000000\build\client.mk:172:0: command 'C:/mozilla-build/python27/python.exe e:/builds/moz2_slave/m-in-w32-000000000000000000000/build/build/pymake/pymake/../make.py -f e:/builds/moz2_slave/m-in-w32-000000000000000000000/build/client.mk realbuild' failed, return code 2
program finished with exit code 2
elapsedTime=56.683000
========= Finished compile failed (results: 2, elapsed: 56 secs) (at 2013-06-25 13:23:37.948129) =========


I don't even...

Comment 25

[(no longer active)]

Backed out because of build bustage on Windows: <https://hg.mozilla.org/integration/mozilla-inbound/rev/301c184ae8e7>

Comment 26

[Gregory Szorc [:gps]]

This initially flabbergasted me because the initial landing didn't break. However, the initial landing seemed to be a clobber build, so the file purger wasn't actually unlinking anything. On the second landing, it was actually doing work.

This is likely a deficiency in mozpack.copier attempting unlink/remove without regard for file permissions. I'm guessing the nss files it's failing on aren't allowed to be removed.

We should teach mozpack about the robust file removal logic (as implemented by mozfile's rmtree as elsewhere in the tree).

Comment 27

[Gregory Szorc [:gps]]

Attachment: Part 0: Teach FileCopier how to remove unwritable files on Windows

I /think/ this will fix things. Someone should probably verify on a Windows
machine just to be sure.

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

Comment 28

[Mike Hommey [:glandium]]

Comment on attachment 768113 [details] [diff] [review]
Part 0: Teach FileCopier how to remove unwritable files on Windows

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

::: python/mozbuild/mozpack/copier.py
@@ +171,5 @@
> +            # Windows requires write access to remove files.
> +            if os.name == 'nt' and not os.access(f, os.W_OK):
> +                # It doesn't matter what we set the permissions to since we
> +                # will remove this file shortly.
> +                os.chmod(f, 0700)

might as well make that 0600

::: python/mozbuild/mozpack/test/test_copier.py
@@ +133,5 @@
>          self.assertEqual(self.all_dirs(self.tmpdir), set(['qux']))
>  
> +    def test_not_writable_delete(self):
> +        """Ensure files without write permission can be deleted."""
> +        with open(os.path.join(self.tmpdir, 'dummy'), 'a'):

you can use self.tmppath('dummy')

@@ +141,5 @@
> +        with open(p, 'a'):
> +            pass
> +
> +        os.chmod(p, 0400)
> +

i'd also test with os.chmod(self.tmpdir, 0400), which makes the directory not writable, that could break unix systems ; and make sure it doesn't.

Comment 29

[Gregory Szorc [:gps]]

Attachment: Part 0: Teach FileCopier how to remove unwritable files on Windows

If we're going to do this, I want to do it right.

Patch now handles directories with bad permissions.

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

Comment 30

[Mike Hommey [:glandium]]

Comment on attachment 768620 [details] [diff] [review]
Part 0: Teach FileCopier how to remove unwritable files on Windows

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

::: python/mozbuild/mozpack/copier.py
@@ +28,5 @@
>          if error.errno != errno.EEXIST:
>              raise
>  
> +    if not os.access(dir, os.W_OK):
> +        os.chmod(dir, 0777)

huh, certainly not 0777. chmod doesn't follow umask.
umask = os.umask(0077)
os.umask(umask)
os.chmod(dir, 0777 & ~umask)

Comment 31

[Gregory Szorc [:gps]]

With latest review nit. Plus, I swapped out shuti.rmtree for mozfile.rmtree in the tests to squash a Windows make check failure.

https://hg.mozilla.org/integration/mozilla-inbound/rev/927424460689
https://hg.mozilla.org/integration/mozilla-inbound/rev/b02a95fdd518
https://hg.mozilla.org/integration/mozilla-inbound/rev/9fc940c4a6dd

Comment 32

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/927424460689
https://hg.mozilla.org/mozilla-central/rev/b02a95fdd518
https://hg.mozilla.org/mozilla-central/rev/9fc940c4a6dd