Skip to content

tail hex parsing, remove fundu dependency #7760

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Apr 23, 2025
Merged

tail hex parsing, remove fundu dependency #7760

merged 8 commits into from
Apr 23, 2025

Conversation

Qelxiros
Copy link
Contributor

  • tail: support hex parsing
  • remove fundu dependency

closes #7670
addresses @sylvestre's comment on #7675

From that issue:

By just using from_str from parse_time.rs naively, only 3 tests fail:

  1. One which exceeds float::MAX: 1.0e100000

  2. A single point: .

  3. Seconds unit: 1.0s

The first looks erroneous to me. I can't get GNU tail to error on my system due to an interval larger than float::MAX.
The second and third are patched in this PR.

@Qelxiros Qelxiros force-pushed the 7670-tail-hex-formatting branch from 713f018 to 997c5ef Compare April 15, 2025 08:02
sylvestre
sylvestre reviewed Apr 15, 2025
View reviewed changes
@sylvestre
Copy link
Contributor

many thanks!

@github-actions GitHub Actions
Copy link

GNU testsuite comparison:

Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)

@drinkcat
Copy link
Collaborator

@eduardorittner FYI

@drinkcat
Copy link
Collaborator

Can we please merge #7694 first? That fixes the second case, and more adjacent similar cases, e.g. e.

@drinkcat
Copy link
Collaborator

drinkcat commented Apr 15, 2025 •
edited
Loading

  1. One which exceeds float::MAX: 1.0e100000
    The first looks erroneous to me. I can't get GNU tail to error on my system due to an interval larger than float::MAX.

Fails for me. That being said, I don't think it's so bad to treat 1.0e100000 as infinity, and IMHO this could be reported to GNU coreutils (since they do parse overflows as infinity in other places).

$ env tail --version
tail (GNU coreutils) 9.6
Copyright (C) 2025 Free Software Foundation, Inc.
...
$ env tail -s 1.0e100000 -f
tail: invalid number of seconds: ‘1.0e100000’

@Qelxiros
Copy link
Contributor Author

I'm happy to rebase against main after #7694 is merged. Ping me here, otherwise I might not see it. I'm using coreutils 9.7 (latest version packaged for Arch Linux), so I'd guess the issue has already been reported.

@github-actions GitHub Actions
Copy link

GNU testsuite comparison:

Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)

@drinkcat
Copy link
Collaborator

I'm happy to rebase against main after #7694 is merged. Ping me here, otherwise I might not see it.

Actually, the easiest would just be if you dropped your changes in num_parser.rs, then there would be any conflict and this could be merged independently (of course . will incorrectly be parsed in the the mean time, but I think that'd be ok).

I'm using coreutils 9.7 (latest version packaged for Arch Linux), so I'd guess the issue has already been reported.

Interesting. I guess I need to update my system ,-) Maybe this is part of the changes they did to cover very short sleep intervals.

Thanks!

@drinkcat
Copy link
Collaborator

Argh, your newly added tests now fail. Hopefully we can just wait for #7694 then rebase this.

@github-actions GitHub Actions
Copy link

GNU testsuite comparison:

Skipping an intermittent issue tests/misc/stdbuf (passes in this run but fails in the 'main' branch)
Skipping an intermittent issue tests/timeout/timeout (passes in this run but fails in the 'main' branch)

@github-actions GitHub Actions
Copy link

GNU testsuite comparison:

Skipping an intermittent issue tests/timeout/timeout (passes in this run but fails in the 'main' branch)

@sylvestre sylvestre requested a review from Copilot April 17, 2025 19:41
Copilot
Copilot AI reviewed Apr 17, 2025
View reviewed changes
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

src/uucore/src/lib/features/parser/parse_time.rs:133

  • Duplicate assertion found for ExtendedBigDecimal overflow handling. Consider removing the duplicate to reduce redundancy.
assert_eq!(from_str("1e92233720368547758080", false), Ok(Duration::MAX));

sylvestre
sylvestre reviewed Apr 17, 2025
View reviewed changes
sylvestre
sylvestre reviewed Apr 17, 2025
View reviewed changes
sylvestre
sylvestre reviewed Apr 17, 2025
View reviewed changes
tests/by-util/test_tail.rs
@@ -4430,7 +4430,6 @@ fn test_follow_when_files_are_pointing_to_same_relative_file_and_file_stays_same
}

#[rstest]
#[case::exponent_exceed_float_max("1.0e100000")]
Copy link
Contributor

Choose a reason for hiding this comment

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

why did you remove it ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See conversation above. TL;DR: It no longer errors in GNU coreutils 9.7, bringing it more in line with other utilities. Should I add it back as a case on a test that expects success?

@sylvestre sylvestre merged commit 044b33d into uutils:main Apr 23, 2025
69 of 70 checks passed
@BrewTestBot BrewTestBot mentioned this pull request May 24, 2025
Merged
@moonfruit moonfruit mentioned this pull request May 26, 2025
Closed
nickorlow pushed a commit to nickorlow/coreutils that referenced this pull request Jul 17, 2025
@Qelxiros @nickorlow
Merge pull request uutils#7760 from Qelxiros/7670-tail-hex-formatting
a711bcb 
tail hex parsing, remove fundu dependency
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sylvestre sylvestre sylvestre left review comments

@drinkcat drinkcat drinkcat left review comments

Copilot code review Copilot Copilot left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

tail: Add support for hex formatting in -s duration
3 participants
@Qelxiros @sylvestre @drinkcat