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

Allow for passing stdin to child #106

Merged
merged 17 commits into from
Oct 6, 2023

Conversation

CompeyDev
Copy link
Contributor

@CompeyDev CompeyDev commented Sep 18, 2023

This PR adds functionality for passing stdin contents to a child process.

Things to finish before merge:

  • Update FromLua implementation for Process options (Rust)
  • Implement writer for stdin (Rust)
  • Update SpawnOptions type (Luau)
  • Include tests (Luau)
  • Proper error handling

Closes #89.

@CompeyDev CompeyDev changed the title Feature/process stdin Allow for passing stdin to child Sep 18, 2023
@CompeyDev
Copy link
Contributor Author

CompeyDev commented Sep 19, 2023

Tests seem to fail because certain linux commands cannot be found on Windows. We can fix this by setting up MinGW on the CI for Windows targets only.

Here is how it would look like:

- name: Set up MinGW
  uses: egor-tensin/setup-mingw@v2
  with:
    platform: x64
  if: ${{matrix.runner-os == 'windows-latest'}}

We might also want to set the cargo-target within the matrix to x86_64-pc-windows-gnu.

CC @filiptibell

@CompeyDev CompeyDev force-pushed the feature/process-stdin branch from cc78af5 to e1c316f Compare September 19, 2023 05:53
@CompeyDev CompeyDev mentioned this pull request Sep 22, 2023
SnorlaxAssist added a commit to SnorlaxAssist/lune that referenced this pull request Sep 24, 2023
Included MinGW installation from lune-org#106 (comment)
Copy link
Collaborator

@filiptibell filiptibell left a comment

Choose a reason for hiding this comment

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

Thank you for the PR! I left a few comments then this should be all good 👍

src/lune/builtins/process/mod.rs Outdated Show resolved Hide resolved
src/lune/builtins/process/options.rs Outdated Show resolved Hide resolved
tests/process/spawn.luau Outdated Show resolved Hide resolved
src/lune/builtins/process/options.rs Outdated Show resolved Hide resolved
filiptibell pushed a commit to SnorlaxAssist/lune that referenced this pull request Sep 25, 2023
Included MinGW installation from lune-org#106 (comment)
@filiptibell
Copy link
Collaborator

Everything looks good code-wise but the windows tests still seem to fail - any idea why that is?

@CompeyDev
Copy link
Contributor Author

Everything looks good code-wise but the windows tests still seem to fail - any idea why that is?

Windows includes additional symbols with echo (not just a newline, but other things such as return carriages, etc. IIRC). I'm not exactly sure what they are, and it's kind of difficult for me to debug them either, considering I don't have access to a Windows instance.

@SnorlaxAssist Could you help out with this? Char codes of these special characters are 239 and 191. What are their significance?

@SnorlaxAssist
Copy link
Contributor

SnorlaxAssist commented Oct 2, 2023

Windows does include additional symbols, although that isn't the problem. The issue is on windows, you would be executing the command powershell -c echo then stdin input would go through but not finish executing, by not "pressing enter" to confirm the command.

So then you would get something like this

cmdlet Write-Output at command pipeline position 1
Supply values for the following parameters:
InputObject[0]: InputObject[1]: Hello from child process!

You can do something like this to confirm the execution & below is the stdout value I got from my virtual machine.

-- Passing stdin strings should work
local input = echoMessage
if IS_WINDOWS then
	-- Confirm input on Windows.
	-- Arg 0 would be default from process.spawn
	-- Confirm message input, and execute with a blank 2nd input.
	input ..= "\n\n"
end
local stdinChild = process.spawn((((not IS_WINDOWS) and "xargs") or "powershell"), {
	"echo",
}, {
	stdin = input,
})
--[[
stdout would includes the stdin value I believe, might be an stdout/stdin bug, not too sure
stdout:
1: Hello from child process!
2: 
3: Hello from child process!
4: 
]]
assert(
	...

@CompeyDev
Copy link
Contributor Author

Unfortunately, that does not pass either! I don't think it has anything to do with what you previously mentioned as lune literally gets a handle to the process and injects to its stdin, it does not run another shell command to pipe stdin (which may require user input, such as an enter).

I think this has something to do with some special chars with codes 239 and 191 getting prepended to the stdout (which I noticed in the CI a bit ago).

@SnorlaxAssist
Copy link
Contributor

Where exactly are you finding those special characters? I don't see those characters in the test & at the output on a VM.

Comment on lines 165 to 174
local stdinChild = process.spawn((((not IS_WINDOWS) and "xargs") or "powershell"), {
"echo",
}, {
stdin = (IS_WINDOWS and (echoMessage .. "\n\n")) or echoMessage,
})

assert(
stdinChild.stdout == (((IS_WINDOWS and `{string.char(239)}{string.char(191)}`) or "") .. echoMessage .. "\n"), -- Windows "echo" adds these "escape codes" to strings
"Stdin passing did not return proper output"
)
Copy link
Contributor

Choose a reason for hiding this comment

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

try this

local stdinChild = process.spawn(not IS_WINDOWS and "xargs" or "powershell", {
	"echo",
}, {
	stdin = echoMessage .. (IS_WINDOWS and "\n\n" or ""),
})

local stdinChildOut = stdinChild.stdout
if IS_WINDOWS then
	stdinChildOut = stdinChildOut:sub(#stdinChildOut - #echoMessage - 1, #stdinChildOut)
end
assert(
	stdinChildOut == echoMessage .. trailingAddition,
	"Stdin passing did not return proper output"
)

Copy link
Contributor

Choose a reason for hiding this comment

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

Btw, on CI the output looks like this

1: Hello from child process!
2: 
3: Hello from child process!
4: 

I assume line 1 is the input & line 3 is the output from child process. and at the end of line 3-4 is \r\n on windows.

@filiptibell
Copy link
Collaborator

filiptibell commented Oct 5, 2023

Seems like those latest test changes worked! Could you merge in latest changes from main?

EDIT: GitHub let me squash merge without any issues anyway 🎉 thank you once again for the PR!

@filiptibell filiptibell merged commit 8865692 into lune-org:main Oct 6, 2023
5 checks passed
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.

process.spawn: writing to stdin
3 participants