-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Redirect: support redirect stderr with piping stdout to next commands. #10851
Conversation
This has troubled me a lot, thanks man bless you |
This reverts commit 1aa3897.
I have no idea how to fix the CI....The CI runs failed because we save stderr in another thread, but we don't know if the thread is finished. The machine is too busy to run this child thread Edit: finally I come up a way to make CI green :-D |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for figuring this out and testing it!
I think we should move towards landing it soon :)
@@ -288,3 +288,38 @@ fn redirection_should_have_a_target() { | |||
); | |||
} | |||
} | |||
|
|||
#[test] | |||
#[cfg(not(windows))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be great if we could convert this test to something that reliably runs on all platforms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking a way to make such output in our testbin.
I'd prefer to make it in separate pr, cause it requires a lot of changes in tests
2. based on 1, make sure that eval process works for stdout properly
nushell#10851) # Description Fixes: nushell#10271 Given the following script: ```shell # test.sh echo aaaaa echo bbbbb 1>&2 echo cc ``` This pr makes the following command possible: ```nushell bash test.sh err> /dev/null | lines | each {|line| $line | str length} ``` ## General idea behind the change: When nushell redirect stderr message to external file 1. it take stdout of external stream, and pass this stream to next command, so it won't block next pipeline command from running. 2. relative stderr stream are handled by `save` command These two streams are handled separately, so we need to delegate a thread to `save` command, or else we'll have a chance to hang nushell, we have meet a similar before: nushell#5625. ### One case to consider What if we're failed to save to an external stream? (Like we don't have a permission to save to a file)? In this case nushell will just print a waning message, and don't stop the following scripts from running. # User-Facing Changes ## Before ```nushell ❯ bash test2.sh err> /dev/null | lines | each {|line| $line | str length} aaaaa cc ``` ## After ```nushell ❯ bash test2.sh err> /dev/null | lines | each {|line| $line | str length} ╭───┬───╮ │ 0 │ 5 │ │ 1 │ 2 │ ╰───┴───╯ ``` BTY, after this pr, the following commands are impossible either, it's important to make sure that the implementation doesn't introduce too much costs: ```nushell ❯ echo a e> a.txt e> a.txt Error: × Can't make stderr redirection twice ╭─[entry #1:1:1] 1 │ echo a e> a.txt e> a.txt · ─┬ · ╰── try to remove one ╰──── ❯ echo a o> a.txt o> a.txt Error: × Can't make stdout redirection twice ╭─[entry nushell#2:1:1] 1 │ echo a o> a.txt o> a.txt · ─┬ · ╰── try to remove one ╰──── ```
nushell#10851) # Description Fixes: nushell#10271 Given the following script: ```shell # test.sh echo aaaaa echo bbbbb 1>&2 echo cc ``` This pr makes the following command possible: ```nushell bash test.sh err> /dev/null | lines | each {|line| $line | str length} ``` ## General idea behind the change: When nushell redirect stderr message to external file 1. it take stdout of external stream, and pass this stream to next command, so it won't block next pipeline command from running. 2. relative stderr stream are handled by `save` command These two streams are handled separately, so we need to delegate a thread to `save` command, or else we'll have a chance to hang nushell, we have meet a similar before: nushell#5625. ### One case to consider What if we're failed to save to an external stream? (Like we don't have a permission to save to a file)? In this case nushell will just print a waning message, and don't stop the following scripts from running. # User-Facing Changes ## Before ```nushell ❯ bash test2.sh err> /dev/null | lines | each {|line| $line | str length} aaaaa cc ``` ## After ```nushell ❯ bash test2.sh err> /dev/null | lines | each {|line| $line | str length} ╭───┬───╮ │ 0 │ 5 │ │ 1 │ 2 │ ╰───┴───╯ ``` BTY, after this pr, the following commands are impossible either, it's important to make sure that the implementation doesn't introduce too much costs: ```nushell ❯ echo a e> a.txt e> a.txt Error: × Can't make stderr redirection twice ╭─[entry nushell#1:1:1] 1 │ echo a e> a.txt e> a.txt · ─┬ · ╰── try to remove one ╰──── ❯ echo a o> a.txt o> a.txt Error: × Can't make stdout redirection twice ╭─[entry nushell#2:1:1] 1 │ echo a o> a.txt o> a.txt · ─┬ · ╰── try to remove one ╰──── ```
Description
Fixes: #10271
Given the following script:
This pr makes the following command possible:
General idea behind the change:
When nushell redirect stderr message to external file
save
commandThese two streams are handled separately, so we need to delegate a thread to
save
command, or else we'll have a chance to hang nushell, we have meet a similar before: #5625.One case to consider
What if we're failed to save to an external stream? (Like we don't have a permission to save to a file)?
In this case nushell will just print a waning message, and don't stop the following scripts from running.
User-Facing Changes
Before
After
BTY, after this pr, the following commands are impossible either, it's important to make sure that the implementation doesn't introduce too much costs: