Closed
Bug 1384396
Opened 7 years ago
Closed 7 years ago
Detect watchman in configure
Categories
(Firefox Build System :: General, enhancement)
Firefox Build System
General
Tracking
(firefox56 fixed)
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: gps, Assigned: gps)
References
Details
Attachments
(3 files)
I asked for this in bug 1384241. I'm going to implement it for Nick.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 8•7 years ago
|
||
mozreview-review |
Comment on attachment 8890185 [details] Bug 1384396 - Add a @depends_all utility function; https://reviewboard.mozilla.org/r/161256/#review166864 If this was new code, I would want to have `depends_any` instead of `depends_if`, since the latter is ambiguous. But it's not new code :( There are only 48 occurences of `depends_if` in the tree, and they seem to be 50-50 split between one argument and multiple arguments. I think that argues for keeping `depends_if` and not bothering renaming or deprecating. Roll on! ::: build/moz.configure/util.configure:408 (Diff revision 1) > > > -# Like @depends, but the decorated function is only called if one of the > -# arguments it would be called with has a positive value (bool(value) is True) > @template > -def depends_if(*args, **kwargs): > +def depends_tmpl(fn, *args, **kwargs): I expected `fn` to be invoked once per argument, but you want to allow early return (with `all` and `any`). Can you find a name or add a comment that conveys that `fn` is invoked once with an iterator? ::: build/moz.configure/util.configure:417 (Diff revision 1) > else: > when = None > def decorator(func): > @depends(*args, when=when) > def wrapper(*args): > - if any(arg for arg in args): > + if fn(arg for arg in args): Is there an advantage to the `for ... in` statement? Should you use `iter`? Should you just pass `args`?
Attachment #8890185 -
Flags: review?(nalexander) → review+
Comment 9•7 years ago
|
||
mozreview-review |
Comment on attachment 8890178 [details] Bug 1384396 - Detect Watchman in configure; https://reviewboard.mozilla.org/r/161242/#review166868
Attachment #8890178 -
Flags: review?(nalexander) → review+
Comment 10•7 years ago
|
||
mozreview-review |
Comment on attachment 8890179 [details] Bug 1384396 - Detect Watchman Mercurial integration in configure; https://reviewboard.mozilla.org/r/161244/#review166874 This looks sensible. Is `moz.configure` smart enough to cache the `hg_config` results, so we're not invoking `hg` for each extension we search for in the future? And I'll assume there's no good way to _ask_ `hg` for the `fsmonitor` state, avoiding baking in special knowledge of `fsmonitor.mode=off` into our script. ::: build/moz.configure/init.configure:367 (Diff revision 3) > +@imports('os') > +def hg_config(build_env, hg): > + env = dict(os.environ) > + env['HGPLAIN'] = '1' > + > + out = check_cmd_output(hg, 'config', env=env, cwd=build_env.topsrcdir) When Mercurial plugins are in bad states, you will get spurious warnings in this output. Can you be looser about the key-value split below, perhaps ignoring lines that don't split properly? ::: moz.configure:395 (Diff revision 3) > + > + for k in ('extensions.fsmonitor', 'extensions.hgext.fsmonitor'): > + if k in hg_config and hg_config[k] != '!': > + ext_enabled = True > + > + mode_disabled = hg_config.get('fsmonitor.mode', 'irrelevant') == 'off' I generally use `None` as a sentinel.
Attachment #8890179 -
Flags: review?(nalexander) → review+
Assignee | ||
Comment 11•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8890185 [details] Bug 1384396 - Add a @depends_all utility function; https://reviewboard.mozilla.org/r/161256/#review166864 > Is there an advantage to the `for ... in` statement? Should you use `iter`? Should you just pass `args`? I don't think so. `any()` and `all()` consume things as generators. Unless there is some wonkiness with the sandbox I'm not aware of. I think we can literally just do `fn(args)`.
Assignee | ||
Comment 12•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8890179 [details] Bug 1384396 - Detect Watchman Mercurial integration in configure; https://reviewboard.mozilla.org/r/161244/#review166874 Yes, moz.configure is smart enough to cache function results. That's why I extracted hg config to its own function and didn't limit it to just specific items. No, there doesn't appear to be a way to ask for fsmonitor state beyond config scanning. That can and should be added upstream. > When Mercurial plugins are in bad states, you will get spurious warnings in this output. Can you be looser about the key-value split below, perhaps ignoring lines that don't split properly? Those warnings go to stderr, which check_cmd_output throws away for us as long as the exit code is 0. HGPLAIN=1 ensures that output is well-defined and backwards compatible. We shouldn't need to be strict about parsing. But if this breaks in the wild, we'll loosen conditions.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 16•7 years ago
|
||
Pushed by gszorc@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0c2bc12f4ebe Add a @depends_all utility function; r=nalexander https://hg.mozilla.org/integration/autoland/rev/2a1b1485ffc7 Detect Watchman in configure; r=nalexander https://hg.mozilla.org/integration/autoland/rev/a8373914cbfd Detect Watchman Mercurial integration in configure; r=nalexander
Comment 17•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0c2bc12f4ebe https://hg.mozilla.org/mozilla-central/rev/2a1b1485ffc7 https://hg.mozilla.org/mozilla-central/rev/a8373914cbfd
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Comment 18•7 years ago
|
||
Backed out for breaking builds when hg username has non-ascii characters (see bug 1384910): https://hg.mozilla.org/mozilla-central/rev/658cba6a971257e2ba39715ec938256dfc414776 https://hg.mozilla.org/mozilla-central/rev/68231b38b01dbd5807ef75478c9c62888754bbd2 https://hg.mozilla.org/mozilla-central/rev/9090379eaa361e44a2664ef6b73029aeeb7009ba
Status: RESOLVED → REOPENED
No longer depends on: 1384910
Flags: needinfo?(gps)
Resolution: FIXED → ---
Assignee | ||
Comment 19•7 years ago
|
||
Python 2's implicit coercion between bytes and unicode is horrible. While the fix is trivial, backing out was the right thing to do because of developer impact. I'll fix this.
Flags: needinfo?(gps)
Comment 20•7 years ago
|
||
Pushed by gszorc@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/abd5c5d03d1c Add a @depends_all utility function; r=nalexander https://hg.mozilla.org/integration/autoland/rev/3db186a5de7d Detect Watchman in configure; r=nalexander https://hg.mozilla.org/integration/autoland/rev/6504499c1689 Detect Watchman Mercurial integration in configure; r=nalexander
Comment 21•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/abd5c5d03d1c https://hg.mozilla.org/mozilla-central/rev/3db186a5de7d https://hg.mozilla.org/mozilla-central/rev/6504499c1689
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Resolution: --- → FIXED
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
•