Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1425593011:
bin/sync-and-gyp: sh->py (make more cross-platform) (Closed)
|
Created:
5 years, 1 month ago by hal.canary Modified:
5 years, 1 month ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
Descriptionbin/sync-and-gyp: sh->py (make more cross-platform)
Committed: https://skia.googlesource.com/skia/+/a4c26c0d3a1ffdd0bab08b1aec24bea025a0d9e2
Patch Set 1 #Patch Set 2 : 2015-11-06 (Friday) 17:43:36 EST #Patch Set 3 : 2015-11-06 (Friday) 17:47:08 EST #
Total comments: 13
Patch Set 4 : 2015-11-09 (Monday) 10:44:58 EST #Patch Set 5 : 2015-11-09 (Monday) 11:13:02 EST #Messages
Total messages: 15 (5 generated)
halcanary@gmail.com changed reviewers: + bungeman@google.com, halcanary@gmail.com
ptal
halcanary@google.com changed reviewers: - halcanary@gmail.com
mtklein@google.com changed reviewers: + mtklein@google.com
lgtm https://codereview.chromium.org/1425593011/diff/40001/bin/sync-and-gyp File bin/sync-and-gyp (right): https://codereview.chromium.org/1425593011/diff/40001/bin/sync-and-gyp#newcode35 bin/sync-and-gyp:35: skia_out = 'out' Seems like we'd want to abspath this too? https://codereview.chromium.org/1425593011/diff/40001/bin/sync-and-gyp#newcode49 bin/sync-and-gyp:49: for var in ['CC', 'CXX', 'GYP_GENERATORS', 'GYP_DEFINES']: Might add in CFLAGS, CPPFLAGS, CXXFLAGS? (Alternatively, the entire environment?) https://codereview.chromium.org/1425593011/diff/40001/bin/sync-and-gyp#newcode62 bin/sync-and-gyp:62: for dir in ['bench', 'gm', 'tests']: Think it makes sense to generalize this to all directories? You're probably going to need to rerun GYP if a new source file shows up anywhere.
https://codereview.chromium.org/1425593011/diff/40001/bin/sync-and-gyp File bin/sync-and-gyp (right): https://codereview.chromium.org/1425593011/diff/40001/bin/sync-and-gyp#newcode35 bin/sync-and-gyp:35: skia_out = 'out' On 2015/11/09 at 15:14:20, mtklein wrote: > Seems like we'd want to abspath this too? No need. It will be relative to `skia_dir`. https://codereview.chromium.org/1425593011/diff/40001/bin/sync-and-gyp#newcode49 bin/sync-and-gyp:49: for var in ['CC', 'CXX', 'GYP_GENERATORS', 'GYP_DEFINES']: On 2015/11/09 at 15:14:20, mtklein wrote: > Might add in CFLAGS, CPPFLAGS, CXXFLAGS? Does gyp respect those? If so, yes. > (Alternatively, the entire environment?) Seems like overkill.
https://codereview.chromium.org/1425593011/diff/40001/bin/sync-and-gyp File bin/sync-and-gyp (right): https://codereview.chromium.org/1425593011/diff/40001/bin/sync-and-gyp#newcode11 bin/sync-and-gyp:11: # Depends on: Python, and Git. Extra comma. Also, aside from the call to git-sync-deps, this doesn't appear to need git. https://codereview.chromium.org/1425593011/diff/40001/bin/sync-and-gyp#newcode49 bin/sync-and-gyp:49: for var in ['CC', 'CXX', 'GYP_GENERATORS', 'GYP_DEFINES']: The environment variables which at least the ninja generator bakes in should be used here. CC, CXX, CPPFLAGS, CFLAGS, CPPFLAGS_host, CFLAGS_host, CC_host, CC_target, CXX_host, CXX_target, AR, AR_host, AR_target, NM, NM_host, NM_target, READELF, READELF_host, READELF_target That's the quick list, there are probably more. https://codereview.chromium.org/1425593011/diff/40001/bin/sync-and-gyp#newcode60 bin/sync-and-gyp:60: hasher.update(f.read()) It seems unfortunate to need to read the content, as opposed to using an already computed hash. https://codereview.chromium.org/1425593011/diff/40001/bin/sync-and-gyp#newcode62 bin/sync-and-gyp:62: for dir in ['bench', 'gm', 'tests']: On 2015/11/09 15:14:20, mtklein wrote: > Think it makes sense to generalize this to all directories? You're probably > going to need to rerun GYP if a new source file shows up anywhere. It does seem unfortunate that this file needs to be kept in sync with the globbing. Also, is scanning all the files worth it? Re-gyping Skia doesn't exactly take a long time. Most of the win of this script is only pulling DEPS when needed? Or is it the principle of the thing? https://codereview.chromium.org/1425593011/diff/40001/bin/sync-and-gyp#newcode78 bin/sync-and-gyp:78: env['SKIA_OUT'] = skia_out Before this created this directory, but this no longer seems to be the case. Does this directory need to be created here?
https://codereview.chromium.org/1425593011/diff/40001/bin/sync-and-gyp File bin/sync-and-gyp (right): https://codereview.chromium.org/1425593011/diff/40001/bin/sync-and-gyp#newcode62 bin/sync-and-gyp:62: for dir in ['bench', 'gm', 'tests']: On 2015/11/09 at 15:30:08, bungeman1 wrote: > On 2015/11/09 15:14:20, mtklein wrote: > > Think it makes sense to generalize this to all directories? You're probably > > going to need to rerun GYP if a new source file shows up anywhere. > > It does seem unfortunate that this file needs to be kept in sync with the globbing. Also, is scanning all the files worth it? Re-gyping Skia doesn't exactly take a long time. Most of the win of this script is only pulling DEPS when needed? Or is it the principle of the thing? Being able to skip re-Gyping Skia on Windows seems like the selling point of the whole CL.
On 2015/11/09 at 15:36:55, mtklein wrote: > https://codereview.chromium.org/1425593011/diff/40001/bin/sync-and-gyp > File bin/sync-and-gyp (right): > > https://codereview.chromium.org/1425593011/diff/40001/bin/sync-and-gyp#newcode62 > bin/sync-and-gyp:62: for dir in ['bench', 'gm', 'tests']: > On 2015/11/09 at 15:30:08, bungeman1 wrote: > > On 2015/11/09 15:14:20, mtklein wrote: > > > Think it makes sense to generalize this to all directories? You're probably > > > going to need to rerun GYP if a new source file shows up anywhere. > > > > It does seem unfortunate that this file needs to be kept in sync with the globbing. Also, is scanning all the files worth it? Re-gyping Skia doesn't exactly take a long time. Most of the win of this script is only pulling DEPS when needed? Or is it the principle of the thing? > > Being able to skip re-Gyping Skia on Windows seems like the selling point of the whole CL. $ time ./gyp_skia GYP_GENERATORS is "ninja" Updating projects from gyp files... real 0.901 user 2.241 sys 0.603 0.9 seconds is too long.
> 0.9 seconds is too long. (And it's much longer on Windows.)
landing now. https://codereview.chromium.org/1425593011/diff/40001/bin/sync-and-gyp File bin/sync-and-gyp (right): https://codereview.chromium.org/1425593011/diff/40001/bin/sync-and-gyp#newcode60 bin/sync-and-gyp:60: hasher.update(f.read()) On 2015/11/09 at 15:30:08, bungeman1 wrote: > It seems unfortunate to need to read the content, as opposed to using an already computed hash. I don't have a better solution. How does git do this? https://codereview.chromium.org/1425593011/diff/40001/bin/sync-and-gyp#newcode78 bin/sync-and-gyp:78: env['SKIA_OUT'] = skia_out On 2015/11/09 at 15:30:08, bungeman1 wrote: > Before this created this directory, but this no longer seems to be the case. Does this directory need to be created here? I created the directory because $(cd "$dir"; pwd) is the posix-shell way of doing os.path.abspath(dir). If successful, `gyp_skia` will create the directory.
The CQ bit was checked by halcanary@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@google.com Link to the patchset: https://codereview.chromium.org/1425593011/#ps80001 (title: "2015-11-09 (Monday) 11:13:02 EST")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1425593011/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1425593011/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://skia.googlesource.com/skia/+/a4c26c0d3a1ffdd0bab08b1aec24bea025a0d9e2 |
