Closed Bug 1363811 Opened 3 years ago Closed 3 years ago

Is the moz.configure lint check doing the right thing for `default=delayed_getattr(...)`?

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set

Tracking

(firefox55 fixed)

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: ted, Assigned: glandium)

References

(Blocks 1 open bug)

Details

Attachments

(6 files)

I landed a patch in bug 1358215 that added an option that looked like:
```
option(env='MOZ_PHOTON_ANIMATIONS',
       help='Enable Photon UI animations',
       default=delayed_getattr(milestone, 'is_nightly'))
```

This worked fine except it failed the moz.configure lint test, like:
[task 2017-05-09T13:45:15.615001Z] 13:45:15     INFO - ConfigureError: Missing @depends for `result`: '--help'

Where `result` here is the function defined inside `delayed_getattr`:
https://dxr.mozilla.org/mozilla-central/rev/b21b974d60d3075ae24f6fb1bae75d0f122f28fc/build/moz.configure/util.configure#381

I changed the patch to instead have:
```
@depends(milestone)
def is_nightly(milestone):
    return milestone.is_nightly

option(env='MOZ_PHOTON_ANIMATIONS',
       help='Enable Photon UI animations',
       default=is_nightly)
```

Which works and passes the lint checks, but I don't really understand the difference. Is the lint check doing the right thing here?
Flags: needinfo?(mh+mozilla)
So, with the limitations in how things are detected, this is the expected behavior, sadly. The problem is that delayed_getattr introduces a closure and that's what we allow missing --help for: https://dxr.mozilla.org/mozilla-central/rev/b8e9b674033bcd1f3a4c59b9d0ee7619c1a17cc5/python/mozbuild/mozbuild/configure/lint.py#84-87

Now, the surprising thing is that the following should trigger the same error but doesn't:
https://dxr.mozilla.org/mozilla-central/rev/b8e9b674033bcd1f3a4c59b9d0ee7619c1a17cc5/mobile/android/moz.configure#64-66

That suggests a bug in the linter.
Flags: needinfo?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #1)
> Now, the surprising thing is that the following should trigger the same
> error but doesn't:
> https://dxr.mozilla.org/mozilla-central/rev/
> b8e9b674033bcd1f3a4c59b9d0ee7619c1a17cc5/mobile/android/moz.configure#64-66
> 
> That suggests a bug in the linter.

Which is now filed as bug 1365477.
So, related to this, there was this part of my message from a few months ago on dev-builds (https://groups.google.com/d/msg/mozilla.dev.builds/XThvzlFdlKU/uUlHt5IxCQAJ):

- It can be cumbersome to have to create @depends functions for
  simple conditionals.

  We have a bunch of templates that take a "when" argument. There also
  is imply_option, set_config, set_define and
  add_old_configure_assignment that all take @depends functions. In many
  places, we do ad-hoc things like
    depends(target)(lambda target: target.os == 'OSX')
  or
    delayed_getattr(milestone, 'is_nightly')

  I know Nick didn't like the magic, but after having written to many of
  those, I'm really considering magic would be nicer. What I was
  thinking was to make calling a depends function possible, and it would
  return a special class that create new pseudo depends function for
  common operations.

  Such that instead of
    depends(target)(lambda target: target.os == 'OSX')
  you could write:
    target().os == 'OSX'

  Instead of
    delayed_getattr(milestone, 'is_nightly')
  you could write:
    milestone().is_nightly

  Instead of
    @depends(a, b)
    def foo(a, b):
        return a or b
  you could write:
    foo = a() or b()

  etc.


This would actually help here. Note that actually already have something like the last example since https://hg.mozilla.org/mozilla-central/rev/9c02acca893d .
The patch queue I just posted implements most of the proposals in comment 3, without the parens (such that milestone.is_nightly is equivalent to delayed_getattr(milestone, 'is_nightly'). The magic for == is not there, though, because it's not possible (python expects __cmp__/__eq__ to return an integer/bool and barfs otherwise)

I'm not sure whether we want the parens, but it's not hard to add them (and retrospectively in what was added in https://hg.mozilla.org/mozilla-central/rev/9c02acca893d).

This fixes the issue with default=delayed_getattr(a, 'b') by making the magic a.b never require a dependency on --help.

One thing missing from the queue is replacing the is_nightly function with milestone.is_nightly.
Comment on attachment 8868430 [details]
Bug 1363811 - Change TestConfigure.test_depends_or to test more cases.

https://reviewboard.mozilla.org/r/140018/#review144252
Attachment #8868430 - Flags: review?(cmanchester) → review+
Comment on attachment 8868431 [details]
Bug 1363811 - Modify the name of the DependsFunction.__or__ implementation method.

https://reviewboard.mozilla.org/r/140020/#review144256

::: python/mozbuild/mozbuild/configure/__init__.py:133
(Diff revision 2)
>  
>      @staticmethod
> -    def first_true(iterable):
> -        # Like the builtin any(), but returns the first element that is true,
> -        # instead of True. If none are true, returns the last element.
> +    def or_impl(iterable):
> +        # Applies "or" to all the items of iterable.
> +        # e.g. if iterable contains a, b and c, returns `a or b or c`.
> +        # Equivalent to reduce(lambda a, b: a or b, iterable, False)

I don't think this line adds much to the explanation. It also wouldn't be equivalent if we ever allowed this function to be called with an empty iterable.
Attachment #8868431 - Flags: review?(cmanchester) → review+
Comment on attachment 8868432 [details]
Bug 1363811 - Allow to combine two DependsFunctions with "&".

https://reviewboard.mozilla.org/r/140022/#review144266
Attachment #8868432 - Flags: review?(cmanchester) → review+
Comment on attachment 8868433 [details]
Bug 1363811 - Allow "direct" access to namespace attributes from DependsFunctions.

https://reviewboard.mozilla.org/r/140024/#review144272
Attachment #8868433 - Flags: review?(cmanchester) → review+
Comment on attachment 8868434 [details]
Bug 1363811 - Replace all uses of delayed_getattr(a, 'b') with a.b.

https://reviewboard.mozilla.org/r/140026/#review144274

Much better!
Attachment #8868434 - Flags: review?(cmanchester) → review+
Comment on attachment 8868451 [details]
Bug 1363811 - Replace is_nightly with milestone.is_nightly.

https://reviewboard.mozilla.org/r/140052/#review144276
Attachment #8868451 - Flags: review?(cmanchester) → review+
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/autoland/rev/fb362996cf3f
Change TestConfigure.test_depends_or to test more cases. r=cmanchester+432261
https://hg.mozilla.org/integration/autoland/rev/aaad90777719
Modify the name of the DependsFunction.__or__ implementation method. r=cmanchester+432261
https://hg.mozilla.org/integration/autoland/rev/d517e034c953
Allow to combine two DependsFunctions with "&". r=cmanchester+432261
https://hg.mozilla.org/integration/autoland/rev/01aa12b787b2
Allow "direct" access to namespace attributes from DependsFunctions. r=cmanchester+432261
https://hg.mozilla.org/integration/autoland/rev/6d99c685aea3
Replace all uses of delayed_getattr(a, 'b') with a.b. r=cmanchester+432261
https://hg.mozilla.org/integration/autoland/rev/d4687badde12
Replace is_nightly with milestone.is_nightly. r=cmanchester+432261
Product: Core → Firefox Build System
Assignee: nobody → mh+mozilla
You need to log in before you can comment on or make changes to this bug.