Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1693963005:
bin/sync-and-gyp: start using gclient (Closed)
|
Created:
4 years, 10 months ago by hal.canary Modified:
4 years, 10 months 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: start using gclient
stop using hard-to-maintain git-sync-deps
BUG=skia:4885
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1693963005
Committed: https://skia.googlesource.com/skia/+/2362c476ef4f05ae2d63c00834f2447480758b74
Patch Set 1 #
Total comments: 7
Patch Set 2 : 2016-02-16 (Tuesday) 12:14:21 EST #
Total comments: 1
Messages
Total messages: 21 (8 generated)
Description was changed from ========== bin/sync-and-gyp: start using gclient stop using hard-to-maintain git-sync-deps ========== to ========== bin/sync-and-gyp: start using gclient stop using hard-to-maintain git-sync-deps GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== bin/sync-and-gyp: start using gclient stop using hard-to-maintain git-sync-deps GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== bin/sync-and-gyp: start using gclient stop using hard-to-maintain git-sync-deps BUG=skia:4885 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
halcanary@google.com changed reviewers: + scroggo@google.com
ptal
https://codereview.chromium.org/1693963005/diff/1/bin/sync-and-gyp File bin/sync-and-gyp (left): https://codereview.chromium.org/1693963005/diff/1/bin/sync-and-gyp#oldcode46 bin/sync-and-gyp:46: subprocess.call(['python', 'tools/git-sync-deps'], env=env) Should we also delete tools/git-sync-deps? https://codereview.chromium.org/1693963005/diff/1/bin/sync-and-gyp File bin/sync-and-gyp (right): https://codereview.chromium.org/1693963005/diff/1/bin/sync-and-gyp#newcode55 bin/sync-and-gyp:55: gclient_config = ''' Why not use the existing gclient config? Mine is slightly different from this one - it includes the line: target_os = ['android'] which downloads the Android toolchains, etc. https://codereview.chromium.org/1693963005/diff/1/bin/sync-and-gyp#newcode69 bin/sync-and-gyp:69: # `gclient sync` is very slow, so skip whenver we can whenever*
mtklein@google.com changed reviewers: + mtklein@google.com
https://codereview.chromium.org/1693963005/diff/1/bin/sync-and-gyp File bin/sync-and-gyp (right): https://codereview.chromium.org/1693963005/diff/1/bin/sync-and-gyp#newcode55 bin/sync-and-gyp:55: gclient_config = ''' On 2016/02/16 15:02:42, scroggo wrote: > Why not use the existing gclient config? Mine is slightly different from this > one - it includes the line: > > target_os = ['android'] > > which downloads the Android toolchains, etc. Nah, target_os = ['android'] doesn't actually do anything right now. Toolchains download on first use.
halcanary@google.com changed reviewers: + mtklein@google.com
https://codereview.chromium.org/1693963005/diff/1/bin/sync-and-gyp File bin/sync-and-gyp (left): https://codereview.chromium.org/1693963005/diff/1/bin/sync-and-gyp#oldcode46 bin/sync-and-gyp:46: subprocess.call(['python', 'tools/git-sync-deps'], env=env) On 2016/02/16 at 15:02:42, scroggo wrote: > Should we also delete tools/git-sync-deps? Maybe. Let's do that in another CL, in case someone is relying on it. https://codereview.chromium.org/1693963005/diff/1/bin/sync-and-gyp File bin/sync-and-gyp (right): https://codereview.chromium.org/1693963005/diff/1/bin/sync-and-gyp#newcode55 bin/sync-and-gyp:55: gclient_config = ''' On 2016/02/16 at 15:02:42, scroggo wrote: > Why not use the existing gclient config? Mine is slightly different from this one - it includes the line: > > target_os = ['android'] > > which downloads the Android toolchains, etc. okay. I'll check to see if the file exists first. https://codereview.chromium.org/1693963005/diff/1/bin/sync-and-gyp#newcode69 bin/sync-and-gyp:69: # `gclient sync` is very slow, so skip whenver we can On 2016/02/16 at 15:02:42, scroggo wrote: > whenever* done
On 2016/02/16 at 15:32:05, mtklein wrote: > https://codereview.chromium.org/1693963005/diff/1/bin/sync-and-gyp > File bin/sync-and-gyp (right): > > https://codereview.chromium.org/1693963005/diff/1/bin/sync-and-gyp#newcode55 > bin/sync-and-gyp:55: gclient_config = ''' > On 2016/02/16 15:02:42, scroggo wrote: > > Why not use the existing gclient config? Mine is slightly different from this > > one - it includes the line: > > > > target_os = ['android'] > > > > which downloads the Android toolchains, etc. > > Nah, target_os = ['android'] doesn't actually do anything right now. Toolchains download on first use. Right. But maybe in the future we'll use target_os to make downloading some deps (e.g. llvm) optional.
On 2016/02/16 15:44:55, Hal Canary wrote: > On 2016/02/16 at 15:32:05, mtklein wrote: > > https://codereview.chromium.org/1693963005/diff/1/bin/sync-and-gyp > > File bin/sync-and-gyp (right): > > > > https://codereview.chromium.org/1693963005/diff/1/bin/sync-and-gyp#newcode55 > > bin/sync-and-gyp:55: gclient_config = ''' > > On 2016/02/16 15:02:42, scroggo wrote: > > > Why not use the existing gclient config? Mine is slightly different from > this > > > one - it includes the line: > > > > > > target_os = ['android'] > > > > > > which downloads the Android toolchains, etc. > > > > Nah, target_os = ['android'] doesn't actually do anything right now. > Toolchains download on first use. > > Right. But maybe in the future we'll use target_os to make downloading some > deps (e.g. llvm) optional. Can we just use this for any feature flag, or does it have to be some built-in list of OSes? E.g. target_os = ['linux', 'android', 'xsan_bot']
On 2016/02/16 at 15:46:51, mtklein wrote: > On 2016/02/16 15:44:55, Hal Canary wrote: > > On 2016/02/16 at 15:32:05, mtklein wrote: > > > https://codereview.chromium.org/1693963005/diff/1/bin/sync-and-gyp > > > File bin/sync-and-gyp (right): > > > > > > https://codereview.chromium.org/1693963005/diff/1/bin/sync-and-gyp#newcode55 > > > bin/sync-and-gyp:55: gclient_config = ''' > > > On 2016/02/16 15:02:42, scroggo wrote: > > > > Why not use the existing gclient config? Mine is slightly different from > > this > > > > one - it includes the line: > > > > > > > > target_os = ['android'] > > > > > > > > which downloads the Android toolchains, etc. > > > > > > Nah, target_os = ['android'] doesn't actually do anything right now. > > Toolchains download on first use. > > > > Right. But maybe in the future we'll use target_os to make downloading some > > deps (e.g. llvm) optional. > > > Can we just use this for any feature flag, or does it have to be some built-in list of OSes? > > E.g. target_os = ['linux', 'android', 'xsan_bot'] That's exactly what I was thinking. Let's ask someone who understands gclient. Maybe someone on the infra team?
On 2016/02/16 at 15:43:44, Hal Canary wrote: > https://codereview.chromium.org/1693963005/diff/1/bin/sync-and-gyp > File bin/sync-and-gyp (left): > > https://codereview.chromium.org/1693963005/diff/1/bin/sync-and-gyp#oldcode46 > bin/sync-and-gyp:46: subprocess.call(['python', 'tools/git-sync-deps'], env=env) > On 2016/02/16 at 15:02:42, scroggo wrote: > > Should we also delete tools/git-sync-deps? > > Maybe. Let's do that in another CL, in case someone is relying on it. > > https://codereview.chromium.org/1693963005/diff/1/bin/sync-and-gyp > File bin/sync-and-gyp (right): > > https://codereview.chromium.org/1693963005/diff/1/bin/sync-and-gyp#newcode55 > bin/sync-and-gyp:55: gclient_config = ''' > On 2016/02/16 at 15:02:42, scroggo wrote: > > Why not use the existing gclient config? Mine is slightly different from this one - it includes the line: > > > > target_os = ['android'] > > > > which downloads the Android toolchains, etc. > > okay. I'll check to see if the file exists first. > > https://codereview.chromium.org/1693963005/diff/1/bin/sync-and-gyp#newcode69 > bin/sync-and-gyp:69: # `gclient sync` is very slow, so skip whenver we can > On 2016/02/16 at 15:02:42, scroggo wrote: > > whenever* > > done I said `done`, but failed to upload the new patch first. PTAL now.
lgtm
The CQ bit was checked by halcanary@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1693963005/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1693963005/20001
Message was sent while issue was closed.
Description was changed from ========== bin/sync-and-gyp: start using gclient stop using hard-to-maintain git-sync-deps BUG=skia:4885 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== bin/sync-and-gyp: start using gclient stop using hard-to-maintain git-sync-deps BUG=skia:4885 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/2362c476ef4f05ae2d63c00834f2447480758b74 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://skia.googlesource.com/skia/+/2362c476ef4f05ae2d63c00834f2447480758b74
Message was sent while issue was closed.
kjlubick@google.com changed reviewers: + kjlubick@google.com
Message was sent while issue was closed.
https://codereview.chromium.org/1693963005/diff/20001/bin/sync-and-gyp File bin/sync-and-gyp (right): https://codereview.chromium.org/1693963005/diff/20001/bin/sync-and-gyp#newcode76 bin/sync-and-gyp:76: sys.stderr.write('\n`gclient sync` failed.\n') Drive by comment, sys is not defined.
Message was sent while issue was closed.
On 2016/02/17 at 13:57:04, kjlubick wrote: > https://codereview.chromium.org/1693963005/diff/20001/bin/sync-and-gyp > File bin/sync-and-gyp (right): > > https://codereview.chromium.org/1693963005/diff/20001/bin/sync-and-gyp#newcode76 > bin/sync-and-gyp:76: sys.stderr.write('\n`gclient sync` failed.\n') > Drive by comment, sys is not defined. Oh, python..... I'll fix this. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
