From d7820cbedbc48053042e1847d00a812ed5d65539 Mon Sep 17 00:00:00 2001 From: frostming Date: Mon, 30 Sep 2019 22:24:42 +0800 Subject: [PATCH 1/5] Correctly combine metasets with OR --- news/68.feature.rst | 1 + src/passa/internals/markers.py | 65 ++++++++++++++++++++++++--- src/passa/internals/specifiers.py | 20 +++++++-- src/passa/models/metadata.py | 75 ++++++++++++++----------------- 4 files changed, 109 insertions(+), 52 deletions(-) create mode 100644 news/68.feature.rst diff --git a/news/68.feature.rst b/news/68.feature.rst new file mode 100644 index 0000000..f33f951 --- /dev/null +++ b/news/68.feature.rst @@ -0,0 +1 @@ +Use logic OR to combine metasets derived from different parents. diff --git a/src/passa/internals/markers.py b/src/passa/internals/markers.py index 6d8839c..00a2d54 100644 --- a/src/passa/internals/markers.py +++ b/src/passa/internals/markers.py @@ -2,7 +2,7 @@ from __future__ import absolute_import, unicode_literals -from packaging.markers import Marker +from packaging.markers import Marker as _Marker from .specifiers import PySpecs, gen_marker import six import distlib.markers @@ -17,8 +17,8 @@ def _ensure_marker(marker): - if not isinstance(marker, Marker): - return Marker(str(marker)) + if not isinstance(marker, _Marker): + return _Marker(str(marker)) return marker @@ -231,7 +231,7 @@ def parse_marker_dict(marker_dict): specs.add(parse_marker_dict(side)) else: # This is the easiest way to go from a string to a PySpec instance - specs.add(PySpecs.from_marker(Marker(side))) + specs.add(PySpecs.from_marker(_Marker(side))) sides.add(specs) if op == "and": # When we are "and"-ing things together, it probably makes the most sense @@ -241,11 +241,64 @@ def parse_marker_dict(marker_dict): sides = reduce(lambda x, y: x | y, sides) if not sides: return sides - return PySpecs.from_marker(Marker(str(sides))) + return PySpecs.from_marker(_Marker(str(sides))) # Actually when we "or" things as well we can also just turn them into a reduced # set using this logic now return reduce(lambda x, y: x & y, sides) else: # At the tip of the tree we are dealing with strings all around and they just need # to be smashed together - return PySpecs.from_marker(Marker(format_string.format(**marker_dict))) + return PySpecs.from_marker(_Marker(format_string.format(**marker_dict))) + + +class Marker(_Marker): + """A customized marker class with support of '&' and '|' operators. + We just try best to reduce duplicaiton when combining, in the following ways: + - A and/or A -> A + - A and B or A -> A + - A and B and A -> A and B + - A or B or A -> A or B + - (C and A or B) and A -> A + """ + def __init__(self, marker=None): + if marker is None: + self._markers = [] + else: + super(Marker, self).__init__(marker) + + def __and__(self, other): + other = _ensure_marker(other) + if not self._markers or self == other: + return type(self)(str(other)) + lhs, rhs = str(self), str(other) + # Need parentheses to wrap markers when the part contains 'or' + if 'or' in self._markers: + lhs = '({})'.format(str(self)) + if 'or' in other._markers: + rhs = '({})'.format(str(other)) + new_marker = type(self)("{} and {}".format(lhs, rhs)) + return new_marker + + def __or__(self, other): + other = _ensure_marker(other) + if not self._markers or self == other: + return type(self)(str(other)) + new_marker = type(self)("{} or {}".format(str(self), str(other))) + return new_marker + + def __bool__(self): + return bool(self._markers) + + def __nonzero__(self): + return self.__bool__() + + def __hash__(self): + return hash(tuple(self._markers)) + + def __eq__(self, other): + if not isinstance(other, _Marker): + return False + return self._markers == other._markers + + def __lt__(self, other): + return self._markers < other._markers diff --git a/src/passa/internals/specifiers.py b/src/passa/internals/specifiers.py index 420df90..57d8502 100644 --- a/src/passa/internals/specifiers.py +++ b/src/passa/internals/specifiers.py @@ -381,6 +381,13 @@ def get_version(v): ranges.add((min_,)) else: ranges.add((min_, max_)) + # Now add the holes between ranges to excludes + sorted_ranges = sorted(ranges, key=lambda x: x[0]) + for k in range(len(sorted_ranges) - 1): + lower = ALL_PYTHON_VERSIONS.index(sorted_ranges[k][-1]) + upper = ALL_PYTHON_VERSIONS.index(sorted_ranges[k + 1][0]) + if lower < upper: + excludes.update({ALL_PYTHON_VERSIONS[i] for i in range(lower + 1, upper)}) return ranges, excludes def create_specset_from_ranges(self, specset=None, ranges=None, excludes=None): @@ -427,7 +434,7 @@ def create_specset_from_ranges(self, specset=None, ranges=None, excludes=None): return new_specset @lru_cache(maxsize=128) - def __and__(self, other): + def __or__(self, other): # Unintuitive perhaps, but this is for "x or y" and needs to handle the # widest possible range encapsulated by the two using the intersection if not isinstance(other, PySpecs): @@ -435,8 +442,13 @@ def __and__(self, other): if self == other: return self new_specset = SpecifierSet() - # In order to do an "or" propertly we need to intersect the "good" versions - intersection = self.get_version_includes() | other.get_version_includes() + if not self.specifierset: + intersection = other.get_version_includes() + elif not other.specifierset: + intersection = self.get_version_includes() + else: + # In order to do an "or" propertly we need to intersect the "good" versions + intersection = self.get_version_includes() | other.get_version_includes() intersection_specset = self.get_specset_from_versions(intersection) # And then we need to union the "bad" versions excludes = self.get_version_excludes() & other.get_version_excludes() @@ -444,7 +456,7 @@ def __and__(self, other): return PySpecs(new_specset) @lru_cache(maxsize=128) - def __or__(self, specset): + def __and__(self, specset): if not isinstance(specset, PySpecs): specset = PySpecs(specset) if str(self) == str(specset): diff --git a/src/passa/models/metadata.py b/src/passa/models/metadata.py index 4d7641a..4c0bd0f 100644 --- a/src/passa/models/metadata.py +++ b/src/passa/models/metadata.py @@ -3,7 +3,6 @@ from __future__ import absolute_import, unicode_literals import copy -import itertools import operator import packaging.markers @@ -14,7 +13,7 @@ from six.moves import reduce from ..internals.markers import ( - get_without_extra, get_without_pyversion, get_contained_pyversions + get_without_extra, get_without_pyversion, get_contained_pyversions, Marker ) from ..internals.specifiers import PySpecs @@ -32,16 +31,16 @@ class MetaSet(object): includes a marker, and a specifier set of Python versions required. """ def __init__(self): - self.markerset = frozenset() + self.marker = Marker() self.pyspecset = PySpecs() def __repr__(self): - return "MetaSet(markerset={0!r}, pyspecset={1!r})".format( - ",".join(sorted(self.markerset)), str(self.pyspecset), + return "MetaSet(marker={0!r}, pyspecset={1!r})".format( + str(self.marker), str(self.pyspecset), ) def __key(self): - return (tuple(self.markerset), hash(tuple(self.pyspecset))) + return (self.marker, hash(tuple(self.pyspecset))) def __hash__(self): return hash(self.__key()) @@ -49,40 +48,15 @@ def __hash__(self): def __eq__(self, other): return self.__key() == other.__key() - def __len__(self): - return len(self.markerset) + len(self.pyspecset) - - def __iter__(self): - return itertools.chain(self.markerset, self.pyspecset) - def __lt__(self, other): return operator.lt(self.__key(), other.__key()) - def __le__(self, other): - return operator.le(self.__key(), other.__key()) - - def __ge__(self, other): - return operator.ge(self.__key(), other.__key()) - - def __gt__(self, other): - return operator.gt(self.__key(), other.__key()) - def __str__(self): - self.markerset = frozenset(filter(None, self.markerset)) - return " and ".join([mkr_part for mkr_part in dedup_markers(itertools.chain( - # Make sure to always use the same quotes so we can dedup properly. - ( - "{0}".format(ms) if " or " in ms else ms - for ms in (str(m).replace('"', "'") for m in self.markerset) - if ms - ), - ( - "{0}".format(str(self.pyspecset)) if self.pyspecset else "", - ))) if mkr_part - ]) + marker = self.marker & self.pyspecset.as_markers + return str(marker) def __bool__(self): - return bool(self.markerset or self.pyspecset) + return bool(self.marker or self.pyspecset) def __nonzero__(self): # Python 2. return self.__bool__() @@ -91,7 +65,7 @@ def __nonzero__(self): # Python 2. def from_tuple(cls, pair): marker, specset = pair pyspecs = PySpecs(specset) - markerset = set() + marker = Marker() if marker: # Returns a PySpec instance or None marker_pyversions = get_contained_pyversions(marker) @@ -100,9 +74,9 @@ def from_tuple(cls, pair): # The remainder of the marker, if there is any cleaned_marker = get_without_pyversion(marker) if cleaned_marker: - markerset.add(str(cleaned_marker)) + marker = marker & cleaned_marker metaset = cls() - metaset.markerset = frozenset(markerset) + metaset.marker = marker metaset.pyspecset = pyspecs return metaset @@ -110,14 +84,29 @@ def __or__(self, other): if not isinstance(other, type(self)): other = self.from_tuple(other) metaset = MetaSet() - markerset = set() + marker = Marker() specset = PySpecs() for meta in (self, other): - if meta.markerset: - markerset |= set(meta.markerset) + if meta.marker: + marker = marker | meta.marker if meta.pyspecset: specset = specset | meta.pyspecset - metaset.markerset = frozenset(markerset) + metaset.marker = marker + metaset.pyspecset = specset + return metaset + + def __and__(self, other): + if not isinstance(other, type(self)): + other = self.from_tuple(other) + metaset = MetaSet() + marker = Marker() + specset = PySpecs() + for meta in (self, other): + if meta.marker: + marker = marker & meta.marker + if meta.pyspecset: + specset = specset & meta.pyspecset + metaset.marker = marker metaset.pyspecset = specset return metaset @@ -144,7 +133,8 @@ def _build_metasets(dependencies, pythons, key, trace, all_metasets): packaging.specifiers.SpecifierSet(python), ) for parent_metaset in parent_metasets: - child_metaset = parent_metaset | metaset + # Use 'and' to connect markers inherited from parent. + child_metaset = parent_metaset & metaset metasets.add(child_metaset) return list(metasets) @@ -178,6 +168,7 @@ def _format_metasets(metasets): # If there is an unconditional route, this needs to be unconditional. if not metasets: return "" + # Use 'or' to combine markers from different parent. combined_metaset = str(MetaSet() | reduce(lambda x, y: x | y, metasets)) if not combined_metaset: return "" From 9f13589fc39b83367f93c19c880e16c0c4447d5e Mon Sep 17 00:00:00 2001 From: frostming Date: Tue, 1 Oct 2019 20:46:53 +0800 Subject: [PATCH 2/5] add test cases for pyspec --- src/passa/internals/markers.py | 15 ++++-------- src/passa/internals/specifiers.py | 2 +- src/passa/models/metadata.py | 8 ++++--- tests/unit/test_specifiers.py | 38 ++++++++++++++++++++++++++++++- 4 files changed, 48 insertions(+), 15 deletions(-) diff --git a/src/passa/internals/markers.py b/src/passa/internals/markers.py index 00a2d54..6b175df 100644 --- a/src/passa/internals/markers.py +++ b/src/passa/internals/markers.py @@ -252,16 +252,9 @@ def parse_marker_dict(marker_dict): class Marker(_Marker): - """A customized marker class with support of '&' and '|' operators. - We just try best to reduce duplicaiton when combining, in the following ways: - - A and/or A -> A - - A and B or A -> A - - A and B and A -> A and B - - A or B or A -> A or B - - (C and A or B) and A -> A - """ + """A customized marker class with support of '&' and '|' operators.""" def __init__(self, marker=None): - if marker is None: + if not marker: self._markers = [] else: super(Marker, self).__init__(marker) @@ -271,7 +264,8 @@ def __and__(self, other): if not self._markers or self == other: return type(self)(str(other)) lhs, rhs = str(self), str(other) - # Need parentheses to wrap markers when the part contains 'or' + # When combining markers with 'and', wrap the part with parentheses + # if there exist 'or's. if 'or' in self._markers: lhs = '({})'.format(str(self)) if 'or' in other._markers: @@ -283,6 +277,7 @@ def __or__(self, other): other = _ensure_marker(other) if not self._markers or self == other: return type(self)(str(other)) + # Just join parts with 'or' since it has the lowest priority. new_marker = type(self)("{} or {}".format(str(self), str(other))) return new_marker diff --git a/src/passa/internals/specifiers.py b/src/passa/internals/specifiers.py index 57d8502..e7490df 100644 --- a/src/passa/internals/specifiers.py +++ b/src/passa/internals/specifiers.py @@ -386,7 +386,7 @@ def get_version(v): for k in range(len(sorted_ranges) - 1): lower = ALL_PYTHON_VERSIONS.index(sorted_ranges[k][-1]) upper = ALL_PYTHON_VERSIONS.index(sorted_ranges[k + 1][0]) - if lower < upper: + if lower < upper - 1: excludes.update({ALL_PYTHON_VERSIONS[i] for i in range(lower + 1, upper)}) return ranges, excludes diff --git a/src/passa/models/metadata.py b/src/passa/models/metadata.py index 4c0bd0f..9137852 100644 --- a/src/passa/models/metadata.py +++ b/src/passa/models/metadata.py @@ -52,7 +52,9 @@ def __lt__(self, other): return operator.lt(self.__key(), other.__key()) def __str__(self): - marker = self.marker & self.pyspecset.as_markers + marker = self.marker + if self.pyspecset: + marker = marker & self.pyspecset return str(marker) def __bool__(self): @@ -65,7 +67,7 @@ def __nonzero__(self): # Python 2. def from_tuple(cls, pair): marker, specset = pair pyspecs = PySpecs(specset) - marker = Marker() + new_marker = Marker() if marker: # Returns a PySpec instance or None marker_pyversions = get_contained_pyversions(marker) @@ -74,7 +76,7 @@ def from_tuple(cls, pair): # The remainder of the marker, if there is any cleaned_marker = get_without_pyversion(marker) if cleaned_marker: - marker = marker & cleaned_marker + new_marker = new_marker & cleaned_marker metaset = cls() metaset.marker = marker metaset.pyspecset = pyspecs diff --git a/tests/unit/test_specifiers.py b/tests/unit/test_specifiers.py index 1b50084..46f20c9 100644 --- a/tests/unit/test_specifiers.py +++ b/tests/unit/test_specifiers.py @@ -2,7 +2,7 @@ from packaging.specifiers import SpecifierSet -from passa.internals.specifiers import cleanup_pyspecs +from passa.internals.specifiers import cleanup_pyspecs, PySpecs @pytest.mark.parametrize("spec, cleaned", [ @@ -28,3 +28,39 @@ def test_cleanup_pyspecs(spec, cleaned): cleaned_specifierset = frozenset(s for s in cleaned) assert cleanup_pyspecs(SpecifierSet(spec)) == cleaned_specifierset + + +@pytest.mark.parametrize("left, right, result", [ + ("", ">=3.6", ">=3.6"), + (">=2.7,!=3.0,!=3.1,!=3.2,!=3.3", ">=3.6", "!=3.0,!=3.1,!=3.2,!=3.3,>=3.6"), + ("<3.0", ">=3.3", "<3.0,>=3.3"), + ("==3.7", ">=3.6", "==3.7,>=3.6") +]) +def test_pyspecs_and_operator(left, right, result): + left = PySpecs(SpecifierSet(left)) + right = PySpecs(SpecifierSet(right)) + rv = left & right + assert str(rv.specifierset) == result + + +@pytest.mark.parametrize("left, right, result", [ + ("", ">=3.6", ">=3.6"), + (">=2.7,!=3.0,!=3.1,!=3.2,!=3.3", ">=3.6", "!=3.0,!=3.1,!=3.2,!=3.3,>=2.7"), + ("==2.7", ">=3.4", "!=3.0,!=3.1,!=3.2,!=3.3,>=2.7"), + (">=2.6,<3.0", ">=3.3,<3.6", "!=3.0,!=3.1,!=3.2,<3.6,>=2.6"), + ("~=3.0", ">=2.7", ">=2.7") +]) +def test_pyspecs_or_operator(left, right, result): + left = PySpecs(SpecifierSet(left)) + right = PySpecs(SpecifierSet(right)) + rv = left | right + assert str(rv.specifierset) == result + + +@pytest.mark.parametrize("spec, marker", [ + ("", ""), + (">=3.3,<3.7", 'python_version < "3.7" and python_version >= "3.3"') +]) +def test_pyspecs_to_marker(spec, marker): + pyspec = PySpecs(SpecifierSet(spec)) + assert str(pyspec) == marker From ac22ce5de0fdaced341af8785091b0ae3eb73261 Mon Sep 17 00:00:00 2001 From: frostming Date: Tue, 1 Oct 2019 21:04:35 +0800 Subject: [PATCH 3/5] Use a more meaningful name --- src/passa/internals/specifiers.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/passa/internals/specifiers.py b/src/passa/internals/specifiers.py index e7490df..58a38e7 100644 --- a/src/passa/internals/specifiers.py +++ b/src/passa/internals/specifiers.py @@ -436,23 +436,23 @@ def create_specset_from_ranges(self, specset=None, ranges=None, excludes=None): @lru_cache(maxsize=128) def __or__(self, other): # Unintuitive perhaps, but this is for "x or y" and needs to handle the - # widest possible range encapsulated by the two using the intersection + # widest possible range encapsulated by the two using the union if not isinstance(other, PySpecs): other = PySpecs(other) if self == other: return self new_specset = SpecifierSet() if not self.specifierset: - intersection = other.get_version_includes() + union = other.get_version_includes() elif not other.specifierset: - intersection = self.get_version_includes() + union = self.get_version_includes() else: # In order to do an "or" propertly we need to intersect the "good" versions - intersection = self.get_version_includes() | other.get_version_includes() - intersection_specset = self.get_specset_from_versions(intersection) + union = self.get_version_includes() | other.get_version_includes() + union_specset = self.get_specset_from_versions(union) # And then we need to union the "bad" versions excludes = self.get_version_excludes() & other.get_version_excludes() - new_specset = self.create_specset_from_ranges(ranges=intersection_specset, excludes=excludes) + new_specset = self.create_specset_from_ranges(ranges=union_specset, excludes=excludes) return PySpecs(new_specset) @lru_cache(maxsize=128) From 3166a7c44b1f6f23b59f69615eb9bfa10ef6851a Mon Sep 17 00:00:00 2001 From: frostming Date: Tue, 1 Oct 2019 22:07:04 +0800 Subject: [PATCH 4/5] disable lru_cache for a functionnot return the same iterator --- src/passa/internals/markers.py | 56 ++++++++++++++++++------------- src/passa/internals/specifiers.py | 1 - 2 files changed, 33 insertions(+), 24 deletions(-) diff --git a/src/passa/internals/markers.py b/src/passa/internals/markers.py index 6b175df..8274f1d 100644 --- a/src/passa/internals/markers.py +++ b/src/passa/internals/markers.py @@ -16,9 +16,9 @@ from backports.functools_lru_cache import lru_cache -def _ensure_marker(marker): - if not isinstance(marker, _Marker): - return _Marker(str(marker)) +def _ensure_marker(marker, cls=_Marker): + if not isinstance(marker, cls): + return cls(str(marker)) return marker @@ -260,26 +260,36 @@ def __init__(self, marker=None): super(Marker, self).__init__(marker) def __and__(self, other): - other = _ensure_marker(other) - if not self._markers or self == other: - return type(self)(str(other)) - lhs, rhs = str(self), str(other) - # When combining markers with 'and', wrap the part with parentheses - # if there exist 'or's. - if 'or' in self._markers: - lhs = '({})'.format(str(self)) - if 'or' in other._markers: - rhs = '({})'.format(str(other)) - new_marker = type(self)("{} and {}".format(lhs, rhs)) - return new_marker + other = _ensure_marker(other, Marker) + if self == other: + marker_string = str(self) + elif not self: + marker_string = str(other) + elif not other: + marker_string = str(self) + else: + lhs, rhs = str(self), str(other) + # When combining markers with 'and', wrap the part with parentheses + # if there exist 'or's. + if 'or' in self._markers: + lhs = '({})'.format(str(self)) + if 'or' in other._markers: + rhs = '({})'.format(str(other)) + marker_string = "{} and {}".format(lhs, rhs) + return type(self)(marker_string) def __or__(self, other): - other = _ensure_marker(other) - if not self._markers or self == other: - return type(self)(str(other)) - # Just join parts with 'or' since it has the lowest priority. - new_marker = type(self)("{} or {}".format(str(self), str(other))) - return new_marker + other = _ensure_marker(other, Marker) + if self == other: + marker_string = str(self) + elif not self: + marker_string = str(other) + elif not other: + marker_string = str(self) + else: + # Just join parts with 'or' since it has the lowest priority. + marker_string = "{} or {}".format(str(self), str(other)) + return type(self)(marker_string) def __bool__(self): return bool(self._markers) @@ -293,7 +303,7 @@ def __hash__(self): def __eq__(self, other): if not isinstance(other, _Marker): return False - return self._markers == other._markers + return str(self) == str(other) def __lt__(self, other): - return self._markers < other._markers + return hash(self) < hash(other) diff --git a/src/passa/internals/specifiers.py b/src/passa/internals/specifiers.py index 58a38e7..eed204f 100644 --- a/src/passa/internals/specifiers.py +++ b/src/passa/internals/specifiers.py @@ -88,7 +88,6 @@ def _get_specs(specset): return result -@lru_cache(maxsize=128) def _group_by_op(specs): specs = [_get_specs(x) for x in list(specs)] flattened = [(op, version) for spec in specs for op, version in spec] From 38472b2f10581ad0917d5b7832b4fa3b6b0c3457 Mon Sep 17 00:00:00 2001 From: frostming Date: Thu, 3 Oct 2019 09:54:43 +0800 Subject: [PATCH 5/5] Use internal marker --- src/passa/models/metadata.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/passa/models/metadata.py b/src/passa/models/metadata.py index 9137852..daf9d7d 100644 --- a/src/passa/models/metadata.py +++ b/src/passa/models/metadata.py @@ -5,7 +5,6 @@ import copy import operator -import packaging.markers import packaging.specifiers import vistir import vistir.misc @@ -175,7 +174,7 @@ def _format_metasets(metasets): if not combined_metaset: return "" # This extra str(Marker()) call helps simplify the expression. - metaset_string = str(packaging.markers.Marker(combined_metaset)) + metaset_string = str(Marker(combined_metaset)) return metaset_string