From 89220bc0e1bc8df0130cf5388488c2b161caf554 Mon Sep 17 00:00:00 2001 From: Harmen Stoppels Date: Mon, 11 Oct 2021 16:07:45 +0200 Subject: Only install env modifications in /.spack (#24081) - Do not store the full list of environment variables in /.spack/spack-build-env.txt because it may contain user secrets. - Only store environment variable modifications upon installation. - Variables like PATH may still contain user and system paths to make spack-build-env.txt sourceable. Variables containing paths are modified through prepending/appending, and if we don't apply these to the current environment variable, we end up with statements like `export PATH=/path/to/spack/bin` with system paths missing, meaning no system binaries are in the path, which is a bad user experience. - Do write the full environment to spack-build-env.txt in the staging dir, but ensure it is readonly for the current user, to make it a bit safer on shared systems. --- lib/spack/spack/analyzers/environment_variables.py | 3 ++ lib/spack/spack/build_environment.py | 47 +++++++++++++--------- lib/spack/spack/installer.py | 19 +++++++-- lib/spack/spack/package.py | 11 +++++ lib/spack/spack/test/cmd/install.py | 9 +++++ lib/spack/spack/test/install.py | 1 + lib/spack/spack/util/environment.py | 18 ++++++--- 7 files changed, 79 insertions(+), 29 deletions(-) (limited to 'lib') diff --git a/lib/spack/spack/analyzers/environment_variables.py b/lib/spack/spack/analyzers/environment_variables.py index 2d235e7a50..21a3d39cad 100644 --- a/lib/spack/spack/analyzers/environment_variables.py +++ b/lib/spack/spack/analyzers/environment_variables.py @@ -10,6 +10,8 @@ an index of key, value pairs for environment variables.""" import os +import llnl.util.tty as tty + from spack.util.environment import EnvironmentModifications from .analyzer_base import AnalyzerBase @@ -43,6 +45,7 @@ class EnvironmentVariables(AnalyzerBase): to remove path prefixes specific to user systems. """ if not os.path.exists(filename): + tty.warn("No environment file available") return mods = EnvironmentModifications.from_sourcing_file(filename) diff --git a/lib/spack/spack/build_environment.py b/lib/spack/spack/build_environment.py index 55ba2d3934..83aa634276 100644 --- a/lib/spack/spack/build_environment.py +++ b/lib/spack/spack/build_environment.py @@ -232,7 +232,7 @@ def clean_environment(): if '/macports/' in p: env.remove_path('PATH', p) - env.apply_modifications() + return env def set_compiler_environment_variables(pkg, env): @@ -765,43 +765,46 @@ def setup_package(pkg, dirty, context='build'): set_module_variables_for_package(pkg) - env = EnvironmentModifications() - - if not dirty: - clean_environment() + # Keep track of env changes from packages separately, since we want to + # issue warnings when packages make "suspicious" modifications. + env_base = EnvironmentModifications() if dirty else clean_environment() + env_mods = EnvironmentModifications() # setup compilers for build contexts need_compiler = context == 'build' or (context == 'test' and pkg.test_requires_compiler) if need_compiler: - set_compiler_environment_variables(pkg, env) - set_wrapper_variables(pkg, env) + set_compiler_environment_variables(pkg, env_mods) + set_wrapper_variables(pkg, env_mods) - env.extend(modifications_from_dependencies( + env_mods.extend(modifications_from_dependencies( pkg.spec, context, custom_mods_only=False)) # architecture specific setup platform = spack.platforms.by_name(pkg.spec.architecture.platform) target = platform.target(pkg.spec.architecture.target) - platform.setup_platform_environment(pkg, env) + platform.setup_platform_environment(pkg, env_mods) if context == 'build': - pkg.setup_build_environment(env) + pkg.setup_build_environment(env_mods) - if (not dirty) and (not env.is_unset('CPATH')): + if (not dirty) and (not env_mods.is_unset('CPATH')): tty.debug("A dependency has updated CPATH, this may lead pkg-" "config to assume that the package is part of the system" " includes and omit it when invoked with '--cflags'.") elif context == 'test': - env.extend( + env_mods.extend( inspect_path( pkg.spec.prefix, spack.user_environment.prefix_inspections(pkg.spec.platform), exclude=is_system_path ) ) - pkg.setup_run_environment(env) - env.prepend_path('PATH', '.') + pkg.setup_run_environment(env_mods) + env_mods.prepend_path('PATH', '.') + + # First apply the clean environment changes + env_base.apply_modifications() # Load modules on an already clean environment, just before applying Spack's # own environment modifications. This ensures Spack controls CC/CXX/... variables. @@ -823,12 +826,16 @@ def setup_package(pkg, dirty, context='build'): implicit_rpaths = pkg.compiler.implicit_rpaths() if implicit_rpaths: - env.set('SPACK_COMPILER_IMPLICIT_RPATHS', - ':'.join(implicit_rpaths)) + env_mods.set('SPACK_COMPILER_IMPLICIT_RPATHS', + ':'.join(implicit_rpaths)) # Make sure nothing's strange about the Spack environment. - validate(env, tty.warn) - env.apply_modifications() + validate(env_mods, tty.warn) + env_mods.apply_modifications() + + # Return all env modifications we controlled (excluding module related ones) + env_base.extend(env_mods) + return env_base def _make_runnable(pkg, env): @@ -1016,8 +1023,8 @@ def _setup_pkg_and_run(serialized_pkg, function, kwargs, child_pipe, if not kwargs.get('fake', False): kwargs['unmodified_env'] = os.environ.copy() - setup_package(pkg, dirty=kwargs.get('dirty', False), - context=context) + kwargs['env_modifications'] = setup_package( + pkg, dirty=kwargs.get('dirty', False), context=context) return_value = function(pkg, kwargs) child_pipe.send(return_value) diff --git a/lib/spack/spack/installer.py b/lib/spack/spack/installer.py index 2f1544ee04..11430bfc94 100644 --- a/lib/spack/spack/installer.py +++ b/lib/spack/spack/installer.py @@ -55,7 +55,7 @@ import spack.package_prefs as prefs import spack.repo import spack.store import spack.util.executable -from spack.util.environment import dump_environment +from spack.util.environment import EnvironmentModifications, dump_environment from spack.util.executable import which from spack.util.timer import Timer @@ -556,8 +556,8 @@ def log(pkg): log_file = os.path.join(os.path.dirname(packages_dir), log_file) fs.install(phase_log, log_file) - # Archive the environment used for the build - fs.install(pkg.env_path, pkg.install_env_path) + # Archive the environment modifications for the build. + fs.install(pkg.env_mods_path, pkg.install_env_path) if os.path.exists(pkg.configure_args_path): # Archive the args used for the build @@ -1730,6 +1730,10 @@ class BuildProcessInstaller(object): # env before starting installation self.unmodified_env = install_args.get('unmodified_env', {}) + # env modifications by Spack + self.env_mods = install_args.get( + 'env_modifications', EnvironmentModifications()) + # timer for build phases self.timer = Timer() @@ -1819,6 +1823,15 @@ class BuildProcessInstaller(object): # Save the build environment in a file before building. dump_environment(pkg.env_path) + # Save just the changes to the environment. This file can be + # safely installed, since it does not contain secret variables. + with open(pkg.env_mods_path, 'w') as env_mods_file: + mods = self.env_mods.shell_modifications( + explicit=True, + env=self.unmodified_env + ) + env_mods_file.write(mods) + for attr in ('configure_args', 'cmake_args'): try: configure_args = getattr(pkg, attr)() diff --git a/lib/spack/spack/package.py b/lib/spack/spack/package.py index 5a0924707d..0e9e07f52e 100644 --- a/lib/spack/spack/package.py +++ b/lib/spack/spack/package.py @@ -73,6 +73,9 @@ _spack_build_logfile = 'spack-build-out.txt' # Filename for the Spack build/install environment file. _spack_build_envfile = 'spack-build-env.txt' +# Filename for the Spack build/install environment modifications file. +_spack_build_envmodsfile = 'spack-build-env-mods.txt' + # Filename of json with total build and phase times (seconds) _spack_times_log = 'install_times.json' @@ -1041,6 +1044,14 @@ class PackageBase(six.with_metaclass(PackageMeta, PackageViewMixin, object)): else: return os.path.join(self.stage.path, _spack_build_envfile) + @property + def env_mods_path(self): + """ + Return the build environment modifications file path associated with + staging. + """ + return os.path.join(self.stage.path, _spack_build_envmodsfile) + @property def metadata_dir(self): """Return the install metadata directory.""" diff --git a/lib/spack/spack/test/cmd/install.py b/lib/spack/spack/test/cmd/install.py index a8a8c8b556..7fcf3658aa 100644 --- a/lib/spack/spack/test/cmd/install.py +++ b/lib/spack/spack/test/cmd/install.py @@ -175,6 +175,15 @@ def test_install_with_source( os.path.join(src, 'configure')) +def test_install_env_variables( + mock_packages, mock_archive, mock_fetch, config, install_mockery +): + spec = Spec('libdwarf') + spec.concretize() + install('libdwarf') + assert os.path.isfile(spec.package.install_env_path) + + @pytest.mark.disable_clean_stage_check def test_show_log_on_error(mock_packages, mock_archive, mock_fetch, config, install_mockery, capfd): diff --git a/lib/spack/spack/test/install.py b/lib/spack/spack/test/install.py index 67d50d2273..a0f8311a22 100644 --- a/lib/spack/spack/test/install.py +++ b/lib/spack/spack/test/install.py @@ -543,6 +543,7 @@ def test_log_install_with_build_files(install_mockery, monkeypatch): with fs.working_dir(log_dir): fs.touch(log_path) fs.touch(spec.package.env_path) + fs.touch(spec.package.env_mods_path) fs.touch(spec.package.configure_args_path) install_path = os.path.dirname(spec.package.install_log_path) diff --git a/lib/spack/spack/util/environment.py b/lib/spack/spack/util/environment.py index ef9f1a8b1b..398af5f018 100644 --- a/lib/spack/spack/util/environment.py +++ b/lib/spack/spack/util/environment.py @@ -128,7 +128,8 @@ def dump_environment(path, environment=None): use_env = environment or os.environ hidden_vars = set(['PS1', 'PWD', 'OLDPWD', 'TERM_SESSION_ID']) - with open(path, 'w') as env_file: + fd = os.open(path, os.O_WRONLY | os.O_CREAT, 0o600) + with os.fdopen(fd, 'w') as env_file: for var, val in sorted(use_env.items()): env_file.write(''.join(['#' if var in hidden_vars else '', env_var_to_source_line(var, val), @@ -596,21 +597,26 @@ class EnvironmentModifications(object): for x in actions: x.execute(env) - def shell_modifications(self, shell='sh'): + def shell_modifications(self, shell='sh', explicit=False, env=None): """Return shell code to apply the modifications and clears the list.""" modifications = self.group_by_name() new_env = os.environ.copy() + if env is None: + env = os.environ + + new_env = env.copy() + for name, actions in sorted(modifications.items()): for x in actions: x.execute(new_env) cmds = '' - for name in set(new_env) | set(os.environ): + for name in sorted(set(modifications)): new = new_env.get(name, None) - old = os.environ.get(name, None) - if new != old: + old = env.get(name, None) + if explicit or new != old: if new is None: cmds += _shell_unset_strings[shell].format(name) else: @@ -989,7 +995,7 @@ def environment_after_sourcing_files(*files, **kwargs): # go with sys.executable. Below we just need a working # Python interpreter, not necessarily sys.executable. python_cmd = executable.which('python3', 'python', 'python2') - python_cmd = python_cmd.name if python_cmd else sys.executable + python_cmd = python_cmd.path if python_cmd else sys.executable dump_cmd = 'import os, json; print(json.dumps(dict(os.environ)))' dump_environment = python_cmd + ' -E -c "{0}"'.format(dump_cmd) -- cgit v1.2.3-60-g2f50