servo: Merge #14715 - Tidy: Check Cargo.lock for packages with same version and different sources (from UK992:tidy-check-lock); r=SimonSapin
authorUK992 <urbankrajnc92@gmail.com>
Mon, 26 Dec 2016 08:57:04 -0800
changeset 478593 2a4854cb07e3dc8ef9e8dee499704d7b1a62c96d
parent 478592 7d177ab333c5c165b72ae5a788c745b728bf8197
child 478594 8fd6f32ce95c5636911be74af879832ee7ab8893
push id44079
push userbmo:gps@mozilla.com
push dateSat, 04 Feb 2017 00:14:49 +0000
reviewersSimonSapin
servo: Merge #14715 - Tidy: Check Cargo.lock for packages with same version and different sources (from UK992:tidy-check-lock); r=SimonSapin <!-- Please describe your changes on the following line: --> r? @Wafflespeanut cc @SimonSapin --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14695 <!-- Either: --> - [X] There are tests for these changes <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> Source-Repo: https://github.com/servo/servo Source-Revision: 37a5e29147f0dc489888377d6f7bb53282dc04f9
servo/python/requirements.txt
servo/python/tidy/servo_tidy/tidy.py
servo/python/tidy/servo_tidy_tests/duplicated_package.lock
servo/python/tidy/servo_tidy_tests/test_tidy.py
servo/servo-tidy.toml
--- a/servo/python/requirements.txt
+++ b/servo/python/requirements.txt
@@ -1,15 +1,15 @@
 blessings == 1.6
 mach == 0.6.0
 mozdebug == 0.1
 mozinfo == 0.8
 mozlog == 3.3
 setuptools == 18.5
-toml == 0.9.1
+toml == 0.9.2
 Mako == 1.0.4
 
 # For Python linting
 flake8 == 2.4.1
 pep8 == 1.5.7
 pyflakes == 0.8.1
 
 # For buildbot checking
--- a/servo/python/tidy/servo_tidy/tidy.py
+++ b/servo/python/tidy/servo_tidy/tidy.py
@@ -28,20 +28,20 @@ CONFIG_FILE_PATH = os.path.join(".", "se
 # Default configs
 config = {
     "skip-check-length": False,
     "skip-check-licenses": False,
     "check-ordered-json-keys": [],
     "lint-scripts": [],
     "ignore": {
         "files": [
-            "./.",   # ignore hidden files
+            os.path.join(".", "."),   # ignore hidden files
         ],
         "directories": [
-            "./.",   # ignore hidden directories
+            os.path.join(".", "."),   # ignore hidden directories
         ],
         "packages": [],
     },
     "check_ext": {}
 }
 
 COMMENTS = ["// ", "# ", " *", "/* "]
 
@@ -85,16 +85,20 @@ WEBIDL_STANDARDS = [
 def is_iter_empty(iterator):
     try:
         obj = iterator.next()
         return True, itertools.chain((obj,), iterator)
     except StopIteration:
         return False, iterator
 
 
+def normilize_paths(paths):
+    return [os.path.join(*path.split('/')) for path in paths]
+
+
 # A simple wrapper for iterators to show progress
 # (Note that it's inefficient for giant iterators, since it iterates once to get the upper bound)
 def progress_wrapper(iterator):
     list_of_stuff = list(iterator)
     total_files, progress = len(list_of_stuff), 0
     for idx, thing in enumerate(list_of_stuff):
         progress = int(float(idx + 1) / total_files * 100)
         sys.stdout.write('\r  Progress: %s%% (%d/%d)' % (progress, idx + 1, total_files))
@@ -118,21 +122,19 @@ class FileList(object):
         for root, _, files in os.walk(self.directory):
             for f in files:
                 yield os.path.join(root, f)
 
     def _git_changed_files(self):
         args = ["git", "log", "-n1", "--merges", "--format=%H"]
         last_merge = subprocess.check_output(args).strip()
         args = ["git", "diff", "--name-only", last_merge, self.directory]
-        file_list = subprocess.check_output(args)
+        file_list = normilize_paths(subprocess.check_output(args).splitlines())
 
-        for f in file_list.splitlines():
-            if sys.platform == 'win32':
-                os.path.join(*f.split('/'))
+        for f in file_list:
             if not any(os.path.join('.', os.path.dirname(f)).startswith(path) for path in self.excluded):
                 yield os.path.join('.', f)
 
     def _filter_excluded(self):
         for root, dirs, files in os.walk(self.directory, topdown=True):
             # modify 'dirs' in-place so that we don't do unnecessary traversals in excluded directories
             dirs[:] = [d for d in dirs if not any(os.path.join(root, d).startswith(name) for name in self.excluded)]
             for rel_path in files:
@@ -294,71 +296,52 @@ def check_flake8(file_name, contents):
     with stdout_redirect(output):
         check_code(contents, ignore=ignore)
     for error in output.getvalue().splitlines():
         _, line_num, _, message = error.split(":", 3)
         yield line_num, message.strip()
 
 
 def check_lock(file_name, contents):
-    def find_reverse_dependencies(dependency, version, content):
-        dependency_prefix = "{} {}".format(dependency, version)
+    def find_reverse_dependencies(name, content):
         for package in itertools.chain([content["root"]], content["package"]):
             for dependency in package.get("dependencies", []):
-                if dependency.startswith(dependency_prefix):
-                    yield package["name"]
+                if dependency.startswith("{} ".format(name)):
+                    yield package["name"], dependency
 
     if not file_name.endswith(".lock"):
         raise StopIteration
 
-    # package names to be neglected (as named by cargo)
+    # Package names to be neglected (as named by cargo)
     exceptions = config["ignore"]["packages"]
 
-    # toml.py has a bug(?) that we trip up in [metadata] sections;
-    # see https://github.com/uiri/toml/issues/61
-    # This should only affect a very few lines (that have embedded ?branch=...),
-    # and most of them won't be in the repo
-    try:
-        content = toml.loads(contents)
-    except:
-        print "WARNING!"
-        print "WARNING! toml parsing failed for Cargo.lock, but ignoring..."
-        print "WARNING!"
-        raise StopIteration
+    content = toml.loads(contents)
 
-    packages = {}
+    packages_by_name = {}
     for package in content.get("package", []):
-        packages.setdefault(package["name"], []).append(package["version"])
+        source = package.get("source", "")
+        if source == r"registry+https://github.com/rust-lang/crates.io-index":
+            source = "crates.io"
+        packages_by_name.setdefault(package["name"], []).append((package["version"], source))
 
-    for (name, versions) in packages.iteritems():
-        if name in exceptions or len(versions) <= 1:
+    for (name, packages) in packages_by_name.iteritems():
+        if name in exceptions or len(packages) <= 1:
             continue
 
-        highest = max(versions)
-        for version in versions:
-            if version != highest:
-                reverse_dependencies = "\n".join(
-                    "\t\t{}".format(n)
-                    for n in find_reverse_dependencies(name, version, content)
-                )
-                substitutions = {
-                    "package": name,
-                    "old_version": version,
-                    "new_version": highest,
-                    "reverse_dependencies": reverse_dependencies
-                }
-                message = """
-duplicate versions for package "{package}"
-\t\033[93mfound dependency on version {old_version}\033[0m
-\t\033[91mbut highest version is {new_version}\033[0m
-\t\033[93mtry upgrading with\033[0m \033[96m./mach cargo-update -p {package}:{old_version}\033[0m
-\tThe following packages depend on version {old_version}:
-{reverse_dependencies}
-""".format(**substitutions).strip()
-                yield (1, message)
+        message = "duplicate versions for package `{}`".format(name)
+        packages.sort()
+        packages_dependencies = list(find_reverse_dependencies(name, content))
+        for version, source in packages:
+            short_source = source.split("#")[0].replace("git+", "")
+            message += "\n\t\033[93mThe following packages depend on version {} from '{}':\033[0m" \
+                       .format(version, short_source)
+            for name, dependency in packages_dependencies:
+                if version in dependency and short_source in dependency:
+                    message += "\n\t\t" + name
+        yield (1, message)
 
 
 def check_toml(file_name, lines):
     if not file_name.endswith("Cargo.toml"):
         raise StopIteration
     ok_licensed = False
     for idx, line in enumerate(lines):
         if idx == 0 and "[workspace]" in line:
@@ -857,33 +840,27 @@ def check_config_file(config_file, print
     # Parse config file
     parse_config(conf_file)
 
 
 def parse_config(content):
     config_file = toml.loads(content)
     exclude = config_file.get("ignore", {})
     # Add list of ignored directories to config
-    config["ignore"]["directories"] += exclude.get("directories", [])
+    config["ignore"]["directories"] += normilize_paths(exclude.get("directories", []))
     # Add list of ignored files to config
-    config["ignore"]["files"] += exclude.get("files", [])
+    config["ignore"]["files"] += normilize_paths(exclude.get("files", []))
     # Add list of ignored packages to config
     config["ignore"]["packages"] = exclude.get("packages", [])
-    # Fix the paths (OS-dependent)
-    config['ignore']['files'] = map(lambda path: os.path.join(*path.split('/')),
-                                    config['ignore']['files'])
-    config['ignore']['directories'] = map(lambda path: os.path.join(*path.split('/')),
-                                          config['ignore']['directories'])
 
     # Add dict of dir, list of expected ext to config
     dirs_to_check = config_file.get("check_ext", {})
     # Fix the paths (OS-dependent)
     for path, exts in dirs_to_check.items():
-        fixed_path = os.path.join(*path.split('/'))
-        config['check_ext'][fixed_path] = exts
+        config['check_ext'][normilize_paths([path])[0]] = exts
 
     # Override default configs
     user_configs = config_file.get("configs", [])
     for pref in user_configs:
         if pref in config:
             config[pref] = user_configs[pref]
 
 
--- a/servo/python/tidy/servo_tidy_tests/duplicated_package.lock
+++ b/servo/python/tidy/servo_tidy_tests/duplicated_package.lock
@@ -10,12 +10,38 @@ source = "registry+https://github.com/ru
 [[package]]
 name = "test"
 version = "0.5.1"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 
 [[package]]
 name = "test2"
 version = "0.1.0"
-source = "git+https://github.com/"
+source = "git+https://github.com/user/test2#c54edsf"
 dependencies = [
  "test 0.4.9 (registry+https://github.com/rust-lang/crates.io-index)",
 ]
+
+[[package]]
+name = "test3"
+version = "0.5.1"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+
+[[package]]
+name = "test3"
+version = "0.5.1"
+source = "git+https://github.com/user/test3#c54edsf"
+
+[[package]]
+name = "test4"
+version = "0.1.0"
+source = "git+https://github.com/user/test4#c54edsf"
+dependencies = [
+ "test3 0.5.1 (registry+https://github.com/rust-lang/crates.io-index)",
+]
+
+[[package]]
+name = "test5"
+version = "0.1.0"
+source = "git+https://github.com/"
+dependencies = [
+ "test3 0.5.1 (git+https://github.com/user/test3)",
+]
--- a/servo/python/tidy/servo_tidy_tests/test_tidy.py
+++ b/servo/python/tidy/servo_tidy_tests/test_tidy.py
@@ -195,23 +195,27 @@ class CheckTidiness(unittest.TestCase):
 
     def test_non_string_list_mapping_buildbot_steps(self):
         errors = tidy.collect_errors_for_files(iterFile('non_string_list_buildbot_steps.yml'), [tidy.check_yaml], [], print_text=False)
         self.assertEqual("List mapped to 'mapping_key' contains non-string element", errors.next()[2])
         self.assertNoMoreErrors(errors)
 
     def test_lock(self):
         errors = tidy.collect_errors_for_files(iterFile('duplicated_package.lock'), [tidy.check_lock], [], print_text=False)
-        msg = """duplicate versions for package "test"
-\t\033[93mfound dependency on version 0.4.9\033[0m
-\t\033[91mbut highest version is 0.5.1\033[0m
-\t\033[93mtry upgrading with\033[0m \033[96m./mach cargo-update -p test:0.4.9\033[0m
-\tThe following packages depend on version 0.4.9:
-\t\ttest2"""
+        msg = """duplicate versions for package `test`
+\t\x1b[93mThe following packages depend on version 0.4.9 from 'crates.io':\x1b[0m
+\t\ttest2
+\t\x1b[93mThe following packages depend on version 0.5.1 from 'crates.io':\x1b[0m"""
         self.assertEqual(msg, errors.next()[2])
+        msg2 = """duplicate versions for package `test3`
+\t\x1b[93mThe following packages depend on version 0.5.1 from 'crates.io':\x1b[0m
+\t\ttest4
+\t\x1b[93mThe following packages depend on version 0.5.1 from 'https://github.com/user/test3':\x1b[0m
+\t\ttest5"""
+        self.assertEqual(msg2, errors.next()[2])
         self.assertNoMoreErrors(errors)
 
     def test_lint_runner(self):
         test_path = base_path + 'lints/'
         runner = tidy.LintRunner(only_changed_files=False, progress=False)
         runner.path = test_path + 'some-fictional-file'
         self.assertEqual([(runner.path, 0, "file does not exist")], list(runner.check()))
         runner.path = test_path + 'not_script'
--- a/servo/servo-tidy.toml
+++ b/servo/servo-tidy.toml
@@ -54,9 +54,9 @@ directories = [
   "./target",
   "./ports/cef",
   "./components/style/gecko_bindings/nsstring_vendor/",
 ]
 
 # Directories that are checked for correct file extension
 [check_ext]
 # directory, list of expected file extensions
-"components/script/dom/webidls" = [".webidl"]
+"./components/script/dom/webidls" = [".webidl"]