Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2510303002:
Add check_gn_headers.py to find missing headers in GN (Closed)
DescriptionAdd check_gn_headers.py to find missing headers in GN
BUG=661774
Review-Url: https://codereview.chromium.org/2510303002
Cr-Commit-Position: refs/heads/master@{#442617}
Committed: https://chromium.googlesource.com/chromium/src/+/037f6e9e17273a1108a38cf00232d083addb6ee9
Patch Set 1 #
Total comments: 9
Patch Set 2 : address comments and add tests #Patch Set 3 : pylint, flake8 #
Total comments: 6
Patch Set 4 : try CQ #Patch Set 5 : cwd #Patch Set 6 : more bots #Patch Set 7 : whitelist #Patch Set 8 : whitelist more #Patch Set 9 : fix json dump #
Total comments: 1
Patch Set 10 : rebase, Skip folders covered by DEPS #Patch Set 11 : update gn_missing_headers.txt with Mac output #Patch Set 12 : pylint and flake8 #Patch Set 13 : add linux and android files from tryjob runs #
Total comments: 12
Patch Set 14 : address comments #Patch Set 15 : remove whitelist file #
Total comments: 4
Patch Set 16 : use gclient, simplify filter #
Total comments: 4
Patch Set 17 : address nits #
Messages
Total messages: 42 (18 generated)
https://codereview.chromium.org/2510303002/diff/1/build/check_gn_headers.py File build/check_gn_headers.py (right): https://codereview.chromium.org/2510303002/diff/1/build/check_gn_headers.py#n... build/check_gn_headers.py:20: IGNORE_REGEX = re.compile('(^(third_party|build)\/)|(.*/gen/.*)') Just curious, why we ignore third_party? https://codereview.chromium.org/2510303002/diff/1/build/check_gn_headers.py#n... build/check_gn_headers.py:31: if is_valid and line.endswith('.h'): Some header files are with extension .hh https://codereview.chromium.org/2510303002/diff/1/build/check_gn_headers.py#n... build/check_gn_headers.py:32: f = os.path.normpath(os.path.join(out_dir, line.strip())) IIRC, normpath is platform-dependent, so path separator is '\\' on windows while it is '/' on Linux. This might cause some issue with the regex used here. https://codereview.chromium.org/2510303002/diff/1/build/check_gn_headers.py#n... build/check_gn_headers.py:50: for f in properties['sources']: nit: properties.get('sources', [])? https://codereview.chromium.org/2510303002/diff/1/build/check_gn_headers.py#n... build/check_gn_headers.py:52: all_headers.add(os.path.normpath('.' + f)) same here for normpath
https://codereview.chromium.org/2510303002/diff/1/build/check_gn_headers.py File build/check_gn_headers.py (right): https://codereview.chromium.org/2510303002/diff/1/build/check_gn_headers.py#n... build/check_gn_headers.py:20: IGNORE_REGEX = re.compile('(^(third_party|build)\/)|(.*/gen/.*)') On 2016/11/18 02:17:28, stgao (slow on Monday) wrote: > Just curious, why we ignore third_party? I guess we shouldn't. https://codereview.chromium.org/2510303002/diff/1/build/check_gn_headers.py#n... build/check_gn_headers.py:31: if is_valid and line.endswith('.h'): On 2016/11/18 02:17:28, stgao (slow on Monday) wrote: > Some header files are with extension .hh Done. https://codereview.chromium.org/2510303002/diff/1/build/check_gn_headers.py#n... build/check_gn_headers.py:32: f = os.path.normpath(os.path.join(out_dir, line.strip())) On 2016/11/18 02:17:28, stgao (slow on Monday) wrote: > IIRC, normpath is platform-dependent, so path separator is '\\' on windows while > it is '/' on Linux. > This might cause some issue with the regex used here. Done. https://codereview.chromium.org/2510303002/diff/1/build/check_gn_headers.py#n... build/check_gn_headers.py:52: all_headers.add(os.path.normpath('.' + f)) On 2016/11/18 02:17:28, stgao (slow on Monday) wrote: > same here for normpath Done.
Description was changed from ========== Add check_gn_headers.py to find missing headers in GN BUG=661774 ========== to ========== Add check_gn_headers.py to find missing headers in GN BUG=661774 ==========
wychen@chromium.org changed reviewers: + stgao@chromium.org
PTAL
I can't see more obvious issues, but I'm not familiar enough with ninja/gn. So I'd suggest asking thakis@, dpranke@ and brettw@ for further review. https://codereview.chromium.org/2510303002/diff/40001/build/check_gn_headers.py File build/check_gn_headers.py (right): https://codereview.chromium.org/2510303002/diff/40001/build/check_gn_headers.... build/check_gn_headers.py:40: if not f.startswith('build'): nit: maybe add a comment to explain why we skip those under build/. https://codereview.chromium.org/2510303002/diff/40001/build/check_gn_headers.... build/check_gn_headers.py:61: if f.endswith('.h') or f.endswith('.hh'): Just to double check: are those generated header files included in both ninja and gn?
https://codereview.chromium.org/2510303002/diff/40001/build/check_gn_headers.py File build/check_gn_headers.py (right): https://codereview.chromium.org/2510303002/diff/40001/build/check_gn_headers.... build/check_gn_headers.py:73: parser.add_argument('args', nargs=argparse.REMAINDER) To integrate into the chromium_trybot recipe for presubmit checking, it might be better to pass in a file path, and dump the list of missing header files into that file in json format. And then the recipe could complain if there are missing header files in gn. IMHO, we should throw an error on CQ only for those missing header files are added by the CL in that try-job. If they are added by another committed CL, it seems better not to complain. This might become a chicken-and-egg problem if a header file is added but "gn analyze" says no compile is needed. However, this should be rare. If this could run fast enough, should we add a checking step on the waterfall instead for a post-submit checking like checkperms?
The CQ bit was checked by wychen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by wychen@chromium.org
Patchset #4 (id:60001) has been deleted
Patchset #4 (id:80001) has been deleted
https://codereview.chromium.org/2510303002/diff/40001/build/check_gn_headers.py File build/check_gn_headers.py (right): https://codereview.chromium.org/2510303002/diff/40001/build/check_gn_headers.... build/check_gn_headers.py:40: if not f.startswith('build'): On 2016/11/19 00:03:50, stgao (slow on Monday) wrote: > nit: maybe add a comment to explain why we skip those under build/. Done. https://codereview.chromium.org/2510303002/diff/40001/build/check_gn_headers.... build/check_gn_headers.py:61: if f.endswith('.h') or f.endswith('.hh'): On 2016/11/19 00:03:50, stgao (slow on Monday) wrote: > Just to double check: are those generated header files included in both ninja > and gn? The generated header files are excluded in the ninja list, since they don't have ../../ prefix. Their dependency should already be taken care of, so we don't have to test them. https://codereview.chromium.org/2510303002/diff/40001/build/check_gn_headers.... build/check_gn_headers.py:73: parser.add_argument('args', nargs=argparse.REMAINDER) On 2016/11/19 00:21:33, stgao (slow on Monday) wrote: > To integrate into the chromium_trybot recipe for presubmit checking, it might be > better to pass in a file path, and dump the list of missing header files into > that file in json format. And then the recipe could complain if there are > missing header files in gn. Done > IMHO, we should throw an error on CQ only for those missing header files are > added by the CL in that try-job. If they are added by another committed CL, it > seems better not to complain. > > This might become a chicken-and-egg problem if a header file is added but "gn > analyze" says no compile is needed. However, this should be rare. > > If this could run fast enough, should we add a checking step on the waterfall > instead for a post-submit checking like checkperms? I tried locally, and it takes ~30 seconds. I think this is fast enough to be included in waterfall. If it's part of the waterfall, we don't really need to filter the error on CQ.
Patchset #11 (id:240001) has been deleted
Patchset #10 (id:220001) has been deleted
wychen@chromium.org changed reviewers: + dpranke@chromium.org
The DEPS filtering is done. PTAL https://codereview.chromium.org/2510303002/diff/200001/build/check_gn_headers.py File build/check_gn_headers.py (right): https://codereview.chromium.org/2510303002/diff/200001/build/check_gn_headers... build/check_gn_headers.py:21: ninja_out = subprocess.check_output(['ninja', '-C', out_dir, '-t', 'deps']) Note to self: Might need universal_newlines=True if we want to support Windows.
Description was changed from ========== Add check_gn_headers.py to find missing headers in GN BUG=661774 ========== to ========== Add check_gn_headers.py to find missing headers in GN This script is run on the following bots on CQ: - linux_chromium_dbg_ng - linux_chromium_rel_ng - linux_android_dbg_ng - linux_android_rel_ng BUG=661774 ==========
Happy-2017 ping!
On 2017/01/04 09:06:19, wychen wrote: > Happy-2017 ping! Sorry for the delay on this! I'll try to get to it tomorrow.
On 2017/01/05 02:59:41, Dirk Pranke wrote: > On 2017/01/04 09:06:19, wychen wrote: > > Happy-2017 ping! > > Sorry for the delay on this! I'll try to get to it tomorrow. Thank you!
This mostly looks pretty good, apart from style comments about the Python. Let me know if you have questions, and sorry again for the delay. https://codereview.chromium.org/2510303002/diff/320001/build/check_gn_headers.py File build/check_gn_headers.py (right): https://codereview.chromium.org/2510303002/diff/320001/build/check_gn_headers... build/check_gn_headers.py:19: def get_d_headers(out_dir): Chromium Python style is (sadly) to use InitialCaps rather than unix_hacker_style hames. https://codereview.chromium.org/2510303002/diff/320001/build/check_gn_headers... build/check_gn_headers.py:25: def extract_d_headers(ninja_out): extract_d_headers is a weird function name. It's not really "extracting" anything, and it's unclear what "d" refers to (same thing with the "get_d_headers" above). I'd call this something like ParseNinjaDepsOutput(). https://codereview.chromium.org/2510303002/diff/320001/build/check_gn_headers... build/check_gn_headers.py:58: def extract_gn_headers(gn): I'd call this something more like "GetHeadersFromGNProjectJSON" https://codereview.chromium.org/2510303002/diff/320001/build/check_gn_headers... build/check_gn_headers.py:80: exec(var_def + deps) Please add a comment about what lines 78-80 are doing (loading the DEPS file and stubbing out the Var() function). https://codereview.chromium.org/2510303002/diff/320001/build/check_gn_headers... build/check_gn_headers.py:119: whitelist = set(open(args.whitelist).read().split('\n')) Please change this to be able to handle a file format that has comments in it (I suggest stripping blank lines and "# comment"s); you'll want to annotate the whitelist sooner or later. https://codereview.chromium.org/2510303002/diff/320001/testing/buildbot/chrom... File testing/buildbot/chromium.linux.json (right): https://codereview.chromium.org/2510303002/diff/320001/testing/buildbot/chrom... testing/buildbot/chromium.linux.json:1301: } As I noted in the bug, it's not immediately clear how to make this work as a test step on the bots. Let's leave this file unmodified and leave the //testing/scripts/check_gn_headers.py file out of this CL, so that we can land the rest.
Patchset #14 (id:340001) has been deleted
Description was changed from ========== Add check_gn_headers.py to find missing headers in GN This script is run on the following bots on CQ: - linux_chromium_dbg_ng - linux_chromium_rel_ng - linux_android_dbg_ng - linux_android_rel_ng BUG=661774 ========== to ========== Add check_gn_headers.py to find missing headers in GN BUG=661774 ==========
https://codereview.chromium.org/2510303002/diff/320001/build/check_gn_headers.py File build/check_gn_headers.py (right): https://codereview.chromium.org/2510303002/diff/320001/build/check_gn_headers... build/check_gn_headers.py:19: def get_d_headers(out_dir): On 2017/01/09 06:47:49, Dirk Pranke wrote: > Chromium Python style is (sadly) to use InitialCaps rather than > unix_hacker_style hames. Done. https://codereview.chromium.org/2510303002/diff/320001/build/check_gn_headers... build/check_gn_headers.py:25: def extract_d_headers(ninja_out): On 2017/01/09 06:47:49, Dirk Pranke wrote: > extract_d_headers is a weird function name. It's not really "extracting" > anything, and it's unclear what "d" refers to (same thing with the > "get_d_headers" above). > > I'd call this something like ParseNinjaDepsOutput(). > Done. https://codereview.chromium.org/2510303002/diff/320001/build/check_gn_headers... build/check_gn_headers.py:58: def extract_gn_headers(gn): On 2017/01/09 06:47:49, Dirk Pranke wrote: > I'd call this something more like "GetHeadersFromGNProjectJSON" I use ParseGNProjectJSON() so it's parallel to ParseNinjaDepsOutput(). https://codereview.chromium.org/2510303002/diff/320001/build/check_gn_headers... build/check_gn_headers.py:80: exec(var_def + deps) On 2017/01/09 06:47:49, Dirk Pranke wrote: > Please add a comment about what lines 78-80 are doing (loading the DEPS file and > stubbing out the Var() function). Done. https://codereview.chromium.org/2510303002/diff/320001/build/check_gn_headers... build/check_gn_headers.py:119: whitelist = set(open(args.whitelist).read().split('\n')) On 2017/01/09 06:47:49, Dirk Pranke wrote: > Please change this to be able to handle a file format that has comments in it (I > suggest stripping blank lines and "# comment"s); you'll want to annotate the > whitelist sooner or later. Great idea!
Thanks for reviewing! https://codereview.chromium.org/2510303002/diff/320001/testing/buildbot/chrom... File testing/buildbot/chromium.linux.json (right): https://codereview.chromium.org/2510303002/diff/320001/testing/buildbot/chrom... testing/buildbot/chromium.linux.json:1301: } On 2017/01/09 06:47:49, Dirk Pranke wrote: > As I noted in the bug, it's not immediately clear how to make this work as a > test step on the bots. Let's leave this file unmodified and leave the > //testing/scripts/check_gn_headers.py file out of this CL, so that we can land > the rest. I've removed them from this CL. According to my design doc, the stale .d file can be eliminated if this checking is only done after a full compile. As for the skipped analysis, even though temporary regression could happen, new errors would eventually be discovered. Quote: "Note that it can go wrong only if a new header is exclusively included by whitelisted headers. If the new header is also included by any cc files, “gn analyze” would trigger compilation. Therefore, the error scenario should be infrequent enough so that sheriffs could revert the culprit without too much difficulty." Do you think it's better to wait until we have PRESUBMIT heuristics to complement the checking on CQ?
https://codereview.chromium.org/2510303002/diff/370004/build/check_gn_headers.py File build/check_gn_headers.py (right): https://codereview.chromium.org/2510303002/diff/370004/build/check_gn_headers... build/check_gn_headers.py:79: """Parse the DEPS file and get the folder names""" It occurred to me that this won't actually work right, since you're not handling recursedeps entries. More importantly, it's assuming too much about the implementation of gclient, which is going to be changing soon to be way more complicated. You're better off calling out to gclient itself to get the list of deps, e.g. via something like `gclient recurse --no-progress -j 1 python -c 'import os; print os.environ["GCLIENT_DEP_PATH"]'`, or by adding a command to gclient to do that in a built-in way (which would be much faster). https://codereview.chromium.org/2510303002/diff/370004/build/check_gn_headers... build/check_gn_headers.py:131: missing = set(filter(lambda x: not StartsWithAny(deps, x), missing)) missing = {x for x in deps if not any(x.startswith(d) for d in deps])}
lgtm w/ nit.
On 2017/01/09 13:14:53, wychen wrote: > Thanks for reviewing! > > https://codereview.chromium.org/2510303002/diff/320001/testing/buildbot/chrom... > File testing/buildbot/chromium.linux.json (right): > > https://codereview.chromium.org/2510303002/diff/320001/testing/buildbot/chrom... > testing/buildbot/chromium.linux.json:1301: } > On 2017/01/09 06:47:49, Dirk Pranke wrote: > > As I noted in the bug, it's not immediately clear how to make this work as a > > test step on the bots. Let's leave this file unmodified and leave the > > //testing/scripts/check_gn_headers.py file out of this CL, so that we can land > > the rest. > > I've removed them from this CL. > > According to my design doc, the stale .d file can be eliminated if this checking > is only done after a full compile. As for the skipped analysis, even though > temporary regression could happen, new errors would eventually be discovered. > > Quote: "Note that it can go wrong only if a new header is exclusively included > by whitelisted headers. If the new header is also included by any cc files, “gn > analyze” would trigger compilation. Therefore, the error scenario should be > infrequent enough so that sheriffs could revert the culprit without too much > difficulty." Unfortunately, I don't think we have a good way to know when to trigger this test step, i.e., which compile targets need to be considered out of date. Put differently, we don't have a way to say "run this script only if we're adding new files". So, that's why I need to think about this more.
On 2017/01/09 20:48:40, Dirk Pranke wrote: > On 2017/01/09 13:14:53, wychen wrote: > > Thanks for reviewing! > > > > > https://codereview.chromium.org/2510303002/diff/320001/testing/buildbot/chrom... > > File testing/buildbot/chromium.linux.json (right): > > > > > https://codereview.chromium.org/2510303002/diff/320001/testing/buildbot/chrom... > > testing/buildbot/chromium.linux.json:1301: } > > On 2017/01/09 06:47:49, Dirk Pranke wrote: > > > As I noted in the bug, it's not immediately clear how to make this work as a > > > test step on the bots. Let's leave this file unmodified and leave the > > > //testing/scripts/check_gn_headers.py file out of this CL, so that we can > land > > > the rest. > > > > I've removed them from this CL. > > > > According to my design doc, the stale .d file can be eliminated if this > checking > > is only done after a full compile. As for the skipped analysis, even though > > temporary regression could happen, new errors would eventually be discovered. > > > > Quote: "Note that it can go wrong only if a new header is exclusively included > > by whitelisted headers. If the new header is also included by any cc files, > “gn > > analyze” would trigger compilation. Therefore, the error scenario should be > > infrequent enough so that sheriffs could revert the culprit without too much > > difficulty." > > Unfortunately, I don't think we have a good way to know when to trigger > this test step, i.e., which compile targets need to be considered out of date. > > Put differently, we don't have a way to say "run this script only if we're > adding > new files". > > So, that's why I need to think about this more. In PS13, I think the custom step is triggered right after a "compile all" step, if compilation is not skipped. We don't really need to know which fine-grained compile targets needs to be considered out of date, right? After all, this script would be added exclusively to the bots that include "all" target, so the compilation would be either all or none. I'm not sure why we'd want to run this script only when we are adding new files. Is this an optimization? Generally speaking, even if a CL doesn't introduce new files, it could still cause missing headers in gn. For example, refactor the gn file and wrongly remove some headers, or forget to wrap OS-dependent headers in #ifdef. To be safe, I think this script should run whenever compile all is run. What's the timing budget we have if this script runs whenever compile all is run? Currently it takes 20 ~ 50 seconds on bots.
On 2017/01/10 02:54:07, wychen wrote: > On 2017/01/09 20:48:40, Dirk Pranke wrote: > > On 2017/01/09 13:14:53, wychen wrote: > > > Thanks for reviewing! > > > > > > > > > https://codereview.chromium.org/2510303002/diff/320001/testing/buildbot/chrom... > > > File testing/buildbot/chromium.linux.json (right): > > > > > > > > > https://codereview.chromium.org/2510303002/diff/320001/testing/buildbot/chrom... > > > testing/buildbot/chromium.linux.json:1301: } > > > On 2017/01/09 06:47:49, Dirk Pranke wrote: > > > > As I noted in the bug, it's not immediately clear how to make this work as > a > > > > test step on the bots. Let's leave this file unmodified and leave the > > > > //testing/scripts/check_gn_headers.py file out of this CL, so that we can > > land > > > > the rest. > > > > > > I've removed them from this CL. > > > > > > According to my design doc, the stale .d file can be eliminated if this > > checking > > > is only done after a full compile. As for the skipped analysis, even though > > > temporary regression could happen, new errors would eventually be > discovered. > > > > > > Quote: "Note that it can go wrong only if a new header is exclusively > included > > > by whitelisted headers. If the new header is also included by any cc files, > > “gn > > > analyze” would trigger compilation. Therefore, the error scenario should be > > > infrequent enough so that sheriffs could revert the culprit without too much > > > difficulty." > > > > Unfortunately, I don't think we have a good way to know when to trigger > > this test step, i.e., which compile targets need to be considered out of date. > > > > Put differently, we don't have a way to say "run this script only if we're > > adding > > new files". > > > > So, that's why I need to think about this more. > > In PS13, I think the custom step is triggered right after a "compile all" step, > if compilation is not skipped. We don't really need to know which fine-grained > compile targets needs to be considered out of date, right? After all, this > script would be added exclusively to the bots that include "all" target, so the > compilation would be either all or none. On the tryservers, builders run the `analyze` step in order to only (re)build the targets that are affected by the change, and to only re-run the test steps affected by the change. But I don't know how to say which targets to rebuild for this particular test, or when we'd need to re-run it. > I'm not sure why we'd want to run this script only when we are adding new files. > Is this an optimization? Generally speaking, even if a CL doesn't introduce new > files, it could still cause missing headers in gn. For example, refactor the gn > file and wrongly remove some headers, or forget to wrap OS-dependent headers in > #ifdef. To be safe, I think this script should run whenever compile all is run. > What's the timing budget we have if this script runs whenever compile all is > run? Currently it takes 20 ~ 50 seconds on bots. That's not an insignificant amount of time for a change that otherwise affects almost nothing. It's doable, but I'd prefer to make things work "properly" if we can figure out what "properly" would mean. I wonder if we're just overthinking this. Can we get 90%+ of this if we just check that whenever a {.cc,.h,.mm,.m} is added or removed to the repo then BUILD files are modified to add or remove the corresponding entries?
On 2017/01/10 03:57:56, Dirk Pranke wrote:
> On the tryservers, builders run the `analyze` step in order to only (re)build
> the targets that are affected by the change, and to only re-run the test steps
> affected by the change. But I don't know how to say which targets to rebuild
> for this particular test, or when we'd need to re-run it.
If the dependency relationship is correctly represented in GN, i.e. there's no
missing headers in GN, "analyze" step should be able to correctly tell which
targets to rebuild. Before that happens, wrongly skipping targets is what we
want to fix, but also the cause of under-triggering of our checking script. The
workaround for this is to enable the checking on bots that have the target
"all", so that we don't have to worry about the target. "When to run the script"
would be the tough issue here.
> That's not an insignificant amount of time for a change that otherwise affects
> almost
> nothing. It's doable, but I'd prefer to make things work "properly" if we can
> figure
> out what "properly" would mean.
> I wonder if we're just overthinking this. Can we get 90%+ of this if we just
> check
> that whenever a {.cc,.h,.mm,.m} is added or removed to the repo then BUILD
files
> are modified to add or remove the corresponding entries?
This script is the costly but accurate checking. The only case it can miss the
error is when "analyze" step wrongly skips compilation. It seems we cannot
afford running this for all the CLs on CQ, so an early-abortion heuristic
targeting those suspicious CLs might be needed in order to save time. How about
running this accurate test only when the CL contains new header files? Other
types like .cc can be ignored because missing a .cc file in GN would cause
errors detectable by other means.
There are a few triggering options on the pareto curve:
- Never (fastest, no control on regression, current situation)
- When the CL contains new header files but no modification of GN files
- When the CL contains new header files (my preference right now)
- When the CL contains new header files or GN files are modified
- Always as long as compilation is triggered (slowest, most accurate, as of
PS13)
- Always, no matter what (not possible, ground truth)
Note that the ground truth is not possible, so it is always possible for a
culprit CL to go through the CQ, and get caught on another unrelated CL on CQ,
or eventually on waterfall. I guess the criteria is to make this infrequent
enough so that the whole process is a net-positive in productivity. WDYT?
Patchset #16 (id:390001) has been deleted
https://codereview.chromium.org/2510303002/diff/370004/build/check_gn_headers.py File build/check_gn_headers.py (right): https://codereview.chromium.org/2510303002/diff/370004/build/check_gn_headers... build/check_gn_headers.py:79: """Parse the DEPS file and get the folder names""" On 2017/01/09 20:46:24, Dirk Pranke wrote: > It occurred to me that this won't actually work right, since you're not handling > recursedeps entries. More importantly, it's assuming too much about the > implementation of gclient, which is going to be changing soon to be way more > complicated. > > You're better off calling out to gclient itself to get the list of deps, e.g. > via something like `gclient recurse --no-progress -j 1 python -c 'import os; > print os.environ["GCLIENT_DEP_PATH"]'`, or by adding a command to gclient to do > that in a built-in way (which would be much faster). Nice catch! Relying on gclient now. https://codereview.chromium.org/2510303002/diff/370004/build/check_gn_headers... build/check_gn_headers.py:131: missing = set(filter(lambda x: not StartsWithAny(deps, x), missing)) On 2017/01/09 20:46:24, Dirk Pranke wrote: > missing = {x for x in deps > if not any(x.startswith(d) for d in deps])} Done.
still lgtm. https://codereview.chromium.org/2510303002/diff/410001/build/check_gn_headers.py File build/check_gn_headers.py (right): https://codereview.chromium.org/2510303002/diff/410001/build/check_gn_headers... build/check_gn_headers.py:91: out.add(line) Nit: This can be just: if line: out.add(line) https://codereview.chromium.org/2510303002/diff/410001/build/check_gn_headers... build/check_gn_headers.py:109: missing = {x for x in missing if not any(x.startswith(d) for d in deps)} Nit: I'd probably use "m", not "x" here. My comment earlier was also wrong, of course, but you caught that.
https://codereview.chromium.org/2510303002/diff/410001/build/check_gn_headers.py File build/check_gn_headers.py (right): https://codereview.chromium.org/2510303002/diff/410001/build/check_gn_headers... build/check_gn_headers.py:91: out.add(line) On 2017/01/10 15:58:23, Dirk Pranke wrote: > Nit: This can be just: > > if line: > out.add(line) Done. https://codereview.chromium.org/2510303002/diff/410001/build/check_gn_headers... build/check_gn_headers.py:109: missing = {x for x in missing if not any(x.startswith(d) for d in deps)} On 2017/01/10 15:58:23, Dirk Pranke wrote: > Nit: I'd probably use "m", not "x" here. My comment earlier was also wrong, of > course, but you caught that. Done.
The CQ bit was checked by wychen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org Link to the patchset: https://codereview.chromium.org/2510303002/#ps430001 (title: "address nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 430001, "attempt_start_ts": 1484067758623390,
"parent_rev": "451a66360d2cea9161108d8ebf7f19555e6a07a0", "commit_rev":
"037f6e9e17273a1108a38cf00232d083addb6ee9"}
Message was sent while issue was closed.
Description was changed from ========== Add check_gn_headers.py to find missing headers in GN BUG=661774 ========== to ========== Add check_gn_headers.py to find missing headers in GN BUG=661774 Review-Url: https://codereview.chromium.org/2510303002 Cr-Commit-Position: refs/heads/master@{#442617} Committed: https://chromium.googlesource.com/chromium/src/+/037f6e9e17273a1108a38cf00232... ==========
Message was sent while issue was closed.
Committed patchset #17 (id:430001) as https://chromium.googlesource.com/chromium/src/+/037f6e9e17273a1108a38cf00232... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
