Auto merge of #14715 - UK992:tidy-check-lock, r=SimonSapin

Tidy: Check Cargo.lock for packages with same version and different sources

<!-- 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. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14715)
<!-- Reviewable:end -->

python/requirements.txt

@@ -4,7 +4,7 @@ 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

python/tidy/servo_tidy/tidy.py

@@ -33,10 +33,10 @@ config = {
     "lint-scripts": [],
     "ignore": {
         "files": [
-            "./.",   # ignore hidden files
+            os.path.join(".", "."),   # ignore hidden files
         ],
         "directories": [
-            "./.",   # ignore hidden directories
+            os.path.join(".", "."),   # ignore hidden directories
         ],
         "packages": [],
     },
@@ -90,6 +90,10 @@ def is_iter_empty(iterator):
         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):
@@ -123,11 +127,9 @@ class FileList(object):
         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():
+        for f in file_list:
-            if sys.platform == 'win32':
-                os.path.join(*f.split('/'))
             if not any(os.path.join('.', os.path.dirname(f)).startswith(path) for path in self.excluded):
                 yield os.path.join('.', f)
 
@@ -299,61 +301,42 @@ def check_flake8(file_name, contents):
 
 
 def check_lock(file_name, contents):
-    def find_reverse_dependencies(dependency, version, content):
+    def find_reverse_dependencies(name, content):
-        dependency_prefix = "{} {}".format(dependency, version)
         for package in itertools.chain([content["root"]], content["package"]):
             for dependency in package.get("dependencies", []):
-                if dependency.startswith(dependency_prefix):
+                if dependency.startswith("{} ".format(name)):
-                    yield package["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;
+    content = toml.loads(contents)
-    # 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
 
-    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():
+    for (name, packages) in packages_by_name.iteritems():
-        if name in exceptions or len(versions) <= 1:
+        if name in exceptions or len(packages) <= 1:
             continue
 
-        highest = max(versions)
+        message = "duplicate versions for package `{}`".format(name)
-        for version in versions:
+        packages.sort()
-            if version != highest:
+        packages_dependencies = list(find_reverse_dependencies(name, content))
-                reverse_dependencies = "\n".join(
+        for version, source in packages:
-                    "\t\t{}".format(n)
+            short_source = source.split("#")[0].replace("git+", "")
-                    for n in find_reverse_dependencies(name, version, content)
+            message += "\n\t\033[93mThe following packages depend on version {} from '{}':\033[0m" \
-                )
+                       .format(version, short_source)
-                substitutions = {
+            for name, dependency in packages_dependencies:
-                    "package": name,
+                if version in dependency and short_source in dependency:
-                    "old_version": version,
+                    message += "\n\t\t" + name
-                    "new_version": highest,
+        yield (1, message)
-                    "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)
 
 
 def check_toml(file_name, lines):
@@ -862,23 +845,17 @@ 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'][normilize_paths([path])[0]] = exts
-        config['check_ext'][fixed_path] = exts
 
     # Override default configs
     user_configs = config_file.get("configs", [])

python/tidy/servo_tidy_tests/duplicated_package.lock

@@ -15,7 +15,33 @@ 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)",
+]

python/tidy/servo_tidy_tests/test_tidy.py

@@ -200,13 +200,17 @@ class CheckTidiness(unittest.TestCase):
 
     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"
+        msg = """duplicate versions for package `test`
-\t\033[93mfound dependency on version 0.4.9\033[0m
+\t\x1b[93mThe following packages depend on version 0.4.9 from 'crates.io':\x1b[0m
-\t\033[91mbut highest version is 0.5.1\033[0m
+\t\ttest2
-\t\033[93mtry upgrading with\033[0m \033[96m./mach cargo-update -p test:0.4.9\033[0m
+\t\x1b[93mThe following packages depend on version 0.5.1 from 'crates.io':\x1b[0m"""
-\tThe following packages depend on version 0.4.9:
-\t\ttest2"""
         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):

servo-tidy.toml

@@ -59,4 +59,4 @@ directories = [
 # 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"]