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

Implement nimble shellenv and nimble shell #1081

Merged
merged 2 commits into from
Apr 27, 2023
Merged

Conversation

yyoncho
Copy link
Contributor

@yyoncho yyoncho commented Mar 21, 2023

Added commands nimble shell and nimble shellenv to allow using shell in the
scope of the project

Added commands `nimble shell` and `nimble shellenv` to allow using shell in the
scope of the project
src/nimble.nim Outdated
let fullInfo = pkg.toFullInfo(options)
for bin, _ in fullInfo.bin:
let folder = fullInfo.getOutputDir(bin).parentDir.quoteShell
result = fmt "{folder}{separator}{result}"
Copy link
Member

Choose a reason for hiding this comment

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

This algorithm is needlessly quadratic. You can construct the list of nimble PATH additions by regular appending and then concatenate it once with the original PATH variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will be fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in the latest commit.

src/nimble.nim Outdated
if shell == "": shell = "powershell"
else:
var shell = getEnv("SHELL")
if shell == "": shell = "/bin/bash"
Copy link
Member

Choose a reason for hiding this comment

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

Launching /usr/bin/env bash would be more portable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will be fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pushed a slightly different fix: I use bash and pass poUserPath. This approach leads to a bit simpler code(no need to track args)

src/nimble.nim Outdated
var shell = getEnv("SHELL")
if shell == "": shell = "/bin/bash"

discard waitForExit startProcess(shell, options = {poParentStreams})
Copy link
Member

Choose a reason for hiding this comment

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

On POSIX systems, using execve (or a similar function) would reduce the system memory usage here:

https://man7.org/linux/man-pages/man2/execve.2.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was under the assumption that poParentStreams will end up using that. I will check that out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tracked the code and it indeed calls execve under the hood. That particular invocation: https://github.com/yyoncho/Nim/blob/optimize-nimble/lib/pure/osproc.nim#L1176

Copy link
Member

@zah zah Apr 27, 2023

Choose a reason for hiding this comment

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

No, you are seeing the classic fork/exec pattern.

No invocation of startProcess will be equivalent to execve because startProcess as an API returns the result of the launched process and the execution of the original program continues. The point of execve is to essentially terminate the execution of the current program by replacing its address space with the launched new process.

@zah zah merged commit 0bd5146 into nim-lang:master Apr 27, 2023
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