-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
tty: add missing newline to output #10252
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
base: main
Are you sure you want to change the base?
Conversation
src/uu/tty/src/tty.rs
Outdated
| let write_result = match name { | ||
| Ok(name) => stdout.write_all_os(name.as_os_str()), | ||
| Ok(name) => { | ||
| stdout.write_all_os(name.as_os_str())?; |
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.
(/bin/tty >&- 2>/dev/null); echo $?
This command shows what happens when theres an error in writing to stdout, the error code that GNU is expecting is 3, but by adding that ? it will make it handled and return a 1. You can get around this by doing:
Ok(name) => stdout
.write_all_os(name.as_os_str())
.and_then(|_| writeln!(stdout)),
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.
target/debug/tty > /dev/full silently returns 3. But GNU have tty: write error: No space left on device.
|
@ChrisDryden can you check now, sir? thank you! |
|
Would you add test to check newline (and exit code of |
|
GNU testsuite comparison: |
|
@oech3 Hi, sir! Please check now and let me know. Thank you! Have a nice weekend. :) |
| if write_result.is_err() || stdout.flush().is_err() { | ||
|
|
||
| if let Err(e) = write_result { | ||
| eprintln!("tty: write error: {}", e); |
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.
Why did you change this?
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.
@ChrisDryden I removed it because I thought it was redundant (flush was already called in line 37), but I've now restored it to match the original pattern.
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 guess we can extract it from OS provided err message.
(Also please cargo fmt)
|
I changed it to print the error message before exiting (as requested in the previous feedback about matching GNU tty's 'write error' output). However, I notice I removed the separate |
CodSpeed Performance ReportMerging this PR will degrade performance by 27.39%Comparing Summary
Performance Changes
Footnotes
|
|
many jobs are failing |
I ran tests locally, and everything worked for me. I don't know why these specific jobs keep failing when I fixed the new line issue. There is no other workaround that I know of, GNU panics if something is wrong but it didn't produce an output so I made custom test like OP mentioned and I don't understand why is it failing now.. Can you suggest fix to this? |
|
I guess you wrote the test writing to I guess it is generic test framefork's issue. |
Thank you, sir. If it's okay on your end, can we close this issue for now and this other issue can be a separate PR? |
|
Please keep open and add |
Understood, sir. Will follow your advice, thank you for the suggestion. |
Fixes #10239