From 209a681f9cb0e955d5659216f262ef5b00861e8a Mon Sep 17 00:00:00 2001 From: Dan Jacques Date: Wed, 12 Jul 2017 11:40:20 -0700 Subject: [PATCH] [git] Remove hard-coded "depot_tools" Git. Several tools, including the "git" recipe module, hard-code a checkout-relative "git.bat" path. Git is a feature that is provided by the system, both to tooling and recipes: 1) For users, "depot_tools" must be on PATH, and during setup it will have installed "git.bat", ensuring that Git tooling is available in PATH. 2) For bots, the system is responsible for providing "git.bat" on PATH. This is typically done at "/b/depot_tools/git.bat", which is sync'd through the "update_scripts" step. By formally treating Git as a system resource, we absolve Windows bots and users from manually installing a depot_tools-local Git, bringing them in line with other platforms. BUG=chromium:590806 TEST=local Change-Id: I93e89855cdd330a2ba7a8cfb8117a1789d1ab54e Reviewed-on: https://chromium-review.googlesource.com/568694 Commit-Queue: Daniel Jacques Reviewed-by: Robbie Iannucci --- fetch.py | 9 ++-- git_common.py | 14 +++++- recipes/README.recipes.md | 28 ++++------- recipes/recipe_modules/git/api.py | 25 ---------- .../examples/full.expected/platform_win.json | 50 +++++++------------ .../recipe_modules/git/resources/git_setup.py | 29 +++++------ .../full.expected/with_wrong_patch.json | 14 +----- .../full.expected/with_wrong_patch_new.json | 14 +----- 8 files changed, 60 insertions(+), 123 deletions(-) diff --git a/fetch.py b/fetch.py index 7483f48a8..60b1c1c6e 100755 --- a/fetch.py +++ b/fetch.py @@ -26,6 +26,8 @@ import subprocess import sys import textwrap +import git_common + from distutils import spawn @@ -88,11 +90,8 @@ class GclientCheckout(Checkout): class GitCheckout(Checkout): def run_git(self, *cmd, **kwargs): - if sys.platform == 'win32' and not spawn.find_executable('git'): - git_path = os.path.join(SCRIPT_PATH, 'git.bat') - else: - git_path = 'git' - return self.run((git_path,) + cmd, **kwargs) + print 'Running: git %s' % (' '.join(pipes.quote(x) for x in cmd)) + return git_common.run(*cmd, **kwargs) class GclientGitCheckout(GclientCheckout, GitCheckout): diff --git a/git_common.py b/git_common.py index 232be9a78..1bb59375f 100644 --- a/git_common.py +++ b/git_common.py @@ -37,9 +37,21 @@ from StringIO import StringIO ROOT = os.path.abspath(os.path.dirname(__file__)) IS_WIN = sys.platform == 'win32' -GIT_EXE = ROOT+'\\git.bat' if IS_WIN else 'git' TEST_MODE = False + +def win_find_git(): + for elem in os.environ.get('PATH', '').split(os.pathsep): + for candidate in ('git.exe', 'git.bat'): + path = os.path.join(elem, candidate) + if os.path.isfile(path): + return path + raise ValueError('Could not find Git on PATH.') + + +GIT_EXE = 'git' if not IS_WIN else win_find_git() + + FREEZE = 'FREEZE' FREEZE_SECTIONS = { 'indexed': 'soft', diff --git a/recipes/README.recipes.md b/recipes/README.recipes.md index efe796f61..7879c4b60 100644 --- a/recipes/README.recipes.md +++ b/recipes/README.recipes.md @@ -366,13 +366,11 @@ Returns: #### **class [GitApi](/recipes/recipe_modules/git/api.py#10)([RecipeApi][recipe_engine/wkt/RecipeApi]):** -— **def [\_\_call\_\_](/recipes/recipe_modules/git/api.py#17)(self, \*args, \*\*kwargs):** +— **def [\_\_call\_\_](/recipes/recipe_modules/git/api.py#13)(self, \*args, \*\*kwargs):** Return a git command step. -— **def [\_\_init\_\_](/recipes/recipe_modules/git/api.py#13)(self, \*args, \*\*kwargs):** - -— **def [bundle\_create](/recipes/recipe_modules/git/api.py#391)(self, bundle_path, rev_list_args=None, \*\*kwargs):** +— **def [bundle\_create](/recipes/recipe_modules/git/api.py#366)(self, bundle_path, rev_list_args=None, \*\*kwargs):** Run 'git bundle create' on a Git repository. @@ -382,11 +380,11 @@ Args: refs in the Git checkout will be bundled. kwargs: Forwarded to '__call__'. -— **def [cat\_file\_at\_commit](/recipes/recipe_modules/git/api.py#58)(self, file_path, commit_hash, remote_name=None, \*\*kwargs):** +— **def [cat\_file\_at\_commit](/recipes/recipe_modules/git/api.py#38)(self, file_path, commit_hash, remote_name=None, \*\*kwargs):** Outputs the contents of a file at a given revision. -— **def [checkout](/recipes/recipe_modules/git/api.py#130)(self, url, ref=None, dir_path=None, recursive=False, submodules=True, submodule_update_force=False, keep_paths=None, step_suffix=None, curl_trace_file=None, can_fail_build=True, set_got_revision=False, remote_name=None, display_fetch_size=None, file_name=None, submodule_update_recursive=True, use_git_cache=False):** +— **def [checkout](/recipes/recipe_modules/git/api.py#110)(self, url, ref=None, dir_path=None, recursive=False, submodules=True, submodule_update_force=False, keep_paths=None, step_suffix=None, curl_trace_file=None, can_fail_build=True, set_got_revision=False, remote_name=None, display_fetch_size=None, file_name=None, submodule_update_recursive=True, use_git_cache=False):** Performs a full git checkout and returns sha1 of checked out revision. @@ -423,7 +421,7 @@ Args: Returns: If the checkout was successful, this returns the commit hash of the checked-out-repo. Otherwise this returns None. -— **def [config\_get](/recipes/recipe_modules/git/api.py#364)(self, prop_name, \*\*kwargs):** +— **def [config\_get](/recipes/recipe_modules/git/api.py#339)(self, prop_name, \*\*kwargs):** Returns: (str) The Git config output, or None if no output was generated. @@ -431,7 +429,7 @@ Args: prop_name: (str) The name of the config property to query. kwargs: Forwarded to '__call__'. -— **def [count\_objects](/recipes/recipe_modules/git/api.py#66)(self, previous_result=None, can_fail_build=False, \*\*kwargs):** +— **def [count\_objects](/recipes/recipe_modules/git/api.py#46)(self, previous_result=None, can_fail_build=False, \*\*kwargs):** Returns `git count-objects` result as a dict. @@ -444,15 +442,11 @@ Args: Returns: A dict of count-object values, or None if count-object run failed. -— **def [ensure\_win\_git\_tooling](/recipes/recipe_modules/git/api.py#39)(self):** - -Ensures that depot_tools/git.bat actually exists. - -— **def [fetch\_tags](/recipes/recipe_modules/git/api.py#52)(self, remote_name=None, \*\*kwargs):** +— **def [fetch\_tags](/recipes/recipe_modules/git/api.py#32)(self, remote_name=None, \*\*kwargs):** Fetches all tags from the remote. -— **def [get\_remote\_url](/recipes/recipe_modules/git/api.py#381)(self, remote_name=None, \*\*kwargs):** +— **def [get\_remote\_url](/recipes/recipe_modules/git/api.py#356)(self, remote_name=None, \*\*kwargs):** Returns: (str) The URL of the remote Git repository, or None. @@ -460,11 +454,11 @@ Args: remote_name: (str) The name of the remote to query, defaults to 'origin'. kwargs: Forwarded to '__call__'. -— **def [get\_timestamp](/recipes/recipe_modules/git/api.py#336)(self, commit='HEAD', test_data=None, \*\*kwargs):** +— **def [get\_timestamp](/recipes/recipe_modules/git/api.py#311)(self, commit='HEAD', test_data=None, \*\*kwargs):** Find and return the timestamp of the given commit. -— **def [new\_branch](/recipes/recipe_modules/git/api.py#404)(self, branch, name=None, upstream=None, \*\*kwargs):** +— **def [new\_branch](/recipes/recipe_modules/git/api.py#379)(self, branch, name=None, upstream=None, \*\*kwargs):** Runs git new-branch on a Git repository, to be used before git cl upload. @@ -474,7 +468,7 @@ Args: upstream (str): to origin/master. kwargs: Forwarded to '__call__'. -— **def [rebase](/recipes/recipe_modules/git/api.py#345)(self, name_prefix, branch, dir_path, remote_name=None, \*\*kwargs):** +— **def [rebase](/recipes/recipe_modules/git/api.py#320)(self, name_prefix, branch, dir_path, remote_name=None, \*\*kwargs):** Run rebase HEAD onto branch Args: diff --git a/recipes/recipe_modules/git/api.py b/recipes/recipe_modules/git/api.py index d6387d53c..5f7b70735 100644 --- a/recipes/recipe_modules/git/api.py +++ b/recipes/recipe_modules/git/api.py @@ -10,18 +10,11 @@ from recipe_engine import recipe_api class GitApi(recipe_api.RecipeApi): _GIT_HASH_RE = re.compile('[0-9a-f]{40}', re.IGNORECASE) - def __init__(self, *args, **kwargs): - super(GitApi, self).__init__(*args, **kwargs) - self.initialized_win_git = False - def __call__(self, *args, **kwargs): """Return a git command step.""" name = kwargs.pop('name', 'git ' + args[0]) infra_step = kwargs.pop('infra_step', True) git_cmd = ['git'] - if self.m.platform.is_win: - self.ensure_win_git_tooling() - git_cmd = [self.package_repo_resource('git.bat')] options = kwargs.pop('git_config_options', {}) for k, v in sorted(options.iteritems()): git_cmd.extend(['-c', '%s=%s' % (k, v)]) @@ -36,19 +29,6 @@ class GitApi(recipe_api.RecipeApi): else: return f.result - def ensure_win_git_tooling(self): - """Ensures that depot_tools/git.bat actually exists.""" - if not self.m.platform.is_win or self.initialized_win_git: - return - with self.m.context(cwd=self.package_repo_resource()): - self.m.python( - 'ensure git tooling on windows', - self.package_repo_resource('bootstrap', 'win', 'git_bootstrap.py'), - ['--verbose'], - infra_step=True, - timeout=300) - self.initialized_win_git = True - def fetch_tags(self, remote_name=None, **kwargs): """Fetches all tags from the remote.""" kwargs.setdefault('name', 'git fetch tags') @@ -197,11 +177,6 @@ class GitApi(recipe_api.RecipeApi): else: remote_name = 'origin' - if self.m.platform.is_win: - self.ensure_win_git_tooling() - git_setup_args += [ - '--git_cmd_path', self.package_repo_resource('git.bat')] - step_suffix = '' if step_suffix is None else ' (%s)' % step_suffix self.m.python( 'git setup%s' % step_suffix, diff --git a/recipes/recipe_modules/git/examples/full.expected/platform_win.json b/recipes/recipe_modules/git/examples/full.expected/platform_win.json index e3f1e0648..167757047 100644 --- a/recipes/recipe_modules/git/examples/full.expected/platform_win.json +++ b/recipes/recipe_modules/git/examples/full.expected/platform_win.json @@ -1,16 +1,4 @@ [ - { - "cmd": [ - "python", - "-u", - "RECIPE_PACKAGE_REPO[depot_tools]\\bootstrap\\win\\git_bootstrap.py", - "--verbose" - ], - "cwd": "RECIPE_PACKAGE_REPO[depot_tools]", - "infra_step": true, - "name": "ensure git tooling on windows", - "timeout": 300 - }, { "cmd": [ "python", @@ -19,15 +7,13 @@ "--path", "[START_DIR]\\src", "--url", - "https://chromium.googlesource.com/chromium/src.git", - "--git_cmd_path", - "RECIPE_PACKAGE_REPO[depot_tools]\\git.bat" + "https://chromium.googlesource.com/chromium/src.git" ], "name": "git setup" }, { "cmd": [ - "RECIPE_PACKAGE_REPO[depot_tools]\\git.bat", + "git", "retry", "fetch", "origin", @@ -43,7 +29,7 @@ }, { "cmd": [ - "RECIPE_PACKAGE_REPO[depot_tools]\\git.bat", + "git", "checkout", "-f", "FETCH_HEAD" @@ -54,7 +40,7 @@ }, { "cmd": [ - "RECIPE_PACKAGE_REPO[depot_tools]\\git.bat", + "git", "rev-parse", "HEAD" ], @@ -68,7 +54,7 @@ }, { "cmd": [ - "RECIPE_PACKAGE_REPO[depot_tools]\\git.bat", + "git", "clean", "-f", "-d", @@ -80,7 +66,7 @@ }, { "cmd": [ - "RECIPE_PACKAGE_REPO[depot_tools]\\git.bat", + "git", "submodule", "sync" ], @@ -90,7 +76,7 @@ }, { "cmd": [ - "RECIPE_PACKAGE_REPO[depot_tools]\\git.bat", + "git", "submodule", "update", "--init", @@ -102,7 +88,7 @@ }, { "cmd": [ - "RECIPE_PACKAGE_REPO[depot_tools]\\git.bat", + "git", "-c", "foo=bar", "count-objects", @@ -115,7 +101,7 @@ }, { "cmd": [ - "RECIPE_PACKAGE_REPO[depot_tools]\\git.bat", + "git", "config", "--get", "remote.origin.url" @@ -130,7 +116,7 @@ }, { "cmd": [ - "RECIPE_PACKAGE_REPO[depot_tools]\\git.bat", + "git", "show", "HEAD", "--format=%at", @@ -143,7 +129,7 @@ }, { "cmd": [ - "RECIPE_PACKAGE_REPO[depot_tools]\\git.bat", + "git", "fetch", "origin", "--tags" @@ -154,7 +140,7 @@ }, { "cmd": [ - "RECIPE_PACKAGE_REPO[depot_tools]\\git.bat", + "git", "status" ], "cwd": "[START_DIR]\\src", @@ -163,7 +149,7 @@ }, { "cmd": [ - "RECIPE_PACKAGE_REPO[depot_tools]\\git.bat", + "git", "status" ], "cwd": "[START_DIR]\\src", @@ -172,7 +158,7 @@ }, { "cmd": [ - "RECIPE_PACKAGE_REPO[depot_tools]\\git.bat", + "git", "status" ], "cwd": "[START_DIR]\\src", @@ -181,7 +167,7 @@ }, { "cmd": [ - "RECIPE_PACKAGE_REPO[depot_tools]\\git.bat", + "git", "new-branch", "refactor" ], @@ -194,7 +180,7 @@ }, { "cmd": [ - "RECIPE_PACKAGE_REPO[depot_tools]\\git.bat", + "git", "new-branch", "feature", "--upstream", @@ -209,7 +195,7 @@ }, { "cmd": [ - "RECIPE_PACKAGE_REPO[depot_tools]\\git.bat", + "git", "rebase", "origin/master" ], @@ -219,7 +205,7 @@ }, { "cmd": [ - "RECIPE_PACKAGE_REPO[depot_tools]\\git.bat", + "git", "bundle", "create", "[START_DIR]\\all.bundle", diff --git a/recipes/recipe_modules/git/resources/git_setup.py b/recipes/recipe_modules/git/resources/git_setup.py index 900352243..172ef6614 100755 --- a/recipes/recipe_modules/git/resources/git_setup.py +++ b/recipes/recipe_modules/git/resources/git_setup.py @@ -11,18 +11,17 @@ import os import subprocess import sys +# Import "git_common" from "depot_tools" root. +DEPOT_TOOLS_ROOT = os.path.abspath(os.path.join( + os.path.dirname(__file__), os.pardir, os.pardir, os.pardir, os.pardir)) +sys.path.insert(0, DEPOT_TOOLS_ROOT) +import git_common -def run_git(git_cmd, *args, **kwargs): - """Runs git with given arguments. - kwargs are passed through to subprocess. - - If the kwarg 'throw' is provided, this behaves as check_call, otherwise will - return git's return value. - """ - logging.info('Running: %s %s %s', git_cmd, args, kwargs) - func = subprocess.check_call if kwargs.pop('throw', True) else subprocess.call - return func((git_cmd,)+args, **kwargs) +def run_git(*cmd, **kwargs): + kwargs['stdout'] = sys.stdout + kwargs['stderr'] = sys.stderr + git_common.run(*cmd, **kwargs) def main(): @@ -31,9 +30,6 @@ def main(): required=True) parser.add_argument('--url', help='URL of remote to make origin.', required=True) - parser.add_argument('--git_cmd_path', - help='Path to the git command to run.', - default='git') parser.add_argument('--remote', help='Name of the git remote.', default='origin') parser.add_argument('-v', '--verbose', action='store_true') @@ -49,11 +45,10 @@ def main(): os.makedirs(path) if os.path.exists(os.path.join(path, '.git')): - run_git(opts.git_cmd_path, 'config', '--remove-section', - 'remote.%s' % remote, cwd=path) + run_git('config', '--remove-section', 'remote.%s' % remote, cwd=path) else: - run_git(opts.git_cmd_path, 'init', cwd=path) - run_git(opts.git_cmd_path, 'remote', 'add', remote, url, cwd=path) + run_git('init', cwd=path) + run_git('remote', 'add', remote, url, cwd=path) return 0 diff --git a/recipes/recipe_modules/tryserver/examples/full.expected/with_wrong_patch.json b/recipes/recipe_modules/tryserver/examples/full.expected/with_wrong_patch.json index a45f2ec8a..d88dc137f 100644 --- a/recipes/recipe_modules/tryserver/examples/full.expected/with_wrong_patch.json +++ b/recipes/recipe_modules/tryserver/examples/full.expected/with_wrong_patch.json @@ -1,19 +1,7 @@ [ { "cmd": [ - "python", - "-u", - "RECIPE_PACKAGE_REPO[depot_tools]\\bootstrap\\win\\git_bootstrap.py", - "--verbose" - ], - "cwd": "RECIPE_PACKAGE_REPO[depot_tools]", - "infra_step": true, - "name": "ensure git tooling on windows", - "timeout": 300 - }, - { - "cmd": [ - "RECIPE_PACKAGE_REPO[depot_tools]\\git.bat", + "git", "diff", "--cached", "--name-only" diff --git a/recipes/recipe_modules/tryserver/examples/full.expected/with_wrong_patch_new.json b/recipes/recipe_modules/tryserver/examples/full.expected/with_wrong_patch_new.json index aaf3156b6..b2f53685d 100644 --- a/recipes/recipe_modules/tryserver/examples/full.expected/with_wrong_patch_new.json +++ b/recipes/recipe_modules/tryserver/examples/full.expected/with_wrong_patch_new.json @@ -1,19 +1,7 @@ [ { "cmd": [ - "python", - "-u", - "RECIPE_PACKAGE_REPO[depot_tools]\\bootstrap\\win\\git_bootstrap.py", - "--verbose" - ], - "cwd": "RECIPE_PACKAGE_REPO[depot_tools]", - "infra_step": true, - "name": "ensure git tooling on windows", - "timeout": 300 - }, - { - "cmd": [ - "RECIPE_PACKAGE_REPO[depot_tools]\\git.bat", + "git", "diff", "--cached", "--name-only"