W3CImportExpectations is currently fairly hard to read, as it mixed Pass and Skip, and also includes the owners.
This could be improved in a few steps:
1. Write a script to create OWNERS files based on the annotations in W3CImportExpectations
2. Delete all [ Pass ] entries, effectively leaving it as a skip-list only
3. Check if there are directories imported that now don't have OWNERS files. If so, add appropriate OWNERS, if any.
W3CImportExpectations is currently fairly hard to read, as it mixed Pass and Skip, and also includes the owners.
This could be improved in a few steps:
1. Write a script to create OWNERS files based on the annotations in W3CImportExpectations
2. Delete all [ Pass ] entries, effectively leaving it as a skip-list only
qyearsley@, WDYT? I guess this would require changes to the importer as well, since it lists owners in the import commits. (I just got one of those emails, very fast import after I added some things, yay.)
Yep, that list of steps looks correct to me!
Note, I think that it might be just as easy to keep directory_owners_extractor.py, but transform it into a module that extracts from OWNERS files instead of W3CImportExpectations. That class already has access to a FileSystem object and has a list_owners method which could be modified to support OWNERS files.
For step 2, sergiyb@ added logic for skipping OWNERS files in Chromium commits before, but we have duplicate logic in the GerritCL class, and this needs to be updated: https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/w3c/gerrit.py?l=173
Owner: robertma@chromium.org Status: Started (was: Available)
Starting to work on this bug for starters. I'm planning to do this in 3 steps/CLs:
1. Batch converting [ Pass ] entries -> OWNERS files
2. All necessary changes in our Python scripts to switch to reading from OWNERS files
3. Removing all [ Pass ] entries after everything is verified to work as intended (and check if there's any new [ Pass ] entries added during this period)
Alright, finally the importer ran again so we can see what extracted owners looks like in practice:
https://chromium-review.googlesource.com/c/585363/
In the CL description:
Directory owners for changes in this CL:
:
external/wpt/html/semantics
dpranke@chromium.org:
external
external
style-dev@chromium.org:
external/wpt/cssom-view
external/wpt/cssom-view
external/wpt/cssom-view
...
Conveniently, these three directories indicate three possible changes we want to make to DirectoryOwnersExtractor (https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/w3c/directory_owners_extractor.py?l=25):
1. If there's no owner email address in an OWNERS file, we can just ignore it and continue. We might want to consider CCing team mailing lists, like dom-dev@, but I believe it would be better in general if directory owners were individual people.
2. We probably do want to stop at external/wpt/ and not consider list owners for that directory or any directories above that. (We don't want to list owners in external/wpt/ or external/.
3. We don't want to list the same directory more than once, so maybe we want to change the list_owners to return a dict of tuple -> set instead of tuple -> list.
Glad to see the first actual run! Found some unexpected cases here.
In reply to comment #13:
1. Empty OWNERS files (or the ones with comments only) should be ignored.
2. Stop the traversal at external/wpt so that we don't spam dpranke@ every day, which was also the previous behavior.
3. Owned directories need to be de-duplicated. I misread the previous implementation and thought it would also have duplicate entries (and regardless, deduplication is surely desired).
Emails listed in "# TEAM:" are ignored, but the script does not differentiate team vs individual owners (the uncommented emails in OWNERS files). I checked W3CImportExpectations: teams like dom-dev@ style-dev@ do have their team mailing lists as owners there (and the previous version would also CC them). So I believe it is WAI if we CC some teams found in (the non-comment part of) OWNERS.
The following revision refers to this bug:
https://chromium.googlesource.com/chromium/src.git/+/57c4e4a54dc9ef0e25673421502570639c923f9b
commit 57c4e4a54dc9ef0e25673421502570639c923f9b
Author: Raphael Kubo da Costa <raphael.kubo.da.costa@intel.com>
Date: Thu Aug 03 11:56:13 2017
[Owners Extractor] Do not assert when LayoutTests/ itself is passed.
Commit 874543453709fb355ef88e85ac4bfe92d1ab99c3 ("[Owners Extractor] Fix
issues found in the actual run") added an assertion to the code to verify
that we are not passing a directory above LayoutTests/external to
find_and_extract_owners().
The assertion gets triggered with valid paths though; for example, changes
to LayoutTests/TestExpectations means //third_party/WebKit/LayoutTests gets
passed to the function, which aborts instead of just skipping that path.
This, in turn, broke our automatic web-platform-tests imports, as a recent
change there meant our scripts had to delete a test from TestExpectations.
For example:
https://luci-milo.appspot.com/buildbot/chromium.infra.cron/wpt-importer/415
leads to
Traceback (most recent call last):
File "/mnt/data/b/rr/tmps9aWJe/w/src/third_party/WebKit/Tools/Scripts/wpt-import", line 18, in <module>
host.exit(importer.main())
File "/mnt/data/b/rr/tmps9aWJe/w/src/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer.py", line 122, in main
self._upload_cl()
File "/mnt/data/b/rr/tmps9aWJe/w/src/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer.py", line 385, in _upload_cl
directory_owners = self.get_directory_owners()
File "/mnt/data/b/rr/tmps9aWJe/w/src/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer.py", line 409, in get_directory_owners
return extractor.list_owners(changed_files)
File "/mnt/data/b/rr/tmps9aWJe/w/src/third_party/WebKit/Tools/Scripts/webkitpy/w3c/directory_owners_extractor.py", line 42, in list_owners
owners_file, owners = self.find_and_extract_owners(self.filesystem.dirname(relpath))
File "/mnt/data/b/rr/tmps9aWJe/w/src/third_party/WebKit/Tools/Scripts/webkitpy/w3c/directory_owners_extractor.py", line 69, in find_and_extract_owners
assert directory.startswith(external_root)
AssertionError
This change just turns the assertion into a check to unbreak the imports, a
better one that increases test coverage should probably be landed later.
TBR=qyearsley@chromium.org,robertma@chromium.org
Bug: 713987
Change-Id: I192f37bf10dad98bc32cf2ff15cefa09e440c4ee
Reviewed-on: https://chromium-review.googlesource.com/599908
Commit-Queue: Raphael Kubo da Costa (rakuco) <raphael.kubo.da.costa@intel.com>
Reviewed-by: Raphael Kubo da Costa (rakuco) <raphael.kubo.da.costa@intel.com>
Cr-Commit-Position: refs/heads/master@{#491704}
[modify] https://crrev.com/57c4e4a54dc9ef0e25673421502570639c923f9b/third_party/WebKit/Tools/Scripts/webkitpy/w3c/directory_owners_extractor.py
[modify] https://crrev.com/57c4e4a54dc9ef0e25673421502570639c923f9b/third_party/WebKit/Tools/Scripts/webkitpy/w3c/directory_owners_extractor_unittest.py
Thank you for catching and fixing this, Raphael!
The assertion was added to prevent other paths out of external/ being passed in and escaping the termination condition, but I didn't realize that LayoutTests/TestExpectations could be legitimately changed during import, too.
I checked the patch and it looks good to me. There's just one question: is there anything else that can be modified that I missed? Now we are expecting the following paths:
LayoutTests/* # TestExpectations
LayoutTests/external/** # Tests imported by TestCopier, and baselines
(where ** includes all the sub-directories recursively and * doesn't)
> There's just one question: is there anything else that can be modified that I missed?
I'm not entirely sure myself. I was tempted to drop the assert altogether (and return (None,None) if we're not inside external/), but I'm not very familiar with the code and thus decided to err on the side of caution.
At the moment, I think the assert is only catching pathological conditions like wpt-import changing a LayoutTests subdirectory that's not in external/, or another part of the tree altogether.
I think that keeping the assert is good, since I believe it should always be true now. (And if it's not true and it catches something else, then that's good :-))
The following revision refers to this bug:
https://chromium.googlesource.com/chromium/src.git/+/0f8e35ed6ebe956f4ad097242d365f18cd412c49
commit 0f8e35ed6ebe956f4ad097242d365f18cd412c49
Author: Raphael Kubo da Costa <raphael.kubo.da.costa@intel.com>
Date: Thu Aug 31 18:18:50 2017
[Owners Extractor] Whitelist LayoutTests/FlagExpectations/.
Follow-up to commits 874543453709fb ("[Owners Extractor] Fix issues found in
the actual run") and 57c4e4a54dc9e ("[Owners Extractor] Do not assert when
LayoutTests/ itself is passed").
It turns out we were still asserting when an expectation file inside
LayoutTests/FlagExpectations/ was updated. A concrete case comes from
https://github.com/w3c/web-platform-tests/commit/27838c0248a1772d4bf6c1f378be63e25f700184,
which moved a test to a different directory. wpt-import adjusted the path in
one of the expectation files in LayoutTests/FlagExpectations/ accordingly,
but the owners extractor complained a forbidden path had been changed.
From
https://luci-milo.appspot.com/buildbot/chromium.infra.cron/wpt-importer/1005:
2017-08-31 08:35:28,494 - Gathering directory owners emails to CC.
Traceback (most recent call last):
File "/mnt/data/b/rr/tmpCMVvWc/w/src/third_party/WebKit/Tools/Scripts/wpt-import", line 25, in <module>
main()
File "/mnt/data/b/rr/tmpCMVvWc/w/src/third_party/WebKit/Tools/Scripts/wpt-import", line 18, in main
host.exit(importer.main())
File "/mnt/data/b/rr/tmpCMVvWc/w/src/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer.py", line 126, in main
self._upload_cl()
File "/mnt/data/b/rr/tmpCMVvWc/w/src/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer.py", line 413, in _upload_cl
directory_owners = self.get_directory_owners()
File "/mnt/data/b/rr/tmpCMVvWc/w/src/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer.py", line 437, in get_directory_owners
return extractor.list_owners(changed_files)
File "/mnt/data/b/rr/tmpCMVvWc/w/src/third_party/WebKit/Tools/Scripts/webkitpy/w3c/directory_owners_extractor.py", line 43, in list_owners
owners_file, owners = self.find_and_extract_owners(self.filesystem.dirname(relpath))
File "/mnt/data/b/rr/tmpCMVvWc/w/src/third_party/WebKit/Tools/Scripts/webkitpy/w3c/directory_owners_extractor.py", line 75, in find_and_extract_owners
directory, external_root)
AssertionError: /mnt/data/b/rr/tmpCMVvWc/w/src/third_party/WebKit/LayoutTests/FlagExpectations must start with /mnt/data/b/rr/tmpCMVvWc/w/src/third_party/WebKit/LayoutTests/external
Bug: 713987
Change-Id: Ib400628398b228910f54ad24a84b6107c5ffec9d
Reviewed-on: https://chromium-review.googlesource.com/645852
Reviewed-by: Philip Jägenstedt <foolip@chromium.org>
Reviewed-by: Quinten Yearsley <qyearsley@chromium.org>
Commit-Queue: Raphael Kubo da Costa (rakuco) <raphael.kubo.da.costa@intel.com>
Cr-Commit-Position: refs/heads/master@{#498922}
[modify] https://crrev.com/0f8e35ed6ebe956f4ad097242d365f18cd412c49/third_party/WebKit/Tools/Scripts/webkitpy/w3c/directory_owners_extractor.py
[modify] https://crrev.com/0f8e35ed6ebe956f4ad097242d365f18cd412c49/third_party/WebKit/Tools/Scripts/webkitpy/w3c/directory_owners_extractor_unittest.py
Comment 1 by foolip@chromium.org
, Apr 21 2017