-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
stty: Basic support for --file/-F #4401
Conversation
I would like to see the CI results on this. I'm not sure how to run the GNU test suite against this. But in my limited testing it works correctly, though the error messages are not the same on failure: $ target/debug/coreutils stty -F /nosuch
stty: No such file or directory
$ stty -F /nosuch
stty: /nosuch: No such file or directory
$ target/debug/coreutils stty -F /bin
thread 'main' panicked at 'Could not get terminal attributes: ENOTTY', src/uu/stty/src/stty.rs:171:44
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
$ stty -F /bin
stty: /bin: Inappropriate ioctl for device There does also seem to be some other differences for stty, but I believe these are not related to $ stty
speed 38400 baud; line = 0;
-brkint -imaxbel iutf8
$ target/debug/coreutils stty
speed 38400 baud; line = 0;
-brkint ixon -imaxbel iutf8
$ stty -F /dev/pts/1
speed 38400 baud; line = 0;
erase = <undef>;
-brkint ixoff -imaxbel iutf8
$ target/debug/coreutils stty -F /dev/pts/1
speed 38400 baud; line = 0;
-brkint ixoff tandem ixon -imaxbel iutf8 |
thanks |
Agreed, though I'm not sure how to write a test of this! It might require creating a pseudo-terminal device just to test against. |
From the CI:
I ran cargo fmt on that file locally and and that does not produce the diff the CI produces. I'm really confused now. I'm using rustfmt from the stable toolchain in rustup. Version is: What did I do wrong when formatting the file locally such that it decides on something else on the CI? Rustfmt is supposed to be stable between versions afaik also. |
Nope, I was struggling with this when I wrote the first version of
Reordering imports is a configuration feature, so maybe you rustfmt is not picking up on that configuration? It should be on by default: https://rust-lang.github.io/rustfmt/?version=v1.5.1&search=#reorder_imports |
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.
Nice! I'd like to think about testing this better indeed. In the meantime, it looks good!
I believe the CI error is spurious. It is for a test unrelated to what I changed:
|
88873d8
to
003098a
Compare
Depending on how many other utilities in coreutils mess with the terminal, it might make sense to make a test harness that opens a pseudo-terminal. Most likely care will need to be taken if both the tester and the test framework are in the same thread to prevent deadlocks due to blocking IO. It might be easier to run it as a parent and child process. That does not sound fun to implement however. How does GNU test this? |
Based on a quick loop at GNU, they just change what they can and reset it at the end. I like a solution with a pseudoterminal better :) |
Spell checking failed on the comment due to referring to the flag "clocal". Not sure how to fix that, as "clocal" is mentioned in POSIX! Do you have a project specific dictionary somewhere? |
Yes, there's a custom dictionary in the |
GNU testsuite comparison:
|
This CI is so annoying. I don't even get what the issue is this time. Spurious/unrelated? |
It's not unrelated and the suggestion in the error message fixes it:
The
The CI makes reviewing a lot easier and you'll probably get used to it very quickly. I also recommend checking out what commands the CI runs and running those locally before pushing. |
eed9a78
to
0fa616b
Compare
Without doubt. However, the contributor experience for this could be better. I don't see a single easy script to run the CI checks locally (that would help, maybe something like cargo xtask). Since running the CI stuff locally appears quite complicated, what would help is if the turn around time from the CI was quicker! Waiting several hours for a maintainer to OK running the CI really isn't a good experience. Because of all the friction contributing to this project, I suspect this will not just be the first time for me, but also the only time. |
I agree. I would like to get some sponsors for this project to have faster builds... |
Good idea! We already have a
Btw, this only applies to first-time contributors and it's a GitHub policy that we can't disable sadly. |
Issue #4253 and not related to my stuff.
Spurious (Android)
Haven't touched that. |
No worries, we can recognize the stuff that's unrelated |
Fixes issue uutils#3863
0fa616b
to
215803e
Compare
Again, the windows error is unrelated to this PR ( The other error is also unrelated to this PR: Is there anything else before this can be merged? |
We'll have to think about the tests separately, but I think this looks good for now! Thanks! |
Fixes issue #3863