Closed
Bug 1132771
Opened 9 years ago
Closed 9 years ago
Define file-based metadata in moz.build files
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(firefox39 fixed)
RESOLVED
FIXED
mozilla39
Tracking | Status | |
---|---|---|
firefox39 | --- | fixed |
People
(Reporter: gps, Assigned: gps)
References
(Blocks 7 open bugs)
Details
Attachments
(18 files, 2 obsolete files)
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 |
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•9 years ago
|
||
/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•9 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•9 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•9 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)
Updated•9 years ago
|
Attachment #8563835 -
Flags: review?(jmuizelaar)
Comment 5•9 years ago
|
||
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.
Assignee | ||
Comment 6•9 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•9 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•9 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•9 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 | ||
Comment 10•9 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•9 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!
Comment 12•9 years ago
|
||
(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@...] ?
Comment 13•9 years ago
|
||
(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 | ||
Comment 14•9 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•9 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 16•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b82df0b1e8cc
Assignee | ||
Comment 17•9 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 18•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=021f3a9f4209
Assignee | ||
Comment 19•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b0279673d8d
Assignee | ||
Comment 20•9 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•9 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•9 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
Comment 23•9 years ago
|
||
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()
Comment 24•9 years ago
|
||
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.
Comment 25•9 years ago
|
||
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).
Comment 26•9 years ago
|
||
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.
Comment 27•9 years ago
|
||
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().
Comment 28•9 years ago
|
||
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.
Comment 29•9 years ago
|
||
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?
Comment 30•9 years ago
|
||
https://reviewboard.mozilla.org/r/3937/#review3393 ::: build/moz.build (Diff revision 6) > +with Files('**'): You could add **/Makefile.in at the top level.
Comment 31•9 years ago
|
||
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
Comment 32•9 years ago
|
||
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.
Comment 33•9 years ago
|
||
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.
Comment 34•9 years ago
|
||
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).
Assignee | ||
Comment 35•9 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•9 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•9 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 ....
Comment 38•9 years ago
|
||
How about making functions and special variables part of the metadata that passes down from sandbox to sandbox?
Assignee | ||
Comment 39•9 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.
Comment 40•9 years ago
|
||
(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•9 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•9 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•9 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•9 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
Comment 45•9 years ago
|
||
https://reviewboard.mozilla.org/r/3827/#review3461 Comment 33 still applies here.
Comment 46•9 years ago
|
||
This replaces the first patch and should apply on top of "Support and test for reading without a config object"
Comment 47•9 years ago
|
||
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.
Comment 48•9 years ago
|
||
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?
Comment 49•9 years ago
|
||
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.
Comment 50•9 years ago
|
||
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.
Comment 51•9 years ago
|
||
https://reviewboard.mozilla.org/r/3821/#review3473 ::: python/mozbuild/mozbuild/frontend/reader.py (Diff revision 9) > + self.external_source_dir = None cf. comment 46
Comment 52•9 years ago
|
||
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
Comment 53•9 years ago
|
||
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.
Comment 54•9 years ago
|
||
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.
Comment 55•9 years ago
|
||
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.
Comment 56•9 years ago
|
||
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?
Comment 57•9 years ago
|
||
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.
Comment 58•9 years ago
|
||
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•9 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•9 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 | ||
Comment 61•9 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 62•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=174056d5acf3
Assignee | ||
Comment 63•9 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 | ||
Comment 64•9 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•9 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
Assignee | ||
Comment 66•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee74ca97837f
Comment 67•9 years ago
|
||
https://reviewboard.mozilla.org/r/3819/#review3533 Ship It!
Comment 68•9 years ago
|
||
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?
Comment 69•9 years ago
|
||
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.
Comment 70•9 years ago
|
||
https://reviewboard.mozilla.org/r/3829/#review3539 Ship It!
Comment 71•9 years ago
|
||
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•9 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.
Comment 73•9 years ago
|
||
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)
Comment 74•9 years ago
|
||
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)
Comment 75•9 years ago
|
||
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.
Comment 76•9 years ago
|
||
https://reviewboard.mozilla.org/r/3939/#review3551 Ship It!
Updated•9 years ago
|
Attachment #8568955 -
Attachment is obsolete: true
Assignee | ||
Comment 77•9 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•9 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•9 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 80•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9922559f2b61
Assignee | ||
Comment 81•9 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•9 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
Comment 83•9 years ago
|
||
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"
Comment 84•9 years ago
|
||
https://reviewboard.mozilla.org/r/3819/#review3655 Ship It!
Comment 85•9 years ago
|
||
https://reviewboard.mozilla.org/r/4041/#review3657 Ship It!
Comment 86•9 years ago
|
||
https://reviewboard.mozilla.org/r/3823/#review3659 Ship It!
Comment 87•9 years ago
|
||
https://reviewboard.mozilla.org/r/3829/#review3661 Ship It!
Comment 88•9 years ago
|
||
https://reviewboard.mozilla.org/r/3825/#review3663 Ship It!
Comment 89•9 years ago
|
||
https://reviewboard.mozilla.org/r/3821/#review3665 Ship It!
Comment 90•9 years ago
|
||
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 91•9 years ago
|
||
https://reviewboard.mozilla.org/r/3937/#review3669 Ship It!
Comment 92•9 years ago
|
||
https://reviewboard.mozilla.org/r/4355/#review3671 Ship It!
Comment 93•9 years ago
|
||
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 | ||
Comment 94•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4013d256b591 https://hg.mozilla.org/integration/mozilla-inbound/rev/acdd5491f10e https://hg.mozilla.org/integration/mozilla-inbound/rev/ed135df39575 https://hg.mozilla.org/integration/mozilla-inbound/rev/6c44edc8208a https://hg.mozilla.org/integration/mozilla-inbound/rev/91d34d3107fa https://hg.mozilla.org/integration/mozilla-inbound/rev/c3a0cb4b45b1 https://hg.mozilla.org/integration/mozilla-inbound/rev/7eed09d39b9f https://hg.mozilla.org/integration/mozilla-inbound/rev/4cc39c54099d https://hg.mozilla.org/integration/mozilla-inbound/rev/b7ec05265c33
Assignee | ||
Comment 95•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/3dac1282a10f
Comment 96•9 years ago
|
||
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.
Comment 97•9 years ago
|
||
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/1cd9344eb281 Hopefully I got it right...
Assignee | ||
Comment 98•9 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)
Comment 99•9 years ago
|
||
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 100•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/58044df73cb0 https://hg.mozilla.org/integration/fx-team/rev/7f95e5b32756 https://hg.mozilla.org/integration/fx-team/rev/d5c17696d9d5 https://hg.mozilla.org/integration/fx-team/rev/3a6dbe970727 https://hg.mozilla.org/integration/fx-team/rev/a6b72bee55a8 https://hg.mozilla.org/integration/fx-team/rev/5a1306a14a8c https://hg.mozilla.org/integration/fx-team/rev/c5a0d6e73532 https://hg.mozilla.org/integration/fx-team/rev/02feef95cffb https://hg.mozilla.org/integration/fx-team/rev/f19c915183c2
Assignee | ||
Comment 101•9 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.
Comment 102•9 years ago
|
||
(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•9 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.
Comment 104•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/58044df73cb0 https://hg.mozilla.org/mozilla-central/rev/7f95e5b32756 https://hg.mozilla.org/mozilla-central/rev/d5c17696d9d5 https://hg.mozilla.org/mozilla-central/rev/3a6dbe970727 https://hg.mozilla.org/mozilla-central/rev/a6b72bee55a8 https://hg.mozilla.org/mozilla-central/rev/5a1306a14a8c https://hg.mozilla.org/mozilla-central/rev/c5a0d6e73532 https://hg.mozilla.org/mozilla-central/rev/02feef95cffb https://hg.mozilla.org/mozilla-central/rev/f19c915183c2
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Comment 105•9 years ago
|
||
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•9 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.)
Blocks: 1139180
Assignee | ||
Comment 107•9 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+
Assignee | ||
Comment 108•9 years ago
|
||
Assignee | ||
Comment 109•9 years ago
|
||
Assignee | ||
Comment 110•9 years ago
|
||
Assignee | ||
Comment 111•9 years ago
|
||
Assignee | ||
Comment 112•9 years ago
|
||
Assignee | ||
Comment 113•9 years ago
|
||
Assignee | ||
Comment 114•9 years ago
|
||
Assignee | ||
Comment 115•9 years ago
|
||
Assignee | ||
Comment 116•9 years ago
|
||
Assignee | ||
Comment 117•9 years ago
|
||
Assignee | ||
Comment 118•9 years ago
|
||
Assignee | ||
Comment 119•9 years ago
|
||
Assignee | ||
Comment 120•9 years ago
|
||
Assignee | ||
Comment 121•9 years ago
|
||
Assignee | ||
Comment 122•9 years ago
|
||
Assignee | ||
Comment 123•9 years ago
|
||
Assignee | ||
Comment 124•9 years ago
|
||
Assignee | ||
Comment 125•9 years ago
|
||
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•