Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

build_env: allowlist rather than blocklist #30015

Open
wants to merge 11 commits into
base: develop
Choose a base branch
from
Open

build_env: allowlist rather than blocklist #30015

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
9b86641
build_env: whitelist rather than blacklist
trws Apr 11, 2022
9ca6062
use dirty in the env modification tests so that the modifications app…
trws Apr 12, 2022
aca3a96
fix windows support, refactor
trws Apr 13, 2022
0eda725
debugging RTD ioctl failures
trws Apr 19, 2022
a954c69
use getpass.getuser() by default, fall back to os.getlogin only for s…
trws Apr 19, 2022
3235e39
comment style fix
trws Apr 20, 2022
2b34b07
pick error type by python version
trws Apr 20, 2022
ebd5d29
refine importerror check
trws Apr 20, 2022
55c7059
clear PKGCONF by default even on cray
trws Apr 21, 2022
cd68fe5
merge from develop (last sync was 2 years ago)
scheibelp Jan 25, 2024
4c3d363
auto style fix (black formatting was introduced since this PR was las…
scheibelp Jan 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
105 changes: 35 additions & 70 deletions lib/spack/spack/build_environment.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,89 +197,54 @@ def clean_environment():
# unlike the other functions so it doesn't overwrite what the modules load.
env = EnvironmentModifications()

# Remove these vars from the environment during build because they
# can affect how some packages find libraries. We want to make
# sure that builds never pull in unintended external dependencies.
env.unset("LD_LIBRARY_PATH")
env.unset("LD_RUN_PATH")
env.unset("DYLD_LIBRARY_PATH")
env.unset("DYLD_FALLBACK_LIBRARY_PATH")

# These vars affect how the compiler finds libraries and include dirs.
env.unset("LIBRARY_PATH")
env.unset("CPATH")
env.unset("C_INCLUDE_PATH")
env.unset("CPLUS_INCLUDE_PATH")
env.unset("OBJC_INCLUDE_PATH")

env.unset("CMAKE_PREFIX_PATH")
env.unset("PYTHONPATH")
env.unset("R_HOME")
env.unset("R_ENVIRON")

env.unset("LUA_PATH")
env.unset("LUA_CPATH")

# Affects GNU make, can e.g. indirectly inhibit enabling parallel build
# env.unset('MAKEFLAGS')

# Avoid that libraries of build dependencies get hijacked.
env.unset("LD_PRELOAD")
env.unset("DYLD_INSERT_LIBRARIES")

# Avoid <packagename>_ROOT user variables overriding spack dependencies
# https://cmake.org/cmake/help/latest/variable/PackageName_ROOT.html
# Spack needs SPACK_ROOT though, so we need to exclude that
for varname in os.environ.keys():
if varname.endswith("_ROOT") and varname != "SPACK_ROOT":
env.unset(varname)
env_name_whitelist = list(
map(
re.compile,
(
r"SPACK",
r"^SPACK_.*", # keep SPACK_ROOT and company
r"EDITOR",
r"PATH",
Copy link
Member

Choose a reason for hiding this comment

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

As observed in #30015 (comment), we'd have to construct a minimal PATH with a set of utilities in order to clean it (i.e. point to some new PATH dir with the minimal set of executables, and start with PATH set to this one entry to achieve the best isolation we can).

r"PATHEXT",
),
)
)

if sys.platform == "win32":
env_name_whitelist.extend(
map(
re.compile,
(
r"SPACKINSTDIR", # One random windows one
r"TMP",
r"TEMP",
r"SYSTEMDRIVE",
r"SYSTEMROOT", # Required for cl.exe
),
)
)

# Clear *everything* not in the whitelist
for n in os.environ.keys():
if not any(r.match(n) for r in env_name_whitelist):
env.unset(n)

# On Cray "cluster" systems, unset CRAY_LD_LIBRARY_PATH to avoid
# interference with Spack dependencies.
# CNL requires these variables to be set (or at least some of them,
# depending on the CNL version).
on_cray, using_cnl = _on_cray()
if on_cray and not using_cnl:
env.unset("CRAY_LD_LIBRARY_PATH")
for varname in os.environ.keys():
if "PKGCONF" in varname:
env.unset(varname)

# Unset the following variables because they can affect installation of
# Autotools and CMake packages.
build_system_vars = [
"CC",
"CFLAGS",
"CPP",
"CPPFLAGS", # C variables
"CXX",
"CCC",
"CXXFLAGS",
"CXXCPP", # C++ variables
"F77",
"FFLAGS",
"FLIBS", # Fortran77 variables
"FC",
"FCFLAGS",
"FCLIBS", # Fortran variables
"LDFLAGS",
"LIBS", # linker variables
]
for v in build_system_vars:
env.unset(v)

# Unset mpi environment vars. These flags should only be set by
# mpi providers for packages with mpi dependencies
mpi_vars = ["MPICC", "MPICXX", "MPIFC", "MPIF77", "MPIF90"]
for v in mpi_vars:
env.unset(v)
if on_cray and using_cnl:
env.restore("CRAY_LD_LIBRARY_PATH")

build_lang = spack.config.get("config:build_language")
if build_lang:
# Override language-related variables. This can be used to force
# English compiler messages etc., which allows parse_log_events to
# show useful matches.
env.set("LC_ALL", build_lang)
else:
env.restore("LC_ALL")

# Remove any macports installs from the PATH. The macports ld can
# cause conflicts with the built-in linker on el capitan. Solves
Expand Down
4 changes: 2 additions & 2 deletions lib/spack/spack/test/build_environment.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,8 +273,8 @@ def platform_pathsep(pathlist):
# Monkeypatch a pkg.compiler.environment with the required modifications
pkg = spack.spec.Spec("cmake").concretized().package
monkeypatch.setattr(pkg.compiler, "environment", modifications)
# Trigger the modifications
spack.build_environment.setup_package(pkg, False)
# Trigger the modifications, set dirty so these aren't removed
spack.build_environment.setup_package(pkg, True)

# Check they were applied
for name, value in expected.items():
Expand Down
10 changes: 10 additions & 0 deletions lib/spack/spack/util/environment.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -469,6 +469,16 @@ def _trace(self) -> Optional[Trace]:

return Trace(filename=filename, lineno=lineno, context=current_context)

def restore(self, name, **kwargs):
"""Stores a request to restore a possibly cleared environment variable to its
current value if that variable is currently set.

Args:
name: name of the environment variable to be restored
"""
if name in os.environ:
self.set(name, os.environ[name])
Copy link
Member

Choose a reason for hiding this comment

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

@trws nothing actually handles this request, does it? In that sense it seems incomplete

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The enclosing class is our list of environment modifications to be made after clearing the environment, so this re-uses the normal set method to add the contents of the current environment to the list to be restored. If there's something wrong with it I'm not sure what.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, that makes sense, e.g. for

env.restore("CRAY_LD_LIBRARY_PATH")

you are adding the original value as a new set operation after unsetting it, the important thing being that the env modifications haven't been executed on the current environment so we are recording the unset followed by the set. Is there a reason to just not unset them in the first place (e.g. include them in the whitelist)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly. I had to dig back through it a bit to try to remember. The end effect is very similar to adding it to the whitelist, I think it was meant to allow packages to explicitly do this in things like setup_build_environment or similar without having to pass around the whitelist. I can't remember why that particular one is done that way though.


@system_env_normalize
def set(self, name: str, value: str, *, force: bool = False, raw: bool = False):
"""Stores a request to set an environment variable.
Expand Down
Loading