From 24996afd00de79d33cea18204fc75ea6c0ad35c5 Mon Sep 17 00:00:00 2001 From: Alexander Thomas Date: Fri, 12 May 2023 08:21:47 +0000 Subject: [PATCH] Revert "Reland "resolve CIPD package names in gclient"" This reverts commit d69302761b70d7bb660bcbd42b1395ee17b4f16b. Reason for revert: Broke Dart CI ``` gclient.py revinfo --output-json output.json --filter='https://chrome-infra-packages.appspot.com/dart/dart-sdk/${platform}' ``` No longer returns: ``` { "sdk/tools/sdks/dart-sdk:dart/dart-sdk/${platform}": { "rev": "git_revision:7a6514d1377175decd3a886fe4190fbbebddac3a", "url": "https://chrome-infra-packages.appspot.com/dart/dart-sdk/${platform}" } } ``` Bad build: https://ci.chromium.org/ui/p/dart/builders/ci.sandbox/vm-aot-msan-linux-release-x64/90/overview Good build: https://ci.chromium.org/ui/p/dart/builders/ci.sandbox/vm-aot-msan-linux-release-x64/89/overview Original change's description: > Reland "resolve CIPD package names in gclient" > > This CL relands the below CL with the addition of handling situations where the CIPD package name has a variable=value and does not equate to true. In this case just return the original value in DEPS. > > This is a reland of commit 8faa5514ec0c4a36d65b44acbd98b2eb66b91405 > > Original change's description: > > resolve CIPD package names in gclient > > > > Currently the package names are not resolved in gclient output, see example below. > > > > This is part of broader work to improve CIPD output, the next CL will replace 'None'. > > > > ``` > > $~ gclient revinfo -a | grep '${platform}\|${arch}' > > > > src/buildtools/linux64:gn/gn/linux-${arch}: None > > src/buildtools/reclient:infra/rbe/client/${platform}: None > > src/third_party/dawn/third_party/ninja:infra/3pp/tools/ninja/${platform}: None > > src/third_party/dawn/tools/golang:infra/3pp/tools/go/${platform}: None > > src/third_party/devtools-frontend-internal/devtools-frontend/third_party/esbuild:infra/3pp/tools/esbuild/${platform}: None > > src/third_party/devtools-frontend/src/third_party/esbuild:infra/3pp/tools/esbuild/${platform}: None > > src/third_party/ninja:infra/3pp/tools/ninja/${platform}: None > > src/tools/luci-go:infra/tools/luci/isolate/${platform}: None > > src/tools/luci-go:infra/tools/luci/swarming/${platform}: None > > src/tools/resultdb:infra/tools/result_adapter/${platform}: None > > ``` > > > > Bug:b/279097790 > > Change-Id: I21abf2579910e9d79d9ee0dcd018ee761e3d3c1c > > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/4463983 > > Reviewed-by: Rachael Newitt > > Reviewed-by: Gavin Mak > > Commit-Queue: Dan Le Febvre > > Bug: b/279097790 > Change-Id: Ib08dac506ce221243c595dde5cb00e8e0d7dc7ed > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/4501582 > Reviewed-by: Gavin Mak > Commit-Queue: Dan Le Febvre Bug: b/279097790 Change-Id: Ibf82354ac5005d9d6279d88aa99c8fb344aa6833 No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/4518113 Owners-Override: Daniel Cheng Commit-Queue: Daniel Cheng Bot-Commit: Rubber Stamper Reviewed-by: Dan Le Febvre --- gclient.py | 9 --------- gclient_scm.py | 14 -------------- testing_support/cipd.bat | 2 +- testing_support/fake_cipd.py | 32 +------------------------------- testing_support/fake_repos.py | 29 ++++------------------------- tests/gclient_cipd_smoketest.py | 24 +++++------------------- tests/gclient_git_smoketest.py | 24 +----------------------- tests/gclient_test.py | 25 +++++++++---------------- 8 files changed, 21 insertions(+), 138 deletions(-) diff --git a/gclient.py b/gclient.py index e1479bacb5..8ca899caac 100755 --- a/gclient.py +++ b/gclient.py @@ -2216,16 +2216,7 @@ class CipdDependency(Dependency): def __init__( self, parent, name, dep_value, cipd_root, custom_vars, should_process, relative, condition): - package = dep_value['package'] - # TODO(dlf): Switch to a batch mechanism using cipd ensure file instead - # of describe and expand all packages not just should_process. - if should_process: - expanded_package = cipd_root.expand_package_name(dep_value['package']) - # If package is not suitable for the platform an empty string will be - # returned. Just use original package name from DEPS file in that case. - if expanded_package: - package = expanded_package version = dep_value['version'] url = urlparse.urljoin( cipd_root.service_url, '%s@%s' % (package, version)) diff --git a/gclient_scm.py b/gclient_scm.py index e0a5a41575..8acc4250c3 100644 --- a/gclient_scm.py +++ b/gclient_scm.py @@ -1596,20 +1596,6 @@ class CipdRoot(object): if os.path.exists(cipd_cache_dir): raise - def expand_package_name(self, package_name_string, **kwargs): - """Run `cipd expand-package-name`. - - CIPD package names can be declared with placeholder variables - such as '${platform}', this cmd will return the package name - with the variables resolved. The resolution is based on the host - the command is executing on. - """ - - kwargs.setdefault('stderr', subprocess2.PIPE) - cmd = ['cipd', 'expand-package-name', package_name_string] - ret = subprocess2.check_output(cmd, **kwargs).decode('utf-8') - return ret.strip() - @contextlib.contextmanager def _create_ensure_file(self): try: diff --git a/testing_support/cipd.bat b/testing_support/cipd.bat index a5d0c55c0b..f5b6df7dd8 100755 --- a/testing_support/cipd.bat +++ b/testing_support/cipd.bat @@ -1,4 +1,4 @@ -@echo off +echo off :: Copyright (c) 2018 The Chromium Authors. All rights reserved. :: Use of this source code is governed by a BSD-style license that can be :: found in the LICENSE file. diff --git a/testing_support/fake_cipd.py b/testing_support/fake_cipd.py index 844b28ee9a..2244e6045f 100644 --- a/testing_support/fake_cipd.py +++ b/testing_support/fake_cipd.py @@ -10,14 +10,8 @@ import re import shutil import sys -ARCH_VAR = 'arch' -OS_VAR = 'os' -PLATFORM_VAR = 'platform' CIPD_SUBDIR_RE = '@Subdir (.*)' -CIPD_ENSURE = 'ensure' -CIPD_EXPAND_PKG = 'expand-package-name' -CIPD_EXPORT = 'export' def parse_cipd(root, contents): @@ -35,32 +29,8 @@ def parse_cipd(root, contents): return tree -def expand_package_name_cmd(package_name): - package_split = package_name.split("/") - suffix = package_split[-1] - # Any use of var equality should return empty for testing. - if "=" in suffix: - if suffix != "${platform=fake-platform-ok}": - return "" - package_name = "/".join(package_split[:-1] + ["${platform}"]) - - for v in [ARCH_VAR, OS_VAR, PLATFORM_VAR]: - var = "${%s}" % v - if package_name.endswith(var): - package_name = package_name.replace(var, "%s-expanded-test-only" % v) - return package_name - - def main(): - cmd = sys.argv[1] - assert cmd in [CIPD_ENSURE, CIPD_EXPAND_PKG, CIPD_EXPORT] - # Handle cipd expand-package-name - if cmd == CIPD_EXPAND_PKG: - # Expecting argument after cmd - assert len(sys.argv) == 3 - # Write result to stdout - sys.stdout.write(expand_package_name_cmd(sys.argv[2])) - return 0 + assert sys.argv[1] in ['ensure', 'export'] parser = argparse.ArgumentParser() parser.add_argument('-ensure-file') parser.add_argument('-root') diff --git a/testing_support/fake_repos.py b/testing_support/fake_repos.py index 37bd3e72fa..0c9797d90a 100755 --- a/testing_support/fake_repos.py +++ b/testing_support/fake_repos.py @@ -657,10 +657,8 @@ hooks = [{ 'origin': 'git/repo_13@3\n' }) - self._commit_git( - 'repo_14', { - 'DEPS': - textwrap.dedent("""\ + self._commit_git('repo_14', { + 'DEPS': textwrap.dedent("""\ vars = {} deps = { 'src/cipd_dep': { @@ -694,28 +692,9 @@ hooks = [{ ], 'dep_type': 'cipd', }, - 'src/cipd_dep_with_cipd_variable_equation_not_ok': { - 'packages': [ - { - 'package': 'package3/${{platform=fake-platform-not-ok}}', - 'version': '1.2', - }, - ], - 'dep_type': 'cipd', - }, - 'src/cipd_dep_with_cipd_variable_equation_ok': { - 'packages': [ - { - 'package': 'package3/${{platform=fake-platform-ok}}', - 'version': '1.4', - }, - ], - 'dep_type': 'cipd', - }, }"""), - 'origin': - 'git/repo_14@2\n' - }) + 'origin': 'git/repo_14@2\n' + }) # A repo with a hook to be recursed in, without use_relative_paths self._commit_git('repo_15', { diff --git a/tests/gclient_cipd_smoketest.py b/tests/gclient_cipd_smoketest.py index 545e92a6cf..289023fc3a 100644 --- a/tests/gclient_cipd_smoketest.py +++ b/tests/gclient_cipd_smoketest.py @@ -33,8 +33,7 @@ class GClientSmokeCipd(gclient_smoketest_base.GClientSmokeBase): tree = self.mangle_git_tree(('repo_14@1', 'src')) tree.update({ - '_cipd': - '\n'.join([ + '_cipd': '\n'.join([ '$ParanoidMode CheckPresence', '$OverrideInstallMode copy', '', @@ -46,29 +45,16 @@ class GClientSmokeCipd(gclient_smoketest_base.GClientSmokeBase): 'package0 0.1', '', '@Subdir src/cipd_dep_with_cipd_variable', - 'package3/platform-expanded-test-only 1.2', - '', - '@Subdir src/cipd_dep_with_cipd_variable_equation_not_ok', - 'package3/${platform=fake-platform-not-ok} 1.2', - '', - '@Subdir src/cipd_dep_with_cipd_variable_equation_ok', - 'package3/platform-expanded-test-only 1.4', + 'package3/${platform} 1.2', '', '', ]), - 'src/another_cipd_dep/_cipd': - '\n'.join([ + 'src/another_cipd_dep/_cipd': '\n'.join([ 'package1 1.1-cr0', 'package2 1.13', ]), - 'src/cipd_dep/_cipd': - 'package0 0.1', - 'src/cipd_dep_with_cipd_variable/_cipd': - 'package3/platform-expanded-test-only 1.2', - 'src/cipd_dep_with_cipd_variable_equation_not_ok/_cipd': - 'package3/${platform=fake-platform-not-ok} 1.2', - 'src/cipd_dep_with_cipd_variable_equation_ok/_cipd': - 'package3/platform-expanded-test-only 1.4', + 'src/cipd_dep/_cipd': 'package0 0.1', + 'src/cipd_dep_with_cipd_variable/_cipd': 'package3/${platform} 1.2', }) self.assertTree(tree) diff --git a/tests/gclient_git_smoketest.py b/tests/gclient_git_smoketest.py index 0b9772ea6e..a50086b0cd 100755 --- a/tests/gclient_git_smoketest.py +++ b/tests/gclient_git_smoketest.py @@ -1261,35 +1261,13 @@ class GClientSmokeGIT(gclient_smoketest_base.GClientSmokeBase): ' "src/cipd_dep_with_cipd_variable": {', ' "packages": [', ' {', - ' "package": "package3/platform-expanded-test-only",', + ' "package": "package3/${{platform}}",', ' "version": "1.2",', ' },', ' ],', ' "dep_type": "cipd",', ' },', '', - ' # "src" -> src/cipd_dep_with_cipd_variable_equation_not_ok', - ' "src/cipd_dep_with_cipd_variable_equation_not_ok": {', - ' "packages": [', - ' {', - ' "package": "package3/${{platform=fake-platform-not-ok}}",', - ' "version": "1.2",', - ' },', - ' ],', - ' "dep_type": "cipd",', - ' },', - '', - ' # "src" -> src/cipd_dep_with_cipd_variable_equation_ok', - ' "src/cipd_dep_with_cipd_variable_equation_ok": {', - ' "packages": [', - ' {', - ' "package": "package3/platform-expanded-test-only",', - ' "version": "1.4",', - ' },', - ' ],', - ' "dep_type": "cipd",', - ' },', - '', '}', '', '# ' + self.git_base + 'repo_14, DEPS', diff --git a/tests/gclient_test.py b/tests/gclient_test.py index 5de4c8d367..252014dec5 100755 --- a/tests/gclient_test.py +++ b/tests/gclient_test.py @@ -46,15 +46,6 @@ def write(filename, content): f.write(content) -class CIPDRootMock(object): - def __init__(self, root_dir, service_url): - self.root_dir = root_dir - self.service_url = service_url - - def expand_package_name(self, package_name): - return package_name - - class SCMMock(object): unit_test = None def __init__(self, parsed_url, root_dir, name, out_fh=None, out_cb=None, @@ -1091,7 +1082,6 @@ class GclientTest(trial_dir.TestCase): options, _ = gclient.OptionParser().parse_args([]) options.ignore_dep_type = 'git' obj = gclient.GClient.LoadCurrentConfig(options) - obj._cipd_root = CIPDRootMock('src', 'https://example.com') self.assertEqual(1, len(obj.dependencies)) sol = obj.dependencies[0] @@ -1102,7 +1092,9 @@ class GclientTest(trial_dir.TestCase): dep = sol.dependencies[0] self.assertIsInstance(dep, gclient.CipdDependency) - self.assertEqual('https://example.com/lemur@version:1234', dep.url) + self.assertEqual( + 'https://chrome-infra-packages.appspot.com/lemur@version:1234', + dep.url) def testDepsFromNotAllowedHostsUnspecified(self): """Verifies gclient works fine with DEPS without allowed_hosts.""" @@ -1247,7 +1239,6 @@ class GclientTest(trial_dir.TestCase): '}') options, _ = gclient.OptionParser().parse_args([]) obj = gclient.GClient.LoadCurrentConfig(options) - obj._cipd_root = CIPDRootMock('src', 'https://example.com') self.assertEqual(1, len(obj.dependencies)) sol = obj.dependencies[0] @@ -1258,7 +1249,9 @@ class GclientTest(trial_dir.TestCase): dep = sol.dependencies[0] self.assertIsInstance(dep, gclient.CipdDependency) - self.assertEqual('https://example.com/lemur@version:1234', dep.url) + self.assertEqual( + 'https://chrome-infra-packages.appspot.com/lemur@version:1234', + dep.url) def testIgnoresCipdDependenciesWhenFlagIsSet(self): """Verifies that CIPD deps are ignored if --ignore-dep-type cipd is set.""" @@ -1411,8 +1404,8 @@ class GclientTest(trial_dir.TestCase): parser = gclient.OptionParser() options, _ = parser.parse_args([]) obj = gclient.GClient('foo', options) - cipd_root = CIPDRootMock(os.path.join(self.root_dir, 'dir1'), - 'https://example.com') + cipd_root = gclient_scm.CipdRoot( + os.path.join(self.root_dir, 'dir1'), 'https://example.com') obj.add_dependencies_and_close( [ gclient.Dependency( @@ -1465,7 +1458,7 @@ class GclientTest(trial_dir.TestCase): parser = gclient.OptionParser() options, _ = parser.parse_args([]) obj = gclient.GClient('src', options) - cipd_root = CIPDRootMock('src', 'https://example.com') + cipd_root = obj.GetCipdRoot() cipd_dep = gclient.CipdDependency( parent=obj,