From e0fbf09239fb25760e07cc984f544ddb98e61f22 Mon Sep 17 00:00:00 2001 From: Harmen Stoppels Date: Fri, 15 Oct 2021 10:00:44 +0200 Subject: [PATCH] EnvironmentModifications: allow disabling stack tracing (#26706) Currently Spack keeps track of the origin in the code of any modification to the environment variables. This is very slow and enabled unconditionally even in code paths where the origin of the modification is never queried. The only place where we inspect the origins of environment modifications is before we start a build, If there's an override of the type `e.set(...)` after incremental changes like `e.append_path(..)`, which is a "suspicious" change. This is very rare though. If an override like this ever happens, it might mean a package is broken. If that leads to build errors, we can just ask the user to run `spack -d install ...` and check the warnings issued by Spack to find the origins of the problem. --- lib/spack/spack/main.py | 2 ++ lib/spack/spack/util/environment.py | 41 +++++++++++++++++++---------- 2 files changed, 29 insertions(+), 14 deletions(-) diff --git a/lib/spack/spack/main.py b/lib/spack/spack/main.py index 5224f423455..484ba4553dc 100644 --- a/lib/spack/spack/main.py +++ b/lib/spack/spack/main.py @@ -43,6 +43,7 @@ import spack.spec import spack.store import spack.util.debug +import spack.util.environment import spack.util.executable as exe import spack.util.path from spack.error import SpackError @@ -478,6 +479,7 @@ def setup_main_options(args): spack.error.debug = True spack.util.debug.register_interrupt_handler() spack.config.set('config:debug', True, scope='command_line') + spack.util.environment.tracing_enabled = True if args.timestamp: tty.set_timestamp(True) diff --git a/lib/spack/spack/util/environment.py b/lib/spack/spack/util/environment.py index 398af5f0186..a0a6c4a3aa0 100644 --- a/lib/spack/spack/util/environment.py +++ b/lib/spack/spack/util/environment.py @@ -21,6 +21,7 @@ import llnl.util.tty as tty from llnl.util.lang import dedupe +import spack.config import spack.platforms import spack.spec import spack.util.executable as executable @@ -45,6 +46,9 @@ } +tracing_enabled = False + + def is_system_path(path): """Predicate that given a path returns True if it is a system path, False otherwise. @@ -365,14 +369,17 @@ class EnvironmentModifications(object): * 'context' : line of code that issued the request that failed """ - def __init__(self, other=None): + def __init__(self, other=None, traced=None): """Initializes a new instance, copying commands from 'other' if it is not None. Args: other (EnvironmentModifications): list of environment modifications to be extended (optional) + traced (bool): enable or disable stack trace inspection to log the origin + of the environment modifications. """ + self.traced = tracing_enabled if traced is None else bool(traced) self.env_modifications = [] if other is not None: self.extend(other) @@ -393,7 +400,12 @@ def _check_other(other): raise TypeError( 'other must be an instance of EnvironmentModifications') - def _get_outside_caller_attributes(self): + def _maybe_trace(self, kwargs): + """Provide the modification with stack trace info so that we can track its + origin to find issues in packages. This is very slow and expensive.""" + if not self.traced: + return + stack = inspect.stack() try: _, filename, lineno, _, context, index = stack[2] @@ -402,8 +414,7 @@ def _get_outside_caller_attributes(self): filename = 'unknown file' lineno = 'unknown line' context = 'unknown context' - args = {'filename': filename, 'lineno': lineno, 'context': context} - return args + kwargs.update({'filename': filename, 'lineno': lineno, 'context': context}) def set(self, name, value, **kwargs): """Stores a request to set an environment variable. @@ -412,7 +423,7 @@ def set(self, name, value, **kwargs): name: name of the environment variable to be set value: value of the environment variable """ - kwargs.update(self._get_outside_caller_attributes()) + self._maybe_trace(kwargs) item = SetEnv(name, value, **kwargs) self.env_modifications.append(item) @@ -425,7 +436,7 @@ def append_flags(self, name, value, sep=' ', **kwargs): value: value to append to the environment variable Appends with spaces separating different additions to the variable """ - kwargs.update(self._get_outside_caller_attributes()) + self._maybe_trace(kwargs) kwargs.update({'separator': sep}) item = AppendFlagsEnv(name, value, **kwargs) self.env_modifications.append(item) @@ -436,7 +447,7 @@ def unset(self, name, **kwargs): Args: name: name of the environment variable to be unset """ - kwargs.update(self._get_outside_caller_attributes()) + self._maybe_trace(kwargs) item = UnsetEnv(name, **kwargs) self.env_modifications.append(item) @@ -450,7 +461,7 @@ def remove_flags(self, name, value, sep=' ', **kwargs): value: value to remove to the environment variable sep: separator to assume for environment variable """ - kwargs.update(self._get_outside_caller_attributes()) + self._maybe_trace(kwargs) kwargs.update({'separator': sep}) item = RemoveFlagsEnv(name, value, **kwargs) self.env_modifications.append(item) @@ -462,7 +473,7 @@ def set_path(self, name, elements, **kwargs): name: name o the environment variable to be set. elements: elements of the path to set. """ - kwargs.update(self._get_outside_caller_attributes()) + self._maybe_trace(kwargs) item = SetPath(name, elements, **kwargs) self.env_modifications.append(item) @@ -473,7 +484,7 @@ def append_path(self, name, path, **kwargs): name: name of the path list in the environment path: path to be appended """ - kwargs.update(self._get_outside_caller_attributes()) + self._maybe_trace(kwargs) item = AppendPath(name, path, **kwargs) self.env_modifications.append(item) @@ -484,7 +495,7 @@ def prepend_path(self, name, path, **kwargs): name: name of the path list in the environment path: path to be pre-pended """ - kwargs.update(self._get_outside_caller_attributes()) + self._maybe_trace(kwargs) item = PrependPath(name, path, **kwargs) self.env_modifications.append(item) @@ -495,7 +506,7 @@ def remove_path(self, name, path, **kwargs): name: name of the path list in the environment path: path to be removed """ - kwargs.update(self._get_outside_caller_attributes()) + self._maybe_trace(kwargs) item = RemovePath(name, path, **kwargs) self.env_modifications.append(item) @@ -506,7 +517,7 @@ def deprioritize_system_paths(self, name, **kwargs): Args: name: name of the path list in the environment. """ - kwargs.update(self._get_outside_caller_attributes()) + self._maybe_trace(kwargs) item = DeprioritizeSystemPaths(name, **kwargs) self.env_modifications.append(item) @@ -517,7 +528,7 @@ def prune_duplicate_paths(self, name, **kwargs): Args: name: name of the path list in the environment. """ - kwargs.update(self._get_outside_caller_attributes()) + self._maybe_trace(kwargs) item = PruneDuplicatePaths(name, **kwargs) self.env_modifications.append(item) @@ -843,6 +854,8 @@ def validate(env, errstream): Args: env: list of environment modifications """ + if not env.traced: + return modifications = env.group_by_name() for variable, list_of_changes in sorted(modifications.items()): set_or_unset_not_first(variable, list_of_changes, errstream)