From fc3e14b3e0c56986227e16d096a4d5477094f8b5 Mon Sep 17 00:00:00 2001 From: Max Desiatov Date: Wed, 9 Nov 2022 15:56:07 +0000 Subject: [PATCH 1/5] update_checkout: re-read config for cross-repo testing When updating versions of some dependencies, it's important to be able to re-read the checkout config to get the new versions to actually check them out. --- .../update_checkout/update_checkout.py | 90 ++++++++++++++----- 1 file changed, 69 insertions(+), 21 deletions(-) diff --git a/utils/update_checkout/update_checkout/update_checkout.py b/utils/update_checkout/update_checkout/update_checkout.py index ea324e6886701..5d042fbbdcd77 100755 --- a/utils/update_checkout/update_checkout/update_checkout.py +++ b/utils/update_checkout/update_checkout/update_checkout.py @@ -97,6 +97,21 @@ def find_rev_by_timestamp(timestamp, repo_name, refspec): def get_branch_for_repo(config, repo_name, scheme_name, scheme_map, cross_repos_pr): + """Infer, fetch, and return a branch corresponding to a given PR, otherwise + return a branch found in the config for this repository name. + + Args: + config (Dict[str, Any]): deserialized `update-checkout-config.json` + repo_name (str): name of the repository for checking out the branch + scheme_name (str): name of the scheme to look up in the config + scheme_map (Dict[str, str]): map of repo names to branches to check out + cross_repos_pr (Dict[str, str]): map of repo ids to PRs to check out + + Returns: + Tuple[str, bool]: a pair of a checked out branch and a boolean + indicating whether this repo matched any `cross_repos_pr`. + """ + cross_repo = False repo_branch = scheme_name if scheme_map: @@ -249,8 +264,17 @@ def get_timestamp_to_match(args): echo=False).strip() -def update_all_repositories(args, config, scheme_name, cross_repos_pr): - scheme_map = None +def get_scheme_map(config, scheme_name): + """Find a mapping from repository IDs to branches in the config. + + Args: + config (Dict[str, Any]): deserialized `update-checkout-config.json` + scheme_name (str): name of the scheme to look up in `config` + + Returns: + Dict[str, str]: a mapping from repos to branches for the given scheme. + """ + if scheme_name: # This loop is only correct, since we know that each alias set has # unique contents. This is checked by validate_config. Thus the first @@ -258,8 +282,12 @@ def update_all_repositories(args, config, scheme_name, cross_repos_pr): # the only possible correct answer. for v in config['branch-schemes'].values(): if scheme_name in v['aliases']: - scheme_map = v['repos'] - break + return v['repos'] + + return None + + +def update_all_repositories(args, config, scheme_name, scheme_map, cross_repos_pr): pool_args = [] timestamp = get_timestamp_to_match(args) for repo_name in config['repos'].keys(): @@ -598,7 +626,7 @@ def main(): clone_with_ssh = args.clone_with_ssh skip_history = args.skip_history skip_tags = args.skip_tags - scheme = args.scheme + scheme_name = args.scheme github_comment = args.github_comment all_repos = args.all_repositories @@ -606,14 +634,6 @@ def main(): config = json.load(f) validate_config(config) - if args.dump_hashes: - dump_repo_hashes(args, config) - return (None, None) - - if args.dump_hashes_config: - dump_repo_hashes(args, config, args.dump_hashes_config) - return (None, None) - cross_repos_pr = {} if github_comment: regex_pr = r'(apple/[-a-zA-Z0-9_]+/pull/\d+|apple/[-a-zA-Z0-9_]+#\d+)' @@ -622,18 +642,46 @@ def main(): repos_with_pr = [pr.replace('/pull/', '#') for pr in repos_with_pr] cross_repos_pr = dict(pr.split('#') for pr in repos_with_pr) + # If branch is None, default to using the default branch alias + # specified by our configuration file. + if scheme_name is None: + scheme_name = config['default-branch-scheme'] + + scheme_map = get_scheme_map(config, scheme_name) + + swift_repo_path = os.path.join(args.source_root, 'swift') + if os.path.exists(swift_repo_path): + with shell.pushd(swift_repo_path, dry_run=False, echo=True): + # Check if `swift` repo itself needs to switch to a cross-repo branch. + branch_name, cross_repo = get_branch_for_repo(config, 'swift', + scheme_name, + scheme_map, + cross_repos_pr) + + if cross_repo: + shell.run(['git', 'checkout', branch_name], echo=True, + prefix="[swift] ") + + # Re-read the config after checkout. + with open(args.config) as f: + config = json.load(f) + validate_config(config) + + if args.dump_hashes: + dump_repo_hashes(args, config) + return (None, None) + + if args.dump_hashes_config: + dump_repo_hashes(args, config, args.dump_hashes_config) + return (None, None) + clone_results = None if clone or clone_with_ssh: - # If branch is None, default to using the default branch alias - # specified by our configuration file. - if scheme is None: - scheme = config['default-branch-scheme'] - skip_repo_list = skip_list_for_platform(config, all_repos) skip_repo_list.extend(args.skip_repository_list) clone_results = obtain_all_additional_swift_sources(args, config, clone_with_ssh, - scheme, + scheme_name, skip_history, skip_tags, skip_repo_list) @@ -646,8 +694,8 @@ def main(): print("You don't have all swift sources. " "Call this script with --clone to get them.") - update_results = update_all_repositories(args, config, scheme, - cross_repos_pr) + update_results = update_all_repositories(args, config, scheme_name, + scheme_map, cross_repos_pr) fail_count = 0 fail_count += check_parallel_results(clone_results, "CLONE") fail_count += check_parallel_results(update_results, "UPDATE") From 21bd961f757e00807b483755edf76bc32ff4f642 Mon Sep 17 00:00:00 2001 From: Max Desiatov Date: Wed, 9 Nov 2022 19:57:32 +0000 Subject: [PATCH 2/5] update_checkout: fix Python linter warnings --- utils/update_checkout/update_checkout/update_checkout.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/utils/update_checkout/update_checkout/update_checkout.py b/utils/update_checkout/update_checkout/update_checkout.py index 5d042fbbdcd77..acf0ebb692ef8 100755 --- a/utils/update_checkout/update_checkout/update_checkout.py +++ b/utils/update_checkout/update_checkout/update_checkout.py @@ -653,14 +653,14 @@ def main(): if os.path.exists(swift_repo_path): with shell.pushd(swift_repo_path, dry_run=False, echo=True): # Check if `swift` repo itself needs to switch to a cross-repo branch. - branch_name, cross_repo = get_branch_for_repo(config, 'swift', + branch_name, cross_repo = get_branch_for_repo(config, 'swift', scheme_name, scheme_map, cross_repos_pr) if cross_repo: shell.run(['git', 'checkout', branch_name], echo=True, - prefix="[swift] ") + prefix="[swift] ") # Re-read the config after checkout. with open(args.config) as f: From ba7b458d386ff919147d614c7c635ed7d64b88a6 Mon Sep 17 00:00:00 2001 From: Max Desiatov Date: Thu, 10 Nov 2022 11:06:51 +0000 Subject: [PATCH 3/5] update-checkout: handle `--skip-repository` option --- .../update_checkout/update_checkout.py | 58 ++++++++++++------- 1 file changed, 38 insertions(+), 20 deletions(-) diff --git a/utils/update_checkout/update_checkout/update_checkout.py b/utils/update_checkout/update_checkout/update_checkout.py index acf0ebb692ef8..4eb1189862a79 100755 --- a/utils/update_checkout/update_checkout/update_checkout.py +++ b/utils/update_checkout/update_checkout/update_checkout.py @@ -75,6 +75,18 @@ def check_parallel_results(results, op): def confirm_tag_in_repo(tag, repo_name): + """Confirm that a given tag exists in a git repository. This function + assumes that the repository is already a current working directory before + it's called. + + Args: + tag (str): tag to look up in the repository + repo_name (str): name the repository for the look up, used for logging + + Returns: + str: returns `tag` argument value or `None` if the tag doesn't exist. + """ + tag_exists = shell.capture(['git', 'ls-remote', '--tags', 'origin', tag], echo=False) if not tag_exists: @@ -255,10 +267,11 @@ def update_single_repository(pool_args): return value -def get_timestamp_to_match(args): - if not args.match_timestamp: +def get_timestamp_to_match(match_timestamp, source_root): + # type: (str, str) -> str | None + if not match_timestamp: return None - with shell.pushd(os.path.join(args.source_root, "swift"), + with shell.pushd(os.path.join(source_root, "swift"), dry_run=False, echo=False): return shell.capture(["git", "log", "-1", "--format=%cI"], echo=False).strip() @@ -289,7 +302,7 @@ def get_scheme_map(config, scheme_name): def update_all_repositories(args, config, scheme_name, scheme_map, cross_repos_pr): pool_args = [] - timestamp = get_timestamp_to_match(args) + timestamp = get_timestamp_to_match(args.match_timestamp, args.source_root) for repo_name in config['repos'].keys(): if repo_name in args.skip_repository_list: print("Skipping update of '" + repo_name + "', requested by user") @@ -498,9 +511,15 @@ def full_target_name(repository, target): raise RuntimeError('Cannot determine if %s is a branch or a tag' % target) -def skip_list_for_platform(config, all_repos): - if all_repos: - return [] # Do not skip any platform-specific repositories +def skip_list_for_platform(config): + """Computes a list of repositories to skip when updating or cloning. + + Args: + config (Dict[str, Any]): deserialized `update-checkout-config.json` + + Returns: + List[str]: a resulting list of repositories to skip. + """ # If there is a platforms key only include the repo if the # platform is in the list @@ -628,7 +647,6 @@ def main(): skip_tags = args.skip_tags scheme_name = args.scheme github_comment = args.github_comment - all_repos = args.all_repositories with open(args.config) as f: config = json.load(f) @@ -649,8 +667,19 @@ def main(): scheme_map = get_scheme_map(config, scheme_name) + clone_results = None + if clone or clone_with_ssh: + skip_repo_list = skip_list_for_platform(config) + skip_repo_list.extend(args.skip_repository_list) + clone_results = obtain_all_additional_swift_sources(args, config, + clone_with_ssh, + scheme_name, + skip_history, + skip_tags, + skip_repo_list) + swift_repo_path = os.path.join(args.source_root, 'swift') - if os.path.exists(swift_repo_path): + if not 'swift' in skip_repo_list and os.path.exists(swift_repo_path): with shell.pushd(swift_repo_path, dry_run=False, echo=True): # Check if `swift` repo itself needs to switch to a cross-repo branch. branch_name, cross_repo = get_branch_for_repo(config, 'swift', @@ -675,17 +704,6 @@ def main(): dump_repo_hashes(args, config, args.dump_hashes_config) return (None, None) - clone_results = None - if clone or clone_with_ssh: - skip_repo_list = skip_list_for_platform(config, all_repos) - skip_repo_list.extend(args.skip_repository_list) - clone_results = obtain_all_additional_swift_sources(args, config, - clone_with_ssh, - scheme_name, - skip_history, - skip_tags, - skip_repo_list) - # Quick check whether somebody is calling update in an empty directory directory_contents = os.listdir(args.source_root) if not ('cmark' in directory_contents or From 0d5bc9ff691baec6e7b261ed87d16cb99d07d19d Mon Sep 17 00:00:00 2001 From: Max Desiatov Date: Thu, 10 Nov 2022 11:46:34 +0000 Subject: [PATCH 4/5] update-checkout: expand docstrings, fix missing options --- .../update_checkout/update_checkout.py | 29 +++++++++++++++---- 1 file changed, 24 insertions(+), 5 deletions(-) diff --git a/utils/update_checkout/update_checkout/update_checkout.py b/utils/update_checkout/update_checkout/update_checkout.py index 4eb1189862a79..b2f9255a00621 100755 --- a/utils/update_checkout/update_checkout/update_checkout.py +++ b/utils/update_checkout/update_checkout/update_checkout.py @@ -268,7 +268,19 @@ def update_single_repository(pool_args): def get_timestamp_to_match(match_timestamp, source_root): - # type: (str, str) -> str | None + # type: (str | None, str) -> str | None + """Computes a timestamp of the last commit on the current branch in + the `swift` repository. + + Args: + match_timestamp (str | None): value of `--match-timestamp` to check. + source_root (str): directory that contains sources of the Swift project. + + Returns: + str | None: a timestamp of the last commit of `swift` repository if + `match_timestamp` argument has a value, `None` if `match_timestamp` is + falsy. + """ if not match_timestamp: return None with shell.pushd(os.path.join(source_root, "swift"), @@ -511,16 +523,22 @@ def full_target_name(repository, target): raise RuntimeError('Cannot determine if %s is a branch or a tag' % target) -def skip_list_for_platform(config): - """Computes a list of repositories to skip when updating or cloning. +def skip_list_for_platform(config, all_repos): + """Computes a list of repositories to skip when updating or cloning, if not + overriden by `--all-repositories` CLI argument. Args: config (Dict[str, Any]): deserialized `update-checkout-config.json` + all_repos (List[str]): repositories not required for current platform. Returns: - List[str]: a resulting list of repositories to skip. + List[str]: a resulting list of repositories to skip or empty list if + `all_repos` is not empty. """ + if all_repos: + return [] # Do not skip any platform-specific repositories + # If there is a platforms key only include the repo if the # platform is in the list skip_list = [] @@ -647,6 +665,7 @@ def main(): skip_tags = args.skip_tags scheme_name = args.scheme github_comment = args.github_comment + all_repos = args.all_repositories with open(args.config) as f: config = json.load(f) @@ -669,7 +688,7 @@ def main(): clone_results = None if clone or clone_with_ssh: - skip_repo_list = skip_list_for_platform(config) + skip_repo_list = skip_list_for_platform(config, all_repos) skip_repo_list.extend(args.skip_repository_list) clone_results = obtain_all_additional_swift_sources(args, config, clone_with_ssh, From 9d299952e6e1f560b13e003a82f9bca1f2084c76 Mon Sep 17 00:00:00 2001 From: Max Desiatov Date: Thu, 10 Nov 2022 11:48:52 +0000 Subject: [PATCH 5/5] update-checkout: add type hints to `confirm_tag_in_repo` --- utils/update_checkout/update_checkout/update_checkout.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/utils/update_checkout/update_checkout/update_checkout.py b/utils/update_checkout/update_checkout/update_checkout.py index b2f9255a00621..8d104a2a941f9 100755 --- a/utils/update_checkout/update_checkout/update_checkout.py +++ b/utils/update_checkout/update_checkout/update_checkout.py @@ -75,6 +75,7 @@ def check_parallel_results(results, op): def confirm_tag_in_repo(tag, repo_name): + # type: (str, str) -> str | None """Confirm that a given tag exists in a git repository. This function assumes that the repository is already a current working directory before it's called. @@ -84,7 +85,8 @@ def confirm_tag_in_repo(tag, repo_name): repo_name (str): name the repository for the look up, used for logging Returns: - str: returns `tag` argument value or `None` if the tag doesn't exist. + str | None: returns `tag` argument value or `None` if the tag doesn't + exist. """ tag_exists = shell.capture(['git', 'ls-remote', '--tags', @@ -280,7 +282,7 @@ def get_timestamp_to_match(match_timestamp, source_root): str | None: a timestamp of the last commit of `swift` repository if `match_timestamp` argument has a value, `None` if `match_timestamp` is falsy. - """ + """ if not match_timestamp: return None with shell.pushd(os.path.join(source_root, "swift"), @@ -687,6 +689,7 @@ def main(): scheme_map = get_scheme_map(config, scheme_name) clone_results = None + skip_repo_list = [] if clone or clone_with_ssh: skip_repo_list = skip_list_for_platform(config, all_repos) skip_repo_list.extend(args.skip_repository_list) @@ -698,7 +701,7 @@ def main(): skip_repo_list) swift_repo_path = os.path.join(args.source_root, 'swift') - if not 'swift' in skip_repo_list and os.path.exists(swift_repo_path): + if 'swift' not in skip_repo_list and os.path.exists(swift_repo_path): with shell.pushd(swift_repo_path, dry_run=False, echo=True): # Check if `swift` repo itself needs to switch to a cross-repo branch. branch_name, cross_repo = get_branch_for_repo(config, 'swift',