Define file-based metadata in moz.build files

RESOLVED FIXED in Firefox 39

Status

defect
RESOLVED FIXED
4 years ago
Last year

People

(Reporter: gps, Assigned: gps)

Tracking

(Blocks 8 bugs)

unspecified
mozilla39
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(18 attachments, 2 obsolete attachments)

39 bytes, text/x-review-board-request
glandium
: review+
Details
39 bytes, text/x-review-board-request
glandium
: review+
Details
39 bytes, text/x-review-board-request
glandium
: review+
Details
39 bytes, text/x-review-board-request
glandium
: review+
Details
39 bytes, text/x-review-board-request
glandium
: review+
Details
39 bytes, text/x-review-board-request
glandium
: review+
Details
39 bytes, text/x-review-board-request
glandium
: review+
Details
39 bytes, text/x-review-board-request
glandium
: review+
Details
39 bytes, text/x-review-board-request
glandium
: review+
Details
39 bytes, text/x-review-board-request
glandium
: review+
Details
39 bytes, text/x-review-board-request
glandium
: review+
Details
39 bytes, text/x-review-board-request
glandium
: review+
Details
39 bytes, text/x-review-board-request
glandium
: review+
Details
39 bytes, text/x-review-board-request
glandium
: review+
Details
39 bytes, text/x-review-board-request
glandium
: review+
Details
39 bytes, text/x-review-board-request
glandium
: review+
Details
39 bytes, text/x-review-board-request
glandium
: review+
Details
39 bytes, text/x-review-board-request
glandium
: review+
Details
Assignee

Description

4 years ago
We want to capture file-based metadata in moz.build files. Bugzilla components. Default reviewers, etc.

See discussion at https://groups.google.com/d/msg/mozilla.dev.platform/iXr70VgapWk/GkTCcKRjNi8J. I plan to use this bug to track getting at least the reading moz.build part of that implemented.
Assignee

Comment 1

4 years ago
Posted file MozReview Request: bz://1132771/gps (obsolete) —
/r/3819 - Bug 1132771 - API to return moz.build files relevant for a set of paths
/r/3821 - Bug 1132771 - Provide a back door to disable reading of GYP files
/r/3823 - Bug 1132771 - Support explicit directory traversal of moz.build files

Pull down these commits:

hg pull review -r fc09e0d45d7120f5958f1cffa1f914855f4e808c
Assignee

Comment 2

4 years ago
Comment on attachment 8563835 [details]
MozReview Request: bz://1132771/gps

/r/3819 - Bug 1132771 - API to return moz.build files relevant for a set of paths
/r/3821 - Bug 1132771 - Provide a back door to disable reading of GYP files
/r/3823 - Bug 1132771 - Support explicit directory traversal of moz.build files
/r/3825 - Bug 1132771 - Add a test for reading all moz.build files in file metadata mode
/r/3827 - Bug 1132771 - Define source flags next to source files in moz.build
/r/3829 - Bug 1132771 - Optionally don't export variables when reading moz.build files

Pull down these commits:

hg pull review -r 8b07005d6b4976fe0a0a55159aff1301e622ad55
Assignee

Comment 3

4 years ago
https://reviewboard.mozilla.org/r/3817/#review3043

I'm shocked by this result, but on my build configuration on OS X, I can read every moz.build file in directory hierarchy traversal mode with this set of patches. It only took 2 fixes.

The bigger challenge will be for moz.build files to execute when no config.status is present. That series is going to be "fun." We may want to defer that until later.
Assignee

Comment 4

4 years ago
Comment on attachment 8563835 [details]
MozReview Request: bz://1132771/gps

/r/3819 - Bug 1132771 - API to return moz.build files relevant for a set of paths
/r/3821 - Bug 1132771 - Provide a back door to disable reading of GYP files
/r/3823 - Bug 1132771 - Support explicit directory traversal of moz.build files
/r/3827 - Bug 1132771 - Define source flags next to source files in moz.build
/r/3829 - Bug 1132771 - Optionally don't export variables when reading moz.build files
/r/3825 - Bug 1132771 - Add a test for reading all moz.build files in file metadata mode

Pull down these commits:

hg pull review -r a0e3538ccc3b0544379220a3346986257a47b9ad
Attachment #8563835 - Flags: review?(mh+mozilla)
Attachment #8563835 - Flags: review?(jmuizelaar)
Attachment #8563835 - Flags: review?(jmuizelaar)
Comment on attachment 8563835 [details]
MozReview Request: bz://1132771/gps

https://reviewboard.mozilla.org/r/3817/#review3049

The moz.build file is programmatically generated. Can you modify the generator instead.
Blocks: 1130343
Assignee

Comment 6

4 years ago
https://reviewboard.mozilla.org/r/3817/#review3061

That's what the comment at the top of the file says. However, I couldn't figure out how exactly where this .flags was coming from. It looked manually inserted to me. The generator script is `gfx/skia/generate_mozbuild.py`, correct?
Assignee

Comment 7

4 years ago
Comment on attachment 8563835 [details]
MozReview Request: bz://1132771/gps

/r/3933 - Bug 1132771 - Allow setter functions in FlagsFactory
/r/3819 - Bug 1132771 - API to return moz.build files relevant for a set of paths
/r/3821 - Bug 1132771 - Provide a back door to disable reading of GYP files
/r/3823 - Bug 1132771 - Support explicit directory traversal of moz.build files
/r/3827 - Bug 1132771 - Define source flags next to source files in moz.build
/r/3829 - Bug 1132771 - Optionally don't export variables when reading moz.build files
/r/3825 - Bug 1132771 - Add a test for reading all moz.build files in file metadata mode
/r/3935 - Bug 1132771 - Add FILES to moz.build with ability to define Bugzilla component

Pull down these commits:

hg pull review -r 60b6da165f340573de4043f1ff2d234b1a878fbd
Assignee

Comment 8

4 years ago
Comment on attachment 8563835 [details]
MozReview Request: bz://1132771/gps

/r/3933 - Bug 1132771 - Allow setter functions in FlagsFactory
/r/3819 - Bug 1132771 - API to return moz.build files relevant for a set of paths
/r/3821 - Bug 1132771 - Provide a back door to disable reading of GYP files
/r/3823 - Bug 1132771 - Support explicit directory traversal of moz.build files
/r/3827 - Bug 1132771 - Define source flags next to source files in moz.build
/r/3829 - Bug 1132771 - Optionally don't export variables when reading moz.build files
/r/3825 - Bug 1132771 - Add a test for reading all moz.build files in file metadata mode
/r/3935 - Bug 1132771 - Add FILES to moz.build with ability to define Bugzilla component

Pull down these commits:

hg pull review -r 60b6da165f340573de4043f1ff2d234b1a878fbd
Assignee

Comment 9

4 years ago
Comment on attachment 8563835 [details]
MozReview Request: bz://1132771/gps

/r/3933 - Bug 1132771 - Allow setter functions in FlagsFactory
/r/3819 - Bug 1132771 - API to return moz.build files relevant for a set of paths
/r/3821 - Bug 1132771 - Provide a back door to disable reading of GYP files
/r/3823 - Bug 1132771 - Support explicit directory traversal of moz.build files
/r/3827 - Bug 1132771 - Define source flags next to source files in moz.build
/r/3829 - Bug 1132771 - Optionally don't export variables when reading moz.build files
/r/3825 - Bug 1132771 - Add a test for reading all moz.build files in file metadata mode
/r/3935 - Bug 1132771 - Add FILES to moz.build with ability to define Bugzilla component
/r/3937 - Bug 1132771 - Define some bug components
/r/3939 - Bug 1132771 - Implement file-info mach command

Pull down these commits:

hg pull review -r d355b8fdac928dc8aa8de32933e672e1ee45251a
Assignee

Updated

4 years ago
Depends on: 1134028
Assignee

Comment 10

4 years ago
Comment on attachment 8563835 [details]
MozReview Request: bz://1132771/gps

/r/3827 - Bug 1132771 - Define source flags next to source files in moz.build
/r/3933 - Bug 1132771 - Allow setter functions in FlagsFactory
/r/3819 - Bug 1132771 - API to return moz.build files relevant for a set of paths
/r/3821 - Bug 1132771 - Provide a back door to disable reading of GYP files
/r/3823 - Bug 1132771 - Support explicit directory traversal of moz.build files
/r/3829 - Bug 1132771 - Optionally don't export variables when reading moz.build files
/r/3825 - Bug 1132771 - Add a test for reading all moz.build files in file metadata mode
/r/3935 - Bug 1132771 - Add FILES to moz.build with ability to define Bugzilla component
/r/3937 - Bug 1132771 - Define some bug components
/r/3939 - Bug 1132771 - Implement file-info mach command
/r/3947 - Bug 1132771 - Support for defining code reviewer metadata

Pull down these commits:

hg pull review -r a76af40bf6600a20583cb78dec1e96fb8c53e728
Assignee

Comment 11

4 years ago
https://reviewboard.mozilla.org/r/3935/#review3125

::: python/mozbuild/mozbuild/frontend/context.py
(Diff revision 3)
> +        ``**/*.cpp`` will match all ``.cpp`` files in child directories.

As I start to define metadata later in the series, I find myself revisiting this variable.

We end up with patterns like:

    FILES['**'].bug_component = ('Core', 'Build Config')
    FILES['**'].reviewers['gps@mozilla.com'] = True
    FILES['**'].reviewers['mh+mozilla@glandium.org'] = True
    
That seems like a lot of redundancy. And I haven't even done things like implement rules for individual files, which is needed for the root directory.

Perhaps we should make FILES a list, with each entry having a pattern or set of files hung off. e.g.

    all_files = FILES.new_entry()
    all_files.pattern = '**'
    all_files.bug_component = ('Core', 'Build Config')
    all_files.reviewers['gps@mozilla.com'] = True
    all_files.reviewers['mh+mozilla@glandium.org'] = True

Let the bikeshedding begin!
(In reply to Gregory Szorc [:gps] from comment #11)
> Perhaps we should make FILES a list, with each entry having a pattern or set
> of files hung off. e.g.
> 
>     all_files = FILES.new_entry()
>     all_files.pattern = '**'

# Will throw if provided pattern(s) already exist in FILES.
# Can say |FILES.new_entry('configure.in', 'Makefile.in', ...)|
all_files = FILES.new_entry('**')

>     all_files.bug_component = ('Core', 'Build Config')
>     all_files.reviewers['gps@mozilla.com'] = True
>     all_files.reviewers['mh+mozilla@glandium.org'] = True

Why not just:

all_files.reviewers = ['gps@...', 'mh_mozilla@...]

?
(In reply to Gregory Szorc [:gps] from comment #11)
> https://reviewboard.mozilla.org/r/3935/#review3125
> 
> ::: python/mozbuild/mozbuild/frontend/context.py
> (Diff revision 3)
> > +        ``**/*.cpp`` will match all ``.cpp`` files in child directories.
> 
> As I start to define metadata later in the series, I find myself revisiting
> this variable.
> 
> We end up with patterns like:
> 
>     FILES['**'].bug_component = ('Core', 'Build Config')
>     FILES['**'].reviewers['gps@mozilla.com'] = True
>     FILES['**'].reviewers['mh+mozilla@glandium.org'] = True

files = FILES['**']
files.bug_component = ...
files.reviewers = ...

or, as something I actually want to do for other things (using "magic" contexts):
with FILES['**']:
    bug_component = ...
    reviewers = ...
Assignee

Updated

4 years ago
Depends on: 1134072
Assignee

Comment 14

4 years ago
Comment on attachment 8563835 [details]
MozReview Request: bz://1132771/gps

/r/3827 - Bug 1132771 - Define source flags next to source files in moz.build
/r/3933 - Bug 1132771 - Allow setter functions in FlagsFactory
/r/3819 - Bug 1132771 - API to return moz.build files relevant for a set of paths
/r/3821 - Bug 1132771 - Provide a back door to disable reading of GYP files
/r/3823 - Bug 1132771 - Support explicit directory traversal of moz.build files
/r/3829 - Bug 1132771 - Optionally don't export variables when reading moz.build files
/r/3825 - Bug 1132771 - Add a test for reading all moz.build files in file metadata mode
/r/3935 - Bug 1132771 - Add FILES to moz.build with ability to define Bugzilla component
/r/3937 - Bug 1132771 - Define some bug components
/r/3939 - Bug 1132771 - Implement file-info mach command
/r/3947 - Bug 1132771 - Support for defining code reviewer metadata

Pull down these commits:

hg pull review -r 411dd2685e2349fcdce65d9585cfc8c8d92f00ab
Assignee

Comment 15

4 years ago
https://reviewboard.mozilla.org/r/3817/#review3181

I've ported this to use context managers and to treat FILES as a context. It works and I like the syntax.

This series is pretty much a port of the existing code to work as context managers. I think I'd like to further change how sub-contexts integrate with moz.build reading. Specifically, I'd like the reader to emit the multiple Context instances directly instead of having to fish them out of variables like FILES. Expect more iterations on this to come.

If you have any thoughts on anything, please speak up. The work feels subject to strong opinions.
Assignee

Comment 17

4 years ago
Comment on attachment 8563835 [details]
MozReview Request: bz://1132771/gps

/r/3827 - Bug 1132771 - Define source flags next to source files in moz.build
/r/3933 - Bug 1132771 - Allow setter functions in FlagsFactory
/r/3819 - Bug 1132771 - API to return moz.build files relevant for a set of paths
/r/3821 - Bug 1132771 - Provide a back door to disable reading of GYP files
/r/3823 - Bug 1132771 - Support explicit directory traversal of moz.build files
/r/3829 - Bug 1132771 - Optionally don't export variables when reading moz.build files
/r/3825 - Bug 1132771 - Add a test for reading all moz.build files in filesystem traversal mode
/r/3935 - Bug 1132771 - Add FILES to moz.build with ability to define Bugzilla component
/r/4037 - Bug 1132771 - Handle FILES contexts in emitter
/r/3937 - Bug 1132771 - Define some bug components
/r/3939 - Bug 1132771 - Implement file-info mach command
/r/3947 - Bug 1132771 - Support for defining code reviewer metadata
/r/4039 - Bug 1132771 - Document sub-contexts in Sphinx
/r/4041 - Bug 1132771 - moz.build fixups to enable execution in no config mode
/r/4043 - Bug 1132771 - Support and test for reading without a config object

Pull down these commits:

hg pull review -r 021f3a9f420911b5b0c54879160f69ad096f672f
Assignee

Comment 20

4 years ago
Comment on attachment 8563835 [details]
MozReview Request: bz://1132771/gps

/r/3827 - Bug 1132771 - Define source flags next to source files in moz.build
/r/3933 - Bug 1132771 - Allow setter functions in FlagsFactory
/r/3819 - Bug 1132771 - API to return moz.build files relevant for a set of paths
/r/3821 - Bug 1132771 - Provide a back door to disable reading of GYP files
/r/3823 - Bug 1132771 - Support explicit directory traversal of moz.build files
/r/3829 - Bug 1132771 - Optionally don't export variables when reading moz.build files
/r/3825 - Bug 1132771 - Add a test for reading all moz.build files in filesystem traversal mode
/r/3935 - Bug 1132771 - Add FILES to moz.build with ability to define Bugzilla component
/r/4037 - Bug 1132771 - Handle FILES contexts in emitter
/r/3937 - Bug 1132771 - Define some bug components
/r/3939 - Bug 1132771 - Implement file-info mach command
/r/3947 - Bug 1132771 - Support for defining code reviewer metadata
/r/4039 - Bug 1132771 - Document sub-contexts in Sphinx
/r/4041 - Bug 1132771 - moz.build fixups to enable execution in no config mode
/r/4043 - Bug 1132771 - Support and test for reading without a config object

Pull down these commits:

hg pull review -r 8b0279673d8de1c69f53944cc1a48a1c9eafe739
Assignee

Comment 21

4 years ago
Comment on attachment 8563835 [details]
MozReview Request: bz://1132771/gps

/r/3827 - Bug 1132771 - Define source flags next to source files in moz.build
/r/3933 - Bug 1132771 - Allow setter functions in FlagsFactory
/r/3819 - Bug 1132771 - API to return moz.build files relevant for a set of paths
/r/3821 - Bug 1132771 - Provide a back door to disable reading of GYP files
/r/3823 - Bug 1132771 - Support explicit directory traversal of moz.build files
/r/3829 - Bug 1132771 - Optionally don't export variables when reading moz.build files
/r/3825 - Bug 1132771 - Add a test for reading all moz.build files in filesystem traversal mode
/r/3935 - Bug 1132771 - Add FILES to moz.build with ability to define Bugzilla component
/r/4037 - Bug 1132771 - Handle FILES contexts in emitter
/r/3937 - Bug 1132771 - Define some bug components
/r/3939 - Bug 1132771 - Implement file-info mach command
/r/3947 - Bug 1132771 - Support for defining code reviewer metadata
/r/4039 - Bug 1132771 - Document sub-contexts in Sphinx
/r/4041 - Bug 1132771 - moz.build fixups to enable execution in no config mode
/r/4043 - Bug 1132771 - Support and test for reading without a config object
/r/4063 - Bug 1132771 - Define some reviewers
/r/4065 - Bug 1132771 - Add ability to audit file metadata

Pull down these commits:

hg pull review -r a760a4b171fd952e17738315088943a87e539bf8
Assignee

Comment 22

4 years ago
Comment on attachment 8563835 [details]
MozReview Request: bz://1132771/gps

/r/3827 - Bug 1132771 - Define source flags next to source files in moz.build
/r/3819 - Bug 1132771 - API to return moz.build files relevant for a set of paths
/r/3821 - Bug 1132771 - Provide a back door to disable reading of GYP files
/r/3823 - Bug 1132771 - Support explicit directory traversal of moz.build files
/r/3829 - Bug 1132771 - Optionally don't export variables when reading moz.build files
/r/3825 - Bug 1132771 - Add a test for reading all moz.build files in filesystem traversal mode
/r/3935 - Bug 1132771 - Add Files to moz.build with ability to define Bugzilla component
/r/4037 - Bug 1132771 - Handle Files contexts in emitter
/r/3937 - Bug 1132771 - Define some bug components
/r/3939 - Bug 1132771 - Implement file-info mach command
/r/3947 - Bug 1132771 - Support for defining code reviewer metadata
/r/4041 - Bug 1132771 - moz.build fixups to enable execution in no config mode
/r/4043 - Bug 1132771 - Support and test for reading without a config object
/r/4063 - Bug 1132771 - Define some reviewers
/r/4065 - Bug 1132771 - Add ability to audit file metadata

Pull down these commits:

hg pull review -r b47b99977c0c10f8dc7db60e11ef59e150dcb3b7
https://reviewboard.mozilla.org/r/3819/#review3377

::: python/mozbuild/mozbuild/frontend/reader.py
(Diff revision 7)
> +        moz.build paths.

Is the latter really useful?

::: python/mozbuild/mozbuild/frontend/reader.py
(Diff revision 7)
> +        for path in sorted(paths, key=len):

note that the normal sorting order for strings places 'foo' before 'foo/bar', so key=len is not necessarily useful.

::: python/mozbuild/mozbuild/frontend/reader.py
(Diff revision 7)
> +            if not path.startswith(root):

I'd do something like this instead (untested):
@memoize
def exists(path):
    return os.path.exists(path)

def itermozbuild(path):
    subpath = ''
    yield 'moz.build'
    for part in mozpath.split(path):
        subpath = mozpath.join(subpath, part)
        yield mozpath.join(subpath, 'moz.build')

for path in sorted(paths):
    path = moz.path.normpath(path)
    if os.path.isabs(path):
        if not mozpath.basedir(path, [root]):
            raise Exception()
        path = moz.path.relpath(path, root)

    relevant[path] = [p for p in itermozbuild(path)
                      if exists(mozpath.join(root, p))]

Note this also works for the case where the given path is not a directory because $path/moz.build doesn't exist in that case (and doesn't do more stats either, since we just replace a stat on $path with a stat with $path/moz.build)

This also has the advantage of not re-stat()ing directories that don't have a moz.build if the original path list contains two paths under such directories.

You could even go further with:
def normalize_path(path):
    path = moz.path.normpath(path)
    if os.path.isabs(path):
        if not mozpath.basedir(path, [root]):
            raise Exception()
        path = moz.path.relpath(path, root)
    return path

return { p: [p for p in itermozbuild(path)
             if exists(mozpath.join(root, p)]
         for path in sorted(normalize_path(p) for p in paths) }

which arguably would be better for sorted()
https://reviewboard.mozilla.org/r/3821/#review3381

::: python/mozbuild/mozbuild/frontend/reader.py
(Diff revision 7)
> -            gyp_dir = context['GYP_DIRS'][target_dir]
> +            for target_dir in context['GYP_DIRS']:

You need to rebase on top of bug 1135984.

You could also just do:
    gyp_dirs = context.get('GYP_DIRS', []) if read_gyp else []
    for target_dir in gyp_dirs:
        ...

which would avoid the reindent.
https://reviewboard.mozilla.org/r/3823/#review3383

::: python/mozbuild/mozbuild/frontend/reader.py
(Diff revision 7)
> +            path_mozbuilds[path] = [mozpath.join(topsrcdir, p) for p in mbpaths]

Note this is likely to completely fail with c-c.

::: python/mozbuild/mozbuild/frontend/reader.py
(Diff revision 7)
> +        dirs = {}

might as well use a defaultdict(set)

::: python/mozbuild/mozbuild/frontend/reader.py
(Diff revision 7)
> +        contexts = {}

defaultdict(list)

::: python/mozbuild/mozbuild/test/frontend/test_reader.py
(Diff revision 7)
> +        reader = self.reader('reader-relevant-mozbuild')

The reader-relevant-mozbuild files are missing.

::: python/mozbuild/mozbuild/test/frontend/test_reader.py
(Diff revision 7)
> +        self.assertEqual(len(paths), 3)

You're testing the same thing twice. You should also test the contents of paths.

::: python/mozbuild/mozbuild/frontend/reader.py
(Diff revision 7)
> +            result[path] = ctxs

result[path] = reduce(lambda x, y: x + y, (contexts[p] for p in paths), [])

or

result[path] = itertools.chain(*(contexts[p] for p in paths))

That said, it seems to me this function should limit itself to returning the contexts (or better, yield them), and leave it to the caller to do classification if it wants to. If different callers need the classification, a helper function can be added for that.

::: python/mozbuild/mozbuild/frontend/reader.py
(Diff revision 7)
> +            dirs = [('EXPLICIT_DIRS', [SourcePath(context, d) for d in reldirs])]

As an alternative to adding yet another parameter to this function, and acting accordingly, you could delay the the assignment of the dirs variable until after the context is yielded, and modify context['DIRS'], context['TEST_DIRS'] and context['GYP_DIRS'] in the caller (which also means you could skip the previous patch).
https://reviewboard.mozilla.org/r/3829/#review3385

::: python/mozbuild/mozbuild/frontend/reader.py
(Diff revision 8)
> -                      read_gyp=True, explicit_dirs=None, metadata={}):
> +                      read_gyp=True, explicit_dirs=None, process_exports=True,

I'd rather have a way to give the sandboxes a different list of functions and make export() a noop through that than add another option to read_mozbuild.
https://reviewboard.mozilla.org/r/3825/#review3387

::: config/tests/test_mozbuild_reading.py
(Diff revision 8)
> +        paths, contexts = reader.read_relevant_mozbuilds(reader.all_mozbuild_paths())

That doesn't test paths matches all_mozbuild_paths().
https://reviewboard.mozilla.org/r/3935/#review3389

::: python/mozbuild/mozbuild/frontend/context.py
(Diff revision 7)
> +class BugzillaComponent(object):

class BugzillaComponent(collections.namedtuple('BugzillaComponent', ('product', 'component'))):
    def __new__(cls, tuple):
        return super(BugzillaComponent, cls)._make(tuple)

We probably want type validation on each value, so some other generic type like namedtuple, with type validation would be better than a specific type.

::: python/mozbuild/mozbuild/frontend/context.py
(Diff revision 7)
> +        'FINAL': (set, set,

Considering most everything else is based on lists and works with FOO += [], FINAL.add() seems an odd case.

::: python/mozbuild/mozbuild/frontend/reader.py
(Diff revision 7)
> +        3. Iterate over entries in ``FILES`` in each moz.build context.

s/FILES/Files/

::: python/mozbuild/mozbuild/frontend/context.py
(Diff revision 7)
> +            d['bug_component'] = None

Wouldn't it be better to return an empty dict in this case?

::: python/mozbuild/mozbuild/frontend/context.py
(Diff revision 7)
> +            file patterns would overrite this root value.

s/overrite/overwrite/.

By reading this text alone, I have absolutely no idea what FINAL does. Reading the code, it becomes clear, but the documentation needs clarification.

That said, how about making this an argument to Files() and make it apply to the entire subcontext. If you need to set other non-final variables for the same pattern, you can use a separate subcontext with the same pattern.

Then you can just say that a Files(final=True) takes precedence over any other defined below it in the same file or in any subdirectories, provided the pattern matches. Or something along those lines.
https://reviewboard.mozilla.org/r/4037/#review3391

::: python/mozbuild/mozbuild/frontend/emitter.py
(Diff revision 3)
> +                if isinstance(out, Files):

How about generalizing to SubContexts?
https://reviewboard.mozilla.org/r/3937/#review3393

::: build/moz.build
(Diff revision 6)
> +with Files('**'):

You could add **/Makefile.in at the top level.
https://reviewboard.mozilla.org/r/3939/#review3395

::: python/mozbuild/mozbuild/frontend/mach_commands.py
(Diff revision 6)
> +        print(yaml.safe_dump(data, default_flow_style=False).rstrip())

I'm not convinced the yaml output is really nice.

For --missing-bugzilla, this makes it:
- path1
- path2

(yes, with the dashes)

Without --missing-bugzilla, this makes it:
path1:
  bug_component:
  - Product
  - Component
path2:
  bug_component:
  - Product
  - Component

As a user of this command, I'd rather have something like this:
./mach file-info --bugzilla 'services/**'
(note the possibility to give wildcards on the command line)

And that would output something like:
Product :: Component:
  path1
  path2
Product2 :: Component2:
  path3
Unknown:
  path4
https://reviewboard.mozilla.org/r/3947/#review3397

::: python/mozbuild/mozbuild/frontend/context.py
(Diff revision 5)
> +        reviewers = sorted(self['REVIEWERS'], key=operator.attrgetter('weight'),

key=lambda x: x.weight would work just as much.

::: python/mozbuild/mozbuild/frontend/context.py
(Diff revision 5)
> +            and the current directory defined ``mary@example.com``, the final set

is Bob being unfaithful to Alice with Mary?

::: python/mozbuild/mozbuild/frontend/context.py
(Diff revision 5)
> +            actually remove the list item.

Why not add -= support to our lists (which I've been thinking to add for a while) and use that instead?

::: python/mozbuild/mozbuild/frontend/context.py
(Diff revision 5)
> +            Weights are integers between -100 and 100, inclusive.

Weights are integers between -100 and 100, with values that are always a multiple of 10. Why not just make it between -10 and 10 then?

::: python/mozbuild/mozbuild/frontend/context.py
(Diff revision 5)
> +        d['reviewers'] = [(unicode(r), r.weight) for r in reviewers]

Note that this likely makes the output of mach file-info even worse than what I noted on the previous patch:
path1:
  bug_component:
  - Product
  - Component
  reviewers:
  - - email1
    - weight1
  - - email2
    - weigth2
path2:
(...)

Better to have a mach file-info --reviewers, and group by reviewer or something.

That said, I'm not entirely sold to in-tree reviewer info. For one, you quickly end up with things out-of-sync between branches (esr, release, beta, aurora, central).

Putting aside the issue of whether or not this really belongs here, this is going to become boring pretty quickly. In most cases, the set of reviewers for a set of files will match the same set of reviewers for other set of files under the same bugzilla component. But you'll still have to define the reviewers each time.

It seems to me, if we are to go ahead with having reviewers in the tree, that the very first basic block should be to have a list of reviewers per bugzilla-component. Then refine with per-tree/per-pattern info.
https://reviewboard.mozilla.org/r/4041/#review3399

::: config/external/icu/moz.build
(Diff revision 4)
> -    for l in CONFIG['ICU_LIB_NAMES']:
> +    for l in (CONFIG['ICU_LIB_NAMES'] or []):

It probably be less awkward to the casual moz.build writers to make that

elif CONFIG['ICU_LIB_NAMES']:
  for ...

::: build/unix/elfhack/inject/moz.build
(Diff revision 4)
> -if CONFIG['TARGET_CPU'].endswith('86'):
> +target = CONFIG['TARGET_CPU'] or ''

This pattern is horrible and counter-intuitive to non-you-me people writing moz.builds. You should carter for this when you create your fake empty config.

I think you can do something like:
class foo(object):
    def __iter__(self):
        return iter(())
    def __unicode__(self):
        return ''

Coercing a foo instance to unicode or list will return an empty string or an empty list.
Then make your config.substs a defaultdict(foo), and that should avoid at least 95% of this patch.
https://reviewboard.mozilla.org/r/4043/#review3401

::: python/mozbuild/mozbuild/frontend/reader.py
(Diff revision 4)
> +        self.substs = ReadOnlyDict({

See review of previous patch (and yes, we have ReadOnlyDefaultDict).
Blocks: 849385
Assignee

Comment 35

4 years ago
https://reviewboard.mozilla.org/r/3823/#review3411

> The reader-relevant-mozbuild files are missing.

They were added as part of the previous commit.

> result[path] = reduce(lambda x, y: x + y, (contexts[p] for p in paths), [])
> 
> or
> 
> result[path] = itertools.chain(*(contexts[p] for p in paths))
> 
> That said, it seems to me this function should limit itself to returning the contexts (or better, yield them), and leave it to the caller to do classification if it wants to. If different callers need the classification, a helper function can be added for that.

I'll use a one-liner here.

However, I disagree about yielding just the contexts. I've built a specialized API for solving the problem at hand. I think YAGNI applies and we can refactor this later if needed.
Assignee

Comment 36

4 years ago
https://reviewboard.mozilla.org/r/3829/#review3415

> I'd rather have a way to give the sandboxes a different list of functions and make export() a noop through that than add another option to read_mozbuild.

I agree with you.

However, that patch is non-trivial. It /could/ be made more trivial if the contexts had a sandbox associated with them post-execution. Then, we could manipulate exports state post read but pre export processing (like how DIRS manipulation will work).

Please let me know what you think.
Assignee

Comment 37

4 years ago
https://reviewboard.mozilla.org/r/3939/#review3435

> I'm not convinced the yaml output is really nice.
> 
> For --missing-bugzilla, this makes it:
> - path1
> - path2
> 
> (yes, with the dashes)
> 
> Without --missing-bugzilla, this makes it:
> path1:
>   bug_component:
>   - Product
>   - Component
> path2:
>   bug_component:
>   - Product
>   - Component
> 
> As a user of this command, I'd rather have something like this:
> ./mach file-info --bugzilla 'services/**'
> (note the possibility to give wildcards on the command line)
> 
> And that would output something like:
> Product :: Component:
>   path1
>   path2
> Product2 :: Component2:
>   path3
> Unknown:
>   path4

Supporting wildcards in arguments is definitely on the feature todo list. I figured since shells support globbing, it was low on the priority list.

I can easily make changes to the output. I went with YAML because it was easy, is mostly human readable, and it can easily be piped into other programs for use. But we can get machine readability by adding --yaml or --json or ....
How about making functions and special variables part of the metadata that passes down from sandbox to sandbox?
Assignee

Comment 39

4 years ago
(In reply to Mike Hommey [:glandium] from comment #38)
> How about making functions and special variables part of the metadata that
> passes down from sandbox to sandbox?

I support that idea. I'd prefer not to bloat scope in this bug, however.
(In reply to Gregory Szorc [:gps] from comment #39)
> I support that idea. I'd prefer not to bloat scope in this bug, however.

And I'd prefer not to bloat read_mozbuild's argument list.
Assignee

Comment 41

4 years ago
https://reviewboard.mozilla.org/r/3947/#review3441

> Note that this likely makes the output of mach file-info even worse than what I noted on the previous patch:
> path1:
>   bug_component:
>   - Product
>   - Component
>   reviewers:
>   - - email1
>     - weight1
>   - - email2
>     - weigth2
> path2:
> (...)
> 
> Better to have a mach file-info --reviewers, and group by reviewer or something.
> 
> That said, I'm not entirely sold to in-tree reviewer info. For one, you quickly end up with things out-of-sync between branches (esr, release, beta, aurora, central).
> 
> Putting aside the issue of whether or not this really belongs here, this is going to become boring pretty quickly. In most cases, the set of reviewers for a set of files will match the same set of reviewers for other set of files under the same bugzilla component. But you'll still have to define the reviewers each time.
> 
> It seems to me, if we are to go ahead with having reviewers in the tree, that the very first basic block should be to have a list of reviewers per bugzilla-component. Then refine with per-tree/per-pattern info.

Here's my throught process.

I think it is nice to have the set of reviewers under version control so you can look at it over time. This establishes some nice potential for auditability in the future (something we've always said we want to do but can't find the time to implement).

I also think it's good to have the set of reviewers defined as close as possible to the things they are reviewing. I think close proximity results in better liklihood for things remaining in sync.

There is a problem with old release branches drifting out of sync with reality. That's why I think tip of central should be used for anything that matters. I plan to eventually build out a web service that exposes metadata for files. And this will be based on what's defined in central. Tools will use this web service to ensure the latest version - not the local version - of the metadata is used. There are some edge cases around what to do when metadata has changed locally. But I think once metadata has been defined, these pains won't be so bad.

Regarding more intelligent "grouping" of reviewers, I had the same thought. I was thinking that my concerns were overblown since the build system is probably one of the more complicated logical components of the tree. I would not be opposed to implementing something to remove some redundancy. But I question whether it should be a v1 feature. I'd kinda like to see Files sub-contexts proving that it's worth addressing now.

> Weights are integers between -100 and 100, with values that are always a multiple of 10. Why not just make it between -10 and 10 then?

I figured this algorithm will likely change over time as we experiment with weighting decisions. I don't like floats and a range of 20 didn't seem like enough granularity.

Keep in mind that we'll eventually need to compute the set of recommended reviewers for a set of changes files (from a patch). This is the weighting algorithm I'm most concerned about being future compatible with.

> Why not add -= support to our lists (which I've been thinking to add for a while) and use that instead?

I can do that, sure.
Assignee

Comment 42

4 years ago
Comment on attachment 8563835 [details]
MozReview Request: bz://1132771/gps

/r/3827 - Bug 1132771 - moz.build fixups to enable execution in no config mode
/r/3819 - Bug 1132771 - API to return moz.build files relevant for a set of paths
/r/3823 - Bug 1132771 - Support reading relevant moz.build files
/r/3829 - Bug 1132771 - Optionally don't export variables when reading moz.build files
/r/3825 - Bug 1132771 - Add a test for reading all moz.build files in filesystem traversal mode
/r/3821 - Bug 1132771 - Support and test for reading without a config object
/r/3935 - Bug 1132771 - Convert Sphinx to sub-contexts
/r/4041 - Bug 1132771 - Implement strongly typed named tuples
/r/4037 - Bug 1132771 - Add Files to moz.build with ability to define Bugzilla component
/r/3937 - Bug 1132771 - Define some bug components
/r/3939 - Bug 1132771 - Implement file-info mach command
/r/3947 - Bug 1132771 - Support for defining code reviewer metadata
/r/4063 - Bug 1132771 - Define some reviewers
/r/4065 - Bug 1132771 - Add ability to audit file metadata

Pull down these commits:

hg pull review -r 1fc4d116123b942fb4a63b5d1be53660e35c8437
Assignee

Comment 43

4 years ago
https://reviewboard.mozilla.org/r/3817/#review3445

I didn't hit all the points of feedback, but I addressed a lot of them. I think the patches early in this series are probably in a good enough state to start receiving ship its.
Assignee

Comment 44

4 years ago
Comment on attachment 8563835 [details]
MozReview Request: bz://1132771/gps

/r/3827 - Bug 1132771 - moz.build fixups to enable execution in no config mode
/r/3819 - Bug 1132771 - API to return moz.build files relevant for a set of paths
/r/3823 - Bug 1132771 - Pass special types down to sandboxes via metadata
/r/3829 - Bug 1132771 - Support reading relevant moz.build files
/r/3825 - Bug 1132771 - Add a test for reading all moz.build files in filesystem traversal mode
/r/3821 - Bug 1132771 - Support and test for reading without a config object
/r/3935 - Bug 1132771 - Convert Sphinx to sub-contexts
/r/4041 - Bug 1132771 - Implement strongly typed named tuples
/r/4037 - Bug 1132771 - Add Files to moz.build with ability to define Bugzilla component
/r/3937 - Bug 1132771 - Define some bug components
/r/3939 - Bug 1132771 - Implement file-info mach command
/r/3947 - Bug 1132771 - Support for defining code reviewer metadata
/r/4063 - Bug 1132771 - Define some reviewers
/r/4065 - Bug 1132771 - Add ability to audit file metadata

Pull down these commits:

hg pull review -r 9781d88dc20cfaf6de6e2bf8d77db19dc298c587
Posted patch Implement comment 33 (obsolete) — Splinter Review
This replaces the first patch and should apply on top of "Support and test for reading without a config object"
https://reviewboard.mozilla.org/r/3819/#review3463

Ship It!

::: python/mozbuild/mozbuild/test/frontend/test_reader.py
(Diff revision 9)
> +                'moz.build', 'd1/moz.build', 'd1/no-intermediate-moz-build/child/moz.build']})

BTW, did you measure how much memory this takes for a the full tree? with all those repetitive strings, that might be quite big, and it might be worth intern()ing the moz.build paths.
https://reviewboard.mozilla.org/r/3823/#review3467

::: python/mozbuild/mozbuild/frontend/reader.py
(Diff revision 10)
>                  'templates': self.metadata.get('templates', {})

Come to think of it, this was already wrong for templates to do that. Instead, the metadata at the time _template_decorator was called should be used.

::: python/mozbuild/mozbuild/frontend/reader.py
(Diff revision 10)
> +        self.special_variables = self.metadata.setdefault('special_variables', {})

You should default to VARIABLES, FUNCTIONS and SUBCONTEXTS here. Because it's better, and because otherwise, test_sandbox breaks.

::: python/mozbuild/mozbuild/frontend/reader.py
(Diff revision 10)
> +            })

No need for this with the proposed change above.

::: python/mozbuild/mozbuild/frontend/reader.py
(Diff revision 10)
> -                if 'templates' in sandbox.metadata:
> +                for key in ('exports', 'functions', 'templates', 'special_variables', 'subcontexts'):

for key in sandbox.metadata: maybe?
https://reviewboard.mozilla.org/r/3829/#review3469

::: python/mozbuild/mozbuild/frontend/reader.py
(Diff revision 10)
> +            'special_variables': SPECIAL_VARIABLES,

with SPECIAL_VARIABLES and SUBCONTEXTS being defaulted, you can drop those two.

::: python/mozbuild/mozbuild/frontend/reader.py
(Diff revision 10)
> +            all_contexts.append(context)

you could fill all_contexts afterwards with itertools.chain(*contexts.values())

Not that it makes a significant difference.

::: python/mozbuild/mozbuild/frontend/reader.py
(Diff revision 10)
> +                dirs[d].add(mozpath.relpath(target_dir, source_dir))

This can most probably be improved to be smarter for files in the same directory, in conjunction with _find_relevant_mozbuilds, but I won't block you on that. Please file a bug, though.
https://reviewboard.mozilla.org/r/3825/#review3471

::: config/tests/test_mozbuild_reading.py
(Diff revision 10)
> +        paths, contexts = reader.read_relevant_mozbuilds(reader.all_mozbuild_paths())

Comment 27 still applies.
https://reviewboard.mozilla.org/r/3821/#review3473

::: python/mozbuild/mozbuild/frontend/reader.py
(Diff revision 9)
> +        self.external_source_dir = None

cf. comment 46
https://reviewboard.mozilla.org/r/3935/#review3475

::: python/mozbuild/mozbuild/frontend/context.py
(Diff revision 9)
> +        'TREES': (dict, dict,

Time for a mozbuild.util.TypedDict? (where the values would be SourcePaths)

Come to think of it, <bikeshedding>the mapping feels backwards. That is, it seems to me we should be mapping source paths to sphinx tree, not the other way around.

Once you do that, you realize PYTHON_PACKAGE_DIRS kind of does the same.

So overall, we could have one mapping, that says that $dir is $feature under sphinx, where $feature == 'python' would have the same effect as using PYTHON_PACKAGE_DIRS.

Once you get there, you realize this could all just be something like:
with Sphinx('path'):
    TREE = 'feature'
</bikeshedding>

Anyways, besides the conflict in context.py for SUBCONTEXTS, this doesn't seem related to this bug and should get its own.

::: python/mozbuild/mozbuild/frontend/emitter.py
(Diff revision 9)
> +                continue

Gah, this should have gone in bug 1134072
https://reviewboard.mozilla.org/r/4041/#review3477

::: python/mozbuild/mozbuild/util.py
(Diff revision 6)
> +                if not isinstance(value, ftype):

related to comment 46, maybe we should have a isinstance wrapper that would return True for isinstance(MyUserString, unicode) (where MyUserString is a subclass of UserString that adds __unicode__, which UserString doesn't have)

::: python/mozbuild/mozbuild/util.py
(Diff revision 6)
> +                    raise TypeError('tuple field not proper type: %s; '

"tuple field not proper type" reads weird.

::: python/mozbuild/mozbuild/util.py
(Diff revision 6)
> +    cls = collections.namedtuple(name, [f[0] for f in fields])

(name for name, typ in fields) reads better.

::: python/mozbuild/mozbuild/util.py
(Diff revision 6)
> +            self = cls.__new__(klass, *args, **kwargs)

I think it would be better to use __init__, so that type(FooBar()) == FooBar. I however don't feel too strongly about it.
https://reviewboard.mozilla.org/r/4037/#review3481

::: python/mozbuild/mozbuild/frontend/context.py
(Diff revision 5)
> -

Don't remove this line.

::: python/mozbuild/mozbuild/frontend/context.py
(Diff revision 5)
> +        return BugzillaComponent.__new__(cls, *t)

I think it would make sense to make TypedNamedTuple itself handle this. I'm pretty sure if we use TypedNamedTuple again, we'll want the same.

::: python/mozbuild/mozbuild/frontend/context.py
(Diff revision 5)
> +            """, None),

This is better, but still isn't clear about directory traversal. I still think this would be simpler if this would be a flag that applies to all variables in the subcontext.

::: python/mozbuild/mozbuild/frontend/context.py
(Diff revision 5)
> +            d['bug_component'] = None

It still seems better to me to not have an entry at all in this case.

::: python/mozbuild/mozbuild/frontend/reader.py
(Diff revision 5)
> +    def get_metadata_for_files(self, paths):

get_files_info would somehow match mach file-info.
https://reviewboard.mozilla.org/r/3937/#review3483

::: moz.build
(Diff revision 8)
> +    BUG_COMPONENT = ('Core', 'Build Config')

This should be FINAL.

I'm tempted to say **/*.mozbuild should be Core::Build Config/FINAL as well. **/moz.build is a different story.
Assignee

Updated

4 years ago
Blocks: 1136552
https://reviewboard.mozilla.org/r/3939/#review3485

::: python/mozbuild/mozbuild/frontend/mach_commands.py
(Diff revision 8)
> +            components[m.get('BUG_COMPONENT', None)].add(p)

dict.get(foo, None) === dict.get(foo)

::: python/mozbuild/mozbuild/frontend/mach_commands.py
(Diff revision 8)
> +        for component, files in sorted(components.items()):

for components, files in sorted(components.items(), key: lambda x: (x is None, x)):
    print('%s :: %s' % (component.product, component.component) if component else 'UNKNOWN')

::: python/mozbuild/mozbuild/frontend/mach_commands.py
(Diff revision 8)
> +            print('')

maybe avoid on the last line. Or simply don't insert that space?
https://reviewboard.mozilla.org/r/3947/#review3487

comment 32 still applies here. Note I think reviewers is scope creep in this bug and could move to another bug, and possibly, whether reviewer info should be in-tree at all should be discussed more broadly.
https://reviewboard.mozilla.org/r/4065/#review3489

This could move to a separate bug. That is, this bug doesn't need to be blocked on this patch.

::: python/mozbuild/mozbuild/frontend/mach_commands.py
(Diff revision 4)
> +    @SubCommand('file-info', 'audit',

The commit message says --audit, but being a subcommand, it's audit.

::: python/mozbuild/mozbuild/frontend/reader.py
(Diff revision 4)
> +        products[p['name']] = (p['is_active'], components)

You could just make that:

products[p['name']] = components if p['is_active'] else None

(or not even fill components when the product is inactive)

::: python/mozbuild/mozbuild/frontend/reader.py
(Diff revision 4)
> +        components = {}

components = {c['name']: c['is_active'] for c in p['components']}

::: python/mozbuild/mozbuild/frontend/reader.py
(Diff revision 4)
> +                    reviewer_exists[reviewer] = bugzilla_user_exists(opener, reviewer)

You might as well memoize bugzilla_user_exists (although the opener gets in the way ; bugzilla helper class?)

T
Assignee

Comment 59

4 years ago
https://reviewboard.mozilla.org/r/3819/#review3511

> BTW, did you measure how much memory this takes for a the full tree? with all those repetitive strings, that might be quite big, and it might be worth intern()ing the moz.build paths.

I did not. I figure we can optimize later if it becomes an issue.
Assignee

Comment 60

4 years ago
https://reviewboard.mozilla.org/r/3823/#review3515

> Come to think of it, this was already wrong for templates to do that. Instead, the metadata at the time _template_decorator was called should be used.

This turns out to be a bit of a rabbit hole. I was thinking I'd cache a copy of the templates dict in the entry inside self.templates. However, a dict isn't hashable and @memoize blows up when it encounters one.

This will require just enough work that I think we should punt to a follow-up bug.
Assignee

Updated

4 years ago
Blocks: 1136966
Assignee

Comment 61

4 years ago
https://reviewboard.mozilla.org/r/3947/#review3531

I'm happy to defer reviewers to another bug if it means landing Files with just BUG_COMPONENT. Will drop these patches.
Assignee

Comment 63

4 years ago
Comment on attachment 8563835 [details]
MozReview Request: bz://1132771/gps

/r/3819 - Bug 1132771 - API to return moz.build files relevant for a set of paths; r=glandium
/r/4041 - Bug 1132771 - Implement strongly typed named tuples
/r/3823 - Bug 1132771 - Pass special types down to sandboxes via metadata
/r/3829 - Bug 1132771 - Support reading relevant moz.build files
/r/3825 - Bug 1132771 - Add a test for reading all moz.build files in filesystem traversal mode
/r/3821 - Bug 1132771 - Support and test for reading without a config object
/r/4037 - Bug 1132771 - Add Files to moz.build with ability to define Bugzilla component
/r/3937 - Bug 1132771 - Define some bug components
/r/3939 - Bug 1132771 - Implement file-info mach command

Pull down these commits:

hg pull review -r 174056d5acf31c10ef009affd7f4437d2166a49f
Assignee

Updated

4 years ago
Blocks: 1137042
Assignee

Updated

4 years ago
Blocks: 1137043
Assignee

Comment 64

4 years ago
Comment on attachment 8563835 [details]
MozReview Request: bz://1132771/gps

/r/3819 - Bug 1132771 - API to return moz.build files relevant for a set of paths; r=glandium
/r/4041 - Bug 1132771 - Implement strongly typed named tuples
/r/3823 - Bug 1132771 - Pass special types down to sandboxes via metadata
/r/3829 - Bug 1132771 - Support reading relevant moz.build files
/r/3825 - Bug 1132771 - Add a test for reading all moz.build files in filesystem traversal mode
/r/3821 - Bug 1132771 - Support and test for reading without a config object
/r/4037 - Bug 1132771 - Add Files to moz.build with ability to define Bugzilla component
/r/3937 - Bug 1132771 - Define some bug components

Pull down these commits:

hg pull review -r ee845e9e0f043f7dc8a4153dca19300da78d2d78
Assignee

Comment 65

4 years ago
Comment on attachment 8563835 [details]
MozReview Request: bz://1132771/gps

/r/3819 - Bug 1132771 - API to return moz.build files relevant for a set of paths; r=glandium
/r/4041 - Bug 1132771 - Implement strongly typed named tuples
/r/3823 - Bug 1132771 - Pass special types down to sandboxes via metadata
/r/3829 - Bug 1132771 - Support reading relevant moz.build files
/r/3825 - Bug 1132771 - Add a test for reading all moz.build files in filesystem traversal mode
/r/3821 - Bug 1132771 - Support and test for reading without a config object
/r/4037 - Bug 1132771 - Add Files to moz.build with ability to define Bugzilla component
/r/3937 - Bug 1132771 - Define some bug components
/r/4355 - Bug 1132771 - Implement file-info mach command

Pull down these commits:

hg pull review -r ee74ca97837fc75f41ea2fdf9fb36947e022da30
https://reviewboard.mozilla.org/r/4041/#review3535

Ship It!

::: python/mozbuild/mozbuild/util.py
(Diff revisions 6 - 7)
> +                return super(TypedTuple, klass).__new__(klass, *args[0])

You could just do args = args[0] and fallthrough.

::: python/mozbuild/mozbuild/test/test_util.py
(Diff revisions 6 - 7)
> +        t2 = FooBar(t)

did you really intend to use t, not t1 here?
https://reviewboard.mozilla.org/r/3823/#review3537

Ship It!

::: python/mozbuild/mozbuild/frontend/reader.py
(Diff revision 11)
> +                'functions': self.metadata.get('functions', {}),

Not sure {} is the right default. OTOH, this should never happen in practice anyways.
https://reviewboard.mozilla.org/r/3825/#review3541

Ship It!

::: config/tests/test_mozbuild_reading.py
(Diff revisions 10 - 11)
> +        self.assertEqual(set(paths.keys()), all_paths)

set(paths) should work too.
Assignee

Comment 72

4 years ago
https://reviewboard.mozilla.org/r/3937/#review3545

> This should be FINAL.
> 
> I'm tempted to say **/*.mozbuild should be Core::Build Config/FINAL as well. **/moz.build is a different story.

I forgot to add **/*.mozbuiod. I can do this before this lands.
https://reviewboard.mozilla.org/r/3821/#review3543

Note it's still worth "fixing" ipc/glue/moz.build, even if that works without the change.

::: python/mozbuild/mozbuild/util.py
(Diff revision 10)
> +    pass

you don't need pass with the docstring.

That being said, It would be better to add something like this:

def __init__(self, s=None):
    super(EmptyValue, self).__init__()

or with no argument at all.

So that you *can't* initialize an EmptyValue with a value. (because currently, you can do EmptyValue('foo') and it's not empty)
https://reviewboard.mozilla.org/r/4037/#review3547

::: build/docs/files-metadata.rst
(Diff revision 7)
> +This working example says, *for all Makefile.in files, set the Bugzilla

maybe be more explicit that this means all Makefile.in files across the tree.

::: build/docs/files-metadata.rst
(Diff revision 7)
> +:py:class:`mozbuild.frontend.context.SubContext`).

Not sure the pythonic implementation details matter. Plus, they might change and this section might remain stale.

::: build/docs/mozbuild-files.rst
(Diff revision 7)
> +with ``DIRS`` in them. For example, to execute a child directory,

DIRS and TEST_DIRS (not GYP_DIRS, it's special)

::: python/mozbuild/mozbuild/frontend/reader.py
(Diff revision 7)
> +    def get_metadata_for_files(self, paths):

get_files_info would somehow match mach file-info. (I feel like a broken record)

::: build/docs/files-metadata.rst
(Diff revision 7)
> +        FINAL += ['BUG_COMPONENT']

I still think this would be simpler if this would be a flag that applies to all variables in the subcontext. (You're not even checking that the values in FINAL correspond to one of the variables allowed in the subcontext)
https://reviewboard.mozilla.org/r/3937/#review3549

::: moz.build
(Diff revision 10)
> +

I'm tempted to say **/*.mozbuild should be Core::Build Config/FINAL as well.
Attachment #8568955 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Blocks: 1137319
Assignee

Comment 77

4 years ago
https://reviewboard.mozilla.org/r/3823/#review3567

> This turns out to be a bit of a rabbit hole. I was thinking I'd cache a copy of the templates dict in the entry inside self.templates. However, a dict isn't hashable and @memoize blows up when it encounters one.
> 
> This will require just enough work that I think we should punt to a follow-up bug.

I filed bug 1137319 to track this.
Assignee

Comment 78

4 years ago
https://reviewboard.mozilla.org/r/4037/#review3569

> I still think this would be simpler if this would be a flag that applies to all variables in the subcontext. (You're not even checking that the values in FINAL correspond to one of the variables allowed in the subcontext)

We will want granularity for finalizing flags. The question is how we should facilitate it.

In my proposal, we lock individual flags on a per context basis.

In your proposal, we'd need separate contexts for each logical unit of flag collections.

e.g.

    with Files('**/Makefile.in'):
        BUG_COMPONENT = ('Core', 'Build Config')
        FINAL = True
        
    with Files('**/Makefile.in'):
        REVIEWERS += [...]
        
I like how separate Files naturally group distinct "rules." However, I'm not crazy about the redundancy in file patterns. This means the same pattern defined multiple times. And that's something that can fall out of sync. I like having just one entity to manage.

I can go both ways on this.

That being said, having `FINAL = True` as a special case that means "apply to all variables" is a good idea. I think I can live with removing the list aspect for now. If we need it later, we can add it back in.

> get_files_info would somehow match mach file-info. (I feel like a broken record)

Sorry. I don't know what you mean here.
Assignee

Comment 79

4 years ago
https://reviewboard.mozilla.org/r/3937/#review3573

> I'm tempted to say **/*.mozbuild should be Core::Build Config/FINAL as well.

Unfortunately, no.

I figure we'll do an annotation on build/ and config/ later.

$ hg files 'glob:**.mozbuild'
b2g/app.mozbuild
b2g/dev/app.mozbuild
browser/app.mozbuild
build/gecko_templates.mozbuild
build/gyp.mozbuild
build/templates.mozbuild
dom/plugins/test/testplugin/testplugin.mozbuild
intl/unicharutil/util/objs.mozbuild
ipc/chromium/chromium-config.mozbuild
media/libopus/sources.mozbuild
media/libvpx/sources.mozbuild
media/mtransport/objs.mozbuild
mobile/android/app.mozbuild
mobile/android/base/android-services.mozbuild
mobile/android/search/search_activity_sources.mozbuild
mobile/android/stumbler/stumbler_sources.mozbuild
mobile/android/tests/background/junit3/background_junit3_sources.mozbuild
python/mozbuild/mozbuild/test/frontend/data/templates/templates.mozbuild
rdf/util/objs.mozbuild
security/pkix/warnings.mozbuild
security/sandbox/objs.mozbuild
toolkit/crashreporter/crashreporter.mozbuild
toolkit/crashreporter/google-breakpad/src/client/windows/crash_generation/objs.mozbuild
toolkit/crashreporter/google-breakpad/src/client/windows/handler/objs.mozbuild
toolkit/crashreporter/google-breakpad/src/client/windows/sender/objs.mozbuild
toolkit/crashreporter/google-breakpad/src/common/windows/objs.mozbuild
toolkit/mozapps/update/common/sources.mozbuild
toolkit/toolkit.mozbuild
tools/update-packaging/app.mozbuild
xpcom/glue/objs.mozbuild
xulrunner/app.mozbuild
Assignee

Comment 81

4 years ago
Comment on attachment 8563835 [details]
MozReview Request: bz://1132771/gps

/r/3819 - Bug 1132771 - API to return moz.build files relevant for a set of paths; r=glandium
/r/4041 - Bug 1132771 - Implement strongly typed named tuples; r=glandium
/r/3823 - Bug 1132771 - Pass special types down to sandboxes via metadata; r=glandium
/r/3829 - Bug 1132771 - Support reading relevant moz.build files; r=glandium
/r/3825 - Bug 1132771 - Add a test for reading all moz.build files in filesystem traversal mode; r=glandium
/r/3821 - Bug 1132771 - Support and test for reading without a config object; r=glandium
/r/4037 - Bug 1132771 - Add Files to moz.build with ability to define Bugzilla component
/r/3937 - Bug 1132771 - Define some bug components
/r/4355 - Bug 1132771 - Implement file-info mach command

Pull down these commits:

hg pull review -r 27d873b57f11f8e2d4b1ae1f1e36a437f1873286
Assignee

Comment 82

4 years ago
Comment on attachment 8563835 [details]
MozReview Request: bz://1132771/gps

/r/3819 - Bug 1132771 - API to return moz.build files relevant for a set of paths; r=glandium
/r/4041 - Bug 1132771 - Implement strongly typed named tuples; r=glandium
/r/3823 - Bug 1132771 - Pass special types down to sandboxes via metadata; r=glandium
/r/3829 - Bug 1132771 - Support reading relevant moz.build files; r=glandium
/r/3825 - Bug 1132771 - Add a test for reading all moz.build files in filesystem traversal mode; r=glandium
/r/3821 - Bug 1132771 - Support and test for reading without a config object; r=glandium
/r/4037 - Bug 1132771 - Add Files to moz.build with ability to define Bugzilla component
/r/3937 - Bug 1132771 - Define some bug components
/r/4355 - Bug 1132771 - Implement file-info mach command

Pull down these commits:

hg pull review -r 8f6c150f2f284097829aa495b4a043827fa5c8cd
https://reviewboard.mozilla.org/r/4037/#review3637

::: build/docs/files-metadata.rst
(Diff revision 9)
> +Files metadata is defined by utilizing the

"using" is good enough

::: build/docs/mozbuild-files.rst
(Diff revision 9)
> -a consumer, and then proceeds to process additional referenced
> -``moz.build`` files from the original file. The consumer then examines
> -the globals/``UPPERCASE`` variables set as part of execution and then
> -converts the data therein to Python class instances.
> -
> -The executed Python sandbox is essentially represented as a dictionary
> -of all the special ``UPPERCASE`` variables populated during its
> +1. Start at the root ``moz.build`` (``<topsrcdir>/moz.build``)
> +2. Evaluate the ``moz.build`` file in a new sandbox
> +3. Emit the main *context* and any *sub-contexts* from the executed
> +   sandbox
> +4. Extract a set of ``moz.build`` files to execute next.
> +5. For each additional ``moz.build`` file, goto #2 and repeat until all
> +   referenced files have executed.

Some of your items end with a period; some don't.

::: build/docs/mozbuild-files.rst
(Diff revision 9)
> +system and doesn't utilize ``DIRS`` variables to control traversal into

"use"

::: build/docs/mozbuild-files.rst
(Diff revision 9)
> +Instead of utilizing content from within the evaluated ``moz.build``

"using"
https://reviewboard.mozilla.org/r/4037/#review3667

Ship It!

::: build/docs/files-metadata.rst
(Diff revisions 7 - 9)
> -component to Core :: Build Config*.
> +in this one and underneath it, set the Bugzilla component to

"all directories in this one and underneath it" sounds weird.

::: python/mozbuild/mozbuild/frontend/context.py
(Diff revisions 7 - 9)
> +       child directories of ``bar/``, such as ``bar/dir1/baz``.

This is actually not true:

>>> mozpath.match('foo/bar/baz', 'foo/*')
True

The real difference between * and ** comes when there is something after it:

>>> mozpath.match('foo/bar/baz', '*/baz')
False
>>> mozpath.match('foo/bar/baz', '**/baz')
True
>>> mozpath.match('foo/bar/baz', 'foo/*/baz')
True
>>> mozpath.match('foo/bar/baz', 'foo/bar/*/baz')
False
>>> mozpath.match('foo/bar/baz', 'foo/bar/**/baz')
True
Comment on attachment 8563835 [details]
MozReview Request: bz://1132771/gps

I think I "ship it!"ed them all.
Attachment #8563835 - Flags: review?(mh+mozilla) → review+
Assignee

Updated

4 years ago
Blocks: 1138283
I'm fairly sure this busted spidermonkey tests. See https://treeherder.mozilla.org/logviewer.html#?job_id=7096652&repo=mozilla-inbound

Failures are like this:

5034 TEST-UNEXPECTED-FAIL | /builds/slave/m-in_lx-d_sm-arm-sim-000000000/src/config/tests/test_mozbuild_reading.py | line 44, test_filesystem_traversal_reading: config.status not available. Run configure.
5041 Exception: config.status not available. Run configure.
5043 make[1]: *** [tests/test_mozbuild_reading.py-run] Error 1
5044 make: *** [check] Error 2

Waiting on retriggers to confirm before I do a backout.
Assignee

Comment 98

4 years ago
*sigh*

In the past, it was sufficient to test build system Python foo by only building optimized builds. This is what I did to earn a green Try run from this commit series: https://treeherder.mozilla.org/#/jobs?repo=try&revision=95aa10d44a52

However, it now appears there is a debug-only SpiderMonkey job. And it just so happens SpiderMonkey builds are special snowflakes from the perspective of the build system and this is what broke this commit series.

Is it fair for me to question the asymmetry between debug and opt here? Do I need to start using extra resources to run *all* builds when I push build system changes to Try?
Flags: needinfo?(ryanvm)
SM jobs run when js/src is touched. Feel free to take it up with sfink and/or other JS folks.
Flags: needinfo?(ryanvm) → needinfo?(sphink)
Assignee

Comment 101

4 years ago
Relanded with the busted test marked as skip.

https://hg.mozilla.org/integration/fx-team/file/f19c915183c2/config/tests/test_mozbuild_reading.py#l36

Sent to fx-team because inbound is closed. There should be no merge conflict since this series was fully backed out from inbound.
Assignee

Updated

4 years ago
Blocks: 1138544
Assignee

Updated

4 years ago
Blocks: 1138579
Assignee

Updated

4 years ago
Blocks: 1138581
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #99)
> SM jobs run when js/src is touched. Feel free to take it up with sfink
> and/or other JS folks.

So you didn't get the jobs when they were needed to detect a regression. The usual reason for that is a try re-push that doesn't touch js/src, even though it's based on an unlanded changeset that does. I should dig up the bug number for that, but it's not relevant here.

Also, I saw a comment that there's a problem because the SM builds don't run on fx-team, but that is not the case. They do run. See eg https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=7a4c862d6cf3&filter-searchStr=spider

In this case, it looks like the problem is that these patches touch other things that can break an SM build. I haven't looked into what the relevant change was, but are there other file patterns that we can add to the SM triggers? (And on the flip side, we could always make them unconditional, but the SM builds are no longer as lightweight as they once were and that would burn a lot of resources.)
Flags: needinfo?(sphink)
Assignee

Comment 103

4 years ago
The file that changed was in config/tests/. This directory runs as part of |make check| in SM builds for hysterical raisins. I'm not sure it is worth the trouble to address it. We may get the fix for free if we ever move defining of Python unit tests to moz.build files.
Assignee

Updated

4 years ago
Blocks: 1139086
Ok, so looking at the js/ subtree:

I want everything under js/public and js/src to default to Core :: JavaScriptEngine. That's simple enough with a js/moz.build Files('public/**') and Files('src/**').

Within js/public, I want a handful of files to be Core :: JavaScript: GC instead of the above default. Can Files() take multiple arguments? It should.

js/src/configure.in should be Core :: Build Config, so that should be set in /moz.build (another use of multi-arg Files()). But generalizing that, it seems like **/*.in should default to Core :: Build Config. Except that I can imagine components using .in files for their own purposes, which exceeds our two levels of precedence. (Marking them FINAL would do no good, since the toplevel file already set them FINAL.)

Or /moz.build could set FINAL for just Makefile.in and configure.in (and moz.build?), and just set **/*.in non-FINALly. But that doesn't work with the above src/** rule, unless I also add in a Files('**/*.in') to reiterate the Core :: Build Config setting. But does that make the general rule to be that a lower-level moz.build file has to reiterate everything set in the higher-level ones? And thus, whenever updating the toplevel /moz.build file, you have to update every moz.build in the tree?

I'm probably exaggerating the problem, but I'd like you to tell me I'm wrong before I go ahead and label the js/ subtree.
Assignee

Comment 106

4 years ago
Bug 1138579 tracks supporting multiple patterns in Files().

Yes, we should probably extend some FINAL build system rules for **/configure.in. Despite landing this bug, I haven't yet annotated all the build files!

**/*.in should definitely *not* be a rule set at the root level. `hg files glob:**.in` lists numerous files that are not related to the build system.

In general, I'd say setting FINAL should be done with care. And more care should be taken the closer to the root moz.build you are.

Why don't you have a go at labeling js/ and we'll see what work we need to do, if any. I think js/ and the build system are in the same more-complex-than-most boat. I wouldn't be surprised if one of us dictates feature requirements. (Fun fact: I implemented FINAL primarily because of Makefile.in files. I think the majority of rules will end up being unqualified * or **, as I think it is somewhat rare for individual files to not belong to the group every other file in a directory belongs to.)
Assignee

Updated

4 years ago
Blocks: 1139218
Assignee

Updated

4 years ago
Blocks: 1168607
Assignee

Updated

4 years ago
Blocks: 1171105
Assignee

Comment 107

4 years ago
Attachment #8563835 - Attachment is obsolete: true
Attachment #8619475 - Flags: review+
Attachment #8619476 - Flags: review+
Attachment #8619477 - Flags: review+
Attachment #8619478 - Flags: review+
Attachment #8619479 - Flags: review+
Attachment #8619480 - Flags: review+
Attachment #8619481 - Flags: review+
Attachment #8619482 - Flags: review+
Attachment #8619483 - Flags: review+
Attachment #8619484 - Flags: review+
Attachment #8619485 - Flags: review+
Attachment #8619486 - Flags: review+
Attachment #8619487 - Flags: review+
Attachment #8619488 - Flags: review+
Attachment #8619489 - Flags: review+
Attachment #8619490 - Flags: review+
Attachment #8619491 - Flags: review+
Attachment #8619492 - Flags: review+

Updated

Last year
Product: Core → Firefox Build System
Assignee

Updated

Last year
Blocks: 1449604
You need to log in before you can comment on or make changes to this bug.