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

envPairs works in vm, nims #18615

Merged
merged 2 commits into from
Sep 29, 2021
Merged

envPairs works in vm, nims #18615

merged 2 commits into from
Sep 29, 2021

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented Jul 30, 2021

all osenv APIs now work and are tested in all backends (c,cpp,vm,nims,js with -d:nodejs)

(note: before PR, getEnv, existsEnv, delEnv, putEnv worked in VM but not envPairs; etc)

@@ -19,29 +21,3 @@ block:
if not isWindows:
doAssert cwd.isAbsolute
doAssert relativePath(getCurrentDir() / "foo", "bar") == "../foo"

import std/sequtils
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these are all covered in tosenv

var gEnv {.importc: "environ", header: "<stdlib.h>".}: cstringArray
var gEnv {.importc: "environ".}: cstringArray

iterator envPairsImpl(): tuple[key, value: string] {.tags: [ReadEnvEffect].} =
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pure re-indentation until this point

@timotheecour timotheecour added the Ready For Review (please take another look): ready for next review round label Jul 30, 2021
@timotheecour timotheecour requested a review from ringabout July 30, 2021 03:35
@timotheecour
Copy link
Member Author

ping @Araq

@timotheecour
Copy link
Member Author

ping @Araq before this bitrots again, I rebased after this had bit-rotted

@Araq Araq merged commit f061971 into nim-lang:devel Sep 29, 2021
@timotheecour timotheecour deleted the pr_envPairs_vm branch September 29, 2021 17:54
timotheecour added a commit to timotheecour/website that referenced this pull request Sep 30, 2021
@timotheecour
Copy link
Member Author

timotheecour commented Sep 30, 2021

@Araq ok to backport this to 1.6 branch? fits well with other improvements to environment variables made in this release, so that we can claim:

osenv APIs now work and are tested in all backends (c,cpp,vm,nims,js with -d:nodejs)

instead of some weird subset of this statement

@Araq
Copy link
Member

Araq commented Sep 30, 2021

I think it's too risky as the diff is quite large, even ignoring the indentation changes.

PMunch pushed a commit to PMunch/Nim that referenced this pull request Mar 28, 2022
* envPairs works in vm, nims

* fixup
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.

3 participants