per-file OWNERS presubmit check depends on upstream's version of src... |
||
Issue descriptionWe had base/task_scheduler/OWNERS and a per-file rule in base/test/OWNERS as per-file *task_scheduler*=file://base/task_scheduler/OWNERS In a CL (https://chromium-review.googlesource.com/c/chromium/src/+/1161088) I'm deleting base/task_scheduler/OWNERS and tweaking the per-file rule in base/test/OWNERS but I can't make the presubmit happy whether I tweak or delete the rule... Assuming it looks up the server's version of base/test/OWNERS such that I can't do this in a single CL... All the logic should be based on local source or this gets in the way of moving files. D:\src\chrome2\src>git cl presubmit Running presubmit commit checks ... Traceback (most recent call last): File "D:\src\depot_tools\metrics.py", line 215, in print_notice_and_exit yield File "D:\src\depot_tools\\git_cl.py", line 6087, in <module> sys.exit(main(sys.argv[1:])) File "D:\src\depot_tools\\git_cl.py", line 6069, in main return dispatcher.execute(OptionParser(), argv) File "D:\src\depot_tools\subcommand.py", line 252, in execute return command(parser, args[1:]) File "D:\src\depot_tools\metrics.py", line 200, in _inner return self._collect_metrics(func, command_name, *args, **kwargs) File "D:\src\depot_tools\metrics.py", line 157, in _collect_metrics result = func(*args, **kwargs) File "D:\src\depot_tools\\git_cl.py", line 4848, in CMDpresubmit parallel=options.parallel) File "D:\src\depot_tools\\git_cl.py", line 1531, in RunHook parallel=parallel) File "D:\src\depot_tools\presubmit_support.py", line 1547, in DoPresubmitChecks results += executer.ExecPresubmitScript(presubmit_script, filename) File "D:\src\depot_tools\presubmit_support.py", line 1454, in ExecPresubmitScript result = eval(function_name + '(*__args)', context) File "<string>", line 1, in <module> File "<string>", line 3349, in CheckChangeOnCommit File "<string>", line 2856, in _CommonChecks File "D:\src\depot_tools\presubmit_canned_checks.py", line 1032, in PanProjectChecks input_api, output_api, source_file_filter=None)) File "D:\src\depot_tools\presubmit_canned_checks.py", line 887, in CheckOwners override_files=input_api.change.OriginalOwnersFiles()) File "D:\src\depot_tools\owners_finder.py", line 40, in __init__ self.db.load_data_needed_for(files) File "D:\src\depot_tools\owners.py", line 215, in load_data_needed_for self._read_owners(self.os_path.join(dirpath, 'OWNERS')) File "D:\src\depot_tools\owners.py", line 300, in _read_owners lineno, '\n'.join(comment + line_comment)) File "D:\src\depot_tools\owners.py", line 362, in _add_entry ('%s does not refer to an existing file.' % directive[5:])) SyntaxErrorInOwnersFile: D:\src\chrome2\src\base\test\OWNERS:1 syntax error: //base/task_scheduler/OWNERS does not refer to an existing file.
,
Aug 6
Yes, changes in OWNERS depend on the upstream version, so that you can't just add yourself as an OWNER and break havoc. The problem seems to be that it is processing //base/test/OWNERS at the upstream revision, but is processing //base/task_scheduler/OWNERS at the local revision. The solution would be to process both of them at the upstream revision.
,
Aug 7
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/46283730e0a62650e38fe29d2b784be249927a8c commit 46283730e0a62650e38fe29d2b784be249927a8c Author: Gabriel Charette <gab@chromium.org> Date: Tue Aug 07 14:43:13 2018 [TaskScheduler] Remove last few references to base/task_scheduler (base/task_scheduler/OWNERS needs to be removed in a follow-up because of crbug.com/870426) Bug: 870426 Change-Id: Id08ebf3a23705c7a0981ec320bee71cdd69d056f Reviewed-on: https://chromium-review.googlesource.com/1163307 Reviewed-by: Robert Liao <robliao@chromium.org> Commit-Queue: Gabriel Charette <gab@chromium.org> Cr-Commit-Position: refs/heads/master@{#581222} [modify] https://crrev.com/46283730e0a62650e38fe29d2b784be249927a8c/base/task/task_scheduler/task_scheduler.h [modify] https://crrev.com/46283730e0a62650e38fe29d2b784be249927a8c/base/task/task_traits.h [modify] https://crrev.com/46283730e0a62650e38fe29d2b784be249927a8c/components/task_scheduler_util/OWNERS
,
Aug 7
> The solution would be to process both of them at the upstream revision. Well, then it wouldn't have caught that I was removing base/task_scheduler/OWNERS with dangling references to it. I liked the fact that it caught that, but then fixing local references should have removed the warning. I understand that changes use the server's version of OWNERS file for owners *checks* but in this case, in a presubmit checking owners files layout, that doesn't seem appropriate (I don't see how it adds security).
,
Aug 7
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/96e8014a2d93903fed255702e40b2fb5c796f213 commit 96e8014a2d93903fed255702e40b2fb5c796f213 Author: Gabriel Charette <gab@chromium.org> Date: Tue Aug 07 15:25:27 2018 [TaskScheduler] Remove unused base/task_scheduler/OWNERS Bug: 870426 Change-Id: I7f9b5eac1ec4146df43492fe37b1607655c1ca42 Reviewed-on: https://chromium-review.googlesource.com/1163308 Reviewed-by: Robert Liao <robliao@chromium.org> Reviewed-by: Gabriel Charette <gab@chromium.org> Commit-Queue: Gabriel Charette <gab@chromium.org> Cr-Commit-Position: refs/heads/master@{#581231} [delete] https://crrev.com/5446e4e6ee93f3f3a1779edef848c0b0c0408c42/base/task_scheduler/OWNERS |
||
►
Sign in to add a comment |
||
Comment 1 by gab@chromium.org
, Aug 2