Rewrite more #2109

AMythicDev · 2021-04-24T17:29:46Z

The more utility seems fairly incomplete. It does not use any modern terminal library like crossterm or termion Reading from standard input is also not implemented along with other flags that are present in the original GNU more. This PR intends to rewrite more using crossterm, adding all the missing flags and making it as close to the original more as possible

There are few differences in the command line interface from the original more like not having the + and - symbols in the -<number>, +<number> and +/<pattern> options from the original more. This is due to the non-standard way of arguments in GNU more, which clap does not allow building

sylvestre · 2021-04-25T07:29:40Z

please let me know when it is ready but it is great to see such progress here :)

AMythicDev · 2021-04-25T08:43:36Z

Sure. I would try to complete it ASAP

sylvestre

I am aware that it is a draft but here are some initial feedback ;)

src/uu/more/src/more.rs

AMythicDev · 2021-04-26T05:09:38Z

Thanks for telling me that. I would change it in further commits

AMythicDev · 2021-04-27T16:49:41Z

I have rewrote the basic usable version of more. Although none of the CLI flags are actually implemented and I will address them in a future PR.

I think that this PR should be merged before I start working on the CLI part. I see there's a merge conflict which I will resolve probably by tomorrow and squash everything so that we could merge this

AMythicDev · 2021-04-28T07:22:22Z

This PR is ready for merging. If you have any suggestions, you can comment it down here. I will address them in my next PR

sylvestre · 2021-04-28T07:47:05Z

Seems that a test is failing on Windows:


---- test_more::test_more_no_arg stdout ----
current_directory_resolved: 
run: D:\a\coreutils\coreutils\target\i686-pc-windows-msvc\debug\coreutils.exe more
thread 'test_more::test_more_no_arg' panicked at 'Command was expected to fail.
stdout = 

 stderr = ', tests\common\util.rs:155:13


failures:
    test_more::test_more_no_arg

Add crossterm as dependency Complete the paging portion Fixed tests cp: extract linux COW logic into function cp: add --reflink support for macOS Fixes #1773 Fix error in Cargo.lock Quit automatically if not much output is left Remove unnecessary redox and windows specific code Handle line wrapping Put everything according to uutils coding standards Add support for multiple files Fix failing test Use the args argument to get cli arguments Fix bug where text is repeated multiple times during printing Add a little prompt Add a top file prompt for multiple files Change println in loops to stdout.write and setup terminal only once Fix bug where all lines were printed in a single row Remove useless file and fix failing test Fix another test

sylvestre

it builds fine and bravo for the tests

However:

more without any arguments cannot be exited?!
Gnu is returning:

LANG=C more
more: bad usage
Try 'more --help' for more information.

more is breaking the display of the terminal (reset is needed to come back in a normal state)
more Makefile. the content isn't rendered correctly.
For example on my system




USEGNU=gmake $*
               all:
                   	@$(USEGNU)
                                  .DEFAULT:
                                           	@$(USEGNU)

                                                          %                     ╭─sylvestre@pinasil ~/dev/debian/coreutils ‹master*› 
                                                     ╰─$

./target/release/coreutils more non-existing
is failing with:

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Os { code: 2, kind: NotFound, message: "No such file or directory" }', /home/sylvestre/dev/debian/coreutils/src/uu/more/src/more.rs:148:63
                                                 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

AMythicDev · 2021-05-06T16:45:22Z

I think, your issues are probably fixed in the last commit, you could review it again and report if anything goes wrong

sylvestre · 2021-05-07T09:54:45Z

src/uu/more/src/more.rs

+            stdin().read_to_string(&mut buff).unwrap();
+            more(&buff, &mut stdout, true);
+        } else {
+            show_usage_error!("bad usage");


bad usage? This is the error that GNU is reporting?

src/uu/more/src/more.rs

src/uu/more/Cargo.toml

src/uu/more/src/more.rs

sylvestre · 2021-05-08T18:19:45Z

It still breaks the terminal for me.

Example with zsh & gnome-terminal:

                                                    [20:18:40]
╭─sylvestre@pinasil ~/dev/debian/coreutils ‹master*› 
╰─$ ./target/release/coreutils more                                  [20:18:42]

more: bad usage
               Try 'more --help' for more information.
                                                      %                         ╭─sylvestre@pinasil ~/dev/debian/coreutils ‹master*› 
                                                     ╰─$                       [./tacd dev/debian/coreutils        
cd: aucun fichier ou dossier de ce type: dev/debian/coreutils
                                                             %                  ╭─sylvestre@pinasil ~/dev/debian/coreutils ‹master*› 
                                                     ╰─$                       [ls:18:48]
1602.patch   Cargo.lock                 Makefile.toml
                                                     2020.patch   Cargo.lock.orig            README.md
                      2056.diff    Cargo.toml                 sample.txt
                                                                        2056.patch   CODE_OF_CONDUCT.md         sample.txt.orig
                                               2057.patch   CONTRIBUTING.md            shuffled_wordlist.txt
                            2109.diff    DEVELOPER_INSTRUCTIONS.md  src
                                                                       2109.diff.1  docs                       target
                                     a            examples                   tests
  a~           GNUmakefile                tmp
                                             b            LICENSE                    util
         build.rs     Makefile
                              %                                                 ╭─sylvestre@pinasil ~/dev/debian/coreutils ‹master*› 
                                                     ╰─$                       [20:18:48]

sylvestre · 2021-05-16T18:40:42Z

Sorry but did you test your change?
Just running the tests is breaking my terminal
Try:

$ cargo test --release --features feat_os_unix
generate a long list of

                                                 test test_unexpand::unexpand_init_1 ... ok
           test test_unexpand::unexpand_init_list_0 ... ok
                                                          test test_unexpand::unexpand_init_list_1 ... ok
                         test test_unexpand::unexpand_read_from_two_file ... ok
                                                                               test test_unexpand::unexpand_read_from_file ... ok
                                                 test test_unexpand::unexpand_spaces_follow_tabs_1 ... ok

AMythicDev · 2021-05-18T07:15:26Z

I fixed the issue with broken terminal. Could you recheck that.

sylvestre · 2021-05-18T08:20:54Z

Could you decrease the minimal version of crossterm, it fails to build with Rust 1.40 with:

error: attributes are not yet allowed on `if` expressions
   --> /home/sylvestre/.cargo/registry/src/github.com-1ecc6299db9ec823/crossterm-0.19.0/src/command.rs:123:9
    |
123 |         #[cfg(windows)]
    |         ^^^^^^^^^^^^^^^

error: aborting due to previous error

AMythicDev · 2021-05-18T09:29:29Z

Sure. I will check that. This might take some time as I would probably have to go through a hit and trial process

sylvestre · 2021-05-18T09:30:32Z

To test
cargo +1.40.0 test --features unix

sylvestre · 2021-05-18T16:19:18Z

Besides that, it looks great :)

AMythicDev · 2021-05-19T09:26:37Z

I have downgraded the minimum version of crossterm and everything compiles correctly with Rust 1.40.

tertsdiepraam · 2021-05-22T15:16:37Z

This rewrite is awesome! Sadly, I'm getting some errors on my machine:

---- test_cp::test_cp_directory_not_recursive stdout ----
current_directory_resolved: 
run: /home/terts/Programming/coreutils/target/debug/coreutils cp hello_dir/ copy_of_hello_world.txt
thread 'test_cp::test_cp_directory_not_recursive' panicked at 'assertion failed: self.stderr_str().contains(cmp.as_ref())', tests/common/util.rs:303:9

---- test_more::test_more_dir_arg stdout ----
current_directory_resolved: 
run: /home/terts/Programming/coreutils/target/debug/coreutils more .
thread 'test_more::test_more_dir_arg' panicked at 'assertion failed: `(left == right)`
  left: `"thread \'main\' panicked at \'called `Result::unwrap()` on an `Err` value: IoError(Os { code: 25, kind: Other, message: \"Inappropriate ioctl for device\" })\', src/uu/more/src/more.rs:179:33\nnote: run with `RUST_BACKTRACE=1` environment variable to display a backtrace"`,
 right: `"more: \'.\' is a directory.\nTry \'more --help\' for more information."`', /home/terts/Programming/coreutils/tests/by-util/test_more.rs:21:9

These seem to be caused by a bug in crossterm that was fixed in version 0.18, exactly the version we can't use because Rust 1.40 does not allow attributes on if expressions. As far as I can tell, Rust 1.43 is the lowest Rust version that supports attributes on if (implemented by this PR).

Apart from this error, really nice job, it looks great when I use the recent versions of crossterm!

@sylvestre Do you think we could set the MSRV to 1.43 to use crossterm and attributes on if? Another driver is that 1.41 introduces the new Cargo.lock format (so no more cargo +1.40.0 update hopefully)

tertsdiepraam · 2021-05-29T10:59:40Z

I've fixed some things up and I think this can be merged now (if the CI is green).

There are a couple of remaining issues that should be dealt with in other PR's:

unicode width needs to be used instead of line.len()
there are many unimplemented arguments
the name of the next file should be shown in the prompt when the end of a file is reached

I'll create issues for these once this is merged.

@arijit79 This PR is amazing, thanks!

sylvestre reviewed Apr 25, 2021

View reviewed changes

src/uu/more/src/more.rs Outdated Show resolved Hide resolved

src/uu/more/src/more.rs Outdated Show resolved Hide resolved

AMythicDev marked this pull request as ready for review April 28, 2021 07:18

AMythicDev and others added 2 commits April 29, 2021 20:23

Merge branch 'master' into implement-more

482e340

sylvestre requested changes May 5, 2021

View reviewed changes

Make a nice error when file does not exist

d2ab0dc

AMythicDev requested a review from sylvestre May 6, 2021 16:43

sylvestre reviewed May 7, 2021

View reviewed changes

sylvestre requested changes May 7, 2021

View reviewed changes

src/uu/more/src/more.rs Outdated Show resolved Hide resolved

src/uu/more/Cargo.toml Outdated Show resolved Hide resolved

src/uu/more/src/more.rs Show resolved Hide resolved

AMythicDev added 2 commits May 16, 2021 22:26

Fix bug with terminal getting weird

22ba21d

Fix clippy warning

c930509

AMythicDev requested a review from sylvestre May 16, 2021 17:02

Fix broken terminal in tests

7a88df9

Downgrade crossterm version

1596c65

tertsdiepraam mentioned this pull request May 22, 2021

more: pipe isn't working #1971

Closed

tertsdiepraam linked an issue May 23, 2021 that may be closed by this pull request

more should respect stdin, return, terminal size #1423

Closed

tertsdiepraam mentioned this pull request May 26, 2021

Bump MSRV to 1.43 #2282

Closed

tertsdiepraam and others added 7 commits May 28, 2021 19:43

More: update crossterm dependency

0a2f74f

Merge branch 'master' into implement-more

fe42808

Add atty to dev-deps for more tests

1c2540a

more: simplify and fix logic for multiple files

101e557

more: simplify main loop

40ee902

more: add todo for unicode width

9d17c1f

more: comment out unimplemented arguments

762da0b

Merge branch 'master' into implement-more

8c5dcd0

sylvestre merged commit 6141fdc into uutils:master May 31, 2021

AMythicDev deleted the implement-more branch May 31, 2021 12:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite more #2109

Rewrite more #2109

AMythicDev commented Apr 24, 2021 •

edited

Loading

sylvestre commented Apr 25, 2021

AMythicDev commented Apr 25, 2021

sylvestre left a comment

AMythicDev commented Apr 26, 2021

AMythicDev commented Apr 27, 2021

AMythicDev commented Apr 28, 2021

sylvestre commented Apr 28, 2021

sylvestre left a comment

AMythicDev commented May 6, 2021

sylvestre May 7, 2021

sylvestre commented May 8, 2021

sylvestre commented May 16, 2021

AMythicDev commented May 18, 2021

sylvestre commented May 18, 2021

AMythicDev commented May 18, 2021

sylvestre commented May 18, 2021

sylvestre commented May 18, 2021

AMythicDev commented May 19, 2021

tertsdiepraam commented May 22, 2021 •

edited

Loading

tertsdiepraam commented May 29, 2021

Rewrite more #2109

Rewrite more #2109

Conversation

AMythicDev commented Apr 24, 2021 • edited Loading

sylvestre commented Apr 25, 2021

AMythicDev commented Apr 25, 2021

sylvestre left a comment

Choose a reason for hiding this comment

AMythicDev commented Apr 26, 2021

AMythicDev commented Apr 27, 2021

AMythicDev commented Apr 28, 2021

sylvestre commented Apr 28, 2021

sylvestre left a comment

Choose a reason for hiding this comment

AMythicDev commented May 6, 2021

sylvestre May 7, 2021

Choose a reason for hiding this comment

sylvestre commented May 8, 2021

sylvestre commented May 16, 2021

AMythicDev commented May 18, 2021

sylvestre commented May 18, 2021

AMythicDev commented May 18, 2021

sylvestre commented May 18, 2021

sylvestre commented May 18, 2021

AMythicDev commented May 19, 2021

tertsdiepraam commented May 22, 2021 • edited Loading

tertsdiepraam commented May 29, 2021

AMythicDev commented Apr 24, 2021 •

edited

Loading

tertsdiepraam commented May 22, 2021 •

edited

Loading