cmake build system: make "generator" a variant by alalazo · Pull Request #35552 · spack/spack

alalazo · 2023-02-17T18:42:01Z

This PR make generator a variant in CMakePackage. This allows us to declare proper dependencies on ninja and gmake, and to select generators at concretization time where it makes sense.

Modifications:

Add a cmake specific generator directive, that builds on variants and conflicts
Modify CMakePackages to use that directive

alalazo · 2023-02-17T18:45:45Z

lib/spack/spack/build_systems/cmake.py

+    allowed_values = ("make", "ninja")
+    if any(x not in allowed_values for x in names):
+        msg = "only 'make' and 'ninja' are allowed for CMake's 'generator' directive"
+        raise ValueError(msg)
+
+    default = default or names[0]
+    not_used = [x for x in allowed_values if x not in names]
+
+    def _values(x):
+        return x in allowed_values
+
+    _values.__doc__ = f"{','.join(names)}"
+
+    variant(
+        "generator",
+        default=default,
+        values=_values,
+        description="the build system generator to use",
+    )
+    for x in not_used:
+        conflicts(f"generator={x}")


This works fine, but can probably cleaned a bit once we have the drop directive from #35045

adamjstewart · 2023-02-17T20:08:48Z

The only reason we didn't do this originally was because the thing getting installed (shouldn't) change, so we didn't want to include it in the hash. But with concretizer reuse the default, maybe this doesn't matter as much.

However, the current implementation is more "make generator a directive" than "make generator a variant". The packages that set generator = "Ninja" aren't doing it because only Ninja works, they're doing it because it's faster. I'm in favor of a normal variant at the base class level, and packages shouldn't override the default. I think we should default to Ninja and only override the variant in packages that won't build with Ninja.

alalazo · 2023-02-17T20:37:46Z

However, the current implementation is more "make generator a directive" than "make generator a variant".

If we have packages that only work with make or only work with ninja we want their generator to be the default, and the other one to be disallowed. That currently means a combination of variant and conflicts, and as soon as #35405 gets in of variant and drop.

The issue are the checks we do on conditional dependencies like:

depends_on("gmake", when="generator=make")

that would raise when only ninja is allowed. The generator wrapper is really a convenience for calling 2 directives with most arguments fixed.

Also forgot to mention that a added benefit is that now the dependency on gmake or ninja is explicit, so I think I can remove a few more directives in packages.

adamjstewart · 2023-02-17T20:42:23Z

Oh, so you're saying if the base class contains:

variant("generator", values=["make", "ninja"])
depends_on("ninja", when="generator=ninja")

and a subclass contains:

variant("generator", values=["make"])

the subclass will raise audit issues because generator=ninja is invalid? That's annoying.

alalazo · 2023-02-17T20:44:02Z

will raise audit issues because generator=ninja is invalid? That's annoying.

Actually it won't concretize too, because we do consistency checks also during concretization 😅

kwryankrattiger · 2023-02-17T22:18:05Z

This is much improved over the current model! Thanks!

alalazo · 2023-02-28T09:44:03Z

@spackbot run pipeline

spackbot-app · 2023-02-28T09:44:11Z

I've started that pipeline for you!

adamjstewart · 2023-03-01T17:38:15Z

In order to avoid the aforementioned concretizer/audit check issue, could we instead use:

variant("generator", values=["make", "ninja"])
depends_on("ninja", when="generator=ninja")

in the base class and:

conflicts("generator=ninja")

in subclasses that only support make? Then we don't need to add a whole new directive just for this.

alalazo · 2023-03-07T17:32:17Z

@adamjstewart For reasons related to clingo it's better if packages that only support Ninja have ninja as the default generator. That means the packages also need to override the variant. Hence, having a declarative way of setting a conflict and declaring a variant while providing only the information that is necessary.

adamjstewart · 2023-03-07T17:39:31Z

For reasons related to clingo

This doesn't tell me a lot, but I trust you. Just trying to avoid adding more and more hyper-specific directives if we don't need them.

alalazo · 2023-03-07T17:45:45Z

This doesn't tell me a lot, but I trust you. Just trying to avoid adding more and more hyper-specific directives if we don't need them.

If we don't make ninja the default, the only value possible would indeed be ninja - but with a penalty in the optimization. That can steer clingo towards weird solutions.

alalazo · 2023-03-17T08:36:45Z

@spackbot run pipeline

spackbot-app · 2023-03-17T08:36:55Z

I've started that pipeline for you!

alalazo · 2023-03-17T13:31:03Z

@spackbot run pipeline

spackbot-app · 2023-03-17T13:31:11Z

I've started that pipeline for you!

haampie · 2023-03-17T17:18:35Z

lib/spack/spack/test/build_systems.py

        with pytest.raises(spack.package_base.InstallError):
-            s.package.builder.std_cmake_args
+            spack.build_systems.cmake.CMakeBuilder.std_args(
+                s.package, generator="Yellow Sticky Notes"


haampie · 2023-03-17T17:35:49Z

lib/spack/spack/build_systems/cmake.py

+
        depends_on("cmake", type="build")
-        depends_on("ninja", type="build", when="platform=windows")
+        depends_on("gmake", type="build", when="generator=make")


I approve of this!

$ spack spec gmake +guile | grep cmake

has nothing on current develop, so it shouldn't break "advanced" gmake builds. gmake ~guile doesn't have deps.

haampie

I grepped for gmake build deps among packages of which there's only two cmake packages that specify a bound on it, so that looks fine. Ninja I think you got right too

alalazo commented Feb 17, 2023

View reviewed changes

alalazo added cmake build-systems labels Feb 17, 2023

spackbot-app bot added conflicts core PR affects Spack core functionality dependencies new-variant new-version python tests General test capability(ies) update-package labels Feb 17, 2023

alalazo force-pushed the features/generator_variant branch from 88bdc67 to 7415f59 Compare February 17, 2023 21:35

alalazo requested review from adamjstewart and haampie February 17, 2023 21:44

alalazo force-pushed the features/generator_variant branch from 7415f59 to e4b7561 Compare March 1, 2023 09:54

alalazo force-pushed the features/generator_variant branch 2 times, most recently from 3c9e0e2 to 4cd497f Compare March 8, 2023 12:08

alalazo added 2 commits March 16, 2023 13:54

cmake build system: make "generator" a variant

34fd54f

Cleanup packages

679859c

alalazo force-pushed the features/generator_variant branch from 4cd497f to 679859c Compare March 16, 2023 13:10

haampie reviewed Mar 17, 2023

View reviewed changes

alalazo mentioned this pull request Mar 17, 2023

Add a new drop directive for extending packages #35045

Closed

4 tasks

haampie approved these changes Mar 18, 2023

View reviewed changes

haampie merged commit d84c6ad into spack:develop Mar 18, 2023

alalazo deleted the features/generator_variant branch March 20, 2023 22:07

alalazo mentioned this pull request Apr 28, 2023

steps: add version 4.1.1, remove others #37250

Merged

alalazo mentioned this pull request Jul 10, 2023

Builtin make on macOS often does not work with any -j besides -j1 (Permission denied) #26669

Closed

3 tasks

yizeyi18 mentioned this pull request Jul 10, 2025

CmakePackages: add build_generator option #22994

Closed

Conversation

alalazo commented Feb 17, 2023

Uh oh!

alalazo Feb 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamjstewart commented Feb 17, 2023

Uh oh!

alalazo commented Feb 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamjstewart commented Feb 17, 2023

Uh oh!

alalazo commented Feb 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kwryankrattiger commented Feb 17, 2023

Uh oh!

alalazo commented Feb 28, 2023

Uh oh!

spackbot-app bot commented Feb 28, 2023

Uh oh!

adamjstewart commented Mar 1, 2023

Uh oh!

alalazo commented Mar 7, 2023

Uh oh!

adamjstewart commented Mar 7, 2023

Uh oh!

alalazo commented Mar 7, 2023

Uh oh!

alalazo commented Mar 17, 2023

Uh oh!

spackbot-app bot commented Mar 17, 2023

Uh oh!

alalazo commented Mar 17, 2023

Uh oh!

spackbot-app bot commented Mar 17, 2023

Uh oh!

haampie Mar 17, 2023

Choose a reason for hiding this comment

Uh oh!

haampie Mar 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haampie left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

alalazo Feb 17, 2023 •

edited

Loading

alalazo commented Feb 17, 2023 •

edited

Loading

alalazo commented Feb 17, 2023 •

edited

Loading

haampie Mar 17, 2023 •

edited

Loading