Updates to GL related packages #192 #2057

kwryankrattiger · 2025-10-22T16:12:23Z

Extracted updates to GL detection from #192

alalazo · 2025-10-23T08:06:56Z

repos/spack_repo/builtin/build_systems/cmake.py

+def _dependencies_cmake_extra_args(pkg: PackageBase, args: List[str]) -> None:
+    for dep in pkg.spec.traverse(deptype=("build", "link")):
+        extra_args = getattr(dep.package, "cmake_extra_args", None)
+        if extra_args:
+            args.extend(extra_args)


Hmm, do we really need something this general only for mesa? Can't we use one of the other callbacks?

I didn't see other callbacks for injecting cmake configure flags. And this is can be used for any package that doesn't provide a cmake config file, which is many, to control options in built-in cmake find modules. Mesa just happens to be the first one.

I can't use link flags, because that will still result in the current issue where libOpenGL and libGL both get linked, and then cause runtime errors.

I am open to other suggestions, but this is fixing one of the longest standing GL issues in Spack so I would rather not hold it up too long. And when I say long standing, I mean since CMake FindOpenGL started supporting GLVND (3.10, circa 2017).

Since it is packages repo API, there is no guarantee on stability, so if we get a better idea later there isn't really much harm in deprecating/dropping it.

@alalazo do you have a better solution here? If not, I would really like to move this forward for the next release of packages as it fixes some bugs that have been around for a couple of years.

I'm looking at this again, and it seems very controversial to me:

Why build, link dependencies only? Don't we ever need run dependencies?

Why do we go beyond direct dependencies?

There is no way to turn off this behavior from a dependent, e.g. if we don't want extra arguments to be injected

The name cmake_extra_args is confusing, since it's not clear these arguments will be injected to transitive dependencies

I agree this can probably be direct deps only. The reason for only build/link is I copied the requirement for python. I didn't convert python to do this because it was outside the immediate scope of the change, but I can to reduce the number of different ways this is handled.

If GL is provided by Spack built mesa, the only option is LEGACY and not having this set for a FindOpenGL will result in borked builds. So there is no option for this, it is what it has to be.

I would say we have now increased the use case to two for this feature, both Mesa and Python. It would acually probably be nice to also provide this for MPI to set MPI_C_COMPILER and MPI_CXX_COMPILIER hints since that is something that is done in a number of places for fix FindMPI. So the usefulness of this type of feature is beyond just GL.

For the API and function, it would probably be named better. set_dependent_cmake_args(self, pkg, args). This would allow something like this, so it is up to the provider whether or not it is optional.

def set_dependent_cmake_args(self, pkg: PackageBase, cmake_args: List[str]): if getattr(pkg, "find_python_hints", True): return cmake_args.extend([...])

alalazo · 2025-11-11T17:07:24Z

repos/spack_repo/builtin/build_systems/cmake.py

+def _dependencies_cmake_extra_args(pkg: PackageBase, args: List[str]) -> None:
+    for dep in pkg.spec.traverse(deptype=("build", "link")):
+        extra_args = getattr(dep.package, "cmake_extra_args", None)
+        if extra_args:
+            args.extend(extra_args)


I'm looking at this again, and it seems very controversial to me:

Why build, link dependencies only? Don't we ever need run dependencies?

Why do we go beyond direct dependencies?

There is no way to turn off this behavior from a dependent, e.g. if we don't want extra arguments to be injected

The name cmake_extra_args is confusing, since it's not clear these arguments will be injected to transitive dependencies

alalazo · 2025-11-11T17:11:31Z

repos/spack_repo/builtin/build_systems/cmake.py


        _conditional_cmake_defaults(pkg, args)
        _maybe_set_python_hints(pkg, args)
+        _dependencies_cmake_extra_args(pkg, args)


do you have a better solution here?

Can't we just deal with this in the same way we do for Python above, and code a _maybe_set_mesa_args? If we get more cases we can think about generalizing the mechanism. In any case it's probably sensible to stick to just direct deps.

I also want to do this for MPI, there are 27 packages where we do the same thing we do for python for MPI. That is at least three packages that I deal with prefequently enough that are trying to inject spack related hints into the CMake configure.

I think it makes some sense to put this on the packages themselves. a) it makes the CMakePackage more adaptable and b) changing the configure flags in the providing package is more likely to trigger a rebuild of the things that depended on it than changing the CMakePackage

Given the implementation, I think we can inline _dependencies_cmake_extra_args.

Also, given we are generalizing the way dependents inject arguments, we should extract the boost related bits of _conditional_cmake_defaults and move them to the boost package.

repos/spack_repo/builtin/packages/python/package.py

alalazo · 2025-11-24T07:34:28Z

repos/spack_repo/builtin/build_systems/cmake.py

-        ]
-    )
+def _dependencies_cmake_extra_args(pkg: PackageBase, args: List[str]) -> None:
+    for dep in pkg.spec.dependencies(deptype=("build", "link")):


We should not use arbitrarily build,link to traverse dependencies. Maybe it's better to decide when to perform a callback using the UseMode defined in spack.build_environment.

Is UseMode in the package API? I am not seeing it imported in package.py.

I am also not sure how that would be used here, this particular function is run as a part of the setup for the cmake configure. My understanding is core shouldn't know anything about since it is a repo defined extension of the package API.

The choice to use build, link here is not arbitrary, we are looking at the dependencies that are needing during the build/link of the package. Packages that marked run shouldn't influence the build imo. If there is an exception to that later that can be addressed, but I am not thinking of any case for that now.

What about test dependencies? Also, is there no case where we should record a PATH or anything of a run dependency?

I am also not sure how that would be used here, this particular function is run as a part of the setup for the cmake configure. My understanding is core shouldn't know anything about since it is a repo defined extension of the package API.

In general core can prescribe new workflows, e.g. "build system methods listed in ... will be called at this point with this signature", so if we want to go for UseMode I guess we can.

What about test dependencies?

I would classify those the same as run for injecting cmake args.

Is an example of a test-only dep that would need to inject cmake arguments? It seems very unlikely to be the case where a test dep that is not also a build or link dep would influence build system flags.

if we want to go for UseMode I guess we can.

I still don't see how this would be better, or even implemented. We are injecting args into build system details that are more or less opaque to core. I am also uncertain how many other cases outside of CMake are going to have this kind of standard configure customization that can be applied like this.

This doesn't seem to be answered:

Also, is there no case where we should record a PATH or anything of a run dependency?

I still feel like build,link is rather arbitrary as a choice. What if a tool needs python or perl only as a run dependency, and those packages want to inject some flag to CMake to be found properly?

Thinking about it the best choice would probably be to iterate over all dependencies, and let each package decide when to inject arguments or not.

The selection of build and link here are not arbitrary at all. As I pointed out previously I was quite intentional picking those.

My feeling is if a package is a run dep only then CMake shouldn't be finding them in the first place, or aware of them at all. If CMake is being used to generate runtime launcher scripts that run python or perl or something else then those should be labeled as build deps since we need the information about the python at build time to generate the files.

I think if there is ever a real case where we need to collect cmake flags from a dependency that is only labeled run then we can update the API accordingly, but I think the use case you are describing will likely only arise in ill-constrained dependencies and we should fix those dependency relationships rather than work around them with more complexity.

Thinking about it the best choice would probably be to iterate over all dependencies, and let each package decide when to inject arguments or not.

☝️ This is such that each package is responsible for deciding when to return arguments (the base class asks all dependencies and each one decides what to return).

If a package knows it doesn't need to inject flags when it is a run dependency (why?) it can inspect pkg and return nothing. Likewise, if the flags we need are different depending on the version of CMake being used (PYTHON_EXECUTABLE?), we can inspect pkg and avoid injecting redundant flags.

Also, we don't account for test dependencies which may be used for build time tests, right? What if python is needed to run some test script etc ? Why are we creating a self imposed constraint instead of being agnostic of the dependency type and let the package decide?

I agree that pkg.spec.dependencies() with all deptypes is more straight-forward and least surprising.

My feeling is if a package is a run dep only then CMake shouldn't be finding them in the first place

In practice this is not the case. There are enough instances of a run type dep where CMake still wants to know things about its executables. Sometimes people like to create absolute shebangs, etc. Specifying those are "build" deps would be wrong conceptually, as those are for the host, whereas the shebangs are for the target arch (would we ever do cross compiling)

Test deps should definitely be included.

UseMode should not be relevant here, given that the package being considered is always the one that's being built. UseMode is only relevant if you care about runtime vs build time and packages a few deps deep from the root.

repos/spack_repo/builtin/build_systems/cmake.py

kwryankrattiger · 2025-11-26T04:26:19Z

@spackbot run pipeline

spackbot-app · 2025-11-26T04:26:28Z

I've started that pipeline for you!

adamjstewart · 2025-11-26T10:04:52Z

repos/spack_repo/builtin/packages/python/package.py

@@ -1289,6 +1280,26 @@ def setup_dependent_package(self, module, dependent_spec):
        module.python_platlib = join_path(dependent_spec.prefix, self.platlib)
        module.python_purelib = join_path(dependent_spec.prefix, self.purelib)

+    def dependent_cmake_args(self, pkg: PackageBase) -> List[str]:


I may be misunderstanding, but it seems like this injects flags to 99% of CMake packages that depend on Python unless they explicitly disable this? I don't love injecting non-standard flags, as they end up as red herring warning messages for downstream packages. I would much rather opt-in, and opt-in one flag at a time. spec['python'] already has an easy and convenient way to grab the Python command. I think packages should manually use this only when needed.

That is already true, I just moved the logic for this from build_systems/cmake.py to python/package.py. This change specifically should amount to a noop.

Without these flags, cmake cannot find spack installed python. It is a similar issue for MPI, which is why you see so many cmake projects passing MPI hints in a few different iterations of the same general form (see the mpi/package.py changes for a flavor of it)

Thanks. I'm okay with no-ops, but I also think we should remove this and only add it to packages that need it. Doesn't have to be in this PR though.

I am not sure what you mean by this.

These flags are only added to projects that directly depend on python (or whatever project has the cmake args function) and are CMake projects. Of those projects, very few don't need to have these hints passed, and even fewer will have issues if the are passed. And for the projects where that is the case, we already of that opt-out option to set the package property find_python_hints = False.

Very few need all 3 flags, and often only need them when +python.

Sure. This is only applied when the spec has a python build or link dep. For packages with a python variant that will usually only be true for the +python. I have a feeling there are vanishingly few cases where the conditions for these flags are not met under those conditions.

We could be smarter about which flags we pass, they could be conditionally applied based on the CMake version. But since it doesn't hurt anything to pass all three all the time I don't know if there is value in making it more complicated.

This is only applied when the spec has a python build or link dep.

I think the Python package should have code to decide when to inject these flags, if that is relevant.

If there is any need to check e.g. the cmake version being used or the dependency types of python, before injecting Python flags, we should do it here. The code in the build system should probably be as agnostic as possible, and just perform a callback on packages implementing the method.

kwryankrattiger · 2025-12-18T16:02:51Z

@spackbot run pipeline

spackbot-app · 2025-12-18T16:02:59Z

I've started that pipeline for you!

Signed-off-by: Ryan Krattiger <ryan.krattiger@kitware.com>

alalazo

Left a few more comments. Summary:

I think we should use this PR only for changes related to the cmake build system, anything else should be extracted in a separate PR and merged
The build system should not make assumption on when to perform the callback depending on the dependency type. It should be on the called package to decide whether it wants to return something or not
No broad Exception catch in build systems, which could mask other errors

I also asked @haampie to give a second review to this PR. Since it touches a part of the repo that is used in many other places.

alalazo · 2026-01-20T14:09:09Z

repos/spack_repo/builtin/build_systems/cmake.py

-        ]
-    )
+def _dependencies_cmake_extra_args(pkg: PackageBase, args: List[str]) -> None:
+    for dep in pkg.spec.dependencies(deptype=("build", "link")):


This doesn't seem to be answered:

Also, is there no case where we should record a PATH or anything of a run dependency?

I still feel like build,link is rather arbitrary as a choice. What if a tool needs python or perl only as a run dependency, and those packages want to inject some flag to CMake to be found properly?

alalazo · 2026-01-20T14:13:50Z

repos/spack_repo/builtin/build_systems/cmake.py

+    for dep in pkg.spec.dependencies(deptype=("build", "link")):
+        try:
+            args.extend(dep.package.dependent_cmake_args(pkg, args))
+        except Exception:


This one is definitely too broad. Can we use a narrower list of exceptions?

Maybe even better would be to not use exceptions at all:

if not hasattr(dep.package, "dependent_cmake_args"): continue

oh wow, I think I was tired when I wrote that or something, 100% agree..

alalazo · 2026-01-20T14:19:20Z

repos/spack_repo/builtin/build_systems/cmake.py

-        ]
-    )
+def _dependencies_cmake_extra_args(pkg: PackageBase, args: List[str]) -> None:
+    for dep in pkg.spec.dependencies(deptype=("build", "link")):


Thinking about it the best choice would probably be to iterate over all dependencies, and let each package decide when to inject arguments or not.

alalazo · 2026-01-20T14:24:28Z

repos/spack_repo/builtin/build_systems/cmake.py


        _conditional_cmake_defaults(pkg, args)
        _maybe_set_python_hints(pkg, args)
+        _dependencies_cmake_extra_args(pkg, args)


Given the implementation, I think we can inline _dependencies_cmake_extra_args.

Also, given we are generalizing the way dependents inject arguments, we should extract the boost related bits of _conditional_cmake_defaults and move them to the boost package.

alalazo · 2026-01-20T14:25:37Z

repos/spack_repo/builtin/packages/apple_glu/package.py

+    def setup_dependent_build_environment(
+        self, env: EnvironmentModifications, dependent_spec: Spec
+    ):
+        env.prepend_path("OpenGL_ROOT", self.prefix)


Can this be extracted in a separate PR to be merged straight away?

Yeah I can do that.

alalazo · 2026-01-20T14:26:53Z

repos/spack_repo/builtin/packages/cairo/package.py

    patch("fontconfig.patch", when="@1.16.0:1.17.2")
    # Don't regenerate docs to avoid a dependency on gtk-doc
-    patch("disable-gtk-docs.patch", when="build_system=autotools ^autoconf@2.70:")
+    patch("disable-gtk-docs.patch", when="build_system=autotools")


Can we extract also this seemingly unrelated change? If we are adding a new feature to the CMake build system, I'd like the PR to contain only changes relevant for that feature.

yeah makes sense, this was all migrated out of another PR, so what is a couple more 😄

alalazo · 2026-01-20T14:33:53Z

repos/spack_repo/builtin/packages/python/package.py

@@ -1289,6 +1280,26 @@ def setup_dependent_package(self, module, dependent_spec):
        module.python_platlib = join_path(dependent_spec.prefix, self.platlib)
        module.python_purelib = join_path(dependent_spec.prefix, self.purelib)

+    def dependent_cmake_args(self, pkg: PackageBase) -> List[str]:


This is only applied when the spec has a python build or link dep.

I think the Python package should have code to decide when to inject these flags, if that is relevant.

If there is any need to check e.g. the cmake version being used or the dependency types of python, before injecting Python flags, we should do it here. The code in the build system should probably be as agnostic as possible, and just perform a callback on packages implementing the method.

alalazo · 2026-01-20T14:39:21Z

repos/spack_repo/builtin/packages/mpi/package.py

    homepage = "https://www.mpi-forum.org/"
    virtual = True

+    def dependent_cmake_args(self, pkg: PackageBase) -> List[str]:


Is this actually being called? If I read code correctly we just iterate on .dependencies:

Spack version 1.2.0.dev0 Python 3.13.2, Linux x86_64 >>> from spack.concretize import concretize_one >>> s = concretize_one("hdf5") >>> s.dependencies() [cmake@3.31.6/ju2z2nr3nclj2ip2rhexrthq6mt7flt6, zlib-ng@2.2.4/3mewdwgmlykrd2ie4cftrpifzovbs2er, pkgconf@2.5.1/iguplooome2oktywbgjvoq5nwxmgdw6y, openmpi@5.0.9/tk7gfkssl7mkbglkbitxu6l4i3gpgysa, gcc@15.1/5mmswryndxgqgzxf4vhd27drooj2zywh, glibc@2.31/nm2n32h34mhcte6fm3s4fjjnnrdyqcvz, gcc-runtime@15.1/4obv4eoaimwzbfcgq3s7lf3wvrpd5emq, compiler-wrapper@1.0/tidruedm3z3lhfee2vyu3baxfpilprh7, gmake@4.4.1/x6dhwcecsmwwctcho5mmmvp4cmbryyqk]

and that doesn't include virtual packages, right?

I think we need to iterate on edges and call the virtual package explicitly if the virtual is provided, if we want this to take effect (unless I'm forgetting some magic we do).

alalazo · 2026-01-22T19:00:47Z

repos/spack_repo/builtin/packages/python/package.py

+
+        # Allow packages to disable these hints be setting a the member variable
+        # ``find_python_hints`` to False. By default this is always applied.
+        if not getattr(pkg, "find_python_hints", True):


Another thought I had is that the CMakePackage class should define an attribute, for instance:

class CMakePackage: disable_hints_from: List[str] = []

and we should check in the base class whether to make the callback or not. This would save use from inventing a new attribute name for each package defining this method, and having this check at the top of each implementation.

For me personally, I think the class attribute is fine, because:

Easier to document

Editor completion in derived classes from CMakePackage

I don't think the list will be long

haampie

At the very least, this PR should remove the MPI related changes. I think it will annoy people to see this compiler wrapper pop up. MPI is just a library like any other dependency.

In general, I would say: split the functionality you're introducing from the use cases you're contributing. Make the PR small, so it's merged faster.

kwryankrattiger requested a review from alalazo October 22, 2025 16:12

spackbot-triage bot added patch update-package labels Oct 22, 2025

spackbot-triage bot requested review from aphecetche and v-dobrev October 22, 2025 16:12

kwryankrattiger mentioned this pull request Oct 22, 2025

Updates to X related packages #192 #2056

Merged

alalazo reviewed Oct 23, 2025

View reviewed changes

kwryankrattiger mentioned this pull request Nov 2, 2025

Updates to Qt6 related packages #192 #2058

Open

alalazo reviewed Nov 11, 2025

View reviewed changes

spackbot-triage bot added the python label Nov 20, 2025

spackbot-triage bot requested review from adamjstewart and scheibelp November 20, 2025 23:10

kwryankrattiger force-pushed the gl_updates_from_192 branch 2 times, most recently from 4fc65ed to a3ddb48 Compare November 21, 2025 15:01

kwryankrattiger mentioned this pull request Nov 21, 2025

Mpi cmake find hints #2535

Draft

alalazo requested changes Nov 24, 2025

View reviewed changes

kwryankrattiger force-pushed the gl_updates_from_192 branch 2 times, most recently from f0e93b2 to a6a39fd Compare November 24, 2025 23:10

kwryankrattiger requested a review from alalazo November 26, 2025 04:24

adamjstewart reviewed Nov 26, 2025

View reviewed changes

kwryankrattiger mentioned this pull request Dec 11, 2025

Paraview 6 updates #192

Open

kwryankrattiger force-pushed the gl_updates_from_192 branch from a6a39fd to 8bc0128 Compare December 17, 2025 21:50

spackbot-triage bot requested a review from adamjstewart December 17, 2025 21:50

kwryankrattiger force-pushed the gl_updates_from_192 branch from 8bc0128 to 572f9c6 Compare December 18, 2025 16:30

kwryankrattiger added 2 commits December 18, 2025 16:25

Updates to GL related packages spack#192

9b4b684

Convert python and mpi to also pass define cmake_args

210c4c6

Signed-off-by: Ryan Krattiger <ryan.krattiger@kitware.com>

Rename api and return args

5627fa4

kwryankrattiger force-pushed the gl_updates_from_192 branch from 572f9c6 to 5627fa4 Compare December 18, 2025 22:25

alalazo reviewed Jan 20, 2026

View reviewed changes

alalazo self-assigned this Jan 20, 2026

alalazo mentioned this pull request Jan 20, 2026

Move build system documentation to spack-packages spack/spack#51874

Open

3 tasks

alalazo reviewed Jan 22, 2026

View reviewed changes

haampie requested changes Jan 23, 2026

View reviewed changes

Updates to GL related packages #192 #2057

Are you sure you want to change the base?

Updates to GL related packages #192 #2057

Uh oh!

Conversation

kwryankrattiger commented Oct 22, 2025

Uh oh!

alalazo Oct 23, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kwryankrattiger Nov 20, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

alalazo Nov 11, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alalazo Nov 24, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haampie Jan 23, 2026 • 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!

Uh oh!

kwryankrattiger commented Nov 26, 2025

Uh oh!

spackbot-app bot commented Nov 26, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kwryankrattiger Dec 4, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

kwryankrattiger commented Dec 18, 2025

Uh oh!

spackbot-app bot commented Dec 18, 2025

alalazo Oct 23, 2025 •

edited

Loading

kwryankrattiger Nov 20, 2025 •

edited

Loading

alalazo Nov 11, 2025 •

edited

Loading

alalazo Nov 24, 2025 •

edited

Loading

haampie Jan 23, 2026 •

edited

Loading

kwryankrattiger Dec 4, 2025 •

edited

Loading