Allow compiler wrapper to modify environment#2275
Conversation
you could just adjust the author of your commit you can get the name and e-mail from |
tgamblin
left a comment
tgamblin
left a comment
There was a problem hiding this comment.
Looks awesome!
Only minor thing is can see is the potential name conflict with SPACK_*.
w.r.t. authorship, I think you could take @davydden's suggestion, or you could preserve @hegner's original commits by just grabbing the head of develop, trying to merge his PR, and resolving the conflict by replacing it with your rewrite. That would have the advantage of giving you some credit for merging, while preserving his original commits too.
lib/spack/spack/build_environment.py
Outdated
| if 'set' in environment: | ||
| env_to_set = environment['set'] | ||
| for key, value in env_to_set.iteritems(): | ||
| env.set('SPACK_%s' % key, value) |
There was a problem hiding this comment.
I think this makes it possible for certain user variables to overwrite Spack's various cc control variables. e.g., what happens if the user says (for who knows what reason) that CC should be set a certain way while the compiler runs -- it will conflict with SPACK_CC. Should this use a different prefix, like SPACK_ENV_SET_ or similar?
There was a problem hiding this comment.
I think this (and the other comments) can be handled by modifying the environment variables in build_environment vs. passing things through to the compiler wrapper. It turns out this was originally done because the wrappers unset these variables.
There was a problem hiding this comment.
It's actually still important that these variables only be set for the compilers. If the compiler depends on some external library, we don't want that to pollute the rest of the build. If you put this in the cc wrapper, only the compiler sees the extra LD_LIBRARY_PATH.
| env.set('%s' % key, value) | ||
| # Let shell know which variables to set | ||
| env_variables = ":".join(env_to_set.keys()) | ||
| env.set('SPACK_ENV_TO_SET', env_variables) |
There was a problem hiding this comment.
It may pollute the compiler namespace a little less if you use a single variable for the bundled user environment. e.g. SPACK_ENV_TO_SET=VAR1=VAULE1:VAR2=VALUE2:...
I can't actually decide if it matters much so this is minor. The approach above adds a lot of variables but they're easy to grep for if you run spack env bash, so that might be better for debugging. This approach adds fewer names to the compiler's environment. I'm not sure either is compelling from an environment size perspective -- the OS size limits are typically for the total environment and not per variable.
lib/spack/env/cc
Outdated
| # | ||
| IFS=':' read -ra varnames <<< "$SPACK_ENV_TO_SET" | ||
| for varname in "${varnames[@]}"; do | ||
| spack_varname="SPACK_$varname" |
There was a problem hiding this comment.
This should probably unset the input variables as it reads them, to limit the env size in the chid process. Not that I expect people to cram tons of info in their compiler settings, but some of our users are... shall we say... aggressive with env vars.
scheibelp
force-pushed
the
features/compiler-ld-lib-path
branch
from
c806d61 to
64b0525
Compare
So far I have left this as is.
Done
IMO yes - done |
This adds the ability to set environment variables in the compiler wrappers. These are specified as part of the compilers.yaml config. The user may also specify RPATHs in compilers.yaml that should be added.
scheibelp
force-pushed
the
features/compiler-ld-lib-path
branch
from
64b0525 to
7d943d1
Compare
|
@scheibelp - I am fine w/ everything as long as the feature enters. Thanks for all the work! :-) |
* Allow compiler wrapper to modify environment This adds the ability to set environment variables in the compiler wrappers. These are specified as part of the compilers.yaml config. The user may also specify RPATHs in compilers.yaml that should be added. * Minor doc tweak
4 participants
This is entirely copied from #943, just modified to be up to date with the current spack. There's probably some better way to handle this since all credit goes to @hegner but for now I'll place this here and mark it WIP.
This adds the ability to set environment variables in the compiler
wrappers. These are specified as part of the compilers.yaml config.