CMakePackage: Add option to get cmake args from dependencies

[kwryankrattiger]

This feature is not complete but it is a minimal implementation to provided the ability for packages to define hints for cmake. Not all hints can be passed as environment variables and cmake argument injection is not possible via existing mechanisms.

As a demonstration of this feature the python hint flags are converted from the ad hoc method in the CMakePackage to using the package callback.

Flags can be disabled using a common class list attribute disable_cmake_hints_from.

Extracted from #2057

[adamjstewart]

Still don't love that it's opt-out instead of opt-in, but I understand why (warnings harm no one, and fixes help everyone). I agree it makes more sense to move this into the Python package.

[haampie]

What I don't like about this conceptually is that the python package imports the cmake module.

Once we do content hashing properly and follow imports, it would mean that Python's hash is affected by changes to the cmake module. That feels wrong to me. I prefer the status quo of having this logic in cmake.py.

Secondly, you've put this under python, but in reality pkg.spec["python"].command picks up python-venv's package. I think the logic is better kept in cmake.py where it chooses python's or python-venv's interpreter path. Putting it in the python package seems to suggest this is about python's interpreter, but it's not.

[alalazo]

I have a few comments on the code, on top of the python-venv vs. python issue Harmen pointed out.¹

Sorry to be pickier than for other PRs, but this is going to be triggered most of the times and is in a code path that is difficult to remove after we merge the PR.

Footnotes

1. The cmdefine issue can be solved by not using the convenience function imo. ↩

[kwryankrattiger]

Secondly, you've put this under python, but in reality pkg.spec["python"].command picks up python-venv's package. I think the logic is better kept in cmake.py where it chooses python's or python-venv's interpreter path. Putting it in the python package seems to suggest this is about python's interpreter, but it's not.

So what is the correct behavior here? I would think that pkg.spec["python"].command is what we want for configuring the dependent.

[kwryankrattiger]

@haampie are you okay if we just put a comment next to the python setup that says something along the lines of

# this can redirect to python-venv if pkg extends python

[haampie]

I would think that pkg.spec["python"].command is what we want for configuring the dependent.

Sure, but it's very confusing to use pkg.spec["python"].command in a method of class Python. Everyone reading that would expect self.command (used 22 times in the same class).

That tells me that python is not a good use case for this feature right now, so I would undo that change.

Maybe you could write it as

class Python:
    def dependent_cmake_args(self, dependent_spec: Spec) -> List[str]:
        if dependent_spec.dependencies("python-venv"):
            return []
        return [... self.command ...]

...

class PythonVenv:
    def dependent_cmake_args(self, dependent_spec: Spec) -> List[str]:
        return [... self.command ...]

but that's also not satisfactory cause packages would have to opt out of two packages that provide hints instead of one.

[kwryankrattiger]

That tells me that python is not a good use case for this feature right now, so I would undo that change.

I disagree on a couple points here.

Personally I don't think it is that confusing. It is reasonably well documented at this point that python has some special handling already and the comment links to the PR that has additional details about why this is the case. I spent a couple of seconds reading the PR and understood what was done well enough to understand why it was written that way.

We could also write it is self.spec.command which is what we do for MPI providers for similar cases.

I really don't like package specific code inside of the CMakePackage. The people maintaining that code are not the same people maintaining the oddities of python or other packages and how they are modeled in Spack. This moves the responsibility and context of python changes into python and out of the build system.

[alalazo]

Current state LGTM. From my point of view, we can merge as soon as @haampie ✔️ too

[haampie]

Still not a convinced that python is currently the best example given the interaction with python-venv, but I can live with it. Few small requests.

lgtm, just small nitpicks

[alalazo]

@spackbot run pipeline

[spackbot-app]

I've started that pipeline for you!

[kwryankrattiger]

CI Failures are unrelated to this change. This change is blocking progress on other work @alalazo @haampie are we okay to force merge this to unblock things?

[alalazo]

Yeah, I'll go on force merging this - there are just a handful of known flaky jobs that went ❌