Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2028413004:
Add a configuration to fetch "ios_internal" project. (Closed)
|
Created:
4 years, 6 months ago by sdefresne Modified:
4 years, 6 months ago CC:
chromium-reviews, dpranke+depot_tools_chromium.org, iannucci+depot_tools_chromium.org Base URL:
https://chromium.googlesource.com/chromium/tools/depot_tools.git@master Target Ref:
refs/heads/master Project:
depot_tools Visibility:
Public. |
DescriptionAdd a configuration to fetch "ios_internal" project.
This allow using "fetch ios_internal" to setup a repository for Chrome
on iOS (this still requires being a Google employee as this fetch a
private downstream repository).
The repository URL was already public as it is in every public bug that
is linked from a downstream CL, e.g. http://crbug.com/459705#c115.
BUG=None
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=300689
Patch Set 1 #
Total comments: 2
Patch Set 2 : Address comments #
Total comments: 6
Patch Set 3 : Address comments #Messages
Total messages: 16 (6 generated)
Description was changed from ========== Add a configuration to fetch "ios_internal" project. This allow using "fetch ios_internal" to setup a repository for Chrome on iOS (this still requires being a Google employee as this fetch a private downstream repository). The repository URL was already public as it is in every public bug that is linked from a CL made in the downstream tree, e.g. crbug/459705#c115. BUG=None ========== to ========== Add a configuration to fetch "ios_internal" project. This allow using "fetch ios_internal" to setup a repository for Chrome on iOS (this still requires being a Google employee as this fetch a private downstream repository). The repository URL was already public as it is in every public bug that is linked from a downstream CL, e.g. http://crbug.com/459705#c115. BUG=None ==========
rohitrao@chromium.org changed reviewers: + rohitrao@chromium.org
https://codereview.chromium.org/2028413004/diff/1/fetch_configs/ios_internal.py File fetch_configs/ios_internal.py (right): https://codereview.chromium.org/2028413004/diff/1/fetch_configs/ios_internal.... fetch_configs/ios_internal.py:20: 'deps_file': '.DEPS.git', Just DEPS? https://codereview.chromium.org/2028413004/diff/1/fetch_configs/ios_internal.... fetch_configs/ios_internal.py:33: spec['target_os'] = ['ios', 'mac'] Why is mac in here?
On 2016/06/02 14:13:12, rohitrao wrote: > https://codereview.chromium.org/2028413004/diff/1/fetch_configs/ios_internal.py > File fetch_configs/ios_internal.py (right): > > https://codereview.chromium.org/2028413004/diff/1/fetch_configs/ios_internal.... > fetch_configs/ios_internal.py:20: 'deps_file': '.DEPS.git', > Just DEPS? > > https://codereview.chromium.org/2028413004/diff/1/fetch_configs/ios_internal.... > fetch_configs/ios_internal.py:33: spec['target_os'] = ['ios', 'mac'] > Why is mac in here? It is not necessary, but explicit is better than implicit.
On 2016/06/02 14:16:00, sdefresne wrote: > On 2016/06/02 14:13:12, rohitrao wrote: > > > https://codereview.chromium.org/2028413004/diff/1/fetch_configs/ios_internal.py > > File fetch_configs/ios_internal.py (right): > > > > > https://codereview.chromium.org/2028413004/diff/1/fetch_configs/ios_internal.... > > fetch_configs/ios_internal.py:20: 'deps_file': '.DEPS.git', > > Just DEPS? fetch_configs/chromium.py uses .DEPS.git. > > > https://codereview.chromium.org/2028413004/diff/1/fetch_configs/ios_internal.... > > fetch_configs/ios_internal.py:33: spec['target_os'] = ['ios', 'mac'] > > Why is mac in here? > > It is not necessary, but explicit is better than implicit.
PTAL
lgtm
sdefresne@chromium.org changed reviewers: + phajdan.jr@chromium.org
phajdan.jr: can you give OWNERS approval?
LGTM w/nits https://codereview.chromium.org/2028413004/diff/20001/fetch_configs/ios_inter... File fetch_configs/ios_internal.py (right): https://codereview.chromium.org/2028413004/diff/20001/fetch_configs/ios_inter... fetch_configs/ios_internal.py:1: # Copyright (c) 2013 The Chromium Authors. All rights reserved. nit: 2016, no "(c)" https://codereview.chromium.org/2028413004/diff/20001/fetch_configs/ios_inter... fetch_configs/ios_internal.py:12: class Chromium(config_util.Config): nit: Don't forget to change the class name. Given ios.py uses IOS here, I recommend IOSInternal for consistency. https://codereview.chromium.org/2028413004/diff/20001/fetch_configs/ios_inter... fetch_configs/ios_internal.py:45: return Chromium().handle_args(argv) nit: Also don't forget to update this one.
Thank you for the review. https://codereview.chromium.org/2028413004/diff/20001/fetch_configs/ios_inter... File fetch_configs/ios_internal.py (right): https://codereview.chromium.org/2028413004/diff/20001/fetch_configs/ios_inter... fetch_configs/ios_internal.py:1: # Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2016/06/02 15:24:53, Paweł Hajdan Jr. wrote: > nit: 2016, no "(c)" Done. https://codereview.chromium.org/2028413004/diff/20001/fetch_configs/ios_inter... fetch_configs/ios_internal.py:12: class Chromium(config_util.Config): On 2016/06/02 15:24:53, Paweł Hajdan Jr. wrote: > nit: Don't forget to change the class name. Given ios.py uses IOS here, I > recommend IOSInternal for consistency. Done. https://codereview.chromium.org/2028413004/diff/20001/fetch_configs/ios_inter... fetch_configs/ios_internal.py:45: return Chromium().handle_args(argv) On 2016/06/02 15:24:53, Paweł Hajdan Jr. wrote: > nit: Also don't forget to update this one. Done.
The CQ bit was checked by sdefresne@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from phajdan.jr@chromium.org, rohitrao@chromium.org Link to the patchset: https://codereview.chromium.org/2028413004/#ps40001 (title: "Address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2028413004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2028413004/40001
Message was sent while issue was closed.
Description was changed from ========== Add a configuration to fetch "ios_internal" project. This allow using "fetch ios_internal" to setup a repository for Chrome on iOS (this still requires being a Google employee as this fetch a private downstream repository). The repository URL was already public as it is in every public bug that is linked from a downstream CL, e.g. http://crbug.com/459705#c115. BUG=None ========== to ========== Add a configuration to fetch "ios_internal" project. This allow using "fetch ios_internal" to setup a repository for Chrome on iOS (this still requires being a Google employee as this fetch a private downstream repository). The repository URL was already public as it is in every public bug that is linked from a downstream CL, e.g. http://crbug.com/459705#c115. BUG=None Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=300689 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as http://src.chromium.org/viewvc/chrome?view=rev&revision=300689 |
