Repository: matze/pkgconfig

Issue:
openssl version parsing error
pkgconfig crashes while checking the openssl version number:
```
>>> pkgconfig.installed('openssl', '>= 1.1.0')
Traceback (most recent call last):
  File "/home/user/w/borg-env/lib/python3.5/site-packages/pkgconfig/pkgconfig.py", line 161, in installed
    result = _compare_versions(modversion, number)
  File "/home/user/w/borg-env/lib/python3.5/site-packages/pkgconfig/pkgconfig.py", line 44, in _compare_versions
    n1 = normalize(v1)
  File "/home/user/w/borg-env/lib/python3.5/site-packages/pkgconfig/pkgconfig.py", line 42, in normalize
    return [int(x) for x in re.sub(r'(\.0+)*$', '', v).split(".")]
  File "/home/user/w/borg-env/lib/python3.5/site-packages/pkgconfig/pkgconfig.py", line 42, in <listcomp>
    return [int(x) for x in re.sub(r'(\.0+)*$', '', v).split(".")]
ValueError: invalid literal for int() with base 10: '0j'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/user/w/borg-env/lib/python3.5/site-packages/pkgconfig/pkgconfig.py", line 164, in installed
    raise ValueError(msg)
ValueError: >= 1.1.0 is not a correct version specifier
```

It's on Debian 9:
```
ii  openssl                              1.1.0j-1~deb9u1                   amd64        Secure Sockets Layer toolkit - cryptographic utility
```

Note: openssl 1.1 is incompatible to 1.0, so while the check for a specific update release (like `j`) is maybe not that important (except for security fixes, early bug fixes), the check for 1.1 vs. 1.0 is fundamentally important.

Discussion:
ThomasWaldmann: hmm, travis seemed rather unhappy. added a test for openssl (fake) package existing.

locally on my machine the tests worked for the 1st changeset.
ThomasWaldmann: OK, seems like it does not test against the `fake-openssl.pc` I put into `data/`:
```
>       assert pkgconfig.modversion('openssl') == '1.1.0j'
mE       AssertionError: assert '1.0.1f' == '1.1.0j'
E         - 1.0.1f
E         + 1.1.0j
```
ThomasWaldmann: Ah, pkg-config seems to get the package name from the `*.pc` filename, not from the `Name: ...` entry inside that file.
ThomasWaldmann: @matze have a look.

Not sure if we want to keep the 2nd and 3rd commit, otoh some more tests would not do harm.

If they are rather unwanted, I can drop them again and collapse the remaining ones.
matze: > Not sure if we want to keep the 2nd and 3rd commit, otoh some more tests would not do harm.

After adding a test for "digit letter digit" squash everything into one commit.
ThomasWaldmann: added tests and comments.

will collapse everything when it is approved by @matze.
matze: LGTM :+1:

Patch:
diff --git a/data/fake-dld-pkg.pc b/data/fake-dld-pkg.pc
new file mode 100644
index 0000000..ba5d551
--- /dev/null
+++ b/data/fake-dld-pkg.pc
@@ -0,0 +1,9 @@
+prefix=/usr
+exec_prefix=${prefix}
+libdir=${exec_prefix}/lib
+includedir=${prefix}/include
+
+Name: BetaPkg
+Description: fake package with a digit-letter-digit version number for testing
+Requires:
+Version: 1.2.3b4
diff --git a/data/fake-openssl.pc b/data/fake-openssl.pc
new file mode 100644
index 0000000..6f35813
--- /dev/null
+++ b/data/fake-openssl.pc
@@ -0,0 +1,10 @@
+prefix=/usr
+exec_prefix=${prefix}
+libdir=${exec_prefix}/lib/x86_64-linux-gnu
+includedir=${prefix}/include
+
+Name: OpenSSL
+Description: Secure Sockets Layer and cryptography libraries and tools
+Requires: libssl libcrypto
+Version: 1.1.0j
+
diff --git a/pkgconfig/pkgconfig.py b/pkgconfig/pkgconfig.py
index 3deb97f..a792a0d 100644
--- a/pkgconfig/pkgconfig.py
+++ b/pkgconfig/pkgconfig.py
@@ -35,11 +35,33 @@ def _compare_versions(v1, v2):
     Compare two version strings and return -1, 0 or 1 depending on the equality
     of the subset of matching version numbers.
 
-    The implementation is taken from the top answer at
+    The implementation is inspired by the top answer at
     http://stackoverflow.com/a/1714190/997768.
     """
     def normalize(v):
-        return [int(x) for x in re.sub(r'(\.0+)*$', '', v).split(".")]
+        # strip trailing .0 or .00 or .0.0 or ...
+        v = re.sub(r'(\.0+)*$', '', v)
+        result = []
+        for part in v.split('.'):
+            # just digits
+            m = re.match(r'^(\d+)$', part)
+            if m:
+                result.append(int(m.group(1)))
+                continue
+            # digits letters
+            m = re.match(r'^(\d+)([a-zA-Z]+)$', part)
+            if m:
+                result.append(int(m.group(1)))
+                result.append(m.group(2))
+                continue
+            # digits letters digits
+            m = re.match(r'^(\d+)([a-zA-Z]+)(\d+)$', part)
+            if m:
+                result.append(int(m.group(1)))
+                result.append(m.group(2))
+                result.append(int(m.group(3)))
+                continue
+        return tuple(result)
 
     n1 = normalize(v1)
     n2 = normalize(v2)
@@ -49,7 +71,7 @@ def _compare_versions(v1, v2):
 
 def _split_version_specifier(spec):
     """Splits version specifiers in the form ">= 0.1.2" into ('0.1.2', '>=')"""
-    m = re.search(r'([<>=]?=?)?\s*((\d*\.)*\d*)', spec)
+    m = re.search(r'([<>=]?=?)?\s*([0-9.a-zA-Z]+)', spec)
     return m.group(2), m.group(1)
 
 


Test patch:
diff --git a/test_pkgconfig.py b/test_pkgconfig.py
index 8786ff3..ea68573 100644
--- a/test_pkgconfig.py
+++ b/test_pkgconfig.py
@@ -8,7 +8,7 @@ PACKAGE_NAME = 'fake-gtk+-3.0'
 
 def test_exists():
     assert pkgconfig.exists(PACKAGE_NAME)
-
+    assert pkgconfig.exists('fake-openssl')
 
 @pytest.mark.parametrize("version,expected",[
     ('3.2.1', True),
@@ -23,8 +23,58 @@ def test_version(version, expected):
     assert pkgconfig.installed(PACKAGE_NAME, version) == expected
 
 
+@pytest.mark.parametrize("version,expected",[
+    ('1.1.0j', True),
+    ('==1.1.0j', True),
+    ('==1.1.0k', False),
+    ('>= 1.1.0', True),
+    ('> 1.2.0', False),
+    ('< 1.2.0', True),
+    ('< 1.1.0', False),
+    ('>= 1.1', True),
+    ('> 1.2', False),
+    ('< 1.2', True),
+    ('< 1.1', False),
+    ('>= 1.1.0c', True),
+    ('>= 1.1.0k', False),
+    # PLEASE NOTE:
+    # the letters have no semantics, except string ordering, see also the
+    # comment in the test below.
+    # comparing release with beta, like "1.2.3" > "1.2.3b" does not work.
+])
+def test_openssl(version, expected):
+    assert pkgconfig.installed('fake-openssl', version) == expected
+
+
+@pytest.mark.parametrize("version,expected",[
+    ('1.2.3b4', True),
+    ('==1.2.3b4', True),
+    ('==1.2.3', False),
+    ('>= 1.2.3b3', True),
+    ('< 1.2.3b5', True),
+    # PLEASE NOTE:
+    # sadly, when looking at all (esp. non-python) libraries out there, there
+    # is no agreement on the semantics of letters appended to version numbers.
+    # e.g. for a release candidate, some might use "c", but other also might
+    # use "rc" or whatever. stuff like openssl does not use the letters to
+    # represent release status, but rather minor updates using a-z.
+    # so, as there is no real standard / agreement, we can NOT assume any
+    # advanced semantics here (like we could for python packages).
+    # thus we do NOT implement any special semantics for the letters,
+    # except string ordering
+    # thus, comparing a version with a letter-digits appendix to one without
+    # may or may not give the desired result.
+    # e.g. python packages use a1 for alpha 1, b2 for beta 2, c3 for release
+    # candidate 3 and <nothing> for release.
+    # we do not implement this semantics, "1.2.3" > "1.2.3b1" does not work.
+])
+def test_dld_pkg(version, expected):
+    assert pkgconfig.installed('fake-dld-pkg', version) == expected
+
+
 def test_modversion():
     assert pkgconfig.modversion(PACKAGE_NAME) == '3.2.1'
+    assert pkgconfig.modversion('fake-openssl') == '1.1.0j'
 
 
 def test_cflags():