Skip to content

Cflags requirements: regression fix#40639

Merged
alalazo merged 3 commits intospack:developfrom
scheibelp:bugfix/cflags-requires2
Oct 31, 2023
Merged

Cflags requirements: regression fix#40639
alalazo merged 3 commits intospack:developfrom
scheibelp:bugfix/cflags-requires2

Conversation

@scheibelp
Copy link
Copy Markdown
Member

@scheibelp scheibelp commented Oct 20, 2023

fixes #39671

Reapply targeted changes from #39880

Based on discussion there, this logic is due for some refactoring. It would be good to have a fix in for the 0.21 release though (and refactoring might take longer).

@spackbot-app spackbot-app bot added core PR affects Spack core functionality new-version tests General test capability(ies) virtual-dependencies labels Oct 20, 2023
@scheibelp
Copy link
Copy Markdown
Member Author

Items meriting refactor that are longer-term (not addressed here):

  • cyclic impose/condition is confusing to read for requirement groups
  • separate numbering system for requirement groups and impose/condition (which overlaps)

@scheibelp scheibelp requested a review from alalazo October 23, 2023 18:03
Copy link
Copy Markdown
Member

@alalazo alalazo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix looks good to me, but the unit test doesn't fail on develop (for the same reason the previous test didn't catch the bug). I suggest using the mock repo instead:

diff --git a/lib/spack/spack/test/concretize_requirements.py b/lib/spack/spack/test/concretize_requirements.py
index f3dad1a195..51747dd992 100644
--- a/lib/spack/spack/test/concretize_requirements.py
+++ b/lib/spack/spack/test/concretize_requirements.py
@@ -91,25 +91,6 @@ class U(Package):
 )
 
 
-_pkgw = (
-    "w",
-    """\
-class W(Package):
-    provides('virtual-w')
-    version('1.0')
-""",
-)
-
-
-_virtualw = (
-    "virtual-w",
-    """\
-class VirtualW(Package):
-    virtual = True
-""",
-)
-
-
 @pytest.fixture
 def create_test_repo(tmpdir, mutable_config):
     repo_path = str(tmpdir)
@@ -123,7 +104,7 @@ def create_test_repo(tmpdir, mutable_config):
         )
 
     packages_dir = tmpdir.join("packages")
-    for pkg_name, pkg_str in [_pkgx, _pkgy, _pkgv, _pkgt, _pkgu, _pkgw]:
+    for pkg_name, pkg_str in [_pkgx, _pkgy, _pkgv, _pkgt, _pkgu]:
         pkg_dir = packages_dir.ensure(pkg_name, dir=True)
         pkg_file = pkg_dir.join("package.py")
         with open(str(pkg_file), "w") as f:
@@ -488,25 +469,25 @@ def test_one_package_multiple_oneof_groups(concretize_scope, test_repo):
 
 
 @pytest.mark.regression("34241")
-def test_require_cflags(concretize_scope, test_repo):
+def test_require_cflags(concretize_scope, mock_packages):
     """Ensures that flags can be required from configuration."""
     conf_str = """\
 packages:
   all:
     providers:
-      virtual-w: [w]
-  y:
+      mpi: [mpich]
+  mpich2:
     require: cflags="-g"
-  virtual-w:
-    require: w cflags="-O1"
+  mpi:
+    require: mpich cflags="-O1"
 """
     update_packages_config(conf_str)
 
-    spec_y = Spec("y").concretized()
+    spec_y = Spec("mpich2").concretized()
     assert spec_y.satisfies("cflags=-g")
 
-    spec_w = Spec("virtual-w").concretized()
-    assert spec_w.satisfies("w cflags=-O1")
+    spec_w = Spec("mpi").concretized()
+    assert spec_w.satisfies("mpich cflags=-O1")
 
 
 def test_requirements_for_package_that_is_not_needed(concretize_scope, test_repo):

I'll be posting benchmark results next

@alalazo alalazo self-assigned this Oct 31, 2023
@alalazo
Copy link
Copy Markdown
Member

alalazo commented Oct 31, 2023

Confirmed that there is no impact on performance.

radiuss-develop.csv
radiuss-pr.csv
radiuss.txt

radiuss

Tested on:

  • Spack: 0.21.0.dev0 (160bfd8)
  • Python: 3.11.5
  • Platform: linux-ubuntu20.04-icelake
  • Concretizer: clingo

… without the fix, although is fragile in the same way)
@alalazo alalazo merged commit 2f2d9ae into spack:develop Oct 31, 2023
@alalazo
Copy link
Copy Markdown
Member

alalazo commented Oct 31, 2023

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core PR affects Spack core functionality new-version tests General test capability(ies) virtual-dependencies

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Compiler flags in spec requirements regression

2 participants