Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years ago by ehmaldonado_webrtc Modified:
4 years ago Reviewers:
kjellander_webrtc CC:
webrtc-reviews_webrtc.org Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd a tool to fix (some) errors reported by gn gen --check.
BUG=webrtc:6828
R=kjellander@webrtc.org
NOTRY=True
Committed: https://crrev.com/01653b1130934b809816f7a5ad3c4b8c73d8d411
Cr-Commit-Position: refs/heads/master@{#15483}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Some improvements. #Patch Set 3 : More improvements! #
Total comments: 2
Patch Set 4 : Use a temporary dir. Style changes. #Patch Set 5 : Revert .gn #
Created: 4 years ago
Messages
Total messages: 17 (6 generated)
Just FYI :) There are some lint errors that I need to fix, and small improvements I can make.
This is very nice! It would probably have saved me an hour of work yesterday. https://codereview.webrtc.org/2555813003/diff/1/tools/gn_check_autofix.py File tools/gn_check_autofix.py (right): https://codereview.webrtc.org/2555813003/diff/1/tools/gn_check_autofix.py#new... tools/gn_check_autofix.py:11: import os Add module documentation that at least mentions that this won't change existing deps entries but rather add a deps += at the end of the target. https://codereview.webrtc.org/2555813003/diff/1/tools/gn_check_autofix.py#new... tools/gn_check_autofix.py:18: nit: Two blank lines between top-level definitions, one blank line between method definitions. https://google.github.io/styleguide/pyguide.html?showone=Blank_Lines#Blank_Lines https://codereview.webrtc.org/2555813003/diff/1/tools/gn_check_autofix.py#new... tools/gn_check_autofix.py:23: def fix_missing(filename, missing): Rename to FixMissing, according to http://dev.chromium.org/chromium-os/python-style-guidelines (one of two exceptions from the Google Python Style guide that Chromium uses): "Naming: Functions and Method names use CapWords() style, not lower_with_under()" https://codereview.webrtc.org/2555813003/diff/1/tools/gn_check_autofix.py#new... tools/gn_check_autofix.py:26: target_re = re.compile(r'(?P<indentation_level>\s*)\w*\("(?P<target_name>\w*)"\) {$') Wrap at column 80 https://codereview.webrtc.org/2555813003/diff/1/tools/gn_check_autofix.py#new... tools/gn_check_autofix.py:27: blocks = "" Use single-quotes consistently for strings. From http://google.github.io/styleguide/pyguide.html?showone=Strings#Strings: "Be consistent with your choice of string quote character within a file. Pick ' or " and stick with it. " https://codereview.webrtc.org/2555813003/diff/1/tools/gn_check_autofix.py#new... tools/gn_check_autofix.py:54: missing = defaultdict(lambda: defaultdict(set)) Use a main() function instead: http://google.github.io/styleguide/pyguide.html?showone=Main#Main https://codereview.webrtc.org/2555813003/diff/1/tools/gn_check_autofix.py#new... tools/gn_check_autofix.py:68: if 'rtc_base' in dep and not 'rtc_base_approved' in dep: Let's remove this as we decided to drop the PRESUBMIT check due to the ongoing refactoring work for base/ https://codereview.webrtc.org/2555813003/diff/1/tools/gn_check_autofix.py#new... tools/gn_check_autofix.py:74: if target == "audio_decoder_unittests_bundle_data": Is this needed? I suggest keeping hardcoded assumptions out of this script. Some manual work is to be expected even after running this script anyway.
Description was changed from ========== Add a tool to fix (some) errors reported by gn gen --check. R=kjellander@webrtc.org ========== to ========== Add a tool to fix (some) errors reported by gn gen --check. BUG=webrtc:6828 R=kjellander@webrtc.org ==========
On 2016/12/07 12:31:38, kjellander_webrtc wrote: > This is very nice! It would probably have saved me an hour of work yesterday. > > https://codereview.webrtc.org/2555813003/diff/1/tools/gn_check_autofix.py > File tools/gn_check_autofix.py (right): > > https://codereview.webrtc.org/2555813003/diff/1/tools/gn_check_autofix.py#new... > tools/gn_check_autofix.py:11: import os > Add module documentation that at least mentions that this won't change existing > deps entries but rather add a deps += at the end of the target. > > https://codereview.webrtc.org/2555813003/diff/1/tools/gn_check_autofix.py#new... > tools/gn_check_autofix.py:18: > nit: Two blank lines between top-level definitions, one blank line between > method definitions. > https://google.github.io/styleguide/pyguide.html?showone=Blank_Lines#Blank_Lines > > https://codereview.webrtc.org/2555813003/diff/1/tools/gn_check_autofix.py#new... > tools/gn_check_autofix.py:23: def fix_missing(filename, missing): > Rename to FixMissing, according to > http://dev.chromium.org/chromium-os/python-style-guidelines > (one of two exceptions from the Google Python Style guide that Chromium uses): > "Naming: Functions and Method names use CapWords() style, not > lower_with_under()" > > https://codereview.webrtc.org/2555813003/diff/1/tools/gn_check_autofix.py#new... > tools/gn_check_autofix.py:26: target_re = > re.compile(r'(?P<indentation_level>\s*)\w*\("(?P<target_name>\w*)"\) {$') > Wrap at column 80 > > https://codereview.webrtc.org/2555813003/diff/1/tools/gn_check_autofix.py#new... > tools/gn_check_autofix.py:27: blocks = "" > Use single-quotes consistently for strings. > From http://google.github.io/styleguide/pyguide.html?showone=Strings#Strings: > "Be consistent with your choice of string quote character within a file. Pick ' > or " and stick with it. " > > https://codereview.webrtc.org/2555813003/diff/1/tools/gn_check_autofix.py#new... > tools/gn_check_autofix.py:54: missing = defaultdict(lambda: defaultdict(set)) > Use a main() function instead: > http://google.github.io/styleguide/pyguide.html?showone=Main#Main > > https://codereview.webrtc.org/2555813003/diff/1/tools/gn_check_autofix.py#new... > tools/gn_check_autofix.py:68: if 'rtc_base' in dep and not 'rtc_base_approved' > in dep: > Let's remove this as we decided to drop the PRESUBMIT check due to the ongoing > refactoring work for base/ > > https://codereview.webrtc.org/2555813003/diff/1/tools/gn_check_autofix.py#new... > tools/gn_check_autofix.py:74: if target == > "audio_decoder_unittests_bundle_data": > Is this needed? I suggest keeping hardcoded assumptions out of this script. Some > manual work is to be expected even after running this script anyway. FYI: I added BUG=webrtc:6828
Made some improvements. Now it puts the deps inside the deps ;) Don't review it yet, I'm still working on it and I'm mostly sharing in case you want to use it :)
On 2016/12/07 12:58:21, ehmaldonado_webrtc wrote: > Made some improvements. Now it puts the deps inside the deps ;) > Don't review it yet, I'm still working on it and I'm mostly sharing in case you > want to use it :) More improvements: Now it writes '..'s as necessary instead of using //webrtc/
Very nice! I'm missing my previous comments being addressed still. https://codereview.webrtc.org/2555813003/diff/40001/.gn File .gn (right): https://codereview.webrtc.org/2555813003/diff/40001/.gn#newcode33 .gn:33: "//webrtc/*", I assume this shouldn't be here? https://codereview.webrtc.org/2555813003/diff/40001/tools/gn_check_autofix.py File tools/gn_check_autofix.py (right): https://codereview.webrtc.org/2555813003/diff/40001/tools/gn_check_autofix.py... tools/gn_check_autofix.py:72: gn_output = Run(['gn', 'gen', 'out/gn', '--check'])[0] Ideally we'd generate a temporary directory here and delete it after execution, how does that sound? I'm afraid we might overwrite an existing dir.
I've clean it up a bit now. I should probably add some documentation. Now you call it like you would call mb, python tools/gn_check_autofix.py -m some_master -b some_bot or python tools/gn_check_autofix.py -c some_mb_config And it will run mb gen in a temporary directory. That way you can check for different configurations.
Using MB is a good idea! My only concern is that for some platforms it'll just fail since some dependencies are missing (unless the user configures target_os = ["unix", "mac", "ios", "android", "win"] but hey, that doesn't really matter here since we don't need to to this tool that user friendly. lgtm
Description was changed from ========== Add a tool to fix (some) errors reported by gn gen --check. BUG=webrtc:6828 R=kjellander@webrtc.org ========== to ========== Add a tool to fix (some) errors reported by gn gen --check. BUG=webrtc:6828 R=kjellander@webrtc.org NOTRY=True ==========
The CQ bit was checked by ehmaldonado@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1481210636688180,
"parent_rev": "a195cd7d32fc9c97b144bec760ea8b019628b094", "commit_rev":
"8029b2164a5b3e6dc2a7f49d5b0c0fb77a2996e7"}
Message was sent while issue was closed.
Description was changed from ========== Add a tool to fix (some) errors reported by gn gen --check. BUG=webrtc:6828 R=kjellander@webrtc.org NOTRY=True ========== to ========== Add a tool to fix (some) errors reported by gn gen --check. BUG=webrtc:6828 R=kjellander@webrtc.org NOTRY=True ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Add a tool to fix (some) errors reported by gn gen --check. BUG=webrtc:6828 R=kjellander@webrtc.org NOTRY=True ========== to ========== Add a tool to fix (some) errors reported by gn gen --check. BUG=webrtc:6828 R=kjellander@webrtc.org NOTRY=True Committed: https://crrev.com/01653b1130934b809816f7a5ad3c4b8c73d8d411 Cr-Commit-Position: refs/heads/master@{#15483} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/01653b1130934b809816f7a5ad3c4b8c73d8d411 Cr-Commit-Position: refs/heads/master@{#15483} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
