Repository navigation
Conversation
|
nice |
|
a few jobs are failing |
|
Yes sorry I updated the pr by error, I still need to work on this |
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Introduces an initial implementation of the wall utility as a new optional workspace crate, along with basic CLI parsing and a small set of utility-specific tests.
Changes:
- Adds new
uu_wallcrate (library + binary) and wires it into workspace features/dependencies - Implements Unix-only
walllogic: message sourcing (stdin/args/file), scanning logged-in users viautmpx, and writing to ttys - Adds initial tests and minor formatting/import cleanups in existing test files
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/tests.rs | Registers the wall test module behind the wall feature flag |
| tests/by-util/test_wall.rs | Adds initial cross-platform tests for wall behavior |
| tests/by-util/test_setsid.rs | Adjusts import ordering (no functional change) |
| tests/by-util/test_lslocks.rs | Reflows an array literal for formatting (no functional change) |
| src/uu/wall/wall.md | Adds minimal help/about text for wall |
| src/uu/wall/src/wall.rs | Implements the core wall command logic and CLI definition |
| src/uu/wall/src/main.rs | Adds binary entrypoint using uucore::bin! |
| src/uu/wall/Cargo.toml | Adds new crate manifest and dependencies for uu_wall |
| Cargo.toml | Enables wall as an optional workspace feature/dependency |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 7 comments.
Suppressed comments (8)
src/uu/wall/Cargo.toml:19
- The nix dependency enables a feature named "feature", which doesn’t appear to be used elsewhere in this workspace and is likely not a valid nix crate feature; this can cause Cargo feature resolution failures.
chrono = { workspace = true }
clap.workspace = true
nix = { workspace = true, features = ["feature", "fs", "hostname", "term"] }
thiserror.workspace = true
src/uu/wall/src/wall.rs:40
- WallError::Stdin is used via
#[from] io::Error, so file-open/read errors also get reported as "cannot read stdin". Consider using a more general message so file input failures aren’t mislabeled.
enum WallError {
#[error("wall: cannot read stdin")]
Stdin(#[from] io::Error),
#[error("wall: encoding error")]
src/uu/wall/src/wall.rs:58
handle_clap_result(...).map_err(|e| USimpleError::new(1, ...))forces exit code 1 for clap’s help/version output and other parsing errors. Other utilities in this repo rely ontry_get_matches_from(args)?so clap/help uses the expected exit codes.
pub fn uumain(args: impl uucore::Args) -> UResult<()> {
let matches = uucore::clap_localization::handle_clap_result(uu_app(), args)
.map_err(|e| USimpleError::new(1, e.to_string()))?; // Clap would have return 101
// Might be considered wrong for --help and --version
let message = get_message(matches.get_many(STRING).unwrap_or_default())?;
src/uu/wall/src/wall.rs:122
- Same as the Linux cfg: prefer
Command::new(uucore::util_name())to follow the established pattern across utilities and keep the displayed name consistent.
pub fn uu_app() -> Command {
Command::new("wall")
.version(uucore::crate_version!())
src/uu/wall/src/wall.rs:101
- Typo in user-facing help text: "branner" → "banner".
.required(false)
.action(ArgAction::SetTrue)
.help("Suppress the intro branner of the broadcast"),
)
src/uu/wall/src/wall.rs:133
- Same user-facing help text issue here (ungrammatical).
.value_name("GROUP")
.help("Send restrict to only users in the group(s)")
.num_args(1)
src/uu/wall/src/wall.rs:218
- On macOS the broadcast header format string is missing the closing ")" after the datetime, which makes the output inconsistent with Linux and likely incorrect.
"\r\nBroadcast message from {}@{} ({tty}) at ({datetime} \r\n\r\n",
tests/by-util/test_wall.rs:24
- This test doesn’t assert anything, so it may not actually execute the command (unlike the other tests in this directory which call
.succeeds()/.fails()). Add an explicit expectation so the test verifies behavior on Linux.
fn test_invalid_file() {
new_ucmd!().arg("not_existing_file.not_existing_extension"); // Should print non-file name as broadcast
}
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved build, option-handling, output-formatting, error-handling, and test issues remain.
16 open issues
nixfeature list includes"feature", which does not correspond to a knownnixcrate…std::fs::File::openexpects anAsRef<Path>, but&OsStringdoes not implementAsRef<Path>;…get_sender()returns aString, so taking a reference to the temporary (&get_sender()) will…--help/--version(which clap reports…Command::new(uucore::util_name())so the app name matches…WallError::Stdinis used as the#[from] io::Errorconversion for multiple I/O operations (stdin…*.mdfiles (they…bidingshould bebinding(or similar). While not user-facing, it…
1 resolved since last review
Review effort: Lite
There was a problem hiding this comment.
🟡 Changes recommended
Active critical, moderate, and test-coverage issues remain, including a dependency configuration that prevents compilation, unsupported-platform build failures, incorrect CLI behavior, and unimplemented options.
19 open issues
nixfeature list includes"feature", which does not correspond to a knownnixcrate…std::fs::File::openexpects anAsRef<Path>, but&OsStringdoes not implementAsRef<Path>;…get_sender()returns aString, so taking a reference to the temporary (&get_sender()) will…--help/--version(which clap reports…Command::new(uucore::util_name())so the app name matches…WallError::Stdinis used as the#[from] io::Errorconversion for multiple I/O operations (stdin…*.mdfiles (they…bidingshould bebinding(or similar). While not user-facing, it…
Review effort: Lite
There was a problem hiding this comment.
🟡 Changes recommended
Several critical platform-specific compilation failures remain in the Unix implementation, including invalid imports, undefined options and symbols, and an undefined error variable.
9 open issues
Command::new(uucore::util_name())so the app name matches…*.mdfiles (they…
14 resolved since last review
nixfeature list includes"feature", which does not correspond to a knownnixcrate…std::fs::File::openexpects anAsRef<Path>, but&OsStringdoes not implementAsRef<Path>;…get_sender()returns aString, so taking a reference to the temporary (&get_sender()) will…--help/--version(which clap reports…WallError::Stdinis used as the#[from] io::Errorconversion for multiple I/O operations (stdin…bidingshould bebinding(or similar). While not user-facing, it…
Review effort: Lite
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical portability/build failures on non-Linux Unix targets, along with a moderate sender-identification bug and malformed help documentation, prevent approval.
8 open issues
Command::new(uucore::util_name())so the app name matches…*.mdfiles (they…
4 resolved since last review
Review effort: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
Human assessment is needed because the change contains critical cross-platform build and terminal-injection risks, along with message-formatting and diagnostic-error issues.
No open issues 🎉
8 resolved since last review
Command::new(uucore::util_name())so the app name matches…*.mdfiles (they…
Review effort: Lite
| let binding = unistd::gethostname().unwrap_or_else(|_| "".into()); | ||
| let hostname = binding.to_string_lossy(); | ||
|
|
||
| let user: String = env::var_os(user) |
There was a problem hiding this comment.
anyone can run USER=root wall hi and the banner says it is from root
please use getlogin() / the uid's passwd entry instead of $USER
| if args.len() == 0 { | ||
| read_from_stdin() | ||
| } else if args.len() == 1 { | ||
| match read_from_file(args.clone().next().unwrap()) { |
There was a problem hiding this comment.
if wall is installed setgid tty, this lets any user read a file only the tty group can read
util-linux refuses to read the file when uid != euid or gid != egid, could you please do the same?
| .collect(); | ||
| // Same here, control characters could also be hidden in the message provided. We still want newline char of course... | ||
| #[cfg(not(target_os = "macos"))] | ||
| let formatted_message = formatted_message.replace('\n', "\r\n\n"); |
There was a problem hiding this comment.
why \r\n\n? this adds an empty line after every line of the message
| while !ttys.is_empty() { | ||
| let remaining = deadline.saturating_duration_since(std::time::Instant::now()); | ||
| if remaining.is_zero() { | ||
| return Err(WallError::TimeoutError.into()); |
There was a problem hiding this comment.
one stuck terminal makes the whole wall fail with exit 1
is that wanted? i would expect it to just give up on that tty
|
|
||
| // #[cfg(all(unix, not(target_os = "linux"), not(target_os = "openbsd")))] | ||
| #[cfg(not(target_os = "linux"))] | ||
| pub fn uu_app() -> Command { |
There was a problem hiding this comment.
almost the same uu_app twice, with the commented-out group block copied in both
could be dedup with cfg on the linux-only args, no? and please remove the commented code
| fn test_invalid_file() { | ||
| new_ucmd!() | ||
| .arg("not_existing_file.not_existing_extension") | ||
| .succeeds(); // Should print non-file name as broadcast |
There was a problem hiding this comment.
this sends a real broadcast to everyone logged in on the machine running the tests
please add tests that check something (--help, -t 0 rejected, invalid utf-8, ...) without hitting the real ttys
| # This file is automatically @generated by Cargo. | ||
| # It is not intended for manual editing. | ||
| version = 3 | ||
| version = 4 |
There was a problem hiding this comment.
unrelated lockfile format bump (and num-conv), please revert
This pull request introduces an implementation of the 'wall' utility. It aims to be a first-draft binary crate since the flags of the wall command are not implemented yet.
New utility implementation:
CLI and mechanics: