-
Notifications
You must be signed in to change notification settings - Fork 6k
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
[core] Implement scoped env setter #50396
Conversation
Signed-off-by: dentiny <[email protected]>
70ddbe1
to
38a74f4
Compare
@dayshah could you review? |
@dentiny don't see it used anywhere -- are there tests you'd update to use this? |
It's used here: ray/src/ray/util/tests/pipe_logger_test.cc Lines 37 to 43 in be1b38f
The PR's still pending review, not merged yet |
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.
lgtm but maybe overkill to have a utility if we only have one use case for it rn?
ScopedEnvSetter::~ScopedEnvSetter() { | ||
unsetEnv(env_name_.c_str()); | ||
if (old_value_.has_value()) { | ||
setEnv(env_name_, *old_value_); |
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.
idk how this will be used but are races with setenv a concern -https://en.cppreference.com/w/cpp/utility/program/getenv?
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.
Only expect to be useful in unit tests at the moment, not a big concern for thread-safety.
Speaking of which, strerror is also not thread-safe, might be a bigger concern.
Signed-off-by: dentiny <[email protected]>
I imagine it'd be useful in unit tests, no intended use cases other than that. |
When working on pipe logger (#50248), I found it necessary to test based on a scoped env, so make a formal util.