(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?

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?

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.

Comment on attachment 8937875 [details] Bug 1426255: combine Files:SCHEDULES correctly; https://reviewboard.mozilla.org/r/208556/#review216836

Pushed by dmitchell@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e4ca69cab96a combine Files:SCHEDULES correctly; r=gps

Backed out changeset e4ca69cab96a (bug 1426255) for bustage failures on /python/mozbuild/mozbuild/test/frontend/test_reader.py::TestBuildReader::test_schedules r=backout a=backout on a CLOSED TREE https://treeherder.mozilla.org/logviewer.html#?job_id=154826946&repo=autoland&lineNumber=33559 https://hg.mozilla.org/integration/autoland/rev/6577b2a480dc6e736b98871e0f17d66508d4d2ed https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=e4ca69cab96aad47b7506a70238c9d0f93d61e04&filter-classifiedState=unclassified&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception

Pushed by dmitchell@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/67ab725f9d50 combine Files:SCHEDULES correctly; r=gps