Don't let runtime env variables of compiler like deps leak into the build environment#40916
Merged
trws merged 4 commits intospack:developfrom Nov 6, 2023
Conversation
Adds `drop` to EnvironmentModifications courtesy of @haampie, and uses it to clear modifications of CC, CXX, F77 and FC made by `setup_{,dependent_}run_environment` routines when producing an environment in BUILD context.
spackbot-app
bot
added
build-environment
core
Member
Author
|
@spackbot run pipeline |
|
I've started that pipeline for you! |
trws
approved these changes
Nov 6, 2023
tgamblin
requested changes
Nov 6, 2023
Member
tgamblin
left a comment
tgamblin
left a comment
There was a problem hiding this comment.
LGTM -- minor request and we're good
| pkg.setup_dependent_run_environment(run_env_mods, spec) | ||
| pkg.setup_run_environment(run_env_mods) | ||
| if self.context == Context.BUILD: | ||
| # Don't let the runtime environment of comiler like dependencies leak into the |
Member
There was a problem hiding this comment.
Suggested change
| # Don't let the runtime environment of comiler like dependencies leak into the | |
| # Don't let the runtime environment of compiler-like dependencies leak into the |
Member
Author
There was a problem hiding this comment.
Haven't you heard about the co-miler?
| pkg.setup_run_environment(env) | ||
| pkg.setup_dependent_run_environment(run_env_mods, spec) | ||
| pkg.setup_run_environment(run_env_mods) | ||
| if self.context == Context.BUILD: |
Member
There was a problem hiding this comment.
Add a TODO here to handle this better when we have proper "compiler" dependencies, mention this is a stopgap
Member
|
Heh ok looks like Tom beat me to it. This is good -- we DO need to sort out how this will work with compiler deps. |
haampie
deleted the
fix/dont-let-runtime-env-variables-mess-up-build-environment
branch
gabrielctn
pushed a commit
to gabrielctn/spack
that referenced
this pull request
Nov 24, 2023
…uild environment (spack#40916) * Test that setup_run_environment changes to CC/CXX/FC/F77 are dropped in build env * compilers set in run env shouldn't impact build Adds `drop` to EnvironmentModifications courtesy of @haampie, and uses it to clear modifications of CC, CXX, F77 and FC made by `setup_{,dependent_}run_environment` routines when producing an environment in BUILD context. * comment / style * comment --------- Co-authored-by: Tom Scogland <[email protected]>
This was referenced Nov 29, 2023
mtaillefumier
pushed a commit
to mtaillefumier/spack
that referenced
this pull request
Dec 14, 2023
…uild environment (spack#40916) * Test that setup_run_environment changes to CC/CXX/FC/F77 are dropped in build env * compilers set in run env shouldn't impact build Adds `drop` to EnvironmentModifications courtesy of @haampie, and uses it to clear modifications of CC, CXX, F77 and FC made by `setup_{,dependent_}run_environment` routines when producing an environment in BUILD context. * comment / style * comment --------- Co-authored-by: Tom Scogland <[email protected]>
Contributor
|
We used setup_run_environment() to clear modifications to PATH. This change breaks that. Is there any way we can now unset PATH modifications for a package? It is set by _make_runnable() and can no longer be unset by setup_run_environment() with env.clear(). Edit: Ah. An explicit removal with env.remove_path() works :-) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When depending on libllvm or libgcc as a build/link type dep, we don't want
their
setup_run_environmentchanges to modify CC / CXX / F77 / FC, sinceSpack determines the actual compiler and should manage those variables.
We still want module files to be generated with those CC / CXX / F77 / FC
variables, so it would be wrong to remove those variables from
setup_run_environment.The alternative would be to setup the compiler env changes last, but quite
some packages already rely on setting CXX in
setup_build_environment,and we also don't want to break that.