Only install env modifications in <prefix>/.spack (#24081)

- Do not store the full list of environment variables in
  <prefix>/.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.

7 files changed, 79 insertions, 29 deletions

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'
 
@@ -1042,6 +1045,14 @@ class PackageBase(six.with_metaclass(PackageMeta, PackageViewMixin, object)):
             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."""
         return spack.store.layout.metadata_path(self.spec)
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)