Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
191133005:
git-sync-deps tool (Closed)
|
Created:
6 years, 9 months ago by hal.canary Modified:
6 years, 8 months ago CC:
skia-review_googlegroups.com, mtklein Base URL:
https://skia.googlesource.com/skia.git@master Visibility:
Public. |
DescriptionThis is an short replacement for gclient.
Makes use of our current DEPS file to allow for a smooth transition.
BUG=skia:291
NOTRY=true
Committed: http://code.google.com/p/skia/source/detail?r=14227
Patch Set 1 #Patch Set 2 : AnotherPatchSet #Patch Set 3 : AnotherPatchSet #
Total comments: 13
Patch Set 4 : AnotherPatchSet #
Total comments: 4
Patch Set 5 : AnotherPatchSet #
Total comments: 13
Patch Set 6 : AnotherPatchSet #Patch Set 7 : AnotherPatchSet #
Total comments: 2
Patch Set 8 : AnotherPatchSet #Patch Set 9 : AnotherPatchSet #
Total comments: 16
Patch Set 10 : AnotherPatchSet #
Total comments: 4
Patch Set 11 : AnotherPatchSet #
Total comments: 1
Messages
Total messages: 39 (0 generated)
https://codereview.chromium.org/191133005/diff/40001/docs/quickstart.md File docs/quickstart.md (right): https://codereview.chromium.org/191133005/diff/40001/docs/quickstart.md#newcode4 docs/quickstart.md:4: This guide assumes you've got `git` and `python` on your path. When did this file get added? I think it's a really, really bad idea to have this file around when we have separately-managed documentation. https://codereview.chromium.org/191133005/diff/40001/tools/grab_deps.py File tools/grab_deps.py (right): https://codereview.chromium.org/191133005/diff/40001/tools/grab_deps.py#newco... tools/grab_deps.py:16: deps = { We should definitely be reading the DEPS file instead of duplicating the list here. https://codereview.chromium.org/191133005/diff/40001/tools/grab_deps.py#newco... tools/grab_deps.py:134: print I'd like to simplify this logic. I think we can make the assumption that nobody is going to modify the third party deps in place. try: subprocess.check_call([git, 'fetch']) subprocess.check_call([git, 'reset', '--hard', hash]) except: shutil.rmtree(directory) if not os.path.exists(directory): subprocess.check_call([git, 'clone', repo]) subprocess.check_call([git, 'reset', '--hard', hash])
https://codereview.chromium.org/191133005/diff/40001/docs/quickstart.md File docs/quickstart.md (right): https://codereview.chromium.org/191133005/diff/40001/docs/quickstart.md#newcode4 docs/quickstart.md:4: This guide assumes you've got `git` and `python` on your path. On 2014/03/10 18:36:12, borenet wrote: > When did this file get added? I think it's a really, really bad idea to have > this file around when we have separately-managed documentation. This file was added here -> https://codereview.chromium.org/146943002/
https://codereview.chromium.org/191133005/diff/40001/docs/quickstart.md File docs/quickstart.md (right): https://codereview.chromium.org/191133005/diff/40001/docs/quickstart.md#newco... docs/quickstart.md:13: * `export PATH="${PATH}:$(pwd)/third_party/externals/depot_tools"` I don't think we need to say that. At least I think people checkout depot_tools earlier and from https://chromium.googlesource.com/chromium/tools/depot_tools/ and put it wherever they want. http://dev.chromium.org/developers/how-tos/install-depot-tools
https://codereview.chromium.org/191133005/diff/40001/.gitignore File .gitignore (right): https://codereview.chromium.org/191133005/diff/40001/.gitignore#newcode9 .gitignore:9: platform_tools/chromeos/third_party/externals please, keep this list sorted.
On 2014/03/10 18:56:29, tfarina wrote: > I don't think we need to say that. At least I think people checkout depot_tools > earlier and from https://chromium.googlesource.com/chromium/tools/depot_tools/ > and put it wherever they want. But what if they aren't Chrome developers?
https://codereview.chromium.org/191133005/diff/40001/tools/grab_deps.py File tools/grab_deps.py (right): https://codereview.chromium.org/191133005/diff/40001/tools/grab_deps.py#newco... tools/grab_deps.py:18: "third_party/externals/depot_tools" : ( Then why we need an 'extra' copy of depot_tools. Was it solely for ninja? If it was, then why not let the dev install ninja standalone? You certainly don't need for that.
On 2014/03/10 19:00:20, tfarina wrote: > Then why we need an 'extra' copy of depot_tools. Was it solely for ninja? If it > was, then why not let the dev install ninja standalone? You certainly don't need > for that. For now, it's for ninja. depot_tools has a binary copy of ninja. I don't know if that's the best way to do that.
https://codereview.chromium.org/191133005/diff/40001/.gitignore File .gitignore (right): https://codereview.chromium.org/191133005/diff/40001/.gitignore#newcode9 .gitignore:9: platform_tools/chromeos/third_party/externals On 2014/03/10 18:57:33, tfarina wrote: > please, keep this list sorted. Done. https://codereview.chromium.org/191133005/diff/40001/tools/grab_deps.py File tools/grab_deps.py (right): https://codereview.chromium.org/191133005/diff/40001/tools/grab_deps.py#newco... tools/grab_deps.py:16: deps = { On 2014/03/10 18:36:12, borenet wrote: > We should definitely be reading the DEPS file instead of duplicating the list > here. Why write a parser? https://codereview.chromium.org/191133005/diff/40001/tools/grab_deps.py#newco... tools/grab_deps.py:134: print On 2014/03/10 18:36:12, borenet wrote: > I'd like to simplify this logic. I think we can make the assumption that nobody > is going to modify the third party deps in place. > > try: > subprocess.check_call([git, 'fetch']) > subprocess.check_call([git, 'reset', '--hard', hash]) > except: > shutil.rmtree(directory) > if not os.path.exists(directory): > subprocess.check_call([git, 'clone', repo]) > subprocess.check_call([git, 'reset', '--hard', hash]) Your logic is fine, but I prefer git-checkout to git-reset-hard.
https://codereview.chromium.org/191133005/diff/40001/tools/grab_deps.py File tools/grab_deps.py (right): https://codereview.chromium.org/191133005/diff/40001/tools/grab_deps.py#newco... tools/grab_deps.py:16: deps = { On 2014/03/10 20:03:08, Hal Canary wrote: > On 2014/03/10 18:36:12, borenet wrote: > > We should definitely be reading the DEPS file instead of duplicating the list > > here. > > Why write a parser? Because certain important users of Skia use DEPS files, and it may be useful for their DEPS files to reference ours.
https://codereview.chromium.org/191133005/diff/40001/tools/grab_deps.py File tools/grab_deps.py (right): https://codereview.chromium.org/191133005/diff/40001/tools/grab_deps.py#newco... tools/grab_deps.py:16: deps = { On 2014/03/10 20:03:08, Hal Canary wrote: > On 2014/03/10 18:36:12, borenet wrote: > > We should definitely be reading the DEPS file instead of duplicating the list > > here. > > Why write a parser? No parser is needed; we can do something like: environment = {} exectfile('DEPS', environment) deps = environment['solutions'] I think this is very worthwhile since it will keep the gclient workflow working, at least in the short term. That will make things much easier on the bots. If we do want to switch all at once, we should remove the DEPS file altogether, but again, I don't think that's the right thing to do right now. https://codereview.chromium.org/191133005/diff/40001/tools/grab_deps.py#newco... tools/grab_deps.py:134: print On 2014/03/10 20:03:08, Hal Canary wrote: > On 2014/03/10 18:36:12, borenet wrote: > > I'd like to simplify this logic. I think we can make the assumption that > nobody > > is going to modify the third party deps in place. > > > > try: > > subprocess.check_call([git, 'fetch']) > > subprocess.check_call([git, 'reset', '--hard', hash]) > > except: > > shutil.rmtree(directory) > > if not os.path.exists(directory): > > subprocess.check_call([git, 'clone', repo]) > > subprocess.check_call([git, 'reset', '--hard', hash]) > > Your logic is fine, but I prefer git-checkout to git-reset-hard. Why? Just curious. IMO reset is nice because it keeps us on the master branch and enforces that there are no local changes. https://codereview.chromium.org/191133005/diff/80001/.gitignore File .gitignore (right): https://codereview.chromium.org/191133005/diff/80001/.gitignore#newcode1 .gitignore:1: *.pyc These changes seem orthogonal to the rest of this CL. Can we do them in a different CL? https://codereview.chromium.org/191133005/diff/80001/tools/DEPS.py File tools/DEPS.py (right): https://codereview.chromium.org/191133005/diff/80001/tools/DEPS.py#newcode1 tools/DEPS.py:1: # Copyright 2014 Google Inc. Again, I think duplicating these is a bad idea.
I don't know how it is done exactly but you can look around and see how they do. Chromium downloads clang-format (and also GN) from google-storage: 'download_from_google_storage --no_resume --platform=win32 --no_auth --bucket chromium-clang-format -s src/third_party/clang_format/bin/win/clang-format.exe.sha1' So in theory you could do the same for ninja.
https://codereview.chromium.org/191133005/diff/80001/.gitignore File .gitignore (right): https://codereview.chromium.org/191133005/diff/80001/.gitignore#newcode1 .gitignore:1: *.pyc On 2014/03/10 20:13:30, borenet wrote: > These changes seem orthogonal to the rest of this CL. Can we do them in a > different CL? Done. https://codereview.chromium.org/191133005/diff/80001/tools/DEPS.py File tools/DEPS.py (right): https://codereview.chromium.org/191133005/diff/80001/tools/DEPS.py#newcode1 tools/DEPS.py:1: # Copyright 2014 Google Inc. On 2014/03/10 20:13:30, borenet wrote: > Again, I think duplicating these is a bad idea. Okay. We now parse DEPS as is.
lgtm https://codereview.chromium.org/191133005/diff/120001/tools/grab_deps.py File tools/grab_deps.py (right): https://codereview.chromium.org/191133005/diff/120001/tools/grab_deps.py#newc... tools/grab_deps.py:6: Add a brief usage? # Run to grab Skia deps, with optional platform support. # python tools/grab_deps.py [android] [chromeos] [barelinux]
Seems pretty good to me, modulo the comments below... https://codereview.chromium.org/191133005/diff/120001/tools/grab_deps.py File tools/grab_deps.py (right): https://codereview.chromium.org/191133005/diff/120001/tools/grab_deps.py#newc... tools/grab_deps.py:26: """TODO(halcanary): document this function. Please document it before committing, unless this is urgent. https://codereview.chromium.org/191133005/diff/120001/tools/grab_deps.py#newc... tools/grab_deps.py:50: for arg in argv: Please add comment like: # Add OS-specific dependencies https://codereview.chromium.org/191133005/diff/120001/tools/grab_deps.py#newc... tools/grab_deps.py:50: for arg in argv: Does it make sense for us to pick up multiple OSes from deps_os? Or should at most one OS flavor be specified? https://codereview.chromium.org/191133005/diff/120001/tools/grab_deps.py#newc... tools/grab_deps.py:52: print arg, '?' Maybe instead of print/exit, do this? raise Exception('arg %s not found within deps_os keys %s' % ( arg, DEPS['deps_os'])) https://codereview.chromium.org/191133005/diff/120001/tools/grab_deps.py#newc... tools/grab_deps.py:58: repo, checkout = dependencies[directory].split('@', 1) maybe "tag", "githash", "version", or "revision" would be more descriptive than "checkout"?
Consider moving this to depot_tools, to avoid duplicating effort?
On 2014/04/11 16:29:18, borenet wrote: > Consider moving this to depot_tools, to avoid duplicating effort? I think we should create the tool within our own repo first, get it working, and then see about merging it into depot_tools.
https://codereview.chromium.org/191133005/diff/120001/tools/grab_deps.py File tools/grab_deps.py (right): https://codereview.chromium.org/191133005/diff/120001/tools/grab_deps.py#newc... tools/grab_deps.py:6: On 2014/04/11 15:58:47, mtklein wrote: > Add a brief usage? > > # Run to grab Skia deps, with optional platform support. > # python tools/grab_deps.py [android] [chromeos] [barelinux] Done. https://codereview.chromium.org/191133005/diff/120001/tools/grab_deps.py#newc... tools/grab_deps.py:26: """TODO(halcanary): document this function. On 2014/04/11 16:16:11, epoger wrote: > Please document it before committing, unless this is urgent. Done. https://codereview.chromium.org/191133005/diff/120001/tools/grab_deps.py#newc... tools/grab_deps.py:50: for arg in argv: On 2014/04/11 16:16:11, epoger wrote: > Does it make sense for us to pick up multiple OSes from deps_os? Or should at > most one OS flavor be specified? Yes, since I might test my branch by cross-compiling to multiple systems. https://codereview.chromium.org/191133005/diff/120001/tools/grab_deps.py#newc... tools/grab_deps.py:50: for arg in argv: On 2014/04/11 16:16:11, epoger wrote: > Please add comment like: > > # Add OS-specific dependencies Done. https://codereview.chromium.org/191133005/diff/120001/tools/grab_deps.py#newc... tools/grab_deps.py:52: print arg, '?' On 2014/04/11 16:16:11, epoger wrote: > Maybe instead of print/exit, do this? > > raise Exception('arg %s not found within deps_os keys %s' % ( > arg, DEPS['deps_os'])) Done. https://codereview.chromium.org/191133005/diff/120001/tools/grab_deps.py#newc... tools/grab_deps.py:58: repo, checkout = dependencies[directory].split('@', 1) On 2014/04/11 16:16:11, epoger wrote: > maybe "tag", "githash", "version", or "revision" would be more descriptive than > "checkout"? I mean 'checkout' as anything that can be passed to `git checkout`. I don't know a better term.
LGTM with or without the following adjustments. Thanks! https://codereview.chromium.org/191133005/diff/120001/tools/grab_deps.py File tools/grab_deps.py (right): https://codereview.chromium.org/191133005/diff/120001/tools/grab_deps.py#newc... tools/grab_deps.py:58: repo, checkout = dependencies[directory].split('@', 1) On 2014/04/11 17:46:51, Hal Canary wrote: > On 2014/04/11 16:16:11, epoger wrote: > > maybe "tag", "githash", "version", or "revision" would be more descriptive > than > > "checkout"? > > I mean 'checkout' as anything that can be passed to `git checkout`. I don't > know a better term. In "git checkout --help", they use the term "tree-ish", which I think is terrible-ish: git checkout [-p|--patch] [<tree-ish>] [--] <pathspec>... When <paths> or --patch are given, git checkout does not switch branches. It updates the named paths in the working tree from the index file or from a named <tree-ish> (most often a commit). In this case, the -b and --track options are meaningless and giving either of them results in an error. The <tree-ish> argument can be used to specify a specific tree-ish (i.e. commit, tag or tree) to update the index for the given paths before updating the working tree. I think your "checkoutable" is better than that, at least! I suppose I would prefer "commit_or_tag" , but if you prefer "checkoutable" that's fine too. https://codereview.chromium.org/191133005/diff/220001/tools/grab_deps.py File tools/grab_deps.py (right): https://codereview.chromium.org/191133005/diff/220001/tools/grab_deps.py#newc... tools/grab_deps.py:19: def useage(): useage -> usage
https://codereview.chromium.org/191133005/diff/220001/tools/grab_deps.py File tools/grab_deps.py (right): https://codereview.chromium.org/191133005/diff/220001/tools/grab_deps.py#newc... tools/grab_deps.py:19: def useage(): On 2014/04/11 18:28:39, epoger wrote: > useage -> usage Done.
The CQ bit was checked by halcanary@google.com
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/halcanary@google.com/191133005/120002
Again, can we consider being good citizens and moving this to depot_tools? Since we've heard that this is something they want to do anyway, it makes sense to try to avoid duplicating their efforts.
On 2014/04/11 18:46:57, borenet wrote: > Again, can we consider being good citizens and moving this to depot_tools? > Since we've heard that this is something they want to do anyway, it makes sense > to try to avoid duplicating their efforts. Sounds great! How do we do that?
On 2014/04/11 18:50:44, Hal Canary wrote: > On 2014/04/11 18:46:57, borenet wrote: > > Again, can we consider being good citizens and moving this to depot_tools? > > Since we've heard that this is something they want to do anyway, it makes > sense > > to try to avoid duplicating their efforts. > > Sounds great! How do we do that? I'd start with checking out https://chromium.googlesource.com/chromium/tools/depot_tools, adding the script and uploading for review. Someone probably has a process in mind which would transform this into "git sync-deps".
On 2014/04/11 18:53:31, borenet wrote: > On 2014/04/11 18:50:44, Hal Canary wrote: > > On 2014/04/11 18:46:57, borenet wrote: > > > Again, can we consider being good citizens and moving this to depot_tools? > > > Since we've heard that this is something they want to do anyway, it makes > > sense > > > to try to avoid duplicating their efforts. > > > > Sounds great! How do we do that? > > I'd start with checking out > https://chromium.googlesource.com/chromium/tools/depot_tools, adding the script > and uploading for review. Someone probably has a process in mind which would > transform this into "git sync-deps". I approve this effort! However this may not be in a good-enough state to land in depot tools. Let's start a (short) design doc on what we'd want/need from a sync-deps tool. In particular I would like to avoid growing a lot of special-case logic (like gclient) and set the expectations of what the tool does (i.e. what is defined behavior and what is undefined behavior) from the very beginning. Initial doc here, feel free to edit: https://docs.google.com/a/chromium.org/document/d/1GsI7NLxfkJeh5Gnbqm3P1-kd0C... (lmk if you need me to add you for permissions)
The CQ bit was unchecked by halcanary@google.com
Since the tree was yellow all weekend I had time to refactor this
program to address some of the points in Robbie's design doc.
* Rename grab_deps to better-sounding git-sync-deps.
* Multithreading.
* Add per-repository sync-deps.disable git variable.
* Add GIT_SYNC_DEPS_QUIET environment variable.
* Document GIT_EXECUTABLE environment variable.
* Add GIT_SYNC_DEPS_PATH environment variable
(with correct default behviour).
* Better usage message.
* Skip `git fetch` if we are already at correct revision.
* Raise error if repository not clean.
Please take a look again!
This is getting closer to what the depot_tools team wants in terms of defined
behavior. For instance, if the tool can not put a repository into a clean state
with the appropriate revision checked out, it will raise an exception.
https://codereview.chromium.org/191133005/diff/250001/tools/git-sync-deps File tools/git-sync-deps (right): https://codereview.chromium.org/191133005/diff/250001/tools/git-sync-deps#new... tools/git-sync-deps:13: Environment Variables: Maybe it would be more straightforward to use argparse/optparse instead of environment variables? (Then the script would automatically provide consistently formatted --help output, with defaults that will stay in sync with the code rather than be maintained separately, etc.) I'm OK either way. https://codereview.chromium.org/191133005/diff/250001/tools/git-sync-deps#new... tools/git-sync-deps:14: GIT_EXECUTABLE if git, git.exe, or git.bat isn't in your path, use this I think this would be more accurate: GIT_EXECUTABLE: path to "git" binary; if unset, will look for one of ['git', 'git.exe', 'git.bat'] in your default path. https://codereview.chromium.org/191133005/diff/250001/tools/git-sync-deps#new... tools/git-sync-deps:17: GIT_SYNC_DEPS_PATH if not set, the DEPS file in this repository This one doesn't quite tell the whole story. Maybe: GIT_SYNC_DEPS_PATH: DEPS file to get the dependency list from; if unset, assumes ../DEPS https://codereview.chromium.org/191133005/diff/250001/tools/git-sync-deps#new... tools/git-sync-deps:78: if os.path.isdir(directory): Could you rearrange it like so, so that the os.path.isdir() branch code is shorter? I think that would be easier to follow... if not os.path.isdir(directory): subprocess.check_call([git, 'clone', '--quiet', repo, directory]) # will fail if repo is bad or if directory is a file # Check to see if this repo is disabled. Quick return. try: disable = subprocess.check_output( ... https://codereview.chromium.org/191133005/diff/250001/tools/git-sync-deps#new... tools/git-sync-deps:89: # Only do a slow git fetch if we are not already on correct revision Sorta-double-negative ("only ... not") and possibly-parenthetical-possibly-restrictive adjective ("slow") makes this ambiguous. Maybe, instead... # Do a git fetch, unless we are already on the correct revision. # (git fetch is slow, so it's good to skip it if we can) https://codereview.chromium.org/191133005/diff/250001/tools/git-sync-deps#new... tools/git-sync-deps:90: def revision(rev): Maybe make this a first-class function? I think that's easier to follow than declaring it inline... def get_commithash(directory, checkoutable) """Returns the commithash corresponding to this tag/branch/commit. Args: directory: ... """ https://codereview.chromium.org/191133005/diff/250001/tools/git-sync-deps#new... tools/git-sync-deps:92: [git, 'rev-list', '-1', rev], cwd=directory) Using the more explicit command-line flag will make it easier to see what's going on here: [git, 'rev-list', '--max-count', '1', rev] https://codereview.chromium.org/191133005/diff/250001/tools/git-sync-deps#new... tools/git-sync-deps:94: at_revision = (revision('HEAD') == revision(checkoutable)) I think the logic would be simpler if we resolved checkoutable to a commithash as close to the top of the function as possible: desired_commithash = get_commithash(directory, checkoutable) Then, the rest of this function could just focus on getting get_commithash(directory, 'HEAD') to equal desired_commithash. https://codereview.chromium.org/191133005/diff/250001/tools/git-sync-deps#new... tools/git-sync-deps:113: if not at_revision: Why don't we just do the "git checkout" right after we do the "git fetch", in the same code branch?
I simplified the logic and gave up on some of the fancy logic. https://codereview.chromium.org/191133005/diff/250001/tools/git-sync-deps File tools/git-sync-deps (right): https://codereview.chromium.org/191133005/diff/250001/tools/git-sync-deps#new... tools/git-sync-deps:14: GIT_EXECUTABLE if git, git.exe, or git.bat isn't in your path, use this On 2014/04/14 19:32:14, epoger wrote: > I think this would be more accurate: > > GIT_EXECUTABLE: path to "git" binary; if unset, will look for one of ['git', > 'git.exe', 'git.bat'] in your default path. Done. https://codereview.chromium.org/191133005/diff/250001/tools/git-sync-deps#new... tools/git-sync-deps:17: GIT_SYNC_DEPS_PATH if not set, the DEPS file in this repository On 2014/04/14 19:32:14, epoger wrote: > This one doesn't quite tell the whole story. Maybe: > > GIT_SYNC_DEPS_PATH: DEPS file to get the dependency list from; if unset, assumes > ../DEPS Done. https://codereview.chromium.org/191133005/diff/250001/tools/git-sync-deps#new... tools/git-sync-deps:78: if os.path.isdir(directory): On 2014/04/14 19:32:14, epoger wrote: > Could you rearrange it like so, so that the os.path.isdir() branch code is > shorter? I think that would be easier to follow... > > if not os.path.isdir(directory): > subprocess.check_call([git, 'clone', '--quiet', repo, directory]) > # will fail if repo is bad or if directory is a file > > # Check to see if this repo is disabled. Quick return. > try: > disable = subprocess.check_output( ... Done. https://codereview.chromium.org/191133005/diff/250001/tools/git-sync-deps#new... tools/git-sync-deps:89: # Only do a slow git fetch if we are not already on correct revision I gave up on saving half a second by checking for correct revision. https://codereview.chromium.org/191133005/diff/250001/tools/git-sync-deps#new... tools/git-sync-deps:90: def revision(rev): On 2014/04/14 19:32:14, epoger wrote: > Maybe make this a first-class function? I think that's easier to follow than > declaring it inline... Simplified out of existence. https://codereview.chromium.org/191133005/diff/250001/tools/git-sync-deps#new... tools/git-sync-deps:92: [git, 'rev-list', '-1', rev], cwd=directory) On 2014/04/14 19:32:14, epoger wrote: > Using the more explicit command-line flag will make it easier to see what's > going on here: > > [git, 'rev-list', '--max-count', '1', rev] Simplified out of existence. https://codereview.chromium.org/191133005/diff/250001/tools/git-sync-deps#new... tools/git-sync-deps:113: if not at_revision: On 2014/04/14 19:32:14, epoger wrote: > Why don't we just do the "git checkout" right after we do the "git fetch", in > the same code branch? Done.
LGTM if you make one typo fix Thanks for writing it, and thanks for simplifying it! https://codereview.chromium.org/191133005/diff/270001/tools/git-sync-deps File tools/git-sync-deps (right): https://codereview.chromium.org/191133005/diff/270001/tools/git-sync-deps#new... tools/git-sync-deps:58: def git_reposiory_sync_is_disabled(git, directory): reposiory -> repository (or just repo) https://codereview.chromium.org/191133005/diff/270001/tools/git-sync-deps#new... tools/git-sync-deps:91: if git_reposiory_sync_is_disabled(git, directory): match above fix to git_reposiory_sync_is_disabled() name
https://codereview.chromium.org/191133005/diff/270001/tools/git-sync-deps File tools/git-sync-deps (right): https://codereview.chromium.org/191133005/diff/270001/tools/git-sync-deps#new... tools/git-sync-deps:58: def git_reposiory_sync_is_disabled(git, directory): On 2014/04/16 19:13:00, epoger wrote: > reposiory -> repository (or just repo) Done. https://codereview.chromium.org/191133005/diff/270001/tools/git-sync-deps#new... tools/git-sync-deps:91: if git_reposiory_sync_is_disabled(git, directory): On 2014/04/16 19:13:00, epoger wrote: > match above fix to git_reposiory_sync_is_disabled() name Done.
The CQ bit was checked by halcanary@google.com
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/halcanary@google.com/191133005/290001
Message was sent while issue was closed.
Change committed as 14227
Message was sent while issue was closed.
lgtm
Message was sent while issue was closed.
If we are phasing out gclient for checking out skia and its third_party dependencies, then please, can we get https://sites.google.com/site/skiadocs/developer-documentation/contributing-c... updated with the new instructions. My understanding is that instead of using gclient config/sync we will now use git-sync-deps.
Message was sent while issue was closed.
https://codereview.chromium.org/191133005/diff/290001/docs/quickstart.md File docs/quickstart.md (right): https://codereview.chromium.org/191133005/diff/290001/docs/quickstart.md#newc... docs/quickstart.md:11: * `python tools/git-sync-deps` Yeah, this isn't right. "gclient sync" is still the preferred workflow, and that needs to be reflected on ALL of our documentation. This is the reason I never liked the addition of this file as long as we still have docs on the web.
Message was sent while issue was closed.
On 2014/04/20 16:53:45, borenet wrote: > https://codereview.chromium.org/191133005/diff/290001/docs/quickstart.md > File docs/quickstart.md (right): > > https://codereview.chromium.org/191133005/diff/290001/docs/quickstart.md#newc... > docs/quickstart.md:11: * `python tools/git-sync-deps` > Yeah, this isn't right. "gclient sync" is still the preferred workflow, and > that needs to be reflected on ALL of our documentation. This is the reason I > never liked the addition of this file as long as we still have docs on the web. Uploaded https://codereview.chromium.org/245283002/ ('warn that docs/quickstart.md is out of sync with official docs') |
