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

Initial implementation of stty #3672

Merged
merged 17 commits into from
Aug 20, 2022
Merged

Initial implementation of stty #3672

merged 17 commits into from
Aug 20, 2022

Conversation

tertsdiepraam
Copy link
Member

@tertsdiepraam tertsdiepraam commented Jun 25, 2022

I got annoyed that there was no stty yet, so I started working on it :)

In this initial version:

  • Only binary flags are implemented (i.e. attributes that are enabled and disabled). This means that attributes like tabN where N is a number between 0 and 3 is missing. This is now supported.
  • --all is implemented.
  • -F/--file and -g/--save are ignored.
  • Special characters, special settings and combined settings are missing.
  • The nix crate is used (not libc). Sadly, here are already some attributes that seem to be missing in nix.

The tests are broken on my machine with the error:

thread 'main' panicked at 'Could not get terminal attributes: ENOTTY', src/uu/stty/src/stty.rs:83:44

So the stdout in cargo test does not seem to be a tty with terminal attributes. This might mean that we can't test stty at all? I'd like to try to find a workaround though.

@sylvestre
Copy link
Contributor

wahou, amazing :)

@anastygnome
Copy link
Contributor

anastygnome commented Jun 28, 2022

@tertsdiepraam can we use crossterm for this (or are we using it ?)

Maybe we will have to use shell scripts for that TTY issue.

https://docs.rs/crossterm/latest/crossterm/index.html

Wahou indeed (pardon my french interjections ;)

@tertsdiepraam
Copy link
Member Author

I'm not currently using crossterm, but nix, which has support for most of the flags that we need for this. crossterm seems to be more about writing to the terminal than changing it's characteristics. The terminal size could come in handy but crossterm doesn't seem to support setting the size. So I'm afraid it will have to stay as low level as it is.

@anastygnome
Copy link
Contributor

@tertsdiepraam I thought so as well, but maybe it could be an inspiration for which calls we should use.

src/uu/stty/src/stty.rs Outdated Show resolved Hide resolved
@tertsdiepraam tertsdiepraam marked this pull request as ready for review July 2, 2022 17:22
@tertsdiepraam
Copy link
Member Author

I've ignored the tests that need to be run from a tty for now, but added some for the argument parsing. I've also added the special settings for the terminal (baudrate, columns, rows and line), but they can only be printed for now (not changed). With those changes this is at least ready to be considered somewhat of a viable first version, though of course there is much to be done :)

@tertsdiepraam
Copy link
Member Author

Apparently it needs some work on mac. It will take me a while to fix it though. If someone else wants to fix it up for mac, feel free to do so in the meantime.

@anastygnome
Copy link
Contributor

anastygnome commented Jul 3, 2022

@tertsdiepraam I belive mac os has no support for the cline field in termios.

Hence, should we ignore it ?

I believe the same problem is the cause for the other errors

@tertsdiepraam
Copy link
Member Author

Yeah I think that makes sense. The other issue with mac is that the baudrate is represented with a u32 instead of the baudrate enum.

fn print_terminal_size(termios: &Termios, opts: &Options) -> nix::Result<()> {
let speed = cfgetospeed(termios);
for (text, baud_rate) in BAUD_RATES {
if *baud_rate == speed {
Copy link
Contributor

@anastygnome anastygnome Jul 3, 2022

Choose a reason for hiding this comment

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

if *baud_rate as u32 == speed as u32

seems to work on my system and should fix this issue, unless there are speeds above 32 bits unsigned integers ??

@sylvestre sylvestre mentioned this pull request Jul 3, 2022
@tertsdiepraam tertsdiepraam force-pushed the stty branch 2 times, most recently from 546e24c to a1250ec Compare August 19, 2022 10:42
@tertsdiepraam
Copy link
Member Author

Okay, so I think it's correct now. I can't test with mac, but I get the correct speed on FreeBSD. There are of course some minor issues and missing functionality with this implementation, but I think we can merge this and I can start opening issues for those.

@sylvestre
Copy link
Contributor

sounds good, let's wait for the CI :)
I can do more testing on mac if you need

@tertsdiepraam
Copy link
Member Author

I'm not sure what's going on with the Cargo.lock format. I can't find anything important that has changes with respect to main.

@sylvestre sylvestre merged commit 8786bb6 into uutils:main Aug 20, 2022
@sylvestre
Copy link
Contributor

Bravo !
would you like to do a release now? :)

@tertsdiepraam
Copy link
Member Author

Yes, I'll start working on it!

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.

None yet

3 participants