1168607 - Support reading moz.build files from arbitrary Mercurial revisions

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Gregory Szorc [:gps]]

I have a few projects that will benefit from reading moz.build file metadata from arbitrary Mercurial revisions. Having to check out a revision in order to evaluate moz.build files is inefficient. I plan on adding a virtual filesystem layer to moz.build reading and to build a Mercurial virtual filesystem that accesses moz.build file data directly from Mercurial, without having to modify files on the filesystem.

Comment 1

[Gregory Szorc [:gps]]

Attachment: MozReview Request: bz://1168607/gps

/r/9397 - Bug 1168607 - Establish a filesystem abstraction layer for moz.build reading
/r/9399 - Bug 1168607 - Define opener on BuildReader
/r/9401 - Bug 1168607 - Add and use exists method on opener
/r/9403 - Bug 1168607 - Add a virtual filesystem layer for reading from Mercurial

Pull down these commits:

hg pull -r 98131b41dcf233ab13ec220f43256ddac511150b https://reviewboard-hg.mozilla.org/gecko/

Comment 2

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/9403/#review8141

I was lazy and didn't implement tests. But I did test this code a little and it appears to work. If you sent back to me with requests to implement tests, I'd completely understand.

Comment 3

[Mike Hommey [:glandium]]

https://reviewboard.mozilla.org/r/9397/#review8169

::: python/mozbuild/mozbuild/frontend/reader.py:178
(Diff revision 1)
> -    def __init__(self, context, metadata={}):
> +    def __init__(self, context, metadata={}, opener=default_opener):

We kind of have an existing interface for this kind of things, with mozpack.files.FileFinder. I think it would make sense to build this on top of that.

Comment 4

[Mike Hommey [:glandium]]

https://reviewboard.mozilla.org/r/9403/#review8171

Where is the code that instantiates this?

::: python/mozbuild/mozbuild/hg.py:9
(Diff revision 1)
> +modules in this file.

Actually, it might be fine to, because a) the MPL2 is GPL-compatible and b) things that call into this file work fine without, since this implements a common interface. This particular file /might/, itself, need to be GPL, but that's not a given. Disclaimer: I haven't actually checked the relevant sections of the GPL and MPL. Please check with Gerv if you feel like importing mercurial modules instead of invoking command lines.

Comment 5

[Gregory Szorc [:gps]]

gerv: I'm curious what your licensing opinion is here.

Here's the summary. We have a Mozilla-authored Python package - "mozbuild." It's a bunch of Python source files. It is all MPL 2.

One of the Python files in mozbuild defines and implements an interface.

We potentially want to write an implementation of that interface that itself calls into Python code that is licensed GPL v2 or later. What implications would this have for licensing?

In case it matters, the GPL v2 or later code is Mercurial and they've written more licensing details at https://mercurial.selenic.com/wiki/License.

I /think/ glandium is correct here in that our interface that calls into the GPL code can be dual licensed under GPL and MPL 2. If so, how does that work with license boilerplate in the file?

Comment 6

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/9397/#review8199

> We kind of have an existing interface for this kind of things, with mozpack.files.FileFinder. I think it would make sense to build this on top of that.

I'll have a go at it.

Comment 7

[Gregory Szorc [:gps]]

https://www.mozilla.org/MPL/2.0/combining-mpl-and-gpl.html seems to answer most of my needinfo.

Comment 8

[Gregory Szorc [:gps]]

Comment on attachment 8610868 [details]
MozReview Request: bz://1168607/gps

/r/9397 - Bug 1168607 - Refactor FileFinder._find_file to not be a generator; r?glandium
/r/9401 - Bug 1168607 - Add get_file method to finders; r?glandium
/r/9399 - Bug 1168607 - Use mozpack Finders for reading moz.build files; r?glandium

Pull down these commits:

hg pull -r 35bd769842de83a6127ba38d67f29dc3f76faecb https://reviewboard-hg.mozilla.org/gecko/

Comment 9

[Mike Hommey [:glandium]]

https://reviewboard.mozilla.org/r/9401/#review8237

::: python/mozbuild/mozpack/files.py:703
(Diff revision 2)
> +    def get_file(self, path):

def get(...) ?

::: python/mozbuild/mozpack/files.py:786
(Diff revision 2)
>                  yield f

I'd rather keep the _find_* functions as generators, and return ((pattern, self.get(pattern)),) for the file case.

::: python/mozbuild/mozpack/files.py:818
(Diff revision 2)
>              return

explicitly return None?

::: python/mozbuild/mozpack/files.py:822
(Diff revision 2)
>                  return

here too.

::: python/mozbuild/mozpack/files.py:712
(Diff revision 2)
> +        raise NotImplementedError('get_file not implemented')

reading the implementations further down, I wonder if we shouldn't have a default implementation that does something like

files = list(self.find(path))
if len(files) != 1:
    return None
path, file = files[0]
return file

And drop the implementations for ComposedFinder and JarFinder, which aren't going to be used anyways.

Comment 10

[Mike Hommey [:glandium]]

https://reviewboard.mozilla.org/r/9399/#review8245

::: python/mozbuild/mozbuild/frontend/reader.py:85
(Diff revision 2)
> +default_finder = FileFinder('/', find_executables=False)

import the one from Sandbox?

::: python/mozbuild/mozbuild/frontend/reader.py:1078
(Diff revision 2)
> -                if not os.path.exists(source):
> +                if not self._finder.get_file(source):

This is going to conflict with bug 991983 :-/

::: python/mozbuild/mozbuild/frontend/sandbox.py:147
(Diff revision 2)
> -            with open(path, 'rt') as fd:
> +            fh = self._finder.get_file(path).open()

Followup for File.open to return a context-aware object?

Comment 11

[Gregory Szorc [:gps]]

Attachment: MozReview Request: Bug 1168607 - Implement a finder that reads from a Mercurial repo; r?glandium

Bug 1168607 - INCOMPLETE Implement a finder that reads from a Mercurial repo

Now that moz.build files use finders for I/O, we can start reading
moz.build data from other sources.

Version control is essentially a filesystem. We implement a finder
that speaks to Mercurial to obtain file data. It is able to obtain
file data from a specific revision in the repository.

We use the hglib package (which uses the Mercurial command server) for
speaking with Mercurial. This adds overhead compared to consuming the
raw Mercurial APIs. However, it also avoids GPL side-effects of
importing Mercurial's Python modules.

Testing shows that performance is good but not great. We may want to
dual license this file under GPL in the future. For now, something is
better than nothing.

Comment 12

[Gregory Szorc [:gps]]

Attachment: MozReview Request: Bug 1168607 - Refactor FileFinder._find_file to not be a generator; r?glandium

Bug 1168607 - Refactor FileFinder._find_file to not be a generator; r?glandium

An upcoming commit will make _find_file a public API. Since this method
only returns a single instance, refactor it to not be a generator.

Comment 13

[Gregory Szorc [:gps]]

Attachment: MozReview Request: Bug 1168607 - Add get method to finders; r=glandium

Bug 1168607 - Add get_file method to finders; r?glandium

Today, the *Finder classes are optimized for doing matching and
returning multiple results. However, sometimes only looking for a
single file is wanted.

This commit implements the "get_file" method on all the *Finder classes.
It returns a BaseFile or None.

FileFinder._find_file has been refactored into FileFinder.get_file
to avoid code duplication.

Comment 14

[Gregory Szorc [:gps]]

Attachment: MozReview Request: Bug 1168607 - Use mozpack Finders for reading moz.build files; r?glandium

Bug 1168607 - Use mozpack Finders for reading moz.build files; r?glandium

Sometimes moz.build data may not come from the local filesystem. To
support defining moz.build data in other backing stores, we switch
moz.build reading to use mozpack's *Finder classes for I/O.

The use of a FileFinder bound to / is sub-optimal. We should ideally
be creating a finder bound to topsrcdir. However, doing this would
require refactoring a lot of path handling in the reader, as that code
makes many assumptions that paths are absolute. This would be excellent
follow-up work.

While I was here, I cleaned up some linter warnings for unused imports.

On my machine, this commit results in a slight slowdown of moz.build
reading. Before, `mach build-backend` was consistently reporting 1.99s
for reading 2,572 moz.build files. After, it is consistently reporting
2.07s, an increase of 4%. This is likely due to a) function call
overhead b) the cost of instantiating a few thousand File instances
c) FileFinder performing an os.path.exists() before returning File
instances. I believe the regression is tolerable.

Comment 15

[Gregory Szorc [:gps]]

Comment on attachment 8611557 [details]
MozReview Request: Bug 1168607 - Implement a finder that reads from a Mercurial repo; r?glandium

Bug 1168607 - INCOMPLETE Implement a finder that reads from a Mercurial repo

Now that moz.build files use finders for I/O, we can start reading
moz.build data from other sources.

Version control is essentially a filesystem. We implement a finder
that speaks to Mercurial to obtain file data. It is able to obtain
file data from a specific revision in the repository.

We use the hglib package (which uses the Mercurial command server) for
speaking with Mercurial. This adds overhead compared to consuming the
raw Mercurial APIs. However, it also avoids GPL side-effects of
importing Mercurial's Python modules.

Testing shows that performance is good but not great. We may want to
dual license this file under GPL in the future. For now, something is
better than nothing.

Comment 16

[Gregory Szorc [:gps]]

Comment on attachment 8611560 [details]
MozReview Request: Bug 1168607 - Refactor FileFinder._find_file to not be a generator; r?glandium

Bug 1168607 - Refactor FileFinder._find_file to not be a generator; r?glandium

An upcoming commit will make _find_file a public API. Since this method
only returns a single instance, refactor it to not be a generator.

Comment 17

[Gregory Szorc [:gps]]

Comment on attachment 8611561 [details]
MozReview Request: Bug 1168607 - Add get method to finders; r=glandium

Bug 1168607 - Add get_file method to finders; r?glandium

Today, the *Finder classes are optimized for doing matching and
returning multiple results. However, sometimes only looking for a
single file is wanted.

This commit implements the "get_file" method on all the *Finder classes.
It returns a BaseFile or None.

FileFinder._find_file has been refactored into FileFinder.get_file
to avoid code duplication.

Comment 18

[Gregory Szorc [:gps]]

Attachment: MozReview Request: Bug 1168607 - Add a read() method to File; r=glandium

Bug 1168607 - Add a read() method to BaseFile; r?glandium

Passing raw file handles around is a bit dangerous because it could lead
to leaking file descriptors. Add a read() method that handles the simple
case of obtaining the full contents of a BaseFile instance.

Comment 19

[Gregory Szorc [:gps]]

Comment on attachment 8611562 [details]
MozReview Request: Bug 1168607 - Use mozpack Finders for reading moz.build files; r?glandium

Bug 1168607 - Use mozpack Finders for reading moz.build files; r?glandium

Sometimes moz.build data may not come from the local filesystem. To
support defining moz.build data in other backing stores, we switch
moz.build reading to use mozpack's *Finder classes for I/O.

The use of a FileFinder bound to / is sub-optimal. We should ideally
be creating a finder bound to topsrcdir. However, doing this would
require refactoring a lot of path handling in the reader, as that code
makes many assumptions that paths are absolute. This would be excellent
follow-up work.

While I was here, I cleaned up some linter warnings for unused imports.

On my machine, this commit results in a slight slowdown of moz.build
reading. Before, `mach build-backend` was consistently reporting 1.99s
for reading 2,572 moz.build files. After, it is consistently reporting
2.07s, an increase of 4%. This is likely due to a) function call
overhead b) the cost of instantiating a few thousand File instances
c) FileFinder performing an os.path.exists() before returning File
instances. I believe the regression is tolerable.

Comment 20

[Gregory Szorc [:gps]]

Comment on attachment 8611557 [details]
MozReview Request: Bug 1168607 - Implement a finder that reads from a Mercurial repo; r?glandium

Bug 1168607 - INCOMPLETE Implement a finder that reads from a Mercurial repo

Now that moz.build files use finders for I/O, we can start reading
moz.build data from other sources.

Version control is essentially a filesystem. We implement a finder
that speaks to Mercurial to obtain file data. It is able to obtain
file data from a specific revision in the repository.

We use the hglib package (which uses the Mercurial command server) for
speaking with Mercurial. This adds overhead compared to consuming the
raw Mercurial APIs. However, it also avoids GPL side-effects of
importing Mercurial's Python modules.

Testing shows that performance is good but not great. We may want to
dual license this file under GPL in the future. For now, something is
better than nothing.

Comment 21

[Mike Hommey [:glandium]]

Comment on attachment 8611561 [details]
MozReview Request: Bug 1168607 - Add get method to finders; r=glandium

https://reviewboard.mozilla.org/r/9401/#review8281

::: python/mozbuild/mozpack/files.py:803
(Diff revision 4)
> -            for f in self._find_dir(pattern):
> +            return self._find_dir(pattern)

You're reverting the change from the first patch. Drop the first patch instead :)

::: python/mozbuild/mozpack/files.py:716
(Diff revision 4)
> +    def get(self, path):

Now that you changed the function name, the commit message needs updating too.

::: python/mozbuild/mozpack/files.py:827
(Diff revision 4)
>      def _find_file(self, path):

I think you can drop _find_file and make _find itself call get.

Comment 22

[Mike Hommey [:glandium]]

Comment on attachment 8611560 [details]
MozReview Request: Bug 1168607 - Refactor FileFinder._find_file to not be a generator; r?glandium

https://reviewboard.mozilla.org/r/9397/#review8283

As per previous comment, this can be dropped/folded.

Comment 23

[Mike Hommey [:glandium]]

Comment on attachment 8611620 [details]
MozReview Request: Bug 1168607 - Add a read() method to File; r=glandium

https://reviewboard.mozilla.org/r/9527/#review8285

::: python/mozbuild/mozpack/files.py:194
(Diff revision 1)
> +        with open(self.path, 'rb') as fh:

This method doesn't work for subclasses that override open (GeneratedFile, DeflatedFile, XPTFile). I get that you may not want to fix the overall issue, which exists for all other uses of these classes, right now, so feel free to move this method to the File class and file a separate bug for this issue.

Comment 24

[Mike Hommey [:glandium]]

Comment on attachment 8611562 [details]
MozReview Request: Bug 1168607 - Use mozpack Finders for reading moz.build files; r?glandium

https://reviewboard.mozilla.org/r/9399/#review8287

::: python/mozbuild/mozbuild/frontend/reader.py:1074
(Diff revision 4)
> -                if not os.path.exists(source.full_path):
> +                if not self._finder.get(source):

Does that actually work? source is the relative path, and the finder is not bound to topsrcdir... and using the full path won't work with the mercurial finder. Seems to me you want something like self._finder.get(mozpath.relpath(source.full_path, self.config.topsrcdir))

::: python/mozbuild/mozbuild/frontend/sandbox.py:31
(Diff revision 4)
> +default_finder = FileFinder('/', find_executables=False)

I've said it in the past, but I think we now have much more uses of FileFinder with find_executables=False than with the default. Time to file a followup to change the default to the opposite of its current value?

Comment 25

[Mike Hommey [:glandium]]

https://reviewboard.mozilla.org/r/9511/#review8289

::: python/mozbuild/mozpack/hg.py:43
(Diff revision 3)
> +        return BytesIO(self._content)

heh, see, your BaseFile.read method was not reading the right file ;)

::: python/mozbuild/mozpack/hg.py:54
(Diff revision 3)
> +        super(MercurialRevisionFinder, self).__init__(**kwargs)

You need to pass a "base" to BaseFinder.__init__. Seems to me that should be repo.

::: python/mozbuild/mozpack/hg.py:57
(Diff revision 3)
> +        self._client = hglib.open(path=repo, encoding=b'utf-8')

hglib doesn't like unicode? sigh.

::: python/mozbuild/mozpack/hg.py:79
(Diff revision 3)
> +    def get_file(self, path):

this should be get, not get_file

::: python/mozbuild/mozpack/hg.py:77
(Diff revision 3)
> +            yield pattern, self._get_file(pattern)

cf. JarFinder._find you need an else case. The contract is that giving the path to a directory yields all files under it.

You'll also note that this function is very similar to JarFinder._find, it's probably worth factoring.

Comment 26

[Gervase Markham [:gerv]]

Hi gps,

This is an interesting one :-) Thanks for asking.

(In reply to Gregory Szorc [:gps] from comment #5)
> We potentially want to write an implementation of that interface that itself
> calls into Python code that is licensed GPL v2 or later. What implications
> would this have for licensing?
> 
> In case it matters, the GPL v2 or later code is Mercurial and they've
> written more licensing details at https://mercurial.selenic.com/wiki/License.

The relevant bit of their opinion is:

"If you intend to distribute software that includes changes to Mercurial's source code or directly calls Mercurial's internals, your work may be considered a 'derived work' according to copyright law, and you will need to license your work under the GPLv2+ and make its source code available."

So that would suggest that the new code needs to be GPLed. 

Now, it's not supposed to be possible to write a "GPL shim" whose purpose is to be the "GPLed bit" and then interface to some non-GPLed code on the other side, thereby avoiding the GPL's obligations with respect to that code which would apply if you just connected the two directly. On the other hand, it would be hard to argue that mozbuild was a derivative work of Mercurial under copyright law if this interface existed before the Mercurial integration piece was written. I assume the interface in mozbuild is not Mercurial-specific?

> I /think/ glandium is correct here in that our interface that calls into the
> GPL code can be dual licensed under GPL and MPL 2. If so, how does that work
> with license boilerplate in the file?

Do we need to distribute Mercurial, the new piece of code and mozbuild together at any point?

If not, the solution is easy. License the new code under the GPL, with an exception which allows combining/linking with mozbuild while mozbuild is still under the MPL 2. Then the two can be distributed together, keeping their licenses. There's no need to dual-license. Instead of the exception, we could dual-license mozbuild MPL/GPL using the powers in MPL 2 section 3.3, but that seems unnecessary. It seems that licensing tweaks to solve this are best constrained to the new code, if possible.

If we need to distribute all three together, then we would need to check with legal that the logic about mozbuild not being a derivative work of Mercurial holds water, then we could do the above plan anyway.

Does that make sense so far?

Gerv

Comment 27

[Mike Hommey [:glandium]]

> On the other hand, it would be hard to argue that mozbuild was a derivative work of Mercurial under copyright law if this interface existed before the Mercurial integration piece was written. I assume the interface in mozbuild is not Mercurial-specific?

Not at all, the interface was added in bug 780561, almost 2.5 years ago.

Comment 28

[Gervase Markham [:gerv]]

I want to alter the above position slightly.

Mozilla distributes Mercurial on Windows, right, as part of the MozillaBuild package? But mozbuild is not in that package, it's in mozilla-central?

If so, I think the situation is as follows. As long as the new_code is under the GPL, we are not violating the GPL license of Mercurial by distributing the new_code in and of itself, either with Mercurial or on its own. The tricky part comes if we want to distribute all three together.

The GPLv2 says in section 2:

"These requirements apply to the modified work as a whole. If identifiable sections of that work are not derived from the Program, and can be reasonably considered independent and separate works in themselves, then this License, and its terms, do not apply to those sections when you distribute them as separate works. But when you distribute the same sections as part of a whole which is a work based on the Program, the distribution of the whole must be on the terms of this License, whose permissions for other licensees extend to the entire whole, and thus to each and every part regardless of who wrote it. "

I think "mozbuild" would be an "identifiable section of the work which is not derived from the Program [Mercurial], which can be considered an independent and separate work in itself".

So Mercurial+new_code+mozbuild all together would be an unredistributable combination under the GPL, unless the distributor took the option, that the MPL 2 allows, to dual-license mozbuild itself under MPL 2/GPL. But if we don't need to distribute all three together, then we don't have to do anything to the license of mozbuild.

Summary: Assuming we don't distribute Mercurial together with mozbuild/new_code, then we can put new_code under the GPL with an exception for linking with mozbuild (I can provide boilerplate) and everything will be fine. 

Gerv

Comment 29

[Gregory Szorc [:gps]]

mozbuild is part of mozilla-central. And, mozilla-central is not distributed as part of MozillaBuild.

So, I think you are saying we would/could:

1. Create a new file in mozilla-central that uses both the Mercurial (GPL) and mozbuild (MPL 2)
2. License said file under GPL with an exception for linking with mozbuild

If so, I'm on board. Please let me know what the license boilerplate looks like.

Also, can this new file be a) located next to b) distributed with mozbuild? I tentatively have it in python/mozbuild/mozpack/hg.py, which is living next to a bunch of MPL 2 .py files. Everything under python/mozbuild currently constitutes the "mozbuild Python package." We don't yet distribute that outside of mozilla-central. However, I'll likely soon have a need for packaging it and distributing it on PyPI, Python's packaging index (like CPAN). Mercurial is also distributed on PyPI.

If we have to isolate the GPL bits from the rest of mozbuild, I understand and that is doable.

Comment 30

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/9401/#review8581

> You're reverting the change from the first patch. Drop the first patch instead :)

Yeah. Looks like I refreshed the wrong commit. I'll drop the first commit and resubmit.

> I think you can drop _find_file and make _find itself call get.

Actually, we can't since that would be mixing yield and a return with argument in the same function. I'd have to re-add the bits of part 1 that iterated and yielded. Your call.

Comment 31

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #30)
> Actually, we can't since that would be mixing yield and a return with
> argument in the same function. I'd have to re-add the bits of part 1 that
> iterated and yielded. Your call.

You don't have to if you do something like:
  file = self.get(pattern)
  return (file,) if file else ()

Comment 32

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/9399/#review8589

> Does that actually work? source is the relative path, and the finder is not bound to topsrcdir... and using the full path won't work with the mercurial finder. Seems to me you want something like self._finder.get(mozpath.relpath(source.full_path, self.config.topsrcdir))

Ideally, yes, we'd bind the finder to topsrcdir. And, yes, this was broken (I needed to use source.full_path).

In the original implementation of the Mercurial finder, I stored the repo path as the "base" of the finder. Consumers basically specified topsrcdir as the path to the repo and the finder stripped the repo base path to construct relative paths corresponding to the path inside Mercurial. A bit hacky, but it worked. I think the current Mercurial finder implementation in this series is broken. I should hopefully make that work shortly...

Comment 33

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/9399/#review8591

> Ideally, yes, we'd bind the finder to topsrcdir. And, yes, this was broken (I needed to use source.full_path).
> 
> In the original implementation of the Mercurial finder, I stored the repo path as the "base" of the finder. Consumers basically specified topsrcdir as the path to the repo and the finder stripped the repo base path to construct relative paths corresponding to the path inside Mercurial. A bit hacky, but it worked. I think the current Mercurial finder implementation in this series is broken. I should hopefully make that work shortly...

Something else that may cause this to mysteriously work is that `os.path.join(relpath, abspath) == abspath`. I wouldn't be surprised if by passing absolute paths into Finder.find() that the base path of the finder was silently ignored. (This is a security concern in other programs.)

Comment 34

[Gregory Szorc [:gps]]

Comment on attachment 8611561 [details]
MozReview Request: Bug 1168607 - Add get method to finders; r=glandium

Bug 1168607 - Add get method to finders; r?glandium

Today, the *Finder classes are optimized for doing matching and
returning multiple results. However, sometimes only looking for a
single file is wanted.

This commit implements the "get" method on all the *Finder classes.
It returns a BaseFile or None.

FileFinder._find_file has been refactored into FileFinder.get
to avoid code duplication.

Comment 35

[Gregory Szorc [:gps]]

Comment on attachment 8611620 [details]
MozReview Request: Bug 1168607 - Add a read() method to File; r=glandium

Bug 1168607 - Add a read() method to File; r?glandium

Passing raw file handles around is a bit dangerous because it could lead
to leaking file descriptors. Add a read() method that handles the simple
case of obtaining the full contents of a File instance.

This should ideally be a method on BaseFile. But this would require
extra work and isn't needed. So we've deferred it until bug 1170329.

Comment 36

[Gregory Szorc [:gps]]

Comment on attachment 8611562 [details]
MozReview Request: Bug 1168607 - Use mozpack Finders for reading moz.build files; r?glandium

Bug 1168607 - Use mozpack Finders for reading moz.build files; r?glandium

Sometimes moz.build data may not come from the local filesystem. To
support defining moz.build data in other backing stores, we switch
moz.build reading to use mozpack's *Finder classes for I/O.

The use of a FileFinder bound to / is sub-optimal. We should ideally
be creating a finder bound to topsrcdir. However, doing this would
require refactoring a lot of path handling in the reader, as that code
makes many assumptions that paths are absolute. This would be excellent
follow-up work.

While I was here, I cleaned up some linter warnings for unused imports.

On my machine, this commit results in a slight slowdown of moz.build
reading. Before, `mach build-backend` was consistently reporting 1.99s
for reading 2,572 moz.build files. After, it is consistently reporting
2.07s, an increase of 4%. This is likely due to a) function call
overhead b) the cost of instantiating a few thousand File instances
c) FileFinder performing an os.path.exists() before returning File
instances. I believe the regression is tolerable.

Comment 37

[Gregory Szorc [:gps]]

Comment on attachment 8611557 [details]
MozReview Request: Bug 1168607 - Implement a finder that reads from a Mercurial repo; r?glandium

Bug 1168607 - Implement a finder that reads from a Mercurial repo

Now that moz.build files use finders for I/O, we can start reading
moz.build data from other sources.

Version control is essentially a filesystem. We implement a finder
that speaks to Mercurial to obtain file data. It is able to obtain
file data from a specific revision in the repository.

We use the hglib package (which uses the Mercurial command server) for
speaking with Mercurial. This adds overhead compared to consuming the
raw Mercurial APIs. However, it also avoids GPL side-effects of
importing Mercurial's Python modules.

Testing shows that performance is good but not great. We may want to
implement a GPL licensed Mercurial finder in the future. For now, we
need to start somewhere.

Comment 38

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/9511/#review8619

This version of the Mercurial finder works. However, it operates on relative paths, which means it is incompatible with the mozbuild reader, which uses absolute paths. I'm holding off on asking for review until after I have that mess sorted out.

Comment 39

[Mike Hommey [:glandium]]

Comment on attachment 8611561 [details]
MozReview Request: Bug 1168607 - Add get method to finders; r=glandium

https://reviewboard.mozilla.org/r/9401/#review8625

::: python/mozbuild/mozpack/files.py:723
(Diff revision 5)
> +        Returns a ``BaseFile`` if at most 1 file exists or ``None`` otherwise.

s/1/one/

::: python/mozbuild/mozpack/files.py:834
(Diff revision 5)
> +            yield path, f

See comment 31

Comment 40

[Mike Hommey [:glandium]]

Comment on attachment 8611620 [details]
MozReview Request: Bug 1168607 - Add a read() method to File; r=glandium

https://reviewboard.mozilla.org/r/9527/#review8627

::: python/mozbuild/mozpack/files.py:232
(Diff revision 2)
> +        # TODO bug 1170329 to implement read() in BaseFile.

You may want to add a read method to BaseFile, make it raise an exception, and put this comment there.

Comment 41

[Mike Hommey [:glandium]]

Comment on attachment 8611562 [details]
MozReview Request: Bug 1168607 - Use mozpack Finders for reading moz.build files; r?glandium

https://reviewboard.mozilla.org/r/9399/#review8629

::: python/mozbuild/mozbuild/frontend/reader.py:1074
(Diff revision 5)
> -                if not os.path.exists(source.full_path):
> +                if not self._finder.get(source.full_path):

Did you test this on Windows?

Comment 42

[Mike Hommey [:glandium]]

Comment on attachment 8611557 [details]
MozReview Request: Bug 1168607 - Implement a finder that reads from a Mercurial repo; r?glandium

https://reviewboard.mozilla.org/r/9511/#review8631

Ship It!

Comment 43

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/9399/#review8693

> Did you test this on Windows?

No. I'll push to try before landing.

Comment 44

[Gregory Szorc [:gps]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e91f770eaf1

Comment 45

[Gregory Szorc [:gps]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d700a78e7e6b

Comment 46

[Gregory Szorc [:gps]]

Comment on attachment 8611561 [details]
MozReview Request: Bug 1168607 - Add get method to finders; r=glandium

Bug 1168607 - Add get method to finders; r=glandium

Today, the *Finder classes are optimized for doing matching and
returning multiple results. However, sometimes only looking for a
single file is wanted.

This commit implements the "get" method on all the *Finder classes.
It returns a BaseFile or None.

FileFinder._find_file has been refactored into FileFinder.get
to avoid code duplication.

Comment 47

[Gregory Szorc [:gps]]

Comment on attachment 8611620 [details]
MozReview Request: Bug 1168607 - Add a read() method to File; r=glandium

Bug 1168607 - Add a read() method to File; r=glandium

Passing raw file handles around is a bit dangerous because it could lead
to leaking file descriptors. Add a read() method that handles the simple
case of obtaining the full contents of a File instance.

This should ideally be a method on BaseFile. But this would require
extra work and isn't needed. So we've deferred it until bug 1170329.

Comment 48

[Gregory Szorc [:gps]]

Comment on attachment 8611562 [details]
MozReview Request: Bug 1168607 - Use mozpack Finders for reading moz.build files; r?glandium

Bug 1168607 - Use mozpack Finders for reading moz.build files; r=glandium

Sometimes moz.build data may not come from the local filesystem. To
support defining moz.build data in other backing stores, we switch
moz.build reading to use mozpack's *Finder classes for I/O.

The use of a FileFinder bound to / is sub-optimal. We should ideally
be creating a finder bound to topsrcdir. However, doing this would
require refactoring a lot of path handling in the reader, as that code
makes many assumptions that paths are absolute. This would be excellent
follow-up work.

While I was here, I cleaned up some linter warnings for unused imports.

On my machine, this commit results in a slight slowdown of moz.build
reading. Before, `mach build-backend` was consistently reporting 1.99s
for reading 2,572 moz.build files. After, it is consistently reporting
2.07s, an increase of 4%. This is likely due to a) function call
overhead b) the cost of instantiating a few thousand File instances
c) FileFinder performing an os.path.exists() before returning File
instances. I believe the regression is tolerable.

Comment 49

[Gregory Szorc [:gps]]

Comment on attachment 8611557 [details]
MozReview Request: Bug 1168607 - Implement a finder that reads from a Mercurial repo; r?glandium

Bug 1168607 - Implement a finder that reads from a Mercurial repo; r?glandium

Now that moz.build files use finders for I/O, we can start reading
moz.build data from other sources.

Version control is essentially a filesystem. We implement a finder
that speaks to Mercurial to obtain file data. It is able to obtain
file data from a specific revision in the repository.

We use the hglib package (which uses the Mercurial command server) for
speaking with Mercurial. This adds overhead compared to consuming the
raw Mercurial APIs. However, it also avoids GPL side-effects of
importing Mercurial's Python modules.

Testing shows that performance is good but not great. We may want to
implement a GPL licensed Mercurial finder in the future. For now, we
need to start somewhere.

Comment 50

[Gregory Szorc [:gps]]

Attachment: MozReview Request: Bug 1168607 - Add mode to MercurialFileFinder to support non-relative paths; r=glandium

Bug 1168607 - Add mode to MercurialFileFinder to support non-relative paths; r?glandium

The moz.build reader uses absolute paths when referencing moz.build
files. *Finder classes expect paths to be relative to a base. When we
switched the reader to use *Finder instances for I/O, we simply provided
a default FileFinder based at the filesystem root. This was quick and
easy. Things worked.

Unfortunately, that solution isn't ideal because not all *Finder
instances can accept absolute paths like that. The
MercurialRevisionFinder is one of them.

Changing the moz.build reader to talk in terms of relative paths is a
lot of work. While this would be ideal, it is significantly easier to
defer the problem until later and hack up MercurialRevisionFinder to
accept absolute paths. This commit does exactly that.

Bug 1171069 has been filed to track converting BuildReader to relative
paths.

Comment 51

[Gregory Szorc [:gps]]

Attachment: MozReview Request: Bug 1168607 - Make `mach file-info` work with Mercurial repos; r?glandium

Bug 1168607 - Make `mach file-info` work with Mercurial repos; r?glandium

This commit adds support for specifying a Mercurial revision with `mach
file-info`. Aside from being a potentially useful feature, it proves
that MercurialRevisionFinder works with BuildReader.

Comment 52

[Mike Hommey [:glandium]]

https://reviewboard.mozilla.org/r/9401/#review8905

::: python/mozbuild/mozpack/files.py:806
(Diff revision 6)
> +            return [(pattern, f)] if f else []

It would be better to return tuples.

Comment 53

[Mike Hommey [:glandium]]

Comment on attachment 8611557 [details]
MozReview Request: Bug 1168607 - Implement a finder that reads from a Mercurial repo; r?glandium

https://reviewboard.mozilla.org/r/9511/#review8907

Ship It!

::: python/mozbuild/mozpack/hg.py:86
(Diff revision 5)
> +                    yield p, self._get(p)

If you're not going to factor with JarFinder._find (cf. comment 25), can you file a followup bug?

::: python/mozbuild/mozpack/hg.py:101
(Diff revision 5)
> +            self._files[path] = f

While the way you're going to use this is not going to be a problem, if someone ends up doing more with it, this combined with the caching in MercurialFile.read is dangerous. File a followup bug?

Comment 54

[Mike Hommey [:glandium]]

Comment on attachment 8614755 [details]
MozReview Request: Bug 1168607 - Add mode to MercurialFileFinder to support non-relative paths; r=glandium

https://reviewboard.mozilla.org/r/10025/#review8909

Ship It!

Comment 55

[Mike Hommey [:glandium]]

Comment on attachment 8614756 [details]
MozReview Request: Bug 1168607 - Make `mach file-info` work with Mercurial repos; r?glandium

https://reviewboard.mozilla.org/r/10027/#review8911

::: python/mozbuild/mozbuild/frontend/mach_commands.py:152
(Diff revision 1)
>          finder = FileFinder(self.topsrcdir, find_executables=False)

You should setup the MercurialRevisionFinder here, since that's where it will have to be when it supports find() and the InvalidPathException you add below is removed.

::: python/mozbuild/mozbuild/frontend/mach_commands.py:91
(Diff revision 1)
> +    @CommandArgument('--hgrev', help='Mercurial revision to look up info from')

'-r', '--rev', help='Version control revision to look up info from' ?
That would avoid having to have two different options when support for git is added (and choosing the right one should be easy)

Comment 56

[Gregory Szorc [:gps]]

Comment on attachment 8611561 [details]
MozReview Request: Bug 1168607 - Add get method to finders; r=glandium

Bug 1168607 - Add get method to finders; r=glandium

Today, the *Finder classes are optimized for doing matching and
returning multiple results. However, sometimes only looking for a
single file is wanted.

This commit implements the "get" method on all the *Finder classes.
It returns a BaseFile or None.

FileFinder._find_file has been refactored into FileFinder.get
to avoid code duplication.

Comment 57

[Gregory Szorc [:gps]]

Comment on attachment 8611620 [details]
MozReview Request: Bug 1168607 - Add a read() method to File; r=glandium

Bug 1168607 - Add a read() method to File; r=glandium

Passing raw file handles around is a bit dangerous because it could lead
to leaking file descriptors. Add a read() method that handles the simple
case of obtaining the full contents of a File instance.

This should ideally be a method on BaseFile. But this would require
extra work and isn't needed. So we've deferred it until bug 1170329.

Comment 58

[Gregory Szorc [:gps]]

Comment on attachment 8611562 [details]
MozReview Request: Bug 1168607 - Use mozpack Finders for reading moz.build files; r?glandium

Bug 1168607 - Use mozpack Finders for reading moz.build files; r=glandium

Sometimes moz.build data may not come from the local filesystem. To
support defining moz.build data in other backing stores, we switch
moz.build reading to use mozpack's *Finder classes for I/O.

The use of a FileFinder bound to / is sub-optimal. We should ideally
be creating a finder bound to topsrcdir. However, doing this would
require refactoring a lot of path handling in the reader, as that code
makes many assumptions that paths are absolute. This would be excellent
follow-up work.

While I was here, I cleaned up some linter warnings for unused imports.

On my machine, this commit results in a slight slowdown of moz.build
reading. Before, `mach build-backend` was consistently reporting 1.99s
for reading 2,572 moz.build files. After, it is consistently reporting
2.07s, an increase of 4%. This is likely due to a) function call
overhead b) the cost of instantiating a few thousand File instances
c) FileFinder performing an os.path.exists() before returning File
instances. I believe the regression is tolerable.

Comment 59

[Gregory Szorc [:gps]]

Comment on attachment 8611557 [details]
MozReview Request: Bug 1168607 - Implement a finder that reads from a Mercurial repo; r?glandium

Bug 1168607 - Implement a finder that reads from a Mercurial repo; r?glandium

Now that moz.build files use finders for I/O, we can start reading
moz.build data from other sources.

Version control is essentially a filesystem. We implement a finder
that speaks to Mercurial to obtain file data. It is able to obtain
file data from a specific revision in the repository.

We use the hglib package (which uses the Mercurial command server) for
speaking with Mercurial. This adds overhead compared to consuming the
raw Mercurial APIs. However, it also avoids GPL side-effects of
importing Mercurial's Python modules.

Testing shows that performance is good but not great. We may want to
implement a GPL licensed Mercurial finder in the future. For now, we
need to start somewhere.

Comment 60

[Gregory Szorc [:gps]]

Comment on attachment 8614755 [details]
MozReview Request: Bug 1168607 - Add mode to MercurialFileFinder to support non-relative paths; r=glandium

Bug 1168607 - Add mode to MercurialFileFinder to support non-relative paths; r=glandium

The moz.build reader uses absolute paths when referencing moz.build
files. *Finder classes expect paths to be relative to a base. When we
switched the reader to use *Finder instances for I/O, we simply provided
a default FileFinder based at the filesystem root. This was quick and
easy. Things worked.

Unfortunately, that solution isn't ideal because not all *Finder
instances can accept absolute paths like that. The
MercurialRevisionFinder is one of them.

Changing the moz.build reader to talk in terms of relative paths is a
lot of work. While this would be ideal, it is significantly easier to
defer the problem until later and hack up MercurialRevisionFinder to
accept absolute paths. This commit does exactly that.

Bug 1171069 has been filed to track converting BuildReader to relative
paths.

Comment 61

[Gregory Szorc [:gps]]

Comment on attachment 8614756 [details]
MozReview Request: Bug 1168607 - Make `mach file-info` work with Mercurial repos; r?glandium

Bug 1168607 - Make `mach file-info` work with Mercurial repos; r?glandium

This commit adds support for specifying a Mercurial revision with `mach
file-info`. Aside from being a potentially useful feature, it proves
that MercurialRevisionFinder works with BuildReader.

Comment 62

[Gregory Szorc [:gps]]

Comment on attachment 8611562 [details]
MozReview Request: Bug 1168607 - Use mozpack Finders for reading moz.build files; r?glandium

Bug 1168607 - Use mozpack Finders for reading moz.build files; r?glandium

Sometimes moz.build data may not come from the local filesystem. To
support defining moz.build data in other backing stores, we switch
moz.build reading to use mozpack's *Finder classes for I/O.

The use of a FileFinder bound to / is sub-optimal. We should ideally
be creating a finder bound to topsrcdir. However, doing this would
require refactoring a lot of path handling in the reader, as that code
makes many assumptions that paths are absolute. This would be excellent
follow-up work.

While I was here, I cleaned up some linter warnings for unused imports.

On my machine, this commit results in a slight slowdown of moz.build
reading. Before, `mach build-backend` was consistently reporting 1.99s
for reading 2,572 moz.build files. After, it is consistently reporting
2.07s, an increase of 4%. This is likely due to a) function call
overhead b) the cost of instantiating a few thousand File instances
c) FileFinder performing an os.path.exists() before returning File
instances. I believe the regression is tolerable.

Comment 63

[Gregory Szorc [:gps]]

Comment on attachment 8611557 [details]
MozReview Request: Bug 1168607 - Implement a finder that reads from a Mercurial repo; r?glandium

Bug 1168607 - Implement a finder that reads from a Mercurial repo; r?glandium

Now that moz.build files use finders for I/O, we can start reading
moz.build data from other sources.

Version control is essentially a filesystem. We implement a finder
that speaks to Mercurial to obtain file data. It is able to obtain
file data from a specific revision in the repository.

We use the hglib package (which uses the Mercurial command server) for
speaking with Mercurial. This adds overhead compared to consuming the
raw Mercurial APIs. However, it also avoids GPL side-effects of
importing Mercurial's Python modules.

Testing shows that performance is good but not great. A follow-up
commit will introduce a GPL licensed Mercurial finder. For now, get
the base functionality in place.

Comment 64

[Gregory Szorc [:gps]]

Comment on attachment 8614755 [details]
MozReview Request: Bug 1168607 - Add mode to MercurialFileFinder to support non-relative paths; r=glandium

Bug 1168607 - Add mode to MercurialFileFinder to support non-relative paths; r=glandium

The moz.build reader uses absolute paths when referencing moz.build
files. *Finder classes expect paths to be relative to a base. When we
switched the reader to use *Finder instances for I/O, we simply provided
a default FileFinder based at the filesystem root. This was quick and
easy. Things worked.

Unfortunately, that solution isn't ideal because not all *Finder
instances can accept absolute paths like that. The
MercurialRevisionFinder is one of them.

Changing the moz.build reader to talk in terms of relative paths is a
lot of work. While this would be ideal, it is significantly easier to
defer the problem until later and hack up MercurialRevisionFinder to
accept absolute paths. This commit does exactly that.

Bug 1171069 has been filed to track converting BuildReader to relative
paths.

Comment 65

[Gregory Szorc [:gps]]

Comment on attachment 8614756 [details]
MozReview Request: Bug 1168607 - Make `mach file-info` work with Mercurial repos; r?glandium

Bug 1168607 - Make `mach file-info` work with Mercurial repos; r?glandium

This commit adds support for specifying a Mercurial revision with `mach
file-info`. Aside from being a potentially useful feature, it proves
that MercurialRevisionFinder works with BuildReader.

Comment 66

[Gregory Szorc [:gps]]

Attachment: MozReview Request: Bug 1168607 - Add a native Mercurial finder; r?glandium

Bug 1168607 - Add a native Mercurial finder; r?glandium

The hglib Mercurial finder was nice. But it is somewhat slow, as it
involves a separate Mercurial process.

This commit introduces a native Mercurial finder that speaks directly
to a Mercurial repository instance. It is significantly faster.

The new code is isolated to its own file because it imports Mercurial
code, which is GPL.

Comment 67

[Gervase Markham [:gerv]]

Sorry for the delay; I've been on holiday.

(In reply to Gregory Szorc [:gps] from comment #29)
> 1. Create a new file in mozilla-central that uses both the Mercurial (GPL)
> and mozbuild (MPL 2)

Well, it can be more than one file; that's OK too.

> 2. License said file under GPL with an exception for linking with mozbuild

Right.

> If so, I'm on board. Please let me know what the license boilerplate looks
> like.

I would go for the following. Sorry it's long; but you only need it for the glue code. Let me know if you see any problems with it.

Copyright (C) 2015 Mozilla Contributors

This program is free software; you can redistribute it and/or
modify it under the terms of the GNU General Public License
as published by the Free Software Foundation; either version 2
of the License, or (at your option) any later version.

This program is distributed in the hope that it will be useful,
but WITHOUT ANY WARRANTY; without even the implied warranty of
MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
GNU General Public License for more details.

You should have received a copy of the GNU General Public License
along with this program; if not, write to the Free Software
Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.

As a special exception, the copyright holders of this code give you 
permission to combine this code with the software known as 'mozbuild', 
and to distribute those combinations without any restriction 
coming from the use of this file. (The General Public License 
restrictions do apply in other respects; for example, they cover 
modification of the file, and distribution when not combined with
mozbuild.) 

If you modify this code, you may extend this exception to your 
version of the code, but you are not obliged to do so. If you 
do not wish to do so, delete this exception statement from your 
version.

(The exception text is based on https://spdx.org/licenses/GPL-2.0-with-GCC-exception .)

> Also, can this new file be a) located next to b) distributed with mozbuild?

Yes and yes - that's what the exception lets you do.

Gerv

Comment 68

[Gregory Szorc [:gps]]

Comment on attachment 8616747 [details]
MozReview Request: Bug 1168607 - Add a native Mercurial finder; r?glandium

Bug 1168607 - Add a native Mercurial finder; r?glandium

The hglib Mercurial finder was nice. But it is somewhat slow, as it
involves a separate Mercurial process.

This commit introduces a native Mercurial finder that speaks directly
to a Mercurial repository instance. It is significantly faster.

The new code is isolated to its own file because it imports Mercurial
code, which is GPL.

Comment 69

[Mike Hommey [:glandium]]

Comment on attachment 8611561 [details]
MozReview Request: Bug 1168607 - Add get method to finders; r=glandium

https://reviewboard.mozilla.org/r/9401/#review9263

Ship It!

Comment 70

[Mike Hommey [:glandium]]

Comment on attachment 8611620 [details]
MozReview Request: Bug 1168607 - Add a read() method to File; r=glandium

https://reviewboard.mozilla.org/r/9527/#review9265

Ship It!

Comment 71

[Mike Hommey [:glandium]]

Comment on attachment 8611562 [details]
MozReview Request: Bug 1168607 - Use mozpack Finders for reading moz.build files; r?glandium

https://reviewboard.mozilla.org/r/9399/#review9267

::: python/mozbuild/mozbuild/frontend/reader.py:337
(Diff revision 8)
> +        #   1. inspect.getfile calls os.path.exists

You mean getsource file here.

::: python/mozbuild/mozbuild/frontend/reader.py:351
(Diff revision 8)
> +            return lines

You lose any advantage of the line cache. So in practice, for a file with plenty of templates (which is the case for our files that contain templates), you'll be opening and reading the file once per template.
OTOH, what we need from getsourcelines is not extremely complex (in fact, we only need something like inspect.getblock(source.splitlines()[func.func_code.co_firstlineno - 1:])), and, we actually already read the executed source in exec_file. It seems to me it would be better to use that source we already read instead of making inspect read through our I/O layer with monkeypatching.

Now, considering timing and all, I might as well get a shot at it and get my own changes to template reading landed.

Comment 72

[Mike Hommey [:glandium]]

Comment on attachment 8611557 [details]
MozReview Request: Bug 1168607 - Implement a finder that reads from a Mercurial repo; r?glandium

https://reviewboard.mozilla.org/r/9511/#review9271

I'm not sure this is very useful considering the other patch that adds mercurial support. Especially considering that hglib is not in the tree, and that hglib requires hg. So that's two dependencies instead of one. And as you noted, it's slower.

::: python/mozbuild/mozpack/test/test_files.py:34
(Diff revision 7)
> +    hglib = None

You should be able to simply do "from mozpack.files import hglib" with the same effect.

::: python/mozbuild/mozpack/files.py:987
(Diff revision 7)
> +            raise Exception('hglib package not found')

There's probably a better exception type for this, but meh.

::: python/mozbuild/mozpack/files.py:1005
(Diff revision 7)
> +        out = self._client.rawcommand([b'files', b'--rev', self._rev])

why stop using the cmdbuilder?

::: python/mozbuild/mozpack/test/test_files.py:1068
(Diff revisions 5 - 7)
> +        c.rawcommand(['update', 'null'])

Please add a comment that this updates to an empty revision, which means it removes anything from the working tree, ensuring that the finder is not picking up files from the filesystem.

Comment 73

[Mike Hommey [:glandium]]

Comment on attachment 8614755 [details]
MozReview Request: Bug 1168607 - Add mode to MercurialFileFinder to support non-relative paths; r=glandium

https://reviewboard.mozilla.org/r/10025/#review9275

> Changing the moz.build reader to talk in terms of relative paths is a lot of work.

Is that actually true? there are few places using the finder in the reader, and they could all use relpath couldn't they?

Comment 74

[Mike Hommey [:glandium]]

Comment on attachment 8614756 [details]
MozReview Request: Bug 1168607 - Make `mach file-info` work with Mercurial repos; r?glandium

https://reviewboard.mozilla.org/r/10027/#review9277

::: python/mozbuild/mozbuild/frontend/mach_commands.py:172
(Diff revision 3)
> +                raise InvalidPathException('cannot use wildcard in version control mode')

Note that if you really wanted to, you could add a mode to find where it only yields paths, in which case the use of wildcards would be supported.

::: python/mozbuild/mozbuild/frontend/mach_commands.py:161
(Diff revision 3)
> +            reader_finder = default_finder

I think this should just be something like:
if repo:
    finder = MercurialRevisionFinder(...)
else:
    finder = FileFinder(self.topsrcdir, find_executables=False)

as in, you don't need two different finders (finder & reader_finder), and you don't need to change the current behavior without mercurial.

Comment 75

[Mike Hommey [:glandium]]

Comment on attachment 8616747 [details]
MozReview Request: Bug 1168607 - Add a native Mercurial finder; r?glandium

https://reviewboard.mozilla.org/r/10465/#review9279

::: python/mozbuild/mozpack/hg.py:32
(Diff revision 2)
> +            ui = hgui.ui()
> +            repo = hg.repository(ui, repo)

You could merge both lines.

::: python/mozbuild/mozpack/hg.py:32
(Diff revision 2)
> +            ui = hgui.ui()
> +            repo = hg.repository(ui, repo)

You could merge both lines.

::: python/mozbuild/mozpack/hg.py:41
(Diff revision 2)
> +        self._root = mozpath.normpath(path).rstrip('/')

normpath already removes trailing slashes.

::: python/mozbuild/mozpack/hg.py:1
(Diff revision 2)
> +# TODO add license

Do it :)

Note that it /might/ make sense to hg mv mozpack/files.py to mozpack/files/__init__.py and put hg.py in mozpack/files.

Comment 76

[Mike Hommey [:glandium]]

Attachment: MozReview Request: Bug 1172800 - Fixup after bug 991983

Bug 1172800 - Fixup after bug 991983

Comment 77

[Mike Hommey [:glandium]]

Attachment: MozReview Request: Bug 1172800 - Move moz.build template handling in a separate class

Bug 1172800 - Move moz.build template handling in a separate class

This will make subsequent refactorings a bit more intelligible.

Comment 78

[Mike Hommey [:glandium]]

Attachment: MozReview Request: Bug 1172800 - Avoid using inspect.getsourcelines() and inspect.getfile()

Bug 1172800 - Avoid using inspect.getsourcelines() and inspect.getfile()

inspect.getsourcelines() and inspect.getfile() involve I/O out of our control.
Our use of those functions, however, doesn't require all their smarts. In fact,
we only use them on function objects, for which we can just do the work
ourselves without involving inspect functions that trigger I/O.

Comment 79

[Mike Hommey [:glandium]]

Attachment: MozReview Request: Bug 1172800 - Create actual functions to execute moz.build templates

Bug 1172800 - Create actual functions to execute moz.build templates

The current mode of execution of templates doesn't allow them to more advanced
control flow, like returning early, returning or yielding values, because that
mode of execution is equivalent to running the code at the top level of a .py
file.

Making the templates executed through a function call, although trickier,
allows those control flows, which will be useful for template as context
managers.

Comment 80

[Mike Hommey [:glandium]]

Sorry for the noise, I used the wrong reviewid for mozreview.

Comment 81

[Mike Hommey [:glandium]]

Comment on attachment 8611562 [details]
MozReview Request: Bug 1168607 - Use mozpack Finders for reading moz.build files; r?glandium

https://reviewboard.mozilla.org/r/9399/#review9283

Bug 1172800 addresses my comments here, so this is a r+ as long as it's rebased on top of that (which essentially means dropping the inspect-related hunks).

Comment 82

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/9511/#review9337

> why stop using the cmdbuilder?

I didn't feel like dealing with the extra import. All it does is build a list in a slightly more Pythonic manner. Meh.

Comment 83

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/10025/#review9339

I concede I was being lazy here. It would be a good follow-up bug.

Comment 84

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/10027/#review9341

> I think this should just be something like:
> if repo:
>     finder = MercurialRevisionFinder(...)
> else:
>     finder = FileFinder(self.topsrcdir, find_executables=False)
> 
> as in, you don't need two different finders (finder & reader_finder), and you don't need to change the current behavior without mercurial.

It's not that simple. The local `finder` is used to expand relative paths whereas the `reader_finder` is anchored at /. Plus you have the bit where the Mercurial finder doesn't work with .find. I'll add a comment.

Comment 85

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/187dfb251703
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a470efc9fc2
https://hg.mozilla.org/integration/mozilla-inbound/rev/4001a09e7285
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab4010e0785f
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b4a8275f7f2
https://hg.mozilla.org/integration/mozilla-inbound/rev/96934755c9c5
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7d7921c9345

Comment 86

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/187dfb251703
https://hg.mozilla.org/mozilla-central/rev/1a470efc9fc2
https://hg.mozilla.org/mozilla-central/rev/4001a09e7285
https://hg.mozilla.org/mozilla-central/rev/ab4010e0785f
https://hg.mozilla.org/mozilla-central/rev/1b4a8275f7f2
https://hg.mozilla.org/mozilla-central/rev/96934755c9c5
https://hg.mozilla.org/mozilla-central/rev/a7d7921c9345

Comment 87

[Gregory Szorc [:gps]]

Attachment: MozReview Request: Bug 1168607 - Refactor FileFinder._find_file to not be a generator; r?glandium

Comment 88

[Gregory Szorc [:gps]]

Attachment: MozReview Request: Bug 1168607 - Add a virtual filesystem layer for reading from Mercurial