Skip to content

Conversation

@aisuneko
Copy link
Contributor

@aisuneko aisuneko commented Mar 22, 2025

This is a basic implementation of the display of each users' idle time, as well as a dedicated time formatting function format_time_elapsed for this feature, as my first PR to the uutils project.

The code's logic is largely based on the original source of procps We'll, no, as that is considered license violation. I mean just the logic is similar, but the code is rather another implementation.

@Krysztal112233
Copy link
Collaborator

Krysztal112233 commented Mar 23, 2025

Nah... this project licensed under MIT and the member who want to contributing CAN'T SEE the GPL licensed code:

Generally, we try to follow what GNU is doing in terms of options and behavior. It is recommended to look at the GNU coreutils manual (on the web, or locally using info ). It is more in depth than the man pages and provides a good description of available features and their implementation details. But remember, you cannot look at the GNU source code!

from our https://github.com/uutils/coreutils/blob/main/CONTRIBUTING.md#writing-code

@Krysztal112233
Copy link
Collaborator

And very thanks for your contributing, but we need to discuss those code will merge into our code base or not

Could you please give us some help?

@sylvestre @cakebaker

@sylvestre
Copy link
Contributor

please don't look at the other implementation. Licenses aren't compatible.
However, here, as you didn't copy how the code is done or its structure, I think we are good

@aisuneko
Copy link
Contributor Author

aisuneko commented Mar 23, 2025

please don't look at the other implementation. Licenses aren't compatible.

Sorry about that. I didn't notice that the licenses are incompatible... I'll pay extra attention to that in future contributions.
(Though IMHO: if we are to "follow what GNU is doing in terms of options and behavior", and in particular, to pass GNU tests, then I think looking at the source code is inevitable to some point (that is to better understand how it works/study it's output because we need to follow it's behavior, not to directly copy it from C to Rust; like, we could implement it in our own way which could be quite different code-wise to the original while retaining its main logic or program outputs to an extent)

@sylvestre
Copy link
Contributor

Looking at test and code are different things.

In general, if you need to look at code, look at the BSD implementation.
for example:
https://github.com/apple-oss-distributions/shell_cmds/blob/main/w/w.c

@aisuneko
Copy link
Contributor Author

aisuneko commented Mar 23, 2025

Looking at test and code are different things.

Then what is our stance on the tests and the scripts/code that might appear in these tests? E.g. Are we allowed to port and run the tests directly in our project?

In general, if you need to look at code, look at the BSD implementation. for example: https://github.com/apple-oss-distributions/shell_cmds/blob/main/w/w.c

Thanks for the explanation.

@Krysztal112233
Copy link
Collaborator

Krysztal112233 commented Mar 24, 2025

Then what is our stance on the tests and the scripts/code that might appear in these tests? E.g. Are we allowed to port and run the tests directly in our project?

The test suite will only check that the inputs, outputs and behaviors are as expected, it only provides the usage of util, not provide the original logic of the util implemented and licensed by GNU and GPL, so it is possible to check the test suite's code

And don't forget to fix the CI failures, that's a great job :)

@aisuneko
Copy link
Contributor Author

Then what is our stance on the tests and the scripts/code that might appear in these tests? E.g. Are we allowed to port and run the tests directly in our project?

The test suite will only check that the inputs, outputs and behaviors are as expected, it only provides the usage of util, not provide the original logic of the util implemented and licensed by GNU and GPL, so it is possible to check the test suite's code

Thanks for clarifying. I might work toward porting the tests to the rest of the uutils projects (as I'm also interested in contributing to uutils for this year's GSoC), so do let me know if possible license infringements exist.

And don't forget to fix the CI failures, that's a great job :)

I suppose that's due to my current implementation is for Linux only which causes the Windows and macOS runners to fail. I don't always have Windows or macOS machines available, so suggestions on ways of porting my code to these two platforms are appreciated ;)

@Krysztal112233
Copy link
Collaborator

Hum you can see the logs of failed CI, the logs will provided all the things you need to fix CI failure

@aisuneko
Copy link
Contributor Author

aisuneko commented Mar 24, 2025

I suppose that's due to my current implementation is for Linux only which causes the Windows and macOS runners to fail.

I said so because I have checked the runner logs:
(though amid some cargo clippy errors which should be quite easy to fix)
Screenshot_20250324-104749_Iceraven

@Krysztal112233
Copy link
Collaborator

Krysztal112233 commented Mar 24, 2025

Use text instead of picture would be better for searching, please use text for pasting logs in future :)

@aisuneko aisuneko requested a review from Krysztal112233 March 24, 2025 12:04
Copy link
Collaborator

@Krysztal112233 Krysztal112233 left a comment

Choose a reason for hiding this comment

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

from CI Style/lint (windows-latest) and Style/lint (macos-latest)

 error[E0433]: failed to resolve: use of undeclared crate or module `process`
   --> src\uu\w\src\w.rs:241:13
    |
241 |             process::exit(1);
    |             ^^^^^^^ use of undeclared crate or module `process`
    |
help: consider importing this module
    |
6   + use std::process;
    |

error: unused variable: `tty`
  --> src\uu\w\src\w.rs:99:20
   |
99 | fn fetch_idle_time(tty: String) -> Result<Duration, std::io::Error> {
   |                    ^^^ help: if this is intentional, prefix it with an underscore: `_tty`
   |
   = note: `-D unused-variables` implied by `-D warnings`
   = help: to override `-D warnings` add `#[allow(unused_variables)]`

please fix this error, this error caused build fail :)

@Krysztal112233
Copy link
Collaborator

Krysztal112233 commented Mar 24, 2025

Cool, to fix the CI failure, the last step:

 error: function `fetch_idle_time` is never used
  --> src/uu/w/src/w.rs:98:4
   |
98 | fn fetch_idle_time(_tty: String) -> Result<Duration, std::io::Error> {
   |    ^^^^^^^^^^^^^^^
   |
   = note: `-D dead-code` implied by `-D warnings`
   = help: to override `-D warnings` add `#[allow(dead_code)]`

And could you please add unit test for testing the time formatting correctly?

@Krysztal112233
Copy link
Collaborator

Krysztal112233 commented Mar 25, 2025

does there any output of w command on your environment? I cannot get any output on my environment

➜  procps git:(main) cargo run -q w  
USER     TTY      LOGIN@   IDLE   JCPU   PCPU WHAT
➜  procps git:(main) w
 15:57:56 up  4:05,  2 users,  load average: 1.45, 2.01, 1.88
USER     TTY      FROM             LOGIN@   IDLE   JCPU   PCPU WHAT
krysztal tty1     -                11:54    4:05m  0.09s  0.09s /usr/bin/startplasma-wayland
krysztal          -                11:54     ?     0.00s  9.93s /usr/lib/systemd/systemd --user
➜  procps git:(aisuneko/main) cargo run -q w    
USER     TTY      LOGIN@   IDLE   JCPU   PCPU WHAT
➜  procps git:(aisuneko/main) w                 
 15:59:04 up  4:06,  2 users,  load average: 1.43, 1.90, 1.86
USER     TTY      FROM             LOGIN@   IDLE   JCPU   PCPU WHAT
krysztal tty1     -                11:54    4:06m  0.09s  0.09s /usr/bin/startplasma-wayland
krysztal          -                11:54     ?     0.00s  9.97s /usr/lib/systemd/systemd --user

hum it seems implemented nothing but header output currently? @sylvestre @cakebaker

@aisuneko
Copy link
Contributor Author

aisuneko commented Mar 25, 2025

@Krysztal112233 It works as intended on my machine, as in #183 (comment) :

[aisuneko@liquidice procps]$ cargo run -q w
USER     TTY      LOGIN@   IDLE   JCPU   PCPU WHAT
aisuneko tty2     Sun23    41:43m 0.22   0.2  /usr/bin/startplasma-wayland
aisuneko pts/0    Sun23    41:43m 11244.5023.31/usr/bin/kded6
aisuneko pts/2    Sun23    41:39m 0.46   0.37 /usr/bin/fish
aisuneko pts/3    Sun23    41:39m 0.46   0.09 sudotio/dev/ttyACM0
aisuneko pts/4    Sun23    17:06m 0.07   0.07 /usr/bin/fish
aisuneko pts/1    Mon24    4:40m  0.25   0.25 /usr/bin/fish
[aisuneko@liquidice procps]$ w
 17:20:25 up 1 day, 17:44,  1 user,  load average: 0.48, 0.58, 0.66
USER     TTY       LOGIN@   IDLE   JCPU   PCPU  WHAT
aisuneko tty2      Sun23   41:44m  0.22s  0.20s /usr/bin/startplasma-wayland
[aisuneko@liquidice procps]$ w -V
w from procps-ng 4.0.5-dirty

Are you using the w binary from procps-ng or a different implementation?

@Krysztal112233
Copy link
Collaborator

Krysztal112233 commented Mar 25, 2025

Weird problem. Our implementation output empty content on Debian.

➜  procps git:(aisuneko/main) cargo run -q w -V
w 0.0.1
➜  procps git:(aisuneko/main) w -V
w from procps-ng 4.0.4

But please don't worry, it's not your problem. :)

I'll spend some time researching why, this pr looks perfect.

Can you add some tests to this file? This test file will ensure that the changes in future don't make regression. https://github.com/uutils/procps/blob/main/tests/by-util/test_w.rs

@cakebaker
Copy link
Contributor

@Krysztal112233 I get a similar output as @aisuneko on my machine using Arch Linux.

Copy link
Collaborator

@Krysztal112233 Krysztal112233 left a comment

Choose a reason for hiding this comment

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

Great work, thanks

@cakebaker cakebaker merged commit 46c2a0a into uutils:main Mar 29, 2025
14 checks passed
@codecov
Copy link

codecov bot commented Mar 29, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 0.00%. Comparing base (eb0871a) to head (f4f7963).
Report is 24 commits behind head on main.

Additional details and impacted files
@@     Coverage Diff     @@
##   main   #361   +/-   ##
===========================
===========================

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@cakebaker
Copy link
Contributor

Thanks for your PR!

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.

4 participants