env activation: use package defined env setup methods#13249
env activation: use package defined env setup methods#13249
Conversation
scheibelp
left a comment
There was a problem hiding this comment.
I have a few requests and a couple questions. I think they are all minor, if it doesn't seem that way let me know.
| ignore_conflicts=True, | ||
| projections=self.projections) | ||
|
|
||
| def __contains__(self, spec): |
There was a problem hiding this comment.
Can you update the PR description/commit messages to mention that you refactored ViewDescriptor to support Spec in view?
There was a problem hiding this comment.
I'll update the PR description. We can avoid having to rewrite history if we update the commit message when we merge it.
| Returns the EnvironmentModifications object that will reverse self | ||
|
|
||
| Only creates reversals for additions to the environment, as reversing | ||
| ``unset`` and ``remove_path`` modifications is impossible. |
There was a problem hiding this comment.
If the reverse logic were migrated to the environment action objects, Unset could record the value of the variable at the time that it is unset. I think adding a TODO is appropriate since this is likely best effort anyhow
There was a problem hiding this comment.
The variable may already be unset.
$ spack env activate foo
...
$ despacktivate
At despacktivate time, the variable is already unset before the python process starts and the EnvironmentModifications object is created.
It would be possible to have environments write a file at activation time from which to restore the previous environment, but I don't think it's worth doing. This best-effort version emulates what the module unload function already does. I suspect that we won't see many (if any) cases in which the setup_run_environment method unsets a variable or removes a path.
There was a problem hiding this comment.
Oh riiight. Er I suppose it could be stored in an intermediate variable like UNSET_<the original variable name>. Or perhaps storing the settings in a file. In either case I don't think that needs to be done here. I do think though that it is possible to handle (just that it doesn't necessarily need to be handled in this PR).
2ae2a1a to
58100e8
Compare
…ages This allows contributors to write mock packages to test backwards compatibility
8c4e7cc to
5264b07
Compare
This PR ensures that environment activation sets all environment variables set by the equivalent `module load` operations, except that the spec prefixes are "rebased" to the view associated with the environment. Currently, Spack blindly adds paths relative to the environment view root to the user environment on activation. Issue spack#12731 points out ways in which this behavior is insufficient. This PR changes that behavior to use the `setup_run_environment` logic for each package to augment the prefix inspections (as in Spack's modulefile generation logic) to ensure that all necessary variables are set to make use of the packages in the environment. See spack#12731 for details on the previous problems in behavior. This PR also updates the `ViewDescriptor` object in `spack.environment` to have a `__contains__` method. This allows for checks like `if spec in self.default_view`. The `__contains__` operator for `ViewDescriptor` objects checks whether the spec satisfies the filters of the View descriptor, not whether the spec is already linked into the underlying `FilesystemView` object.
Fixed #15884. Spack asks every package linked into an environment to tell us how environment variables should be modified when a spack environment is activated. As part of this, specs in an environment are symlinked into the environment's view (see #13249), and the package calculates environment modifications with *the default view as the prefix*. All of this works nicely for pointing the user's environment at the view *if* every package is successfully linked. Unfortunately, right now we only track what specs "should" be in a view, not which specs actually are. So we end up calculating environment modifications on things that aren't linked into thee view, and the exception isn't caught, so lots of spack commands end up failing. This fixes the issue by ignoring and warning about specs where calculating environment modifications fails. So we can still keep using Spack even if the current environment is incomplete. We should probably also just avoid computing env modifications *entirely* for unlinked packages, but right now that is a slow operation (requires a lot of YAML parsing). We should revisit that when we have some better state management for views, but the fix adopted here will still be necessary, as we want spack commands to be resilient to other types of bugs in `setup_run_environment()` and friends. That code is in packages and we have to assume it could be buggy when we call it outside of builds (as it might fail more than just the build).
Fixed #15884. Spack asks every package linked into an environment to tell us how environment variables should be modified when a spack environment is activated. As part of this, specs in an environment are symlinked into the environment's view (see #13249), and the package calculates environment modifications with *the default view as the prefix*. All of this works nicely for pointing the user's environment at the view *if* every package is successfully linked. Unfortunately, right now we only track what specs "should" be in a view, not which specs actually are. So we end up calculating environment modifications on things that aren't linked into thee view, and the exception isn't caught, so lots of spack commands end up failing. This fixes the issue by ignoring and warning about specs where calculating environment modifications fails. So we can still keep using Spack even if the current environment is incomplete. We should probably also just avoid computing env modifications *entirely* for unlinked packages, but right now that is a slow operation (requires a lot of YAML parsing). We should revisit that when we have some better state management for views, but the fix adopted here will still be necessary, as we want spack commands to be resilient to other types of bugs in `setup_run_environment()` and friends. That code is in packages and we have to assume it could be buggy when we call it outside of builds (as it might fail more than just the build).
Fixed #15884. Spack asks every package linked into an environment to tell us how environment variables should be modified when a spack environment is activated. As part of this, specs in an environment are symlinked into the environment's view (see #13249), and the package calculates environment modifications with *the default view as the prefix*. All of this works nicely for pointing the user's environment at the view *if* every package is successfully linked. Unfortunately, right now we only track what specs "should" be in a view, not which specs actually are. So we end up calculating environment modifications on things that aren't linked into thee view, and the exception isn't caught, so lots of spack commands end up failing. This fixes the issue by ignoring and warning about specs where calculating environment modifications fails. So we can still keep using Spack even if the current environment is incomplete. We should probably also just avoid computing env modifications *entirely* for unlinked packages, but right now that is a slow operation (requires a lot of YAML parsing). We should revisit that when we have some better state management for views, but the fix adopted here will still be necessary, as we want spack commands to be resilient to other types of bugs in `setup_run_environment()` and friends. That code is in packages and we have to assume it could be buggy when we call it outside of builds (as it might fail more than just the build).
Fixed #15884. Spack asks every package linked into an environment to tell us how environment variables should be modified when a spack environment is activated. As part of this, specs in an environment are symlinked into the environment's view (see #13249), and the package calculates environment modifications with *the default view as the prefix*. All of this works nicely for pointing the user's environment at the view *if* every package is successfully linked. Unfortunately, right now we only track what specs "should" be in a view, not which specs actually are. So we end up calculating environment modifications on things that aren't linked into thee view, and the exception isn't caught, so lots of spack commands end up failing. This fixes the issue by ignoring and warning about specs where calculating environment modifications fails. So we can still keep using Spack even if the current environment is incomplete. We should probably also just avoid computing env modifications *entirely* for unlinked packages, but right now that is a slow operation (requires a lot of YAML parsing). We should revisit that when we have some better state management for views, but the fix adopted here will still be necessary, as we want spack commands to be resilient to other types of bugs in `setup_run_environment()` and friends. That code is in packages and we have to assume it could be buggy when we call it outside of builds (as it might fail more than just the build).
Fixed #15884. Spack asks every package linked into an environment to tell us how environment variables should be modified when a spack environment is activated. As part of this, specs in an environment are symlinked into the environment's view (see #13249), and the package calculates environment modifications with *the default view as the prefix*. All of this works nicely for pointing the user's environment at the view *if* every package is successfully linked. Unfortunately, right now we only track what specs "should" be in a view, not which specs actually are. So we end up calculating environment modifications on things that aren't linked into thee view, and the exception isn't caught, so lots of spack commands end up failing. This fixes the issue by ignoring and warning about specs where calculating environment modifications fails. So we can still keep using Spack even if the current environment is incomplete. We should probably also just avoid computing env modifications *entirely* for unlinked packages, but right now that is a slow operation (requires a lot of YAML parsing). We should revisit that when we have some better state management for views, but the fix adopted here will still be necessary, as we want spack commands to be resilient to other types of bugs in `setup_run_environment()` and friends. That code is in packages and we have to assume it could be buggy when we call it outside of builds (as it might fail more than just the build).
Fixed spack#15884. Spack asks every package linked into an environment to tell us how environment variables should be modified when a spack environment is activated. As part of this, specs in an environment are symlinked into the environment's view (see spack#13249), and the package calculates environment modifications with *the default view as the prefix*. All of this works nicely for pointing the user's environment at the view *if* every package is successfully linked. Unfortunately, right now we only track what specs "should" be in a view, not which specs actually are. So we end up calculating environment modifications on things that aren't linked into thee view, and the exception isn't caught, so lots of spack commands end up failing. This fixes the issue by ignoring and warning about specs where calculating environment modifications fails. So we can still keep using Spack even if the current environment is incomplete. We should probably also just avoid computing env modifications *entirely* for unlinked packages, but right now that is a slow operation (requires a lot of YAML parsing). We should revisit that when we have some better state management for views, but the fix adopted here will still be necessary, as we want spack commands to be resilient to other types of bugs in `setup_run_environment()` and friends. That code is in packages and we have to assume it could be buggy when we call it outside of builds (as it might fail more than just the build).
Fixes #12731
This PR ensures that environment activation sets all environment variables set by the equivalent
module loadoperations, except that the spec prefixes are "rebased" to the view associated with the environment.Currently, Spack blindly adds paths relative to the environment view root to the user environment on activation. Issue #12731 points out ways in which this behavior is insufficient.
This PR changes that behavior to use the
setup_run_environmentlogic for each package to augment the prefix inspections (as in Spack's modulefile generation logic) to ensure that all necessary variables are set to make use of the packages in the environment.See #12731 for details on the previous problems in behavior.
This PR also updates the
ViewDescriptorobject inspack.environmentto have a__contains__method. This allows for checks likeif spec in self.default_view. The__contains__operator forViewDescriptorobjects checks whether the spec satisfies the filters of the View descriptor, not whether the spec is already linked into the underlyingFilesystemViewobject.