Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2001613002:
[iOS/GN] Add template for generate_localizable_strings tool. (Closed)
Description[iOS/GN] Add template for generate_localizable_strings tool.
BUG=459705
Committed: https://crrev.com/a95ac7082963239e2833c4ba335af2a60daa5486
Cr-Commit-Position: refs/heads/master@{#395097}
Patch Set 1 #
Total comments: 2
Dependent Patchsets: Messages
Total messages: 18 (7 generated)
sdefresne@chromium.org changed reviewers: + justincohen@chomium.org
Please take a look and send to CQ if LGTY. Note that "action" target can only invoke python script (and there is no generalised python script to just re-invoke a built target).
Description was changed from ========== [iOS/GN] Add template for generate_localizable_strings tool. BUG=459705 ========== to ========== [iOS/GN] Add template for generate_localizable_strings tool. BUG=459705 ==========
sdefresne@chromium.org changed reviewers: + stkhapugin@chromium.org - justincohen@chomium.org
-justincohen, +stkhapugin: changing reviewer due to OOO. Please take a look and send to CQ if LGTY.
justincohen@chromium.org changed reviewers: + justincohen@chromium.org
lgtm with questions. https://codereview.chromium.org/2001613002/diff/1/ios/chrome/tools/strings/ge... File ios/chrome/tools/strings/generate_localizable_strings.py (right): https://codereview.chromium.org/2001613002/diff/1/ios/chrome/tools/strings/ge... ios/chrome/tools/strings/generate_localizable_strings.py:7: comment explaining what the script does. https://codereview.chromium.org/2001613002/diff/1/ios/chrome/tools/strings/ge... ios/chrome/tools/strings/generate_localizable_strings.py:19: os.execv(tool, [tool, '-c', config_file, '-I', include_dir, '-p', I guess gn can't call out to exec itself? This is a pretty general script. Why not just do this in GN?
On 2016/05/20 15:39:01, justincohen wrote: > lgtm with questions. > > https://codereview.chromium.org/2001613002/diff/1/ios/chrome/tools/strings/ge... > File ios/chrome/tools/strings/generate_localizable_strings.py (right): > > https://codereview.chromium.org/2001613002/diff/1/ios/chrome/tools/strings/ge... > ios/chrome/tools/strings/generate_localizable_strings.py:7: > comment explaining what the script does. > > https://codereview.chromium.org/2001613002/diff/1/ios/chrome/tools/strings/ge... > ios/chrome/tools/strings/generate_localizable_strings.py:19: os.execv(tool, > [tool, '-c', config_file, '-I', include_dir, '-p', > I guess gn can't call out to exec itself? This is a pretty general script. Why > not just do this in GN? I think we force the use of python script (i.e. we do not want people using unix tools like "cp" or whatever that are not porable) because it needs to be portable to other OSes (Windows mostly). This particular action is iOS specific, but most of the actions are not. Using a script here also allow me to use positional arguments which make the .gni file more readable. I personally would like to have a way to launch a binary without writing a wrapper python script (even if this is just by re-using a shared "exec.py' script that just call os.execvp). Maybe we can add such a script to //build/ (next to build/cp.py and similar scripts).
On 2016/05/20 15:39:01, justincohen wrote: > lgtm with questions. > > https://codereview.chromium.org/2001613002/diff/1/ios/chrome/tools/strings/ge... > File ios/chrome/tools/strings/generate_localizable_strings.py (right): > > https://codereview.chromium.org/2001613002/diff/1/ios/chrome/tools/strings/ge... > ios/chrome/tools/strings/generate_localizable_strings.py:7: > comment explaining what the script does. > > https://codereview.chromium.org/2001613002/diff/1/ios/chrome/tools/strings/ge... > ios/chrome/tools/strings/generate_localizable_strings.py:19: os.execv(tool, > [tool, '-c', config_file, '-I', include_dir, '-p', > I guess gn can't call out to exec itself? This is a pretty general script. Why > not just do this in GN? I think we force the use of python script (i.e. we do not want people using unix tools like "cp" or whatever that are not porable) because it needs to be portable to other OSes (Windows mostly). This particular action is iOS specific, but most of the actions are not. Using a script here also allow me to use positional arguments which make the .gni file more readable. I personally would like to have a way to launch a binary without writing a wrapper python script (even if this is just by re-using a shared "exec.py' script that just call os.execvp). Maybe we can add such a script to //build/ (next to build/cp.py and similar scripts). I'll file a bug to add such a script but will land this CL as is if you don't mind.
lgtm, is it worth adding a bug to do what you suggest later?
On 2016/05/20 15:49:13, justincohen wrote: > lgtm, is it worth adding a bug to do what you suggest later? => https://bugs.chromium.org/p/chromium/issues/detail?id=613589
The CQ bit was checked by sdefresne@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2001613002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2001613002/1
Message was sent while issue was closed.
Description was changed from ========== [iOS/GN] Add template for generate_localizable_strings tool. BUG=459705 ========== to ========== [iOS/GN] Add template for generate_localizable_strings tool. BUG=459705 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== [iOS/GN] Add template for generate_localizable_strings tool. BUG=459705 ========== to ========== [iOS/GN] Add template for generate_localizable_strings tool. BUG=459705 Committed: https://crrev.com/a95ac7082963239e2833c4ba335af2a60daa5486 Cr-Commit-Position: refs/heads/master@{#395097} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/a95ac7082963239e2833c4ba335af2a60daa5486 Cr-Commit-Position: refs/heads/master@{#395097} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
