Skip to content

Commit

Permalink
[sonic-package-manager] switch from poetry-semver to semantic_version…
Browse files Browse the repository at this point in the history
… due to bugs found in poetry-semver (sonic-net#1710)

#### What I did

I replaced underneeth a library which I used for semver functionality.

#### How I did it

Tried to keep existing Version/VersionConstraint API but replaced library used underneeth.
I also added two UT that failed with poetry-semver and show the motivation for this change.

UT description:

- ```test_invalid_version```

This test is to verify that **invalid** semantic version strings are rejected with exception by ```Version.parse```.
E.g. "1.2.3-0123" is not a valid semantic version due to this (https://semver.org/#spec-item-9):
```
A pre-release version MAY be denoted by appending a hyphen and a series of dot separated identifiers immediately following the patch version. Identifiers MUST comprise only ASCII alphanumerics and hyphens [0-9A-Za-z-]. Identifiers MUST NOT be empty. Numeric identifiers MUST NOT include leading zeroes
```

- ```test_version_comparison```

This test checks whether the implementation correctly compare two version strings (this is needed due to the logic that checks if we are upgrading or downgrading). According to https://semver.org/#spec-item-11, "1.0.0-alpha" < "1.0.0".

#### How to verify it

Run UT, run some basic sonic-package-manager commands.
  • Loading branch information
stepanblyschak committed Aug 17, 2021
1 parent 7821a3f commit 2c7cfaa
Show file tree
Hide file tree
Showing 9 changed files with 162 additions and 39 deletions.
2 changes: 1 addition & 1 deletion setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@
'netaddr>=0.8.0',
'netifaces>=0.10.7',
'pexpect>=4.8.0',
'poetry-semver>=0.1.0',
'semantic-version>=2.8.5',
'prettyprinter>=0.18.0',
'pyroute2>=0.5.14, <0.6.1',
'requests>=2.25.0',
Expand Down
63 changes: 56 additions & 7 deletions sonic_package_manager/constraint.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,26 @@
""" Package version constraints module. """

import re
from abc import ABC
from dataclasses import dataclass, field
from typing import Dict, Union

import semver
import semantic_version

from sonic_package_manager.version import Version

class VersionConstraint(semver.VersionConstraint, ABC):
""" Extends VersionConstraint from semver package. """

@staticmethod
def parse(constraint_expression: str) -> 'VersionConstraint':
class VersionConstraint:
""" Version constraint representation. """

def __init__(self, *args, **kwargs):
self._constraint = semantic_version.SimpleSpec(*args, **kwargs)

@property
def expression(self):
return self._constraint.expression

@classmethod
def parse(cls, constraint_expression: str) -> 'VersionConstraint':
""" Parse version constraint.
Args:
Expand All @@ -23,7 +31,48 @@ def parse(constraint_expression: str) -> 'VersionConstraint':
The resulting VersionConstraint object.
"""

return semver.parse_constraint(constraint_expression)
return cls(constraint_expression)

def allows(self, version: Version) -> bool:
""" Checks if other version is allowed by this constraint
Args:
version: Version to check against this constraint.
Returns:
Boolean wether this constraint allows version.
"""

return self._constraint.match(version._version)

def is_exact(self) -> bool:
""" Is the version constraint exact, meaning only one version is allowed.
Returns:
Boolean wether this constraint is exact.
"""

clause = self._constraint.clause
return hasattr(clause, 'target') and clause.operator == '=='

def get_exact_version(self) -> Version:
""" Returns an exact version for this constraint if it is exact constraint.
Returns:
Exact version in case this constraint is exact.
Raises:
AttributeError: when constraint is not exact
"""

return self._constraint.clause.target

def __str__(self):
return self._constraint.__str__()

def __repr__(self):
return self._constraint.__repr__()

def __eq__(self, other):
return self._constraint.__eq__(other._constraint)


@dataclass
Expand Down
18 changes: 9 additions & 9 deletions sonic_package_manager/manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,6 @@
from sonic_package_manager.utils import DockerReference
from sonic_package_manager.version import (
Version,
VersionRange,
version_to_tag,
tag_to_version
)
Expand Down Expand Up @@ -122,13 +121,14 @@ def package_constraint_to_reference(constraint: PackageConstraint) -> PackageRef
# Allow only specific version for now.
# Later we can improve package manager to support
# installing packages using expressions like 'package>1.0.0'
if version_constraint == VersionRange(): # empty range means any version
if version_constraint.expression == '*':
return PackageReference(package_name, None)
if not isinstance(version_constraint, Version):
if not version_constraint.is_exact():
raise PackageManagerError(f'Can only install specific version. '
f'Use only following expression "{package_name}=<version>" '
f'to install specific version')
return PackageReference(package_name, version_to_tag(version_constraint))
version = version_constraint.get_exact_version()
return PackageReference(package_name, version_to_tag(version))


def parse_reference_expression(expression):
Expand Down Expand Up @@ -156,7 +156,7 @@ def validate_package_base_os_constraints(package: Package, sonic_version_info: D

version = Version.parse(sonic_version_info[component])

if not constraint.allows_all(version):
if not constraint.allows(version):
raise PackageSonicRequirementError(package.name, component, constraint, version)


Expand All @@ -178,7 +178,7 @@ def validate_package_tree(packages: Dict[str, Package]):

installed_version = dependency_package.version
log.debug(f'dependency package is installed {dependency.name}: {installed_version}')
if not dependency.constraint.allows_all(installed_version):
if not dependency.constraint.allows(installed_version):
raise PackageDependencyError(package.name, dependency, installed_version)

dependency_components = dependency.components
Expand All @@ -197,7 +197,7 @@ def validate_package_tree(packages: Dict[str, Package]):
log.debug(f'dependency package {dependency.name}: '
f'component {component} version is {component_version}')

if not constraint.allows_all(component_version):
if not constraint.allows(component_version):
raise PackageComponentDependencyError(package.name, dependency, component,
constraint, component_version)

Expand All @@ -209,7 +209,7 @@ def validate_package_tree(packages: Dict[str, Package]):

installed_version = conflicting_package.version
log.debug(f'conflicting package is installed {conflict.name}: {installed_version}')
if conflict.constraint.allows_all(installed_version):
if conflict.constraint.allows(installed_version):
raise PackageConflictError(package.name, conflict, installed_version)

for component, constraint in conflicting_package.components.items():
Expand All @@ -220,7 +220,7 @@ def validate_package_tree(packages: Dict[str, Package]):
log.debug(f'conflicting package {dependency.name}: '
f'component {component} version is {component_version}')

if constraint.allows_all(component_version):
if constraint.allows(component_version):
raise PackageComponentConflictError(package.name, dependency, component,
constraint, component_version)

Expand Down
2 changes: 1 addition & 1 deletion sonic_package_manager/manifest.py
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ def unmarshal(self, value):

# TODO: add description for each field
SCHEMA = ManifestRoot('root', [
ManifestField('version', ParsedMarshaller(Version), Version(1, 0, 0)),
ManifestField('version', ParsedMarshaller(Version), Version.parse('1.0.0')),
ManifestRoot('package', [
ManifestField('version', ParsedMarshaller(Version)),
ManifestField('name', DefaultMarshaller(str)),
Expand Down
66 changes: 63 additions & 3 deletions sonic_package_manager/version.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,70 @@

""" Version and helpers routines. """

import semver
import semantic_version

Version = semver.Version
VersionRange = semver.VersionRange

class Version:
""" Version class represents SemVer 2.0 compliant version """

@classmethod
def parse(cls, version_string: str) -> 'Version':
""" Construct Version from version_string.
Args:
version_string: SemVer compatible version string.
Returns:
Version object.
Raises:
ValueError: when version_string does not follow SemVer.
"""

return cls(version_string)

def __init__(self, *args, **kwargs):
self._version = semantic_version.Version(*args, **kwargs)

@property
def major(self):
return self._version.major

@property
def minor(self):
return self._version.minor

@property
def patch(self):
return self._version.patch

def __str__(self):
return self._version.__str__()

def __repr__(self):
return self._version.__repr__()

def __iter__(self):
return self._version.__iter__()

def __cmp__(self, other):
return self._version.__cmp__(other._version)

def __eq__(self, other):
return self._version.__eq__(other._version)

def __ne__(self, other):
return self._version.__ne__(other._version)

def __lt__(self, other):
return self._version.__lt__(other._version)

def __le__(self, other):
return self._version.__le__(other._version)

def __gt__(self, other):
return self._version.__gt__(other._version)

def __ge__(self, other):
return self._version.__ge__(other._version)


def version_to_tag(ver: Version) -> str:
Expand Down
22 changes: 18 additions & 4 deletions tests/sonic_package_manager/test_constraint.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,22 @@
#!/usr/bin/env python

import pytest

from sonic_package_manager import version
from sonic_package_manager.constraint import PackageConstraint
from sonic_package_manager.version import Version, VersionRange
from sonic_package_manager.constraint import PackageConstraint, VersionConstraint
from sonic_package_manager.version import Version


@pytest.mark.parametrize('invalid_version', ['1.2.3-0123', '1.2', '1.0.0+artiary+version'])
def test_invalid_version(invalid_version):
with pytest.raises(Exception):
Version.parse(invalid_version)


@pytest.mark.parametrize(('newer', 'older'),
[('0.1.1', '0.1.1-alpha')])
def test_version_comparison(newer, older):
assert Version.parse(newer) > Version.parse(older)


def test_constraint():
Expand All @@ -28,7 +42,7 @@ def test_constraint_strict():


def test_constraint_match():
package_constraint = PackageConstraint.parse('swss==1.2*.*')
package_constraint = PackageConstraint.parse('swss==1.2.*')
assert package_constraint.name == 'swss'
assert not package_constraint.constraint.allows(Version.parse('1.1.1'))
assert package_constraint.constraint.allows(Version.parse('1.2.0'))
Expand All @@ -47,7 +61,7 @@ def test_constraint_multiple():
def test_constraint_only_name():
package_constraint = PackageConstraint.parse('swss')
assert package_constraint.name == 'swss'
assert package_constraint.constraint == VersionRange()
assert package_constraint.constraint == VersionConstraint('*')


def test_constraint_from_dict():
Expand Down
6 changes: 3 additions & 3 deletions tests/sonic_package_manager/test_database.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ def test_database_get_package(fake_db):
assert swss_package.built_in
assert swss_package.repository == 'docker-orchagent'
assert swss_package.default_reference == '1.0.0'
assert swss_package.version == Version(1, 0, 0)
assert swss_package.version == Version.parse('1.0.0')


def test_database_get_package_not_builtin(fake_db):
Expand Down Expand Up @@ -52,11 +52,11 @@ def test_database_add_package_existing(fake_db):
def test_database_update_package(fake_db):
test_package = fake_db.get_package('test-package-2')
test_package.installed = True
test_package.version = Version(1, 2, 3)
test_package.version = Version.parse('1.2.3')
fake_db.update_package(test_package)
test_package = fake_db.get_package('test-package-2')
assert test_package.installed
assert test_package.version == Version(1, 2, 3)
assert test_package.version == Version.parse('1.2.3')


def test_database_update_package_non_existing(fake_db):
Expand Down
21 changes: 11 additions & 10 deletions tests/sonic_package_manager/test_manager.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#!/usr/bin/env python

import re
from unittest.mock import Mock, call

import pytest
Expand All @@ -26,8 +27,8 @@ def test_installation_dependencies(package_manager, fake_metadata_resolver, mock
manifest = fake_metadata_resolver.metadata_store['Azure/docker-test']['1.6.0']['manifest']
manifest['package']['depends'] = ['swss^2.0.0']
with pytest.raises(PackageInstallationError,
match='Package test-package requires swss>=2.0.0,<3.0.0 '
'but version 1.0.0 is installed'):
match=re.escape('Package test-package requires swss^2.0.0 '
'but version 1.0.0 is installed')):
package_manager.install('test-package')


Expand Down Expand Up @@ -80,8 +81,8 @@ def test_installation_components_dependencies_not_satisfied(package_manager, fak
},
]
with pytest.raises(PackageInstallationError,
match='Package test-package requires libswsscommon >=1.1.0,<2.0.0 '
'in package swss>=1.0.0 but version 1.0.0 is installed'):
match=re.escape('Package test-package requires libswsscommon ^1.1.0 '
'in package swss>=1.0.0 but version 1.0.0 is installed')):
package_manager.install('test-package')


Expand All @@ -98,8 +99,8 @@ def test_installation_components_dependencies_implicit(package_manager, fake_met
},
]
with pytest.raises(PackageInstallationError,
match='Package test-package requires libswsscommon >=2.1.0,<3.0.0 '
'in package swss>=1.0.0 but version 1.0.0 is installed'):
match=re.escape('Package test-package requires libswsscommon ^2.1.0 '
'in package swss>=1.0.0 but version 1.0.0 is installed')):
package_manager.install('test-package')


Expand All @@ -125,8 +126,8 @@ def test_installation_breaks(package_manager, fake_metadata_resolver):
manifest = fake_metadata_resolver.metadata_store['Azure/docker-test']['1.6.0']['manifest']
manifest['package']['breaks'] = ['swss^1.0.0']
with pytest.raises(PackageInstallationError,
match='Package test-package conflicts with '
'swss>=1.0.0,<2.0.0 but version 1.0.0 is installed'):
match=re.escape('Package test-package conflicts with '
'swss^1.0.0 but version 1.0.0 is installed')):
package_manager.install('test-package')


Expand Down Expand Up @@ -294,7 +295,7 @@ def test_manager_upgrade(package_manager, sonic_fs):

package_manager.install('test-package-6=2.0.0')
upgraded_package = package_manager.get_installed_package('test-package-6')
assert upgraded_package.entry.version == Version(2, 0, 0)
assert upgraded_package.entry.version == Version.parse('2.0.0')
assert upgraded_package.entry.default_reference == package.entry.default_reference


Expand All @@ -304,7 +305,7 @@ def test_manager_package_reset(package_manager, sonic_fs):

package_manager.reset('test-package-6')
upgraded_package = package_manager.get_installed_package('test-package-6')
assert upgraded_package.entry.version == Version(1, 5, 0)
assert upgraded_package.entry.version == Version.parse('1.5.0')


def test_manager_migration(package_manager, fake_db_for_migration):
Expand Down
1 change: 0 additions & 1 deletion tests/sonic_package_manager/test_manifest.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@

from sonic_package_manager.constraint import ComponentConstraints
from sonic_package_manager.manifest import Manifest, ManifestError
from sonic_package_manager.version import VersionRange


def test_manifest_v1_defaults():
Expand Down

0 comments on commit 2c7cfaa

Please sign in to comment.