skill: add basic implementation of `skill` #364

maplestarplayl · 2025-03-24T07:54:55Z

This pr add the skill command, but only works on Linux

maplestarplayl · 2025-03-24T07:58:57Z

Although the main functionality has been completed, some edge cases and potential improvements still need to be considered, such as command usage clarification, test coverage, more elegant implementation and performance optimization.

Cargo.toml

Krysztal112233 · 2025-03-24T09:57:25Z

src/uu/skill/src/command.rs

+pub const SIGNALS: &[&str] = &[
+    "HUP", "INT", "QUIT", "ILL", "TRAP", "ABRT", "BUS", "FPE", "KILL", "USR1", "SEGV", "USR2",
+    "PIPE", "ALRM", "TERM", "STKFLT", "CHLD", "CONT", "STOP", "TSTP", "TTIN", "TTOU", "URG",
+    "XCPU", "XFSZ", "VTALRM", "PROF", "WINCH", "POLL", "PWR", "SYS",
+];
+const DEFAULT_SIGNAL: &str = "TERM";


You can use the signals defined in uucore: https://github.com/uutils/coreutils/blob/main/src/uucore/src/lib/features/signals.rs

Ok, got it.
But I found that the defined ALL_SIGNALS have 32 signals in linux , which is not consistent with the Linux Manual (has 31).
Do you know what causes this difference and how should I handle this?
Thanks for help

please use text (code blocks) instead of picture, using text will make it easier to be searched :)

this post can help you to understand the EXIT "signal", which can be considered as a trap to capture something. it's an extension of shell, bash, zsh, fish, etc..

https://unix.stackexchange.com/questions/385030/are-exit-debug-return-and-err-signals

https://fishshell.com/docs/current/cmds/trap.html

please use text (code blocks) instead of picture, using text will make it easier to be searched :)

Sorry, my mistake. I’ll stick to using text in our future conversations.

this post can help you to understand the EXIT "signal", which can be considered as a trap to capture something. it's an extension of shell, bash, zsh, fish, etc..

https://unix.stackexchange.com/questions/385030/are-exit-debug-return-and-err-signals

https://fishshell.com/docs/current/cmds/trap.html

After reviewing these articles, it appears that signals like EXIT are specific to the shell and not applicable to all processes. Therefore, in the context of the skill command, I have excluded them. Does this approach seem correct?

I think it can be processed in this way

Sorry, my mistake. I’ll stick to using text in our future

Please don't be nervous, it's not a mistake :)

src/uu/skill/src/util.rs

Krysztal112233 · 2025-03-24T10:02:21Z

src/uu/skill/src/command.rs

+    }
+}
+
+pub fn parse_command(args: &mut impl uucore::Args) -> Vec<OsString> {


could you please explain what this function does?

Ok, I can explain, i know it looks a little wired :)

Issue Explanation:

The core problem lies in the signal format expected by skill, which follows the structure:
skill [signal] [options] <expression

However, the signal is passed in the form -HUP instead of -s HUP, preventing clap from automatically parsing it from the arguments.

Solution Approach:

To handle this correctly:

Signal as an Indexed Argument:
We must define signal as an indexed argument in clap so it can be properly parsed.

Expression as an Indexed Argument
Since the expression is also passed in a similar way(not an option), it must also be treated as an indexed argument.

But after doing things above, another problem comes in: Clap's Restriction on Optional Arguments:
clap enforces a rule where if a later indexed argument is required, earlier ones cannot be optional.
This means signal must be marked as required to comply with clap's parsing logic, which is conflict with skill's Default Behavior:
The standard skill implementation (procps-ng 3.3.17) allows signal to be optional with a default value.
To reconcile this with clap's requirements, we preprocess the arguments before passing them to clap, ensuring signal is always provided (even if defaulted) to satisfy parsing constraints.

Summary of the Issue

The root cause of these challenges stems from the fact that skill's argument format (as defined in procps-ng 3.3.17) cannot be directly modeled using clap's parsing rules.

However, this doesn’t imply any limitation in clap—rather, skill’s unconventional syntax (e.g., -HUP instead of -s HUP) deviates from typical CLI conventions, making parsing more complex.

Whatever, I will add more comments to the code to make it more reasonable :)
Also, if there are other more elegant way to process that, please tell me, thanks!

@cakebaker can you give us some help? seems like this is something clap can't handle.

@maplestarplayl and if it is possible, could you write some document(comment) and unit tests to make sure it doesn't regressing in future?

@maplestarplayl and if it is possible, could you write some document(comment) and unit tests to make sure it doesn't regressing in future?

Have added corresponding tests in the command.rs file

Krysztal112233 · 2025-03-24T10:03:19Z

Great work, and please don't forget run rustfmt before commit :)

src/uu/skill/src/command.rs

maplestarplayl · 2025-04-01T13:59:17Z

Refactor the util functions with pgrep's process.rs, and add corresponding tests for command and user usage in skill command

src/uu/skill/Cargo.toml

cakebaker · 2025-04-02T14:14:04Z

Can you please run cargo clippy? And the CI shows some issues on macOS :|

cakebaker · 2025-04-02T14:17:31Z

tests/by-util/test_skill.rs

+}
+#[cfg(target_os = "linux")]


Suggested change

}

#[cfg(target_os = "linux")]

}

#[cfg(target_os = "linux")]

cakebaker · 2025-04-02T14:21:06Z

tests/by-util/test_skill.rs

+}
+#[cfg(target_os = "linux")]


Suggested change

}

#[cfg(target_os = "linux")]

}

#[cfg(target_os = "linux")]

cakebaker · 2025-04-02T14:21:51Z

tests/by-util/test_skill.rs

+}
+#[cfg(target_os = "linux")]
+#[test]
+fn test_mutiple_signal() {


A typo:

Suggested change

fn test_mutiple_signal() {

fn test_multiple_signal() {

cakebaker · 2025-04-02T14:23:12Z

tests/by-util/test_skill.rs

+}
+#[cfg(target_os = "linux")]


Suggested change

}

#[cfg(target_os = "linux")]

}

#[cfg(target_os = "linux")]

cakebaker · 2025-04-02T14:24:35Z

tests/by-util/test_skill.rs

+}
+#[cfg(target_os = "linux")]


Suggested change

}

#[cfg(target_os = "linux")]

}

#[cfg(target_os = "linux")]

cakebaker · 2025-04-02T14:29:55Z

tests/by-util/test_skill.rs

+#[test]
+fn test_list_option() {
+    new_ucmd!()
+        .arg("-l")
+        .succeeds()
+        .no_stderr()
+        .stdout_contains("HUP INT QUIT ILL TRAP ABRT BUS FPE KILL USR1 SEGV USR2 PIPE ALRM TERM STKFLT CHLD CONT STOP TSTP TTIN TTOU URG XCPU XFSZ VTALRM PROF WINCH POLL PWR SYS");
+}
+
+#[cfg(target_os = "linux")]
+#[test]
+fn test_list_option_long() {
+    new_ucmd!()
+        .arg("--list")
+        .succeeds()
+        .no_stderr()
+        .stdout_contains("HUP INT QUIT ILL TRAP ABRT BUS FPE KILL USR1 SEGV USR2 PIPE ALRM TERM STKFLT CHLD CONT STOP TSTP TTIN TTOU URG XCPU XFSZ VTALRM PROF WINCH POLL PWR SYS");
+}


Here it might make sense to combine the functions into one and simply loop through the args, something like:

for arg in ["-l", "--list"] { new_ucmd!() .arg(arg) ... }

cakebaker · 2025-04-02T14:31:28Z

tests/by-util/test_skill.rs

+#[cfg(target_os = "linux")]
+#[test]
+fn test_table_option() {
+    new_ucmd!()
+        .arg("-L")
+        .succeeds()
+        .no_stderr()
+        .stdout_contains("1 HUP     2 INT     3 QUIT    4 ILL     5 TRAP    6 ABRT    7 BUS")
+        .stdout_contains("8 FPE     9 KILL   10 USR1   11 SEGV   12 USR2   13 PIPE   14 ALRM")
+        .stdout_contains("15 TERM   16 STKFLT 17 CHLD   18 CONT   19 STOP   20 TSTP   21 TTIN")
+        .stdout_contains("22 TTOU   23 URG    24 XCPU   25 XFSZ   26 VTALRM 27 PROF   28 WINCH")
+        .stdout_contains("29 POLL   30 PWR    31 SYS");
+}
+
+#[cfg(target_os = "linux")]
+#[test]
+fn test_table_option_long() {
+    new_ucmd!()
+        .arg("--table")
+        .succeeds()
+        .no_stderr()
+        .stdout_contains("1 HUP     2 INT     3 QUIT    4 ILL     5 TRAP    6 ABRT    7 BUS")
+        .stdout_contains("8 FPE     9 KILL   10 USR1   11 SEGV   12 USR2   13 PIPE   14 ALRM")
+        .stdout_contains("15 TERM   16 STKFLT 17 CHLD   18 CONT   19 STOP   20 TSTP   21 TTIN")
+        .stdout_contains("22 TTOU   23 URG    24 XCPU   25 XFSZ   26 VTALRM 27 PROF   28 WINCH")
+        .stdout_contains("29 POLL   30 PWR    31 SYS");
+}


Same with these two functions.

cakebaker · 2025-04-02T14:36:18Z

tests/by-util/test_skill.rs

+
+#[cfg(target_os = "linux")]
+#[test]
+fn test_mutiple_options() {
+    new_ucmd!()
+        .arg("-nv") // no action + verbose
+        .arg("1")
+        .succeeds()
+        .stdout_contains("Would send signal TERM to process 1");
+}


This function is identical to test_default_signal, so it can be removed.

Suggested change

#[cfg(target_os = "linux")]

#[test]

fn test_mutiple_options() {

new_ucmd!()

.arg("-nv") // no action + verbose

.arg("1")

.succeeds()

.stdout_contains("Would send signal TERM to process 1");

}

cakebaker · 2025-04-02T14:39:30Z

src/uu/skill/src/util.rs

+use uu_pgrep::process::{walk_process, ProcessInformation};
+#[cfg(target_os = "linux")]


Suggested change

use uu_pgrep::process::{walk_process, ProcessInformation};

#[cfg(target_os = "linux")]

use uu_pgrep::process::{walk_process, ProcessInformation};

#[cfg(target_os = "linux")]

cakebaker · 2025-04-02T14:54:22Z

src/uu/skill/src/util.rs

+
+#[cfg(target_os = "linux")]
+pub fn get_process_owner(process: &mut ProcessInformation) -> Option<String> {
+    // let status_path = format!("/proc/{}/status", pid);


I would either remove this line or add a TODO with a reason why it's commented out.

cakebaker · 2025-04-02T15:13:20Z

src/uu/skill/src/command.rs

+
+// Mainly used to parse the signal to make sure it is valid
+// and insert the default signal if it's not present
+pub fn parse_command(args: &mut impl uucore::Args) -> Vec<String> {


As this function can fail, I would return a Result.

cakebaker · 2025-04-02T15:19:11Z

src/uu/skill/src/command.rs

+            eprintln!("Invalid signal: {}", &signal);
+            std::process::exit(2);


See my previous comment about returning a Result. We usually use something like:

return Err(USimpleError::new(2, format!("Invalid signal: {}", &signal)));

cakebaker · 2025-04-02T15:19:34Z

src/uu/skill/src/command.rs

+        eprintln!("Too many signals");
+        std::process::exit(2);


See my previous comment.

cakebaker

Overall good job :)

skill: add basic implementation of sload

1932114

Krysztal112233 changed the title ~~skill: add basic implementation of sload~~ skill: add basic implementation of skill Mar 24, 2025

Krysztal112233 reviewed Mar 24, 2025

View reviewed changes

Krysztal112233 requested review from cakebaker and sylvestre March 24, 2025 10:03

cakebaker linked an issue Mar 26, 2025 that may be closed by this pull request

Add command skill #29

Open

maplestarplayl added 2 commits March 27, 2025 07:07

skill: refactor command parsing and improve test coverage

a4efca1

Merge branch 'main' into main

1892fa0

Krysztal112233 reviewed Mar 29, 2025

View reviewed changes

src/uu/skill/src/command.rs Outdated Show resolved Hide resolved

src/uu/skill/src/command.rs Outdated Show resolved Hide resolved

maplestarplayl added 2 commits April 1, 2025 21:44

Merge branch 'uutils:main' into main

27ea243

skill: refactor util with pgrep's process and add tests

ea1e43f

cakebaker reviewed Apr 2, 2025

View reviewed changes

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

maplestarplayl added 2 commits April 2, 2025 19:04

skill: add missing newline

017aa84

Merge branch 'uutils:main' into main

927bb53

cakebaker reviewed Apr 2, 2025

View reviewed changes

Merge branch 'main' into main

02c93a3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

skill: add basic implementation of `skill` #364

skill: add basic implementation of `skill` #364

maplestarplayl commented Mar 24, 2025

maplestarplayl commented Mar 24, 2025

Krysztal112233 Mar 24, 2025

maplestarplayl Mar 25, 2025

Krysztal112233 Mar 25, 2025

Krysztal112233 Mar 25, 2025 •

edited

Loading

maplestarplayl Mar 27, 2025

maplestarplayl Mar 27, 2025

Krysztal112233 Mar 27, 2025

Krysztal112233 Mar 27, 2025

Krysztal112233 Mar 24, 2025

maplestarplayl Mar 25, 2025

Krysztal112233 Mar 25, 2025

Krysztal112233 Mar 25, 2025

maplestarplayl Mar 27, 2025

Krysztal112233 commented Mar 24, 2025 •

edited

Loading

maplestarplayl commented Apr 1, 2025

cakebaker commented Apr 2, 2025

cakebaker Apr 2, 2025

cakebaker Apr 2, 2025

cakebaker Apr 2, 2025

cakebaker Apr 2, 2025

cakebaker Apr 2, 2025

cakebaker Apr 2, 2025

cakebaker Apr 2, 2025

cakebaker Apr 2, 2025

cakebaker Apr 2, 2025

cakebaker Apr 2, 2025

cakebaker Apr 2, 2025

cakebaker Apr 2, 2025

cakebaker Apr 2, 2025

cakebaker left a comment

		use uu_pgrep::process::{walk_process, ProcessInformation};
		#[cfg(target_os = "linux")]

		eprintln!("Invalid signal: {}", &signal);
		std::process::exit(2);

skill: add basic implementation of skill #364

Are you sure you want to change the base?

skill: add basic implementation of skill #364

Conversation

maplestarplayl commented Mar 24, 2025

maplestarplayl commented Mar 24, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Krysztal112233 Mar 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Issue Explanation:

Solution Approach:

Summary of the Issue

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Krysztal112233 commented Mar 24, 2025 • edited Loading

maplestarplayl commented Apr 1, 2025

cakebaker commented Apr 2, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cakebaker left a comment

Choose a reason for hiding this comment

skill: add basic implementation of `skill` #364

skill: add basic implementation of `skill` #364

Krysztal112233 Mar 25, 2025 •

edited

Loading

Krysztal112233 commented Mar 24, 2025 •

edited

Loading