Closed Bug 1426255 Opened 6 years ago Closed 6 years ago

file-info schedules is incorrect

Categories

(Firefox Build System :: Task Configuration, task)

task
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla59

People

(Reporter: dustin, Assigned: dustin)

References

Details

Attachments

(1 file)

(sandbox) dustin@lamport ~/p/m-c (676474b) $ cat -n moz.build | grep -C1 test-verify
    58  with Files("**/*.js"):
    59      SCHEDULES.inclusive += ['test-verify']
    60  
    61  with Files("**/*.html"):
    62      SCHEDULES.inclusive += ['test-verify']
    63  
    64  with Files("**/*.xhtml"):
    65      SCHEDULES.inclusive += ['test-verify']
    66  
    67  with Files("**/*.xul"):
    68      SCHEDULES.inclusive += ['test-verify']
    69  
(sandbox) dustin@lamport ~/p/m-c (676474b) $ cat -n testing/mochitest/moz.build | grep -C2 SCHEDULES
   166  with Files("**"):
   167      BUG_COMPONENT = ("Testing", "Mochitest")
   168      SCHEDULES.exclusive = ['mochitest', 'robocop']
   169  
   170  with Files("*remote*"):
(sandbox) dustin@lamport ~/p/m-c (676474b) $ ./mach file-info schedules testing/mochitest/tests/Harness_sanity/test_spawn_task.html
robocop, mochitest


Expected: robocop, mochitest, test-verify.
Comment on attachment 8937875 [details]
Bug 1426255: combine Files:SCHEDULES correctly;

https://reviewboard.mozilla.org/r/208556/#review214366

I need to think a bit more about the scheduling implications here. But there are plenty of nits to focus on (yes, I realize I'm doing this review in the wrong "order").

::: python/mozbuild/mozbuild/frontend/context.py:839
(Diff revision 1)
> +    # The `Files` context uses |= to combine SCHEDULES from multiple levels; at this
> +    # point the immutability is no longer needed so we use plain lists

I grok the reasoning here. But I think this will lead to trouble. I think the type of the stored variables should be consistent.

::: python/mozbuild/mozbuild/frontend/context.py:842
(Diff revision 1)
>          return list(sorted(set(self._inclusive) | set(self._exclusive)))
>  
> +    # The `Files` context uses |= to combine SCHEDULES from multiple levels; at this
> +    # point the immutability is no longer needed so we use plain lists
> +    def __ior__(self, other):
> +        rv = Schedules()

The semantics of `__ior__` are violated here. ``__ior__`` is supposed to do an in-place union and then return the original object (`self`).

The implementation here is a proper implementation of `__or__`, which does return a new object.

It's quite possible other `Files` code is wrong here. If you point me at other wrong code, I'll consider looking the other way regarding this nit pick. Although I'd prefer to fix the code to honor proper semantics.

::: python/mozbuild/mozbuild/frontend/context.py:845
(Diff revision 1)
> +    # point the immutability is no longer needed so we use plain lists
> +    def __ior__(self, other):
> +        rv = Schedules()
> +        rv._inclusive = list(sorted(set(self._inclusive) | set(other._inclusive)))
> +        if other._exclusive == self._exclusive:
> +            rv._exclusive = self._exclusive

This will copy a reference, which is probably not what is desired. This should create a new collection instance.

::: python/mozbuild/mozbuild/frontend/context.py:847
(Diff revision 1)
> +            rv._exclusive = other._exclusive
> +        elif other._exclusive == schedules.EXCLUSIVE_COMPONENTS:
> +            rv._exclusive = self._exclusive

ditto

::: python/mozbuild/mozbuild/frontend/context.py:854
(Diff revision 1)
> +            rv._exclusive = self._exclusive
> +        else:
> +            msg = 'Two Files sections have set SCHEDULES.exclusive to different' \
> +                'values; these cannot be combined: {} and {}'
> +            msg = msg.format(self._exclusive, other._exclusive)
> +            raise RuntimeError(msg)

`ValueError` is more appropriate.

::: python/mozbuild/mozbuild/test/frontend/test_reader.py:521
(Diff revision 1)
> +    def test_schedules_conflicting_excludes(self):
> +        reader = self.reader('schedules')
> +
> +        # bad.osx is defined explicitly, and matches *.osx, and the two have
> +        # conflicting SCHEDULES.exclusive settings
> +        with self.assertRaises(RuntimeError):

Could you please use `assertRaisesRegexp` to check for an expected error string?
Attachment #8937875 - Flags: review?(gps) → review-
I switched to just implementing __or__: I don't want to mutate the Schedules object in-place as it might later be combined with different sub-contexts.

By the way, that's not my understanding of the semantics of __iadd__ -- I think that behavior is optional but not required.  Do you have a docs link?
https://stackoverflow.com/questions/1047021/overriding-in-python-iadd-method clarifies it for me: it's certainly possible to return something other than `self`, but in that case it's better to just implement __add__ which Python will use if __iadd__ is not defined.
Greg, did you have a chance to look at this again after revision?
Flags: needinfo?(gps)
No, because I haven't looked at my review queue since December. But I'm going through my review queue today, so I should get to it.
Flags: needinfo?(gps)
Comment on attachment 8937875 [details]
Bug 1426255: combine Files:SCHEDULES correctly;

https://reviewboard.mozilla.org/r/208556/#review216836
Attachment #8937875 - Flags: review?(gps) → review+
Pushed by dmitchell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e4ca69cab96a
combine Files:SCHEDULES correctly; r=gps
Pushed by dmitchell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/67ab725f9d50
combine Files:SCHEDULES correctly; r=gps
https://hg.mozilla.org/mozilla-central/rev/67ab725f9d50
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Flags: needinfo?(dustin)
Product: TaskCluster → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: