New issue
Advanced search Search tips

Issue 870426 link

Starred by 1 user

Issue metadata

Status: Available
Owner: ----
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

per-file OWNERS presubmit check depends on upstream's version of src...

Project Member Reported by gab@chromium.org, Aug 2

Issue description

We 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.
 
Summary: per-file OWNERS presubmit check depends on upstream's version of src... (was: per-file OWNERS presubmit check depends on server's version of src...)
Actually, appears to be the upstream's (not the server's) version of src that matters (uploading a follow-up CL removing base/task_scheduler/OWNERS works)
Status: Available (was: Untriaged)
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.
Project Member

Comment 3 by bugdroid1@chromium.org, 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

> 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).
Project Member

Comment 5 by bugdroid1@chromium.org, 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