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

Add last Utility #65

Open
wants to merge 26 commits into
base: main
Choose a base branch
from
Open

Add last Utility #65

wants to merge 26 commits into from

Conversation

Puffy1215
Copy link

I implemented the last utility in #18.

Limitations

  • I only implemented support for Unix
  • Several flags from GNU last were not implemented.
  • There are some slight output differences between GNU last and this implementation. They can be easily cleared up given more time. For example GNU last prints the relative time since the last crash, while this doesn't do that quite yet.

Tests
I added some tests too to go along with the PR. They are kind of simplistic though in that they only do a check to verify there was some output. The timestamp checks only do a simplistic regex check on the output.

Comment on lines 5 to 7

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

Suggested change
# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html


// "%b %e %H:%M"
let time_format: Vec<time::format_description::FormatItem> =
time::format_description::parse(description).unwrap();
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

why not handling the error ? unwrap seems risky, no ? :)

// "%b %e %H:%M"
let time_format: Vec<time::format_description::FormatItem> =
time::format_description::parse(description).unwrap();
ut.login_time().format(&time_format).unwrap() // LC_ALL=C
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

same

if counter >= self.limit && self.limit > 0 {
break;
}
// println!("|{}| |{}| |{}|", ut.user(), time_string(&ut), ut.tty_device());
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

please remove the comments

@sylvestre
Copy link
Sponsor Contributor

i guess you saw it but the builds are failing

@Puffy1215
Copy link
Author

i guess you saw it but the builds are failing

Windows and macOS is still broken. I'll work on fixing those. I will also fix the still existing linting issues.

Copy link

codecov bot commented Jul 17, 2024

Codecov Report

Attention: Patch coverage is 49.85994% with 179 lines in your changes missing coverage. Please review.

Project coverage is 60.05%. Comparing base (6c383c3) to head (a249bc5).
Report is 9 commits behind head on main.

Files Patch % Lines
src/uu/last/src/platform/unix.rs 39.24% 150 Missing and 28 partials ⚠️
src/uu/last/src/last.rs 95.23% 0 Missing and 1 partial ⚠️

❗ There is a different number of reports uploaded between BASE (6c383c3) and HEAD (a249bc5). Click for more details.

HEAD has 2 uploads less than BASE
Flag BASE (6c383c3) HEAD (a249bc5)
macos_latest 1 0
windows_latest 1 0
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #65      +/-   ##
==========================================
- Coverage   69.35%   60.05%   -9.30%     
==========================================
  Files          14       18       +4     
  Lines        1142     1467     +325     
  Branches      164      233      +69     
==========================================
+ Hits          792      881      +89     
- Misses        336      460     +124     
- Partials       14      126     +112     
Flag Coverage Δ
macos_latest ?
ubuntu_latest 60.05% <49.85%> (-3.82%) ⬇️
windows_latest ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Puffy1215
Copy link
Author

Puffy1215 commented Jul 22, 2024

It looks like I have everything working now. The existing errors appear to be from other utilities.

failures:
    test_mountpoint::test_invalid_arg
    test_rev::test_invalid_arg

@sylvestre
Copy link
Sponsor Contributor

dunno if it is hard to do but could you please add tests with an invalid /var/log/wtmp to check that the error management is properly done ? thanks

@Puffy1215
Copy link
Author

dunno if it is hard to do but could you please add tests with an invalid /var/log/wtmp to check that the error management is properly done ? thanks

Added one test so far that matches the behavior with the linux org implementation. That seems like the only test I could get working so far. There were some discrepancies I noticed that I think are beyond my implementation, and involve the UtmpxIter code.

For example, if you have a file of arbitrary bytes (Say a file of one long string of all 'a' characters) which can fit into Utmpx struct, the linux org implementation is able to read it into the fields and display it. However, with our implementation, it does not load any of them into a struct, and so displays nothing.

@sylvestre
Copy link
Sponsor Contributor

a bunch of jobs are failing, could you please have a look?

Like:


error: length comparison to zero
   --> src/uu/last/src/platform/unix.rs:190:16
    |
190 |             if ut_stack.len() == 0 {
    |                ^^^^^^^^^^^^^^^^^^^ help: using `is_empty` is clearer and more explicit: `ut_stack.is_empty()`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#len_zero
    = note: `-D clippy::len-zero` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(clippy::len_zero)]`

error: useless conversion to the same type: `i64`
   --> src/uu/last/src/platform/unix.rs:275:62
    |
275 |         let time = time::OffsetDateTime::from_unix_timestamp(secs.into())
    |                                                              ^^^^^^^^^^^ help: consider removing `.into()`: `secs`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#useless_conversion
    = note: `-D clippy::useless-conversion` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(clippy::useless_conversion)]`

error: useless conversion to the same type: `u64`
   --> src/uu/last/src/platform/unix.rs:277:36
    |
277 |             + Duration::from_nanos(nsecs.into());
    |                                    ^^^^^^^^^^^^ help: consider removing `.into()`: `nsecs`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#useless_conversion

error: useless conversion to the same type: `uucore::utmpx::time::OffsetDateTime`
   --> src/uu/last/src/platform/unix.rs:284:13
    |
284 |             OffsetDateTime::from(time).replace_offset(offset) + Duration::from_secs(offset_secs);
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider removing `OffsetDateTime::from()`: `time`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#useless_conversion

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

2 participants