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

osproc.execCmdEx now takes an optional input for stdin, env, workingDir #14211

Merged
merged 2 commits into from
May 13, 2020

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented May 4, 2020

osproc.execCmdEx now takes an optional input for stdin. This is a common use case, and avoids the bashism input | cmd for common cases where input is small enough to avoid blocking (eg on OSX, 140_000 does not block but 150_000 blocks)

  • as noted, writing stdin by chunks (in case of large stdin) would require selectors (kqueue/epoll etc) otherwise you'll end up blocking either when writing to stdin (pipe full) or reading from stdout (output empty). This can be fixed in future work (actually I'm advocating for a osproc2 that fixes all the design issues with osproc, but that's left as future work).

  • the runnableExample comment is intentionally not a doc comment but may be useful for people looking at the code (it hits ^[[0m is inserted in stderr for echo code | nim c - timotheecour/Nim#152 depending on your user config)

  • the when (NimMajor, NimMinor, NimPatch) < (1, 3, 3): doAssert input.len == 0 is a compromise for the sake of simplicity (avoiding to rewrite a separate template etc)

  • env, workingDir are also added for more consistency with rest of osproc and to avoid more bashisms (eg "cd $dir && cmd" or other existing kludges)

@Araq
Copy link
Member

Araq commented May 4, 2020

This needs to have a test so that we know it works on all CI'ed OSes.

@timotheecour timotheecour force-pushed the pr_execCmdEx_stdin branch from d7bedac to 56d16a3 Compare May 4, 2020 09:53
@timotheecour
Copy link
Member Author

PTAL, done (ya, nim doc isn't run on all CI OS's anymore...)

@timotheecour timotheecour force-pushed the pr_execCmdEx_stdin branch 2 times, most recently from 11a3260 to 40b1aed Compare May 6, 2020 11:54
@timotheecour timotheecour changed the title osproc.execCmdEx now takes an optional input for stdin osproc.execCmdEx now takes an optional input for stdin, env, workingDir May 6, 2020
@timotheecour timotheecour force-pushed the pr_execCmdEx_stdin branch 5 times, most recently from 1dbbbd7 to 3054713 Compare May 11, 2020 01:39
@timotheecour timotheecour force-pushed the pr_execCmdEx_stdin branch 3 times, most recently from 038ad1e to a953da6 Compare May 12, 2020 06:44
@Araq Araq merged commit 041ee92 into nim-lang:devel May 13, 2020
@timotheecour timotheecour deleted the pr_execCmdEx_stdin branch May 13, 2020 11:59
EchoPouet pushed a commit to EchoPouet/Nim that referenced this pull request Jun 13, 2020
…rkingDir (nim-lang#14211)

* `osproc.execCmdEx` now takes an optional `input` for stdin

* execCmdEx now also takes an optional ``workingDir` and `env`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants