From e9ee9eaf500c052910b77d2c428ec88574f283be Mon Sep 17 00:00:00 2001
From: Peter Scheibel <scheibel1@llnl.gov>
Date: Wed, 4 Dec 2019 23:27:08 -0700
Subject: patching: do strict version range checking (#13989)

* apply strict constraint checks for patches, otherwise Spack may incorrectly treat a version range constraint as satisfied when mixing x.y and x.y.z versions
* add mixed version checks to version comparison tests
---
 lib/spack/spack/spec.py                                |  4 ++--
 lib/spack/spack/test/patch.py                          | 15 +++++++++++++++
 lib/spack/spack/test/versions.py                       |  2 ++
 var/spack/repos/builtin.mock/packages/patch/biz.patch  |  1 +
 var/spack/repos/builtin.mock/packages/patch/package.py |  3 +++
 5 files changed, 23 insertions(+), 2 deletions(-)
 create mode 100644 var/spack/repos/builtin.mock/packages/patch/biz.patch

diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py
index 050f027679..6a16a308db 100644
--- a/lib/spack/spack/spec.py
+++ b/lib/spack/spack/spec.py
@@ -2182,7 +2182,7 @@ class Spec(object):
             # Add any patches from the package to the spec.
             patches = []
             for cond, patch_list in s.package_class.patches.items():
-                if s.satisfies(cond):
+                if s.satisfies(cond, strict=True):
                     for patch in patch_list:
                         patches.append(patch)
             if patches:
@@ -2201,7 +2201,7 @@ class Spec(object):
 
             patches = []
             for cond, dependency in pkg_deps[dspec.spec.name].items():
-                if dspec.parent.satisfies(cond):
+                if dspec.parent.satisfies(cond, strict=True):
                     for pcond, patch_list in dependency.patches.items():
                         if dspec.spec.satisfies(pcond):
                             for patch in patch_list:
diff --git a/lib/spack/spack/test/patch.py b/lib/spack/spack/test/patch.py
index c3df33dc8b..aae0687ec1 100644
--- a/lib/spack/spack/test/patch.py
+++ b/lib/spack/spack/test/patch.py
@@ -24,6 +24,7 @@ from spack.spec import Spec
 foo_sha256 = 'b5bb9d8014a0f9b1d61e21e796d78dccdf1352f23cd32812f4850b878ae4944c'
 bar_sha256 = '7d865e959b2466918c9863afca942d0fb89d7c9ac0c99bafc3749504ded97730'
 baz_sha256 = 'bf07a7fbb825fc0aae7bf4a1177b2b31fcf8a3feeaf7092761e18c859ee52a9c'
+biz_sha256 = 'a69b288d7393261e613c276c6d38a01461028291f6e381623acc58139d01f54d'
 
 # url patches
 url1_sha256 = 'abcd1234abcd1234abcd1234abcd1234abcd1234abcd1234abcd1234abcd1234'
@@ -105,6 +106,20 @@ def test_patch_in_spec(mock_packages, config):
             tuple(spec.variants['patches']._patches_in_order_of_appearance))
 
 
+def test_patch_mixed_versions_subset_constraint(mock_packages, config):
+    """If we have a package with mixed x.y and x.y.z versions, make sure that
+       a patch applied to a version range of x.y.z versions is not applied to
+       an x.y version.
+    """
+    spec1 = Spec('patch@1.0.1')
+    spec1.concretize()
+    assert biz_sha256 in spec1.variants['patches'].value
+
+    spec2 = Spec('patch@1.0')
+    spec2.concretize()
+    assert biz_sha256 not in spec2.variants['patches'].value
+
+
 def test_patch_order(mock_packages, config):
     spec = Spec('dep-diamond-patch-top')
     spec.concretize()
diff --git a/lib/spack/spack/test/versions.py b/lib/spack/spack/test/versions.py
index b39da0c698..60e4497a8e 100644
--- a/lib/spack/spack/test/versions.py
+++ b/lib/spack/spack/test/versions.py
@@ -266,6 +266,8 @@ def test_contains():
     assert_in('1.3.5-7', '1.2:1.4')
     assert_not_in('1.1', '1.2:1.4')
     assert_not_in('1.5', '1.2:1.4')
+    assert_not_in('1.5', '1.5.1:1.6')
+    assert_not_in('1.5', '1.5.1:')
 
     assert_in('1.4.2', '1.2:1.4')
     assert_not_in('1.4.2', '1.2:1.4.0')
diff --git a/var/spack/repos/builtin.mock/packages/patch/biz.patch b/var/spack/repos/builtin.mock/packages/patch/biz.patch
new file mode 100644
index 0000000000..71a8a61460
--- /dev/null
+++ b/var/spack/repos/builtin.mock/packages/patch/biz.patch
@@ -0,0 +1 @@
+this patch is never applied, it is used to check spec semantics on when the concretizer chooses to include a patch
diff --git a/var/spack/repos/builtin.mock/packages/patch/package.py b/var/spack/repos/builtin.mock/packages/patch/package.py
index 37ea72fb1a..e6c8b33dcf 100644
--- a/var/spack/repos/builtin.mock/packages/patch/package.py
+++ b/var/spack/repos/builtin.mock/packages/patch/package.py
@@ -13,11 +13,14 @@ class Patch(Package):
     url = "http://www.example.com/patch-1.0.tar.gz"
 
     version('1.0', '0123456789abcdef0123456789abcdef')
+    version('1.0.1')
+    version('1.0.2')
     version('2.0', '0123456789abcdef0123456789abcdef')
 
     patch('foo.patch')
     patch('bar.patch', when='@2:')
     patch('baz.patch')
+    patch('biz.patch', when='@1.0.1:1.0.2')
 
     def install(self, spec, prefix):
         pass
-- 
cgit v1.2.3-70-g09d2