浏览代码

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 <renewitt@google.com>
> > Reviewed-by: Gavin Mak <gavinmak@google.com>
> > Commit-Queue: Dan Le Febvre <dlf@google.com>
>
> Bug: b/279097790
> Change-Id: Ib08dac506ce221243c595dde5cb00e8e0d7dc7ed
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/4501582
> Reviewed-by: Gavin Mak <gavinmak@google.com>
> Commit-Queue: Dan Le Febvre <dlf@google.com>

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 <dcheng@chromium.org>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Reviewed-by: Dan Le Febvre <dlf@google.com>
Alexander Thomas 2 年之前
父节点
当前提交
24996afd00

+ 0 - 9
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))

+ 0 - 14
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:

+ 1 - 1
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.

+ 1 - 31
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')

+ 4 - 25
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', {

+ 5 - 19
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)
 

+ 1 - 23
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',

+ 9 - 16
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,