Rework variant sorting to more accurately follow versioning

Signed-off-by: Jonah Newton <jonah@jonahnewton.com.au>

Author: jonahjnewton

src/rez/data/tests/solver/packages/missing_variant_requires/1/package.py

@@ -5,6 +5,6 @@ def commands():
     pass
 
 variants = [
-    ["noexist"],
-    ["nada"]
+    ["nada"],
+    ["noexist"]
 ]

src/rez/data/tests/solver/packages/test_prio_both_versions_equal_extra_package/1.0/package.py

@@ -0,0 +1,4 @@
+name = "test_prio_both_versions_equal_extra_package"
+version = "1.0"
+
+variants = [["python-2.6.0", "nada"], ["python-2.6.0"]]

src/rez/solver.py

@@ -400,14 +400,19 @@ def sort(self):
 
         version_priority:
         - sort by highest versions of packages shared with request;
-        - THEN least number of additional packages added to solve;
-        - THEN highest versions of additional packages;
-        - THEN alphabetical on name of additional packages;
-        - THEN variant index.
+        - THEN highest versions of additional packages common to all variants;
+        - THEN highest number of packages shared with request;
+        - THEN lowest number of additional packages not common to all variants;
+        - THEN alphabetical on name of any remaining additional packages.
+        - THEN variant index;
 
         intersection_priority:
         - sort by highest number of packages shared with request;
-        - THEN sort according to version_priority
+        - THEN highest versions of packages shared with request;
+        - THEN highest versions of additional packages common to all variants;
+        - THEN lowest number of additional packages not common to all variants;
+        - THEN alphabetical on name of any remaining additional packages.
+        - THEN variant index;
 
         Note:
             In theory 'variant.index' should never factor into the sort unless
@@ -420,9 +425,28 @@ def sort(self):
         if self.sorted:
             return
 
+        requested_names = {
+            request.name for request in self.solver.request_list
+            if not request.conflict
+        }
+
+        common_additional_names = None
+        for variant in self.variants:
+            variant_additional_names = {
+                request.name
+                for request in variant.requires_list
+                if not request.conflict and request.name not in requested_names
+            }
+
+            if common_additional_names is None:
+                common_additional_names = variant_additional_names
+            else:
+                common_additional_names &= variant_additional_names
+
+        common_additional_names = common_additional_names or set()
+
         def key(variant):
             requested_key = []
-            names = set()
 
             for i, request in enumerate(self.solver.request_list):
                 if not request.conflict:
@@ -431,26 +455,32 @@ def key(variant):
                         orderer = get_orderer(req.name, orderers=self.solver.package_orderers or {})
                         range_key = orderer.sort_key(req.name, req.range)
                         requested_key.append((-i, range_key))
-                        names.add(req.name)
 
-            additional_key = []
+            common_additional_key = []
+            non_common_additional_key = []
             for request in variant.requires_list:
-                if not request.conflict and request.name not in names:
+                if not request.conflict and request.name not in requested_names:
                     orderer = get_orderer(request.name, orderers=self.solver.package_orderers)
                     range_key = orderer.sort_key(request.name, request.range)
-                    additional_key.append((range_key, request.name))
+                    key_item = (range_key, request.name)
+                    if request.name in common_additional_names:
+                        common_additional_key.append(key_item)
+                    else:
+                        non_common_additional_key.append(key_item)
 
+            
             if (VariantSelectMode[config.variant_select_mode] == VariantSelectMode.version_priority):
                 k = (requested_key,
-                     additional_key,
+                     common_additional_key,
+                     len(requested_key),
+                     -len(non_common_additional_key),
                      variant.index)
             else:  # VariantSelectMode.intersection_priority
                 k = (len(requested_key),
-                     requested_key,
-                     -len(additional_key),
-                     additional_key,
-                     variant.index)
-
+                    requested_key,
+                    common_additional_key,
+                    -len(non_common_additional_key),
+                    variant.index)
             return k
 
         self.variants.sort(key=key, reverse=True)

src/rez/tests/test_completion.py

@@ -54,7 +54,7 @@ def _eq(prefix, expected_completions):
                  "test_variant_split_start", "test_variant_split_mid1",
                  "test_variant_split_mid2", "test_variant_split_end", "missing_variant_requires",
                  "test_weakly_reference_requires", "test_weakly_reference_variant", 
-                 "test_prio_both_base", "test_prio_both_extra_package", "test_prio_lower_extra_package", "test_prio_higher_extra_package", "test_prio_both_base_reverse", "test_prio_both_extra_package_reverse", "test_prio_lower_extra_package_reverse", "test_prio_higher_extra_package_reverse"])
+                 "test_prio_both_base", "test_prio_both_extra_package", "test_prio_lower_extra_package", "test_prio_higher_extra_package", "test_prio_both_base_reverse", "test_prio_both_extra_package_reverse", "test_prio_lower_extra_package_reverse", "test_prio_higher_extra_package_reverse", "test_prio_both_versions_equal_extra_package"])
         _eq("py", ["pybah", "pydad", "pyfoo", "pymum", "pyodd", "pyson",
             "pysplit", "python", "pyvariants"])
         _eq("pys", ["pyson", "pysplit"])

src/rez/tests/test_packages.py

@@ -63,6 +63,7 @@
     'test_weakly_reference_variant-2.0',
     'test_prio_both_extra_package-1.0',
     'test_prio_both_base-1.0',
+    'test_prio_both_versions_equal_extra_package-1.0',
     'test_prio_lower_extra_package-1.0',
     'test_prio_higher_extra_package-1.0',
     'test_prio_both_extra_package_reverse-1.0',

src/rez/tests/test_solver.py

@@ -260,7 +260,7 @@ def test_12_missing_variant_requires(self):
             self._solve(["missing_variant_requires"], [])
 
         config.override("error_on_missing_variant_requires", False)
-        self._solve(["missing_variant_requires"], ["nada[]", "missing_variant_requires-1[1]"])
+        self._solve(["missing_variant_requires"], ["nada[]", "missing_variant_requires-1[0]"])
 
     def test_13_resolve_weakly_reference_requires(self):
         """Test resolving a package with a weakly referenced requirement."""
@@ -334,6 +334,19 @@ def test_15_version_priority_extra_packages(self):
                     ["python-2.7.0[]", "test_prio_both_base-1.0[1]", "nada[]"], 
                     perms_same_packages=True)
 
+        self._solve(["test_prio_both_versions_equal_extra_package"],
+                    ["python-2.6.0[]", "test_prio_both_versions_equal_extra_package-1.0[1]"], 
+                    perms_same_packages=True)
+        self._solve(["test_prio_both_versions_equal_extra_package", "python"],
+                    ["python-2.6.0[]", "test_prio_both_versions_equal_extra_package-1.0[1]"], 
+                    perms_same_packages=True)
+        self._solve(["test_prio_both_versions_equal_extra_package", "python", "nada"],
+                    ["python-2.6.0[]", "nada[]", "test_prio_both_versions_equal_extra_package-1.0[0]"],
+                    perms_same_packages=True)
+        self._solve(["test_prio_both_versions_equal_extra_package", "nada"],
+                    ["nada[]", "python-2.6.0[]", "test_prio_both_versions_equal_extra_package-1.0[0]"], 
+                    perms_same_packages=True)
+        
         self._solve(["test_prio_lower_extra_package_reverse"],
                     ["python-2.7.0[]", "test_prio_lower_extra_package_reverse-1.0[0]"], 
                     perms_same_packages=True)
@@ -405,7 +418,7 @@ def test_16_intersection_priority_extra_packages(self):
                     perms_same_packages=True)
 
         self._solve(["test_prio_higher_extra_package"],
-                    ["python-2.6.0[]", "test_prio_higher_extra_package-1.0[0]"],
+                    ["nada[]", "python-2.7.0[]", "test_prio_higher_extra_package-1.0[1]"],
                     perms_same_packages=True)
         self._solve(["test_prio_higher_extra_package", "python"],
                     ["python-2.7.0[]", "nada[]", "test_prio_higher_extra_package-1.0[1]"])
@@ -465,7 +478,7 @@ def test_16_intersection_priority_extra_packages(self):
                     perms_same_packages=True)
 
         self._solve(["test_prio_higher_extra_package_reverse"],
-                    ["python-2.6.0[]", "test_prio_higher_extra_package_reverse-1.0[1]"],
+                    ["nada[]", "python-2.7.0[]", "test_prio_higher_extra_package_reverse-1.0[0]"],
                     perms_same_packages=True)
         self._solve(["test_prio_higher_extra_package_reverse", "python"],
                     ["python-2.7.0[]", "nada[]", "test_prio_higher_extra_package_reverse-1.0[0]"])