Move external python glue logic into core#50605
Merged
haampie merged 10 commits intospack:developfrom May 22, 2025
Merged
Conversation
…tion; this moves that logic into core from the python build system
…ic is copied between the mock/real python build system
scheibelp
commented
May 22, 2025
tgamblin
approved these changes
May 22, 2025
Member
tgamblin
left a comment
tgamblin
left a comment
There was a problem hiding this comment.
This LGTM. I think this is the right change to keep the current behavior until we can add proper dependencies for externals, but not expose it as part of the package API in 1.0.
Member
|
@haampie if you like this one, can you approve and merge? I do not think we need to do a rebuild or anything with gitlab, as this logic only applies to externals. |
haampie
approved these changes
May 22, 2025
kwryankrattiger
added a commit
that referenced
this pull request
May 22, 2025
This reverts commit 6328fb3.
haampie
pushed a commit
that referenced
this pull request
May 22, 2025
scheibelp
added a commit
that referenced
this pull request
May 30, 2025
Redo of #50605 (Move external python glue logic into core) This concerns logic that attaches a `python` dependency to some external packages (because they expect an instance of `python` in their DAG). #50605 was intended to be a simple relocation of logic from the package repo to the core. That PR caused a failure because it was attempting to attach python to a bigger set of dependents: originally this logic only ran for packages that were a subclass of `PythonPackage`, and that PR made it run for all packages that `extends(python)`; notably that included LLVM, and in particular this occurred inside of an environment that set `unify: false`; attaching Python can involve looking for an instance of Python on the system, and normal external detection runs with a process pool, which is prohibited in the context of `concretize_separately` (which itself uses a process pool with daemon processes). This PR reapplies the logic from #50605 with two additional changes: * Match the previous inclusion/exclusion semantics by only attaching Python to external packages that are a subclass of `PythonPackage` * Update external Python search to run within the current process (so that it can run as part of `concretize_separately`)
kshea21
pushed a commit
to kshea21/spack
that referenced
this pull request
Jun 18, 2025
Redo of spack#50605 (Move external python glue logic into core) This concerns logic that attaches a `python` dependency to some external packages (because they expect an instance of `python` in their DAG). spack#50605 was intended to be a simple relocation of logic from the package repo to the core. That PR caused a failure because it was attempting to attach python to a bigger set of dependents: originally this logic only ran for packages that were a subclass of `PythonPackage`, and that PR made it run for all packages that `extends(python)`; notably that included LLVM, and in particular this occurred inside of an environment that set `unify: false`; attaching Python can involve looking for an instance of Python on the system, and normal external detection runs with a process pool, which is prohibited in the context of `concretize_separately` (which itself uses a process pool with daemon processes). This PR reapplies the logic from spack#50605 with two additional changes: * Match the previous inclusion/exclusion semantics by only attaching Python to external packages that are a subclass of `PythonPackage` * Update external Python search to run within the current process (so that it can run as part of `concretize_separately`)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This originally lived in the
pythonbuild system and in this PR moves into coreThis was originally plugged in generically: any extendable package could implement
_update_external_dependenciesand potentially resolve analogous issues. No other package in builtin did this (but a if a private repo took advantage of this, this PR would break them).The changes are nearly a verbatim relocation (although one edge case for the old concretizer is removed), plus some renaming of methods to reflect the fact that it's just for python and not generic.
This logic is already tested (
spack unit-test -k external_python).