Closed Bug 1384396 Opened 7 years ago Closed 7 years ago

Detect watchman in configure

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set
normal

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 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 on attachment 8890178 [details]
Bug 1384396 - Detect Watchman in configure;

https://reviewboard.mozilla.org/r/161242/#review166868
Attachment #8890178 - Flags: review?(nalexander) → 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+
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)`.
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.
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
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
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Depends on: 1384910
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)
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
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.