Skip to content

Conversation

dan-hipschman
Copy link
Contributor

This partially fixes #1280. With this patch, the GNU compatibility tests for numfmt --zero-terminated pass:

$ bash util/run-gnu-test.sh tests/misc/numfmt.pl
...
z1...
z3...
z2...
z4...
z5...
...

One thing to note is that this patch does an unwrap on String::from_utf8 because BufRead.split(0) results in Vec<u8> instead of String. This isn't a regression though because the rest of the program already assumes input can be put into String. Here's an example from the main branch:

$ printf "\xED\xBF\xBF" | cargo run -q --bin coreutils numfmt
numfmt: stream did not contain valid UTF-8

I don't see a bug report for that so I'm happy to file one. I just didn't want to do more than one thing in this PR.

&prefix[1..]
} else {
prefix
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is probably just some implementation detail, but one of the GNU compatibility tests depends on this behavior. As per the guidelines, I haven't examined the GNU implementation for numfmt, but here are some manual tests I did for compatibility:

$ printf "A\nB 1001 C\x00D E\n2002 F\x00" | ./src/numfmt -z --field=3 --to=si | cat --show-all # official test
A B 1.1k C^@D E 2.1k F^@⏎
$ printf "A \nB 1001 C\x00D E\n2002 F\x00" | ./src/numfmt -z --field=3 --to=si | cat --show-all
A $
B 1.1k C^@D E 2.1k F^@⏎
$ printf "A\n B 1001 C\x00D E\n2002 F\x00" | ./src/numfmt -z --field=3 --to=si | cat --show-all
A  B 1.1k C^@D E 2.1k F^@⏎

The above output is from the GNU version of numfmt, but it's identical to the uutils version output (with this patch of course).

Copy link

github-actions bot commented Apr 8, 2025

GNU testsuite comparison:

Skip an intermittent issue tests/misc/stdbuf (fails in this run but passes in the 'main' branch)
Skipping an intermittent issue tests/timeout/timeout (passes in this run but fails in the 'main' branch)
Congrats! The gnu test tests/chmod/symlinks is no longer failing!

@cakebaker
Copy link
Contributor

There is some compilation error:

error[E0063]: missing field `zero_terminated` in initializer of `options::NumfmtOptions`
   --> src/uu/numfmt/src/numfmt.rs:424:9
    |
424 |         NumfmtOptions {
    |         ^^^^^^^^^^^^^ missing `zero_terminated`

@dan-hipschman dan-hipschman force-pushed the numfmt/add-zero-terminated-option branch from ea20122 to 81911f9 Compare April 8, 2025 17:05
Copy link

github-actions bot commented Apr 8, 2025

GNU testsuite comparison:

Skip an intermittent issue tests/misc/stdbuf (fails in this run but passes in the 'main' branch)
Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)
Congrats! The gnu test tests/misc/tee is no longer failing!

@dan-hipschman
Copy link
Contributor Author

dan-hipschman commented Apr 8, 2025

There is some compilation error:

@cakebaker Thanks, this should be fixed now. I'm new to this project and didn't realize there are tests within numfmt.rs that aren't run by make UTILS=numfmt test (they are run with cargo test -puu_numfmt).

CI is passing now except for what looks like an unrelated test for tail which may be flaky / have a race condition (there's some timing related code in the test, and also comments above the test suggesting it fails sometimes):

 thread 'test_tail::test_follow_when_files_are_pointing_to_same_relative_file_and_file_stays_same_size' panicked at tests/by-util/test_tail.rs:4424:10:
Expected stderr to be empty, but it's:
tail: /tmp/.tmpOE165n/data: file truncated

Unfortunately it doesn't look like I have permission to rerun it, unless I'm missing something. So I'll have to wait until a maintainer can rerun it to verify. (I could push an empty commit, but don't want to pollute the commit log just for this.)

@sylvestre sylvestre merged commit 45c978f into uutils:main Apr 10, 2025
68 of 69 checks passed
@sylvestre
Copy link
Contributor

Thanks!

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.

numfmt: implement missing options
3 participants