935987 - Add preprocessor support to install manifests

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Brian O'Keefe [:bokeefe]]

Attachment: Add preprocessed files to install manifests

I was trying to fix bug 772828 by moving all of the things that go into dist/bin/res into mozbuild, but it turns out one file needs to go through the preprocessor first. I didn't want to leave that one file in a Makefile.in, so I wired up install manifests to invoke the preprocessor.

Try wasn't terribly happy on Windows when the preprocessor was still in /config, so I had to move it first, but I'll put that patch in bug 924343.

The green try run: https://tbpl.mozilla.org/?tree=Try&rev=63ec57f73dbb

Comment 1

[Gregory Szorc [:gps]]

Comment on attachment 828648 [details] [diff] [review]
Add preprocessed files to install manifests

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

This is mostly good. There are two things holding it back:

1) I'd like to see better handling of defines. Install manifests need to be processed quickly. I have a feeling once we start adding preprocessed entries for the entire tree that the overhead of string parsing will add up. I'd much prefer to perform this string processing once, at build config/manifest generation time.

2) glandium is touching Preprocessor.py in bug 935305. You two need to coordinate. I suppose this means manifests will some day need to expose derived dependencies. That could be... fun.

::: python/mozbuild/mozpack/files.py
@@ +332,5 @@
> +        '''
> +        Invokes the preprocessor to create the destination file
> +        skip_if_older is ignored.
> +        '''
> +        from mozbuild.action.preprocessor import Preprocessor

This should be a file-level import. I also don't think Preprocessor should exist under mozbuild.action. But I guess that's due to a separate patch.

@@ +341,5 @@
> +            assert isinstance(dest, Dest)
> +
> +        pp = Preprocessor()
> +        opts = pp.getCommandLineParser()
> +        opts.parse_args(args=self.defines.split(' '))

I thought these 2 lines weren't needed, then I saw what the preprocessor is doing behind the scenes. Oy.

I'd feel much better if we had a proper API on the preprocessor to set defines so we avoided the overhead of argument parsing.

@@ +347,5 @@
> +        pp.setMarker(self.marker)
> +        pp.out = dest
> +        pp.do_include(self.file, False)
> +
> +        dest.close()

I don't believe we should close dest here - isn't it done in the caller?

::: python/mozbuild/mozpack/manifests.py
@@ +208,5 @@
>          destination file does not exist.
>          """
>          self._add_entry(dest, (self.OPTIONAL_EXISTS,))
>  
> +    def add_preprocess(self, source, dest, marker='#', defines=''):

I don't like using strings for defines when it is really an enumerable.

Comment 2

[Brian O'Keefe [:bokeefe]]

(In reply to Gregory Szorc [:gps] from comment #1)
> 1) I'd like to see better handling of defines. Install manifests need to be
> processed quickly. I have a feeling once we start adding preprocessed
> entries for the entire tree that the overhead of string parsing will add up.
> I'd much prefer to perform this string processing once, at build
> config/manifest generation time.

Alright, I'll see what I can do about that. I was mostly trying to account for ACDEFINES being a string (in bug 772828), but it looks like I can get at those as a dict directly.

> 2) glandium is touching Preprocessor.py in bug 935305. You two need to
> coordinate. I suppose this means manifests will some day need to expose
> derived dependencies. That could be... fun.

I'll rebase this on top of bug 935305, since that's much further along (plus has some fixes I'll need)

> @@ +347,5 @@
> > +        pp.setMarker(self.marker)
> > +        pp.out = dest
> > +        pp.do_include(self.file, False)
> > +
> > +        dest.close()
> 
> I don't believe we should close dest here - isn't it done in the caller?

It isn't, or at least it appeared that way from the tests. They were saying the file was empty, even though looking at the file after the test exited it had the right content. Really, the close should go in the preprocessor, like glandium did in attachment 829203 [details] [diff] [review] (bug 935305 comment 13).

Comment 3

[Brian O'Keefe [:bokeefe]]

Attachment: Part 0: Reindent preprocessor.py

I'm going to touch this in the next patch anyway, so I may as well fix the two-space indents.

Comment 4

[Brian O'Keefe [:bokeefe]]

Attachment: Part 1: Give Preprocessor a more pythonic API

Having a sane API makes working with the preprocessor a lot easier. The key changes here:
* Preprocessor.__init__ now accepts a dictionary of defines, so you don't need parse_command_line to deal with them
* There's a processFile entrypoint, that preprocesses everything in the inputs and writes them to output, instead of setting things up just right and calling do_include in a loop
* The output object is now somewhat magic:
  - sys.stdout is the default, like before
  - If output is an object with an 'open' attribute, call open on it, then treat it like a stream
  - Otherwise, it's probably a path to a file, so call open(output) and use the result.
* If output isn't sys.stdout, it's closed after all of the inputs are processes.

Comment 5

[Brian O'Keefe [:bokeefe]]

Attachment: Part 2: Add preprocessed files to install manifests

This covers the interesting parts:

> 1) I'd like to see better handling of defines. Install manifests need to be
> processed quickly. I have a feeling once we start adding preprocessed
> entries for the entire tree that the overhead of string parsing will add up.
> I'd much prefer to perform this string processing once, at build
> config/manifest generation time.

Defines are used as a dictionary most places, so I did that here as well. There wasn't a nice way to store them as text, so I used Pickle. Inconveniently, pickle kept inserting newlines, which was messing up the manifests, so I base64 encoded it to work around that.

That doesn't hurt performance a whole lot. In a quick microbenchmark, setting up the preprocessor 10,000 times in a loop (open file, parse, set defines on preprocessor, close file):
- With the string parsing, the old patch way, it took about 20 seconds (on my Windows machine)
- With base64decode, cPickle.loads, and the new preprocessor API, it took around 2.5 seconds.

I also fixed preprocessed files so they depend on the source and the manifest - if either changes (the manifest because it has the defines), then we regenerate the file. That has tests, too.

> I suppose this means manifests will some day need to expose
> derived dependencies. That could be... fun.

I didn't go quite as far as putting in derived dependencies, but they should work if we can get them down to the PreprocessedFile instance. Maybe an extra field in the manifest...

Comment 6

[Mike Hommey [:glandium]]

Comment on attachment 831822 [details] [diff] [review]
Part 1: Give Preprocessor a more pythonic API

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

::: python/mozbuild/mozbuild/preprocessor.py
@@ +398,5 @@
> +            self.out = get_output_file(output)
> +
> +        if depfile:
> +            if sys.stdin in inputs:
> +               raise Preprocessor.Error(self, "--depend doesn't work with stdin", None)

Exceptions related to command line options emitted when you're calling processFile() as an API is weird.

Comment 7

[Mike Hommey [:glandium]]

(In reply to Brian O'Keefe from comment #5)

> Defines are used as a dictionary most places, so I did that here as well.
> There wasn't a nice way to store them as text, so I used Pickle.

JSON?

Comment 8

[Brian O'Keefe [:bokeefe]]

(In reply to Mike Hommey [:glandium] from comment #6)
> Comment on attachment 831822 [details] [diff] [review]
> Part 1: Give Preprocessor a more pythonic API
> 
> Review of attachment 831822 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: python/mozbuild/mozbuild/preprocessor.py
> @@ +398,5 @@
> > +            self.out = get_output_file(output)
> > +
> > +        if depfile:
> > +            if sys.stdin in inputs:
> > +               raise Preprocessor.Error(self, "--depend doesn't work with stdin", None)
> 
> Exceptions related to command line options emitted when you're calling
> processFile() as an API is weird.

I agree. I thought I had updated those, but obviously not.

(In reply to Mike Hommey [:glandium] from comment #7)
> (In reply to Brian O'Keefe from comment #5)
> 
> > Defines are used as a dictionary most places, so I did that here as well.
> > There wasn't a nice way to store them as text, so I used Pickle.
> 
> JSON?

I forgot JSON was an option; it's certainly much simpler than my pickle nonsense. I benchmarked that using the same test: it's roughly 3.2 seconds to load the JSON 10000 times, so marginally slower than pickling, but still a huge improvement over string manipulations.


Also, apparently try is too fast, so the tests for re-preprocessing when a dependency changes fail intermittently. I'll fix the tests to work around that.
(Broken-ish) Try push: https://tbpl.mozilla.org/?tree=Try&rev=54d0783fa906

Comment 9

[Gregory Szorc [:gps]]

Comment on attachment 831817 [details] [diff] [review]
Part 0: Reindent preprocessor.py

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

rs=me.

::: python/mozbuild/mozbuild/preprocessor.py
@@ +1,1 @@
> +"""

We should add an MPL header while we're here.

Comment 10

[Gregory Szorc [:gps]]

Comment on attachment 831822 [details] [diff] [review]
Part 1: Give Preprocessor a more pythonic API

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

This looks good except for the bit where you are throwing exceptions related to command line arguments inside a library function. Fix that and this will get r+.

::: python/mozbuild/mozbuild/preprocessor.py
@@ +390,5 @@
> +                try:
> +                    os.makedirs(dir)
> +                except OSError as error:
> +                    if error.errno != errno.EEXIST:
> +                        raise

No need to check os.path.exists. Eliminate it and save an extra system call.

@@ +408,5 @@
> +            except:
> +                raise Preprocessor.Error(self, "--depend requires the "
> +                                               "mozbuild.makeutil module", None)
> +            self.depfile = get_output_file(depfile)
> +        

Nit: Whitespace

@@ +464,5 @@
>              return open(path, 'wb')
>  
>          p = self.getCommandLineParser()
>          options, args = p.parse_args(args=args)
> +        

Nit: whitespace

@@ +472,2 @@
>          includes.extend(args)
> +        

Nit: whitespace

Comment 11

[Brian O'Keefe [:bokeefe]]

Attachment: Part 0: Reindent preprocessor.py (rs=gps)

(In reply to Gregory Szorc [:gps] from comment #9)
> Comment on attachment 831817 [details] [diff] [review]
> Part 0: Reindent preprocessor.py
> 
> Review of attachment 831817 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> rs=me.
> 
> ::: python/mozbuild/mozbuild/preprocessor.py
> @@ +1,1 @@
> > +"""
> 
> We should add an MPL header while we're here.

Done. Carried forward rs=gps

Comment 12

[Brian O'Keefe [:bokeefe]]

Attachment: Part 1: Give Preprocessor a more pythonic API

(In reply to Gregory Szorc [:gps] from comment #10)
> Comment on attachment 831822 [details] [diff] [review]
> Part 1: Give Preprocessor a more pythonic API
> 
> Review of attachment 831822 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good except for the bit where you are throwing exceptions related
> to command line arguments inside a library function. Fix that and this will
> get r+.

I've improved the exceptions, so they don't reference the command line options anymore.

> ::: python/mozbuild/mozbuild/preprocessor.py
> @@ +390,5 @@
> > +                try:
> > +                    os.makedirs(dir)
> > +                except OSError as error:
> > +                    if error.errno != errno.EEXIST:
> > +                        raise
> 
> No need to check os.path.exists. Eliminate it and save an extra system call.

Done.

> Nit: whitespace

Fixed.

Comment 13

[Brian O'Keefe [:bokeefe]]

Attachment: Part 2: Add preprocessed files to install manifests

Now with 100% less absurd pickling, plus happier try: https://tbpl.mozilla.org/?tree=Try&rev=d15850886d57

Comment 14

[Gregory Szorc [:gps]]

I apologize for not having my regular low review latency on this bug. I hope to a full review by next week.

I have been thinking about this patch some. Part 2 does "scare" me a little bit. Since we need to capture dependencies so we don't re-preprocess unless files have changed, we will need to bake some dependency smarts into the manifest processing. You appear to have partially done this, but you don't capture the actual dependency list emitted from the preprocessor: the captured dependencies are those for the manifest itself, not what the preprocessor says they are.

To do this right, we'd need to update the manifest if the preprocessor dependencies change. I think that sucks. IMO the manifest file should be immutable and idempotent.

I think I'd be more receptive to seeing a preprocessor processing split into its "component." Maybe we should have a dedicated manifest for preprocessing. Or, maybe we should just write an auto-generated make file containing rules for every preprocessed file in the tree. The preprocessor can write make deps files. That seems like the path of least resistance. Although, we should also consider how we port this to Tup and other build backends.

Comment 15

[Gregory Szorc [:gps]]

Comment on attachment 8334279 [details] [diff] [review]
Part 1: Give Preprocessor a more pythonic API

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

I still think the handling of stdin/stdout could be improved. I'd much rather see argument validation performed before calling processFile() and just have processFile() accept open file objects. Let the caller worry about filtering stdin/stdout and closing the handles.

That being said, this patch is a step in the right direction and isn't "wrong," so r+. Extra credit if you keep improving it, however :)

I'll land part 0 for you so things don't get bit rotted.

::: python/mozbuild/mozbuild/preprocessor.py
@@ +377,5 @@
> +
> +        If ``depfile`` is specified, a Makefile-like rule is written listing
> +        ``inputs`` as dependencies for ``output``.
> +        """
> +        def get_output_file(out):

I've seen this function somewhere before. I foresee a followup bug to split it out into util.py.

@@ +790,5 @@
>  
>  def preprocess(includes=[sys.stdin], defines={},
>                 output = sys.stdout,
>                 line_endings='\n', marker='#'):
> +    pp = Preprocessor(line_endings, defines, marker)

Nit: Use named arguments.

Comment 16

[Gregory Szorc [:gps]]

Part 0 landed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/1e12bf6424fe

Comment 17

[Gregory Szorc [:gps]]

I pushed a followup to convert DOS line endings to UNIX. Please ensure your tools are configured to save UNIX line endings.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ebbe6a7c9c7e

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1e12bf6424fe
https://hg.mozilla.org/mozilla-central/rev/ebbe6a7c9c7e

Comment 19

[Brian O'Keefe [:bokeefe]]

Comment on attachment 8334275 [details] [diff] [review]
Part 0: Reindent preprocessor.py (rs=gps)

(In reply to Gregory Szorc [:gps] from comment #17)
> I pushed a followup to convert DOS line endings to UNIX. Please ensure your
> tools are configured to save UNIX line endings.
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/ebbe6a7c9c7e

Thanks for that. Evidently, reindent.py decided to 'fix' line endings too. I'll remember for next time.

Comment 20

[Brian O'Keefe [:bokeefe]]

Attachment: Part 1: Give Preprocessor a more pythonic API

(In reply to Gregory Szorc [:gps] from comment #15)
> Comment on attachment 8334279 [details] [diff] [review]
> Part 1: Give Preprocessor a more pythonic API
> 
> Review of attachment 8334279 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I still think the handling of stdin/stdout could be improved. I'd much
> rather see argument validation performed before calling processFile() and
> just have processFile() accept open file objects. Let the caller worry about
> filtering stdin/stdout and closing the handles.
> 
> That being said, this patch is a step in the right direction and isn't
> "wrong," so r+. Extra credit if you keep improving it, however :)

I was going to just update this patch, and then see how using file objects worked in a new patch, but using the file objects turned out to make things so much simpler I just went right to that.

There's also a new computeDependencies function, which isn't used until Part 2, which just runs the preprocessor without any output.

Comment 21

[Brian O'Keefe [:bokeefe]]

Attachment: Part 2: Add preprocessed files to install manifests

(In reply to Gregory Szorc [:gps] from comment #14)
> I have been thinking about this patch some. Part 2 does "scare" me a little
> bit. Since we need to capture dependencies so we don't re-preprocess unless
> files have changed, we will need to bake some dependency smarts into the
> manifest processing. You appear to have partially done this, but you don't
> capture the actual dependency list emitted from the preprocessor: the
> captured dependencies are those for the manifest itself, not what the
> preprocessor says they are.

I completely misread that code in the preprocessor - I fixed it so the right
dependencies are getting captured now.

> To do this right, we'd need to update the manifest if the preprocessor
> dependencies change. I think that sucks. IMO the manifest file should be
> immutable and idempotent.
> 
> I think I'd be more receptive to seeing a preprocessor processing split into
> its "component." Maybe we should have a dedicated manifest for
> preprocessing. Or, maybe we should just write an auto-generated make file
> containing rules for every preprocessed file in the tree. The preprocessor
> can write make deps files. That seems like the path of least resistance.
> Although, we should also consider how we port this to Tup and other build
> backends.

I adjusted the way the preprocessor works with manifests a bit. I'm still using
the InstallManifest for the preprocessed file, but dependencies go in a parallel
.deps file now. So you end up with:

_build_manifests/install/
  dist_bin
  dist_bin.deps
  (etc)

Where the manifest itself only changes if DEFINES changes, and the .deps file is
updated when the manifest is built. The PreprocessedFile class checks whether
the dependencies were modified, so this will work for Tup and friends, until it
gets ported over.

I haven't gone down the rabbit hole of files somewhere along the include chain
being added or deleted - at the moment, adding a file means the file that
includes it is newer, so the target will get regenerated, but the dependencies
aren't updated; deleting a file probably just causes mass confusion.

So, just f? to see if this is more in line with what you were thinking (or at
least moving in the right direction).

Comment 22

[Gregory Szorc [:gps]]

Comment on attachment 8338685 [details] [diff] [review]
Part 1: Give Preprocessor a more pythonic API

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

Only minor nits stand between you and final review.

::: python/mozbuild/mozbuild/preprocessor.py
@@ +367,5 @@
>          rv.LE = self.LE
>          rv.out = self.out
>          return rv
>  
> +    def processFile(self, input, output, depfile = None):

Nit: no spaces around =

@@ +387,5 @@
> +    def computeDependencies(self, input):
> +        """
> +        Reads the ``input`` stream, and computes the dependencies for that input.
> +        """
> +        self.out = None

I'm tempted to say we should just have the output passed around everywhere instead of attached to the class instance. Things like overriding self.out = None to compute dependencies seem fragile.

Either pass ``output`` around everywhere or modify this function to try..finally so self.out's value isn't lost during calling.

@@ +728,5 @@
>              return matchobj.group(0)
>          return self.varsubst.sub(repl, aLine)
>      def filter_attemptSubstitution(self, aLine):
>          return self.filter_substitution(aLine, fatal=False)
> +

Introduced whitespace?

Comment 23

[Gregory Szorc [:gps]]

Comment on attachment 8338696 [details] [diff] [review]
Part 2: Add preprocessed files to install manifests

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

I am internally going back and forth on whether adding preprocessed files to install manifests is a good idea. On one hand, your patch is terrific and seems to be 90% towards a working implementation. On the other hand, this is a lot of complicated logic and a voice in my head keeps saying "maybe we should move preprocessing into its own entity and just have the install manifests contain optional/required existing entries."

The part that is swaying me towards accepting the current approach is mozpack.files.PreprocessedFile.

Aside from the bit rot from bug 934739 (sorry about that - there is no excuse for me taking so long to look at this patch), the only glaring issue I see is the lack of updating the dependency file after things are first executed. You'll need to store the updated dependency information on every invocation or else things won't get properly rebuilt. Here's a test that you should add:

1) input -> output
2) Process install manifest/registry
3) Modify 'input' to add a #include to "included"
4) Process install manifest/registry
5) Modify 'included' to output something
6) Process install manifest/registry
7) Ensure 'output' contains the modification from 'included'

Please also consider splitting the mozpack.files work into its own patch - you should be able to land that without much hassle. The only considerations for the manifest part of the patch are how to propagate dependency info back to the manifest. Perhaps you could have each input file write its own .pp file. Then, you just include several hundred of them without needing to worry about updating a monolithic dependency file.

Comment 24

[Brian O'Keefe [:bokeefe]]

Attachment: Part 1: Give Preprocessor a more pythonic API

(In reply to Gregory Szorc [:gps] (in southeast Asia until Dec 14) from comment #22)
> I'm tempted to say we should just have the output passed around everywhere
> instead of attached to the class instance. Things like overriding self.out =
> None to compute dependencies seem fragile.
> 
> Either pass ``output`` around everywhere or modify this function to
> try..finally so self.out's value isn't lost during calling.

Passing ``output`` around was quickly becoming 'touch every function in the preprocessor', so I just went with the try..finally option.

Comment 25

[Brian O'Keefe [:bokeefe]]

Attachment: Part 2: Add preprocessed file support to mozpack

(In reply to Gregory Szorc [:gps] from comment #23)
> Please also consider splitting the mozpack.files work into its own patch -
> you should be able to land that without much hassle. The only considerations
> for the manifest part of the patch are how to propagate dependency info back
> to the manifest. Perhaps you could have each input file write its own .pp
> file. Then, you just include several hundred of them without needing to
> worry about updating a monolithic dependency file.

This part is just the mozpack.files changes (and appropriate tests). I shuffled the dependency tracking a bit, so it now lives entirely in the PreprocessedFile class, and you just have to pass in where you want to store the dependency file.

Comment 26

[Brian O'Keefe [:bokeefe]]

Attachment: Part 3: Add preprocessed files to install manifests

(In reply to Gregory Szorc [:gps] from comment #23)
> the only glaring issue
> I see is the lack of updating the dependency file after things are first
> executed. You'll need to store the updated dependency information on every
> invocation or else things won't get properly rebuilt.

Trying to make the dependency information part of the manifest (or a file along side the manifest) kept becoming less and less straightforward, so I took that as I sign that I was doing it wrong. With the new Part 2, dependency files are handled by the PreprocessedFile class. With that, I've switched to storing the location of that dependency file in the manifest. That gets rid of the monolithic dependency file along side the manifest and removes some of the magic it was doing.

> Here's a test that you should add:

I added that test, which also proved that the other test was wrong (I had the mtime bits wrong, so the source was always newer than the output).

Try was happy: https://tbpl.mozilla.org/?tree=Try&rev=39365c754a92
The win b2g failure was bizarre, but a re-trigger was fine, so I assumed it wasn't my patch.

Comment 27

[Gregory Szorc [:gps]]

Comment on attachment 8350034 [details] [diff] [review]
Part 1: Give Preprocessor a more pythonic API

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

This looks good. I'd like to see this landed ASAP.

Comment 28

[Gregory Szorc [:gps]]

Comment on attachment 8350038 [details] [diff] [review]
Part 2: Add preprocessed file support to mozpack

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

This is pretty close! Just internal optimizations and test cases.

::: python/mozbuild/mozpack/files.py
@@ +44,5 @@
>          self.mode = None
>  
> +    @property
> +    def name(self):
> +        return self.path

Why can't we just access .path directly?

@@ +318,5 @@
> +        self.marker = marker
> +        self.defines = defines
> +        self.extra_depends = []
> +        if extra_depends:
> +            self.extra_depends.extend(extra_depends)

I'd just write:

self.extra_depends = list(extra_depends or [])

@@ +334,5 @@
> +
> +        # If a dependency file was specified, and it exists, add any
> +        # dependencies from that fild to our list
> +        if self.depfile and os.path.exists(self.depfile):
> +            target = dest.name.replace(os.sep, '/')

target = mozpath.normpath(dest.name)

@@ +338,5 @@
> +            target = dest.name.replace(os.sep, '/')
> +            with open(self.depfile, 'rb') as fileobj:
> +                for rule in makeutil.read_dep_makefile(fileobj):
> +                    if target in rule.targets():
> +                        pp_deps.extend(rule.dependencies())

Perhaps pp_deps should be a set?

@@ +344,5 @@
> +        skip = False
> +        if dest.exists() and skip_if_older:
> +            skip = True
> +
> +            # If a dependency file was specified, and it doesn't exists,

Nit: exist

@@ +351,5 @@
> +            if self.depfile and not os.path.exists(self.depfile):
> +                skip = False
> +
> +            for dep in pp_deps:
> +                skip &= BaseFile.is_older(dep, dest.path)

We should optimize this to minimize the number of stats.

First, we're computing the mtime of dest.path every time. While the syscall result should be cached by the OS, it's still inefficient.

Second, we're computing the mtime for every dependency, even if we already know we can't skip.

We might want to factor this out into another function and invert the result to "can't skip" and return fast in the case where we need to do work.

::: python/mozbuild/mozpack/test/test_files.py
@@ +411,5 @@
> +            tmp.write('quux')
> +        time = os.path.getmtime(dest) + 1
> +        os.utime(incl, (time, time))
> +        f.copy(dest)
> +        self.assertEqual('quux', open(dest, 'rb').read())

Please add a test case where the destination path is a symlink. I think you'll find the existing implementation will open the symlink and modify the target of that symlink instead of writing a replacement file in the destination path.

Please note we may have existing bugs for other classes that don't do this. However, the major use case is a symlink is replaced by a preprocessed file, so this needs to work and be tested.

Comment 29

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #28)
> Comment on attachment 8350038 [details] [diff] [review]
> Part 2: Add preprocessed file support to mozpack
> 
> Review of attachment 8350038 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is pretty close! Just internal optimizations and test cases.
> 
> ::: python/mozbuild/mozpack/files.py
> @@ +44,5 @@
> >          self.mode = None
> >  
> > +    @property
> > +    def name(self):
> > +        return self.path
> 
> Why can't we just access .path directly?

File handler coming from open() have name. The preprocessor uses fh.name iirc.

Comment 30

[Brian O'Keefe [:bokeefe]]

(In reply to Gregory Szorc [:gps] from comment #27)
> Comment on attachment 8350034 [details] [diff] [review]
> Part 1: Give Preprocessor a more pythonic API
> 
> Review of attachment 8350034 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good. I'd like to see this landed ASAP.

Thanks. I can't land myself, so I added checkin-needed. (To whomever happens to land this, just Part 1 please.)

Comment 31

[Brian O'Keefe [:bokeefe]]

Attachment: Part 2: Add preprocessed file support to mozpack

(In reply to Mike Hommey [:glandium] from comment #29)
> (In reply to Gregory Szorc [:gps] from comment #28)
> > Comment on attachment 8350038 [details] [diff] [review]
> > Part 2: Add preprocessed file support to mozpack
> > 
> > Review of attachment 8350038 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This is pretty close! Just internal optimizations and test cases.
> > 
> > ::: python/mozbuild/mozpack/files.py
> > @@ +44,5 @@
> > >          self.mode = None
> > >  
> > > +    @property
> > > +    def name(self):
> > > +        return self.path
> > 
> > Why can't we just access .path directly?
> 
> File handler coming from open() have name. The preprocessor uses fh.name
> iirc.

Yes, that was why.

(In reply to Gregory Szorc [:gps] from comment #28)
> Comment on attachment 8350038 [details] [diff] [review]
> Part 2: Add preprocessed file support to mozpack
> 
> Review of attachment 8350038 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +351,5 @@
> > +            if self.depfile and not os.path.exists(self.depfile):
> > +                skip = False
> > +
> > +            for dep in pp_deps:
> > +                skip &= BaseFile.is_older(dep, dest.path)
> 
> We should optimize this to minimize the number of stats.
> 
> First, we're computing the mtime of dest.path every time. While the syscall
> result should be cached by the OS, it's still inefficient.
> 
> Second, we're computing the mtime for every dependency, even if we already
> know we can't skip.
> 
> We might want to factor this out into another function and invert the result
> to "can't skip" and return fast in the case where we need to do work.

Done. There's now an 'any_newer' procedure up in BaseFile that takes care of this.

> ::: python/mozbuild/mozpack/test/test_files.py
> @@ +411,5 @@
> > +            tmp.write('quux')
> > +        time = os.path.getmtime(dest) + 1
> > +        os.utime(incl, (time, time))
> > +        f.copy(dest)
> > +        self.assertEqual('quux', open(dest, 'rb').read())
> 
> Please add a test case where the destination path is a symlink. I think
> you'll find the existing implementation will open the symlink and modify the
> target of that symlink instead of writing a replacement file in the
> destination path.
> 
> Please note we may have existing bugs for other classes that don't do this.
> However, the major use case is a symlink is replaced by a preprocessed file,
> so this needs to work and be tested.

I blame Windows. I forgot that was something I'd even need to worry about. I've fixed it to remove the existing destination if it's a symlink, and added a test for that. I'm temporarily running on blind faith that the test works, seeing as I don't have a Linux/OSX box handy. I'll confirm when I have access to my Linux box this afternoon.

Comment 32

[Brian O'Keefe [:bokeefe]]

Attachment: Interdiff of Part 2

Comment 33

[Brian O'Keefe [:bokeefe]]

Attachment: Part 2: Add preprocessed file support to mozpack

(In reply to Brian O'Keefe from comment #31)
> I'm temporarily running on blind faith that the test
> works, seeing as I don't have a Linux/OSX box handy. I'll confirm when I
> have access to my Linux box this afternoon.

And, of course, that didn't work, because I copy/pasted the preprocessor line and forgot to fix the variable names. The new version fixes that.

(The interdiff is still mostly valid; just pretend line 438 has 'pp_source' in place of 'src' and 'None' in place of 'deps')

Comment 34

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/96cc0aec0437

Comment 35

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/96cc0aec0437

Comment 36

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8350034 [details] [diff] [review]
Part 1: Give Preprocessor a more pythonic API

Someone forgot to add the checkin+ when this landed

Comment 37

[Gregory Szorc [:gps]]

Comment on attachment 8350769 [details] [diff] [review]
Part 2: Add preprocessed file support to mozpack

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

Excellent! There's a lot of comments, but they are minor. I trust you can address them on your own before commit.

symlinks and mtimes can be highly finicky. I highly recommend performing one last try push before landing.

::: python/mozbuild/mozpack/files.py
@@ +73,5 @@
>      Base interface and helper for file copying. Derived class may implement
>      their own copy function, or rely on BaseFile.copy using the open() member
>      function and/or the path property.
>      '''
> +    @classmethod

Use @staticmethod.

@classmethod gets the containing class as an argument. @staticmethod doesn't. You don't do anything with the class, so this can be @staticmethod.

@@ +86,5 @@
> +        # enough precision.
> +        return int(os.path.getmtime(first) * 1000) \
> +                <= int(os.path.getmtime(second) * 1000)
> +
> +    @classmethod

@staticmethod

@@ +355,5 @@
> +            try:
> +                if os.path.islink(dest.path):
> +                    os.remove(dest.path)
> +            except EnvironmentError:
> +                raise

This except block does nothing.

If EnvironmentError is raised, it is re-raised.
If any other exception is raised, it's uncaught.

@@ +360,5 @@
> +
> +        pp_deps = set(self.extra_depends)
> +
> +        # If a dependency file was specified, and it exists, add any
> +        # dependencies from that fild to our list

Nit: s/fild/file/. Punctuate comments as full sentences. (We do the latter to help non-English speakers feeding comments into translators to get more accurate results since full sentences tend to translate better.)

@@ +370,5 @@
> +                        pp_deps.update(rule.dependencies())
> +
> +        skip = False
> +        if dest.exists() and skip_if_older:
> +            skip = True

Kill assignment since it is overridden below.

::: python/mozbuild/mozpack/test/test_files.py
@@ +339,5 @@
> +        with open(src, 'wb') as tmp:
> +            tmp.write('#ifdef FOO\ntest\n#endif')
> +
> +        f = PreprocessedFile(src, depfile_path=None, marker='#', defines={'FOO': True})
> +        f.copy(dest)

self.assertTrue(f.copy(dest))

@@ +357,5 @@
> +            tmp.write('#ifdef FOO\ntest\n#endif')
> +
> +        # Initial copy
> +        f = PreprocessedFile(src, depfile_path=depfile, marker='#', defines={'FOO': True})
> +        f.copy(dest)

self.assertTrue(f.copy(dest))

@@ +360,5 @@
> +        f = PreprocessedFile(src, depfile_path=depfile, marker='#', defines={'FOO': True})
> +        f.copy(dest)
> +
> +        # Ensure subsequent copies won't trigger writes
> +        f.copy(DestNoWrite(dest))

self.assertFalse(f.copy(DestNoWrite(dest)))

@@ +369,5 @@
> +        with open(src, 'wb') as tmp:
> +            tmp.write('#ifdef FOO\nfooo\n#endif')
> +        time = os.path.getmtime(dest) - 1
> +        os.utime(src, (time, time))
> +        f.copy(DestNoWrite(dest))

self.assertFalse(f.copy(...

@@ +373,5 @@
> +        f.copy(DestNoWrite(dest))
> +        self.assertEqual('test\n', open(dest, 'rb').read())
> +
> +        # skip_if_older=False is expected to force a copy in this situation.
> +        f.copy(dest, skip_if_older=False)

You know what I'm going to type.

@@ +393,5 @@
> +            tmp.write('foo bar')
> +
> +        # Initial copy
> +        f = PreprocessedFile(src, depfile_path=deps, marker='#', defines={'FOO': True})
> +        f.copy(dest)

self.assertTrue

@@ +400,5 @@
> +        with open(src, 'wb') as tmp:
> +            tmp.write('#include incl\n')
> +        time = os.path.getmtime(dest) + 1
> +        os.utime(src, (time, time))
> +        f.copy(dest)

self.assertTrue

@@ +410,5 @@
> +        with open(incl, 'wb') as tmp:
> +            tmp.write('quux')
> +        time = os.path.getmtime(dest) + 1
> +        os.utime(incl, (time, time))
> +        f.copy(dest)

self.assertTrue

@@ +411,5 @@
> +            tmp.write('quux')
> +        time = os.path.getmtime(dest) + 1
> +        os.utime(incl, (time, time))
> +        f.copy(dest)
> +        self.assertEqual('quux', open(dest, 'rb').read())

Can we perform an additional f.copy() to ensure we don't incur preprocessing again?

@@ +435,5 @@
> +        with open(pp_source, 'wb') as tmp:
> +            tmp.write('#define FOO\nPREPROCESSED')
> +
> +        f = PreprocessedFile(pp_source, depfile_path=None, marker='#', defines={'FOO': True})
> +        f.copy(dest)

self.assertTrue

Comment 38

[Gregory Szorc [:gps]]

Comment on attachment 8350044 [details] [diff] [review]
Part 3: Add preprocessed files to install manifests

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

This is essentially an r+. There's one too many nits for my liking. Next patch should be a rubber stamp r+ methinks.

::: python/mozbuild/mozpack/manifests.py
@@ +95,5 @@
>  
>          If path is defined, the manifest will be populated with data from the
>          file path.
>  
> +        If fileobj is defined, the manifest will be populated with data read

Thank you for fixing the docs.

@@ +186,5 @@
> +
> +        Complex data types, such as ``dict``, need to be converted into a text
> +        representation before they can be written to a file.
> +        """
> +        return json.dumps(data)

You'll want to specify sort_keys=True to ensure file content is stable across invocations.

@@ +194,5 @@
> +
> +        Complex data types, such as ``dict``, need to be converted into a text
> +        representation before they can be written to a file.
> +        """
> +        return json.loads(data)

_store_object and _load_object could be named better. Maybe _encode_field_entry and _decode_field_entry.

@@ +205,5 @@
>  
>          It is an error if both are specified.
>          """
>          with _auto_fileobj(path, fileobj, 'wb') as fh:
> +            fh.write('4\n')

Let's just factor out the current version into a class variable. FWIW, I have another patch sitting around where glandium insisted I do this. You better land this code soon :)

@@ +268,5 @@
>          """
>          self._add_entry(mozpath.join(base, pattern, dest),
>              (self.PATTERN_COPY, base, pattern, dest))
>  
> +    def add_preprocess(self, source, dest, deps, marker='#', defines=''):

Should defines default to {}?

@@ +283,5 @@
> +
> +    def _add_preprocess_encoded(self, source, dest, deps, marker, defines):
> +        """Add a preprocessed file to this manifest, using the file encoded format.
> +        """
> +        self._add_entry(dest, (self.PREPROCESS, source, deps, marker, defines))

I'm not sure what the value of having this function is. Just inline the call to encode/decode the field in the 3 places it is needed.

@@ +294,5 @@
>  
> +    def _get_deps(self, dest):
> +        deps = []
> +        if self._source_file:
> +            deps.extend([self._source_file])

And this means that every time the manifest is updated that we'll have to preprocess everything again. That's... unfortunate. It would be nice to optimize that away some how. Though that requires complexity. Not sure it's worth it.

@@ +295,5 @@
> +    def _get_deps(self, dest):
> +        deps = []
> +        if self._source_file:
> +            deps.extend([self._source_file])
> +        return deps

return [self._source_file] if self._source_file else []

Or perhaps this should be a set:

return {self._source_file} if self._source_file else set()

( {} without ':' creates sets, not dicts )

::: python/mozbuild/mozpack/test/test_manifests.py
@@ +197,5 @@
> +        manifest = self.tmppath('m')
> +        deps = self.tmppath('m.pp')
> +        dest = self.tmppath('dest')
> +        include = self.tmppath('p_incl')
> +        os.mkdir(dest)

FileCopier.copy() will create dest if it doesn't exist. Remove this mkdir.

@@ +229,5 @@
> +            self.assertEqual(fh.read(), 'PASS1\n')
> +
> +        # Create a second manifest with the preprocessed file, then apply it.
> +        # Since this manifest does not exist on the disk, the should not be a
> +        # dependency on it, and the preprocessed file should not be modified.

Nit: bad english.

@@ +237,5 @@
> +        m2.populate_registry(c)
> +        c.copy(dest)
> +
> +        with open(self.tmppath('dest/p_dest'), 'rt') as fh:
> +            self.assertEqual(fh.read(), 'PASS1\n')

An alternate way to do this would be to examine the c.copy() result and ensure the path doesn't exist in result.updated_files.

@@ +281,5 @@
> +        dest = self.tmppath('dest')
> +        source = self.tmppath('p_source')
> +        destfile = self.tmppath('dest/p_dest')
> +        include = self.tmppath('p_incl')
> +        os.mkdir(dest)

Kill this mkdir

@@ +339,5 @@
> +        m.populate_registry(c)
> +        c.copy(dest)
> +
> +        with open(destfile, 'rt') as fh:
> +            self.assertEqual(fh.read(), 'SOURCE\nINCLUDE MODIFIED\n')

Some more comments in this test couldn't hurt.

Comment 39

[Brian O'Keefe [:bokeefe]]

Attachment: Part 2: Add preprocessed files to mozpack (r=gps)

With all the comments addressed, so r+ carried forward. I'll wait for part 3 and get these landed together.

(In reply to Gregory Szorc [:gps] from comment #37)
> Comment on attachment 8350769 [details] [diff] [review]
> Part 2: Add preprocessed file support to mozpack
> 
> Review of attachment 8350769 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Excellent! There's a lot of comments, but they are minor. I trust you can
> address them on your own before commit.
> 
> symlinks and mtimes can be highly finicky. I highly recommend performing one
> last try push before landing.

Sure, more try never hurts. This one came back green: https://tbpl.mozilla.org/?tree=Try&rev=a785b3c9cce4

> ::: python/mozbuild/mozpack/files.py
> @@ +73,5 @@
> >      Base interface and helper for file copying. Derived class may implement
> >      their own copy function, or rely on BaseFile.copy using the open() member
> >      function and/or the path property.
> >      '''
> > +    @classmethod
> 
> Use @staticmethod.
> 
> @classmethod gets the containing class as an argument. @staticmethod
> doesn't. You don't do anything with the class, so this can be @staticmethod.

Ah, that's how that works. Python is still new to me.

> @@ +355,5 @@
> > +            try:
> > +                if os.path.islink(dest.path):
> > +                    os.remove(dest.path)
> > +            except EnvironmentError:
> > +                raise
> 
> This except block does nothing.
> 
> If EnvironmentError is raised, it is re-raised.
> If any other exception is raised, it's uncaught.

Whoops. I took out the useless try block.

> self.assertTrue(f.copy(dest))

I added all of these.

> @@ +411,5 @@
> > +            tmp.write('quux')
> > +        time = os.path.getmtime(dest) + 1
> > +        os.utime(incl, (time, time))
> > +        f.copy(dest)
> > +        self.assertEqual('quux', open(dest, 'rb').read())
> 
> Can we perform an additional f.copy() to ensure we don't incur preprocessing
> again?

Sure, that seems like a good check.

Comment 40

[Brian O'Keefe [:bokeefe]]

Comment on attachment 8358159 [details] [diff] [review]
Part 2: Add preprocessed files to mozpack (r=gps)

It would help if I could pick the right line from the autocomplete list...

Comment 41

[Brian O'Keefe [:bokeefe]]

Attachment: Part 2: Add preprocessed files to install manifests

(In reply to Gregory Szorc [:gps] from comment #38)
> Comment on attachment 8350044 [details] [diff] [review]
> Part 3: Add preprocessed files to install manifests
> 
> Review of attachment 8350044 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is essentially an r+. There's one too many nits for my liking. Next
> patch should be a rubber stamp r+ methinks.
> 
> ::: python/mozbuild/mozpack/manifests.py
> @@ +95,5 @@
> >  
> >          If path is defined, the manifest will be populated with data from the
> >          file path.
> >  
> > +        If fileobj is defined, the manifest will be populated with data read
> 
> Thank you for fixing the docs.
> 
> @@ +186,5 @@
> > +
> > +        Complex data types, such as ``dict``, need to be converted into a text
> > +        representation before they can be written to a file.
> > +        """
> > +        return json.dumps(data)
> 
> You'll want to specify sort_keys=True to ensure file content is stable
> across invocations.

At some point, I knew I needed that, but I lost it somewhere along the line.

> @@ +194,5 @@
> > +
> > +        Complex data types, such as ``dict``, need to be converted into a text
> > +        representation before they can be written to a file.
> > +        """
> > +        return json.loads(data)
> 
> _store_object and _load_object could be named better. Maybe
> _encode_field_entry and _decode_field_entry.

I switched to your suggestions for names. I couldn't think of anything better.

> @@ +205,5 @@
> >  
> >          It is an error if both are specified.
> >          """
> >          with _auto_fileobj(path, fileobj, 'wb') as fh:
> > +            fh.write('4\n')
> 
> Let's just factor out the current version into a class variable. FWIW, I
> have another patch sitting around where glandium insisted I do this. You
> better land this code soon :)

Okay, theres a new CURRENT_VERSION variable now.

> @@ +268,5 @@
> >          """
> >          self._add_entry(mozpath.join(base, pattern, dest),
> >              (self.PATTERN_COPY, base, pattern, dest))
> >  
> > +    def add_preprocess(self, source, dest, deps, marker='#', defines=''):
> 
> Should defines default to {}?

Yeah, it should.

> @@ +283,5 @@
> > +
> > +    def _add_preprocess_encoded(self, source, dest, deps, marker, defines):
> > +        """Add a preprocessed file to this manifest, using the file encoded format.
> > +        """
> > +        self._add_entry(dest, (self.PREPROCESS, source, deps, marker, defines))
> 
> I'm not sure what the value of having this function is. Just inline the call
> to encode/decode the field in the 3 places it is needed.

I don't think there's much point anymore, so I pulled it out.

> @@ +294,5 @@
> >  
> > +    def _get_deps(self, dest):
> > +        deps = []
> > +        if self._source_file:
> > +            deps.extend([self._source_file])
> 
> And this means that every time the manifest is updated that we'll have to
> preprocess everything again. That's... unfortunate. It would be nice to
> optimize that away some how. Though that requires complexity. Not sure it's
> worth it.

Yeah, that's because I'm storing the defines in the manifest. It was the least bad way I could come up with to get the output to be regenerated when the defines change...

> @@ +295,5 @@
> > +    def _get_deps(self, dest):
> > +        deps = []
> > +        if self._source_file:
> > +            deps.extend([self._source_file])
> > +        return deps
> 
> return [self._source_file] if self._source_file else []
> 
> Or perhaps this should be a set:
> 
> return {self._source_file} if self._source_file else set()
> 
> ( {} without ':' creates sets, not dicts )

Ah ha! I've learned how Python does a ternary operator. And yeah, that should be a set.

> ::: python/mozbuild/mozpack/test/test_manifests.py
> @@ +197,5 @@
> > +        manifest = self.tmppath('m')
> > +        deps = self.tmppath('m.pp')
> > +        dest = self.tmppath('dest')
> > +        include = self.tmppath('p_incl')
> > +        os.mkdir(dest)
> 
> FileCopier.copy() will create dest if it doesn't exist. Remove this mkdir.

Removed.

> @@ +229,5 @@
> > +            self.assertEqual(fh.read(), 'PASS1\n')
> > +
> > +        # Create a second manifest with the preprocessed file, then apply it.
> > +        # Since this manifest does not exist on the disk, the should not be a
> > +        # dependency on it, and the preprocessed file should not be modified.
> 
> Nit: bad english.

Blah, I should learn to type.

> @@ +237,5 @@
> > +        m2.populate_registry(c)
> > +        c.copy(dest)
> > +
> > +        with open(self.tmppath('dest/p_dest'), 'rt') as fh:
> > +            self.assertEqual(fh.read(), 'PASS1\n')
> 
> An alternate way to do this would be to examine the c.copy() result and
> ensure the path doesn't exist in result.updated_files.

I checked that it's not in updated_files and is in existing files.

> @@ +281,5 @@
> > +        dest = self.tmppath('dest')
> > +        source = self.tmppath('p_source')
> > +        destfile = self.tmppath('dest/p_dest')
> > +        include = self.tmppath('p_incl')
> > +        os.mkdir(dest)
> 
> Kill this mkdir

Removed.

> @@ +339,5 @@
> > +        m.populate_registry(c)
> > +        c.copy(dest)
> > +
> > +        with open(destfile, 'rt') as fh:
> > +            self.assertEqual(fh.read(), 'SOURCE\nINCLUDE MODIFIED\n')
> 
> Some more comments in this test couldn't hurt.

I've added a few more comments.

Comment 42

[Gregory Szorc [:gps]]

Comment on attachment 8358180 [details] [diff] [review]
Part 2: Add preprocessed files to install manifests

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

I'm a little bit worried about timing bugs leading to test failures. But, I suppose we can deal with that if it is actually an issue. Worst case, we can skip the tests until they are fixed in a follow-up bug.

::: python/mozbuild/mozpack/manifests.py
@@ +278,5 @@
> +
> +        ``source`` will be passed through preprocessor.py, and the output will be
> +        written to ``dest``.
> +        """
> +        self._add_entry(dest, 

Nit: trailing whitespace

::: python/mozbuild/mozpack/test/test_manifests.py
@@ +249,5 @@
> +        os.utime(self.tmppath('dest/p_dest'), (time, time))
> +        m2 = InstallManifest(path=manifest)
> +        c = FileCopier()
> +        m2.populate_registry(c)
> +        c.copy(dest)

Nit: verify result

@@ +268,5 @@
> +        time = os.path.getmtime(include) - 1
> +        os.utime(self.tmppath('dest/p_dest'), (time, time))
> +        c = FileCopier()
> +        m2.populate_registry(c)
> +        c.copy(dest)

Nit: verify result

@@ +289,5 @@
> +        os.utime(include, (time, time))
> +
> +        with open(source, 'wt') as fh:
> +            fh.write('#define SRC\nSOURCE\n')
> +        time = os.path.getmtime(source) - 3

I'd just calbirate mtimes from one file. Reading from multiple sources seems more prone to timing intermittent failures.

@@ +305,5 @@
> +        # our preprocessed file.
> +        m = InstallManifest(path=manifest)
> +        c = FileCopier()
> +        m.populate_registry(c)
> +        c.copy(dest)

Verify result.

@@ +310,5 @@
> +
> +        with open(destfile, 'rt') as fh:
> +            self.assertEqual(fh.read(), 'SOURCE\n')
> +
> +        # Next, modify the source to #INCLUDE anothe file.

Nit: anothe

Comment 43

[Brian O'Keefe [:bokeefe]]

Attachment: Part 3: Add preprocessed files to install manifests (r=gps)

(In reply to Gregory Szorc [:gps] from comment #42)
With nits addressed; this should be good to go. I noticed I obsoleted the wrong patch earlier (I blame autocomplete), so I'll fix that up in a second (before requesting checkin-needed).

Comment 44

[Brian O'Keefe [:bokeefe]]

Comment on attachment 8350034 [details] [diff] [review]
Part 1: Give Preprocessor a more pythonic API

This one shouldn't be obsolete, especially since it's landed

Comment 45

[Gregory Szorc [:gps]]

Comment on attachment 8358159 [details] [diff] [review]
Part 2: Add preprocessed files to mozpack (r=gps)

This fails Python tests:

TEST-UNEXPECTED-FAIL | /Users/gps/src/firefox/python/mozbuild/mozpack/test/test_files.py | line 445, test_replace_symlink: global name 'src' is not defined
TEST-PASS | /Users/gps/src/firefox/python/mozbuild/mozpack/test/test_files.py | test_xpt_file
ERROR: test_replace_symlink (__main__.TestPreprocessedFile)
Traceback (most recent call last):
  File "/Users/gps/src/firefox/python/mozbuild/mozpack/test/test_files.py", line 445, in test_replace_symlink
    f = PreprocessedFile(src, depfile_path=deps, marker='#', defines={'FOO': True})
NameError: global name 'src' is not defined

Comment 46

[Gregory Szorc [:gps]]

Comment on attachment 8358159 [details] [diff] [review]
Part 2: Add preprocessed files to mozpack (r=gps)

I manually fixed the patch.

Comment 47

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cbb166c4a60d
https://hg.mozilla.org/integration/mozilla-inbound/rev/48289161bc17

Comment 48

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/cbb166c4a60d
https://hg.mozilla.org/mozilla-central/rev/48289161bc17