-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Replace calls to putenv
with setenv
#18530
Conversation
this PR also improves #18533 but doesn't fix it; followup PR welcome to remove tracking of |
Welcome!, nice code. 🙂:+1:
If you are at it, maybe check the |
Looks like the new behavior of |
See https://man7.org/linux/man-pages/man3/setenv.3.html#ERRORS, I could match on the error code to be able to throw a ValueError instead if EINVAL is returned. |
@brightly-salty see my reply in timotheecour#782 (comment); your PR fixes timotheecour#782 ; please add a test using
out of scope for this PR, and probably not a good idea; other OS calls don't do that |
Co-authored-by: Timothee Cour <[email protected]>
Co-authored-by: Timothee Cour <[email protected]>
Co-authored-by: Timothee Cour <[email protected]>
Co-authored-by: Timothee Cour <[email protected]>
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
in future work, we could add an overload that exposes the overwrite param, it's a useful thing to be able to not overwrite if env var already exists; eg:
proc putEnv*(key, val: string, overwrite = true)
This needs to be tested on MSVC too, see also https://stackoverflow.com/questions/17258029/c-setenv-undefined-identifier-in-visual-studio |
Note: I need to test this manually for MSCV before merging. |
Can confirm that the tests work on Windows with MSCV. |
Thanks for fixing it @brightly-salty 🙂 |
* Replace calls to C `putenv` with C `setenv` to remove possible memory leaks * Add test of correct behaviour on invalid input * Fix style in tests/stdlib/tos.nim Co-authored-by: Timothee Cour <[email protected]> * Update tests/stdlib/tos.nim Co-authored-by: Timothee Cour <[email protected]> * Update tests/stdlib/tos.nim Co-authored-by: Timothee Cour <[email protected]> * Add comment with bug number to tests/stdlib/tos.nim Co-authored-by: Timothee Cour <[email protected]> * Fix possible msvc arch issues Co-authored-by: Timothee Cour <[email protected]>
Fixes #18502 which could lead to memory leaks if left unchecked
in
os.putenv(k, v)
, replaces calls to Cputenv("k=v")
with calls to Csetenv(k, v, 1)
First time contributor here, let me know if anything else is needed of me