Skip to content
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

Remove tracking of environment from osenv.nim v2 #18575

Merged
merged 5 commits into from
Jul 29, 2021

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented Jul 25, 2021

the 1st commit is squashed from #18535 and keeps the author's name

note

code to support empty values on windows (putEnv("foo", "")) is inspired from facebook/folly@5d8ca09, see also https://github.com/facebook/folly/blob/master/folly/portability/Stdlib.cpp

this is the best approach I found after extensive research; it allows having same semantics across backends (c,cpp,js,vm) and os's (osx, linux, windows)

future work

links

@timotheecour timotheecour changed the title WIP Remove tracking of environment from osenv.nim v2 Jul 25, 2021
@timotheecour timotheecour marked this pull request as ready for review July 25, 2021 18:27
@timotheecour timotheecour added the Ready For Review (please take another look): ready for next review round label Jul 27, 2021
@Araq
Copy link
Member

Araq commented Jul 28, 2021

Does this code still work?

putEnv("key", "value")
assert getEnv("key") == "value"

Because 11 years ago when I wrote the code, it didn't and I had to come up with the environment workaround...

@timotheecour
Copy link
Member Author

timotheecour commented Jul 28, 2021

Does this code still work?

yes, i just double-checked; if that didn't work nothing would; more importantly, it's covered in the tests, see tosenv.nim which has increased coverage compared to before this PR (c,cpp,js,vm + threads + more testing)

EDIT: #18530 (which was merged last week) is also an ingredient that makes this work, by avoiding having to hold on to the strings by using setenv instead of putenv.

@timotheecour
Copy link
Member Author

@Araq PTAL

@Araq Araq merged commit 6b3c77e into nim-lang:devel Jul 29, 2021
@timotheecour timotheecour deleted the pull_18535_osenv_windows branch July 29, 2021 21:27
PMunch pushed a commit to PMunch/Nim that referenced this pull request Mar 28, 2022
* Remove unnecessary environment tracking

* try to fix windows

* fix delEnv

* make putEnv work on windows even with empty values; improve tests: add tests, add js, vm testing

* [skip ci] fix changelog

Co-authored-by: Caden Haustein <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready For Review (please take another look): ready for next review round
Projects
None yet
Development

Successfully merging this pull request may close these issues.

getEnv + friends gives SIGSEGV or wrong results; osenv should not try to keep track of environment
3 participants