Skip to content

env activation: use package defined env setup methods#13249

Merged
tgamblin merged 4 commits intodevelopfrom
features/env-activate-use-setup-run-env
Oct 23, 2019
Merged

env activation: use package defined env setup methods#13249
tgamblin merged 4 commits intodevelopfrom
features/env-activate-use-setup-run-env

Conversation

@becker33
Copy link
Copy Markdown
Member

@becker33 becker33 commented Oct 17, 2019

Fixes #12731

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 #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 #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.

@scheibelp scheibelp self-assigned this Oct 17, 2019
Copy link
Copy Markdown
Member

@scheibelp scheibelp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you update the PR description/commit messages to mention that you refactored ViewDescriptor to support Spec in view?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

@becker33 becker33 force-pushed the features/env-activate-use-setup-run-env branch from 2ae2a1a to 58100e8 Compare October 17, 2019 23:54
@becker33 becker33 force-pushed the features/env-activate-use-setup-run-env branch from 8c4e7cc to 5264b07 Compare October 22, 2019 21:36
@tgamblin tgamblin closed this Oct 23, 2019
@tgamblin tgamblin reopened this Oct 23, 2019
@tgamblin tgamblin merged commit 95a48b2 into develop Oct 23, 2019
@tgamblin tgamblin deleted the features/env-activate-use-setup-run-env branch October 23, 2019 06:36
jrmadsen pushed a commit to jrmadsen/spack that referenced this pull request Oct 30, 2019
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.
tgamblin added a commit that referenced this pull request May 5, 2020
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).
tgamblin added a commit that referenced this pull request May 7, 2020
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).
tgamblin added a commit that referenced this pull request May 7, 2020
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).
tgamblin added a commit that referenced this pull request May 7, 2020
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).
becker33 pushed a commit that referenced this pull request Jul 11, 2020
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).
vjranagit pushed a commit to vjranagit/spack that referenced this pull request Jan 18, 2026
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).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Spack Environments does not set all env vars during activation

4 participants