-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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.
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
build_env: allowlist rather than blocklist #30015
base: develop
Are you sure you want to change the base?
Changes from all commits
9b86641
9ca6062
aca3a96
0eda725
a954c69
3235e39
2b34b07
ebd5d29
55c7059
cd68fe5
4c3d363
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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]) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, that makes sense, e.g. for
you are adding the original value as a new There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
|
There was a problem hiding this comment.
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).