Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2781603003:
Add script fix_gn_headers.py to fix GN missing headers (Closed)
DescriptionAdd script fix_gn_headers.py to fix GN missing headers
The script is based on CLs authored by thakis@chromium.org:
- https://codereview.chromium.org/2770693003/
- https://codereview.chromium.org/2771373003/
BUG=661774
Review-Url: https://codereview.chromium.org/2781603003
Cr-Commit-Position: refs/heads/master@{#460256}
Committed: https://chromium.googlesource.com/chromium/src/+/b663358c89e05334e8cfff0f1f855fe4c72c192a
Patch Set 1 : from thakis #Patch Set 2 : my amendments #
Total comments: 10
Patch Set 3 : address comments #
Dependent Patchsets: Messages
Total messages: 20 (10 generated)
wychen@chromium.org changed reviewers: + thakis@chromium.org
PTAL
lgtm I feel this would fit in better in build/, but it makes sense to put it next to check_gn_headers.py. Maybe they could both move to tools in a future CL. https://codereview.chromium.org/2781603003/diff/20001/build/fix_gn_headers.py File build/fix_gn_headers.py (right): https://codereview.chromium.org/2781603003/diff/20001/build/fix_gn_headers.py... build/fix_gn_headers.py:32: if not (filename.endswith('.h') or filename.endswith('.hh')): Do we have any headers ending in .hh? https://codereview.chromium.org/2781603003/diff/20001/build/fix_gn_headers.py... build/fix_gn_headers.py:57: if lines[linenr - 2] == new: why this? https://codereview.chromium.org/2781603003/diff/20001/build/fix_gn_headers.py... build/fix_gn_headers.py:64: for l in reversed(sorted(edits[gnfile].keys())): Ah, that was the bug! I had assumed the keys were ordered, but you're right, they aren't. Write this as `sorted(edits[gnfile].keys(), reverse=True)` though. https://codereview.chromium.org/2781603003/diff/20001/build/fix_gn_headers.py... build/fix_gn_headers.py:72: """Add header files to the sources list in the closest GN file. "to the _first_ sources list" Mention somehow that this usually does the wrong thing for _test files if the test and the main target are in the same .gn file. https://codereview.chromium.org/2781603003/diff/20001/build/fix_gn_headers.py... build/fix_gn_headers.py:105: help="missing headers, output of check_gn_headers.py") nit: I'd make this one a positional argument
Patchset #3 (id:40001) has been deleted
Did you mean fitting in better in src/tools/? check_gn_headers.py was intended for waterfall and trybots, so I put it in build/. https://codereview.chromium.org/2781603003/diff/20001/build/fix_gn_headers.py File build/fix_gn_headers.py (right): https://codereview.chromium.org/2781603003/diff/20001/build/fix_gn_headers.py... build/fix_gn_headers.py:32: if not (filename.endswith('.h') or filename.endswith('.hh')): On 2017/03/28 14:37:55, Nico wrote: > Do we have any headers ending in .hh? Yes, we do, and some of them are part of the GN missing headers. https://codereview.chromium.org/2781603003/diff/20001/build/fix_gn_headers.py... build/fix_gn_headers.py:57: if lines[linenr - 2] == new: On 2017/03/28 14:37:55, Nico wrote: > why this? This and the line above is for deduplication. lines[linenr] is the line after the match, and lines[linenr-2] is before. Depending on whether that match is a cc or mm, the .h could be before or after. https://codereview.chromium.org/2781603003/diff/20001/build/fix_gn_headers.py... build/fix_gn_headers.py:64: for l in reversed(sorted(edits[gnfile].keys())): On 2017/03/28 14:37:55, Nico wrote: > Ah, that was the bug! I had assumed the keys were ordered, but you're right, > they aren't. Write this as `sorted(edits[gnfile].keys(), reverse=True)` though. Done. https://codereview.chromium.org/2781603003/diff/20001/build/fix_gn_headers.py... build/fix_gn_headers.py:72: """Add header files to the sources list in the closest GN file. On 2017/03/28 14:37:55, Nico wrote: > "to the _first_ sources list" > > Mention somehow that this usually does the wrong thing for _test files if the > test and the main target are in the same .gn file. Done. https://codereview.chromium.org/2781603003/diff/20001/build/fix_gn_headers.py... build/fix_gn_headers.py:105: help="missing headers, output of check_gn_headers.py") On 2017/03/28 14:37:55, Nico wrote: > nit: I'd make this one a positional argument Done.
Yes, I meant tools/, sorry. Some other scripts in tools are called by bots too. On Mar 28, 2017 1:52 PM, <wychen@chromium.org> wrote: Did you mean fitting in better in src/tools/? check_gn_headers.py was intended for waterfall and trybots, so I put it in build/. https://codereview.chromium.org/2781603003/diff/20001/ build/fix_gn_headers.py File build/fix_gn_headers.py (right): https://codereview.chromium.org/2781603003/diff/20001/ build/fix_gn_headers.py#newcode32 build/fix_gn_headers.py:32: if not (filename.endswith('.h') or filename.endswith('.hh')): On 2017/03/28 14:37:55, Nico wrote: > Do we have any headers ending in .hh? Yes, we do, and some of them are part of the GN missing headers. https://codereview.chromium.org/2781603003/diff/20001/ build/fix_gn_headers.py#newcode57 build/fix_gn_headers.py:57: if lines[linenr - 2] == new: On 2017/03/28 14:37:55, Nico wrote: > why this? This and the line above is for deduplication. lines[linenr] is the line after the match, and lines[linenr-2] is before. Depending on whether that match is a cc or mm, the .h could be before or after. https://codereview.chromium.org/2781603003/diff/20001/ build/fix_gn_headers.py#newcode64 build/fix_gn_headers.py:64: for l in reversed(sorted(edits[gnfile].keys())): On 2017/03/28 14:37:55, Nico wrote: > Ah, that was the bug! I had assumed the keys were ordered, but you're right, > they aren't. Write this as `sorted(edits[gnfile].keys(), reverse=True)` though. Done. https://codereview.chromium.org/2781603003/diff/20001/ build/fix_gn_headers.py#newcode72 build/fix_gn_headers.py:72: """Add header files to the sources list in the closest GN file. On 2017/03/28 14:37:55, Nico wrote: > "to the _first_ sources list" > > Mention somehow that this usually does the wrong thing for _test files if the > test and the main target are in the same .gn file. Done. https://codereview.chromium.org/2781603003/diff/20001/ build/fix_gn_headers.py#newcode105 build/fix_gn_headers.py:105: help="missing headers, output of check_gn_headers.py") On 2017/03/28 14:37:55, Nico wrote: > nit: I'd make this one a positional argument Done. https://codereview.chromium.org/2781603003/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by wychen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org Link to the patchset: https://codereview.chromium.org/2781603003/#ps60001 (title: "address comments")
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 commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by wychen@chromium.org
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 commit-bot@chromium.org
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't started yet, builder probably lacks capacity)
The CQ bit was checked by wychen@chromium.org
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": 60001, "attempt_start_ts": 1490748468975400,
"parent_rev": "e80b960e27af6495ff3dd79cbfcede18d48da85a", "commit_rev":
"b663358c89e05334e8cfff0f1f855fe4c72c192a"}
Message was sent while issue was closed.
Description was changed from ========== Add script fix_gn_headers.py to fix GN missing headers The script is based on CLs authored by thakis@chromium.org: - https://codereview.chromium.org/2770693003/ - https://codereview.chromium.org/2771373003/ BUG=661774 ========== to ========== Add script fix_gn_headers.py to fix GN missing headers The script is based on CLs authored by thakis@chromium.org: - https://codereview.chromium.org/2770693003/ - https://codereview.chromium.org/2771373003/ BUG=661774 Review-Url: https://codereview.chromium.org/2781603003 Cr-Commit-Position: refs/heads/master@{#460256} Committed: https://chromium.googlesource.com/chromium/src/+/b663358c89e05334e8cfff0f1f85... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/chromium/src/+/b663358c89e05334e8cfff0f1f85... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
