Detect watchman in configure

RESOLVED FIXED in Firefox 56

Status

enhancement
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: gps, Assigned: gps)

Tracking

Trunk
mozilla56
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(3 attachments)

Assignee

Description

2 years ago
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

2 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

2 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

2 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

2 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

2 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

2 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

2 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
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Depends on: 1384910
Assignee

Comment 19

2 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

2 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

Updated

a year ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.