Remove clap for echo #7603

cerdelen · 2025-03-28T15:24:09Z

removed clap as a parsing tool for echo. Added some tests from previous issues comment sections regarding double hyphen problematics.

…roblematic interactions with hyphens

github-actions · 2025-03-28T16:10:18Z

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/misc/usage_vs_getopt (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 · 2025-03-28T16:55:58Z

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/misc/usage_vs_getopt (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)

Copilot

Pull Request Overview

This PR removes the dependency on clap for echo’s argument parsing and introduces a custom flag parsing logic alongside additional tests to verify handling of double hyphen and flag-like arguments.

Removed clap-specific parsing code and replaced it with custom logic in echo.rs
Updated the POSIXLY_CORRECT check to compare against "1"
Added new tests in tests/by-util/test_echo.rs for various argument scenarios

Reviewed Changes

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

File	Description
tests/by-util/test_echo.rs	Added tests covering parsing of flags and double hyphen cases
src/uu/echo/src/echo.rs	Removed clap usage; implemented custom parsing and updated env var check

Comments suppressed due to low confidence (2)

tests/by-util/test_echo.rs:296

This test case omits a trailing newline in its expected output, which is inconsistent with similar tests that include a newline. Please verify whether a trailing newline should be expected here.

.stdout_only("- --");

src/uu/echo/src/echo.rs:101

Changing the POSIXLY_CORRECT check from verifying the existence of the variable to comparing its value to "1" may affect behavior if the variable is set to any truthy value other than "1". Please confirm that this change aligns with the intended behavior.

posixly_correct == "1"

sylvestre · 2025-03-28T21:19:15Z

src/uu/echo/src/echo.rs

-fn is_echo_flag(arg: &OsString) -> bool {
-    matches!(arg.to_str(), Some("-e" | "-E" | "-n"))
+struct EchoFlags {
+    pub n: bool,


please make this more explicit

Is this better?

yes for the name but your comments are a bit hard to understand IMHO :)

github-actions · 2025-03-29T07:18:39Z

GNU testsuite comparison:

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

github-actions · 2025-03-29T10:50:40Z

GNU testsuite comparison:

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

tertsdiepraam · 2025-03-29T11:29:21Z

src/uu/echo/src/echo.rs

 }

 #[uucore::main]
 pub fn uumain(args: impl uucore::Args) -> UResult<()> {
-    let is_posixly_correct = env::var("POSIXLY_CORRECT").is_ok();
+    // Check POSIX compatibility mode
+    let is_posixly_correct = if let Ok(posixly_correct) = env::var("POSIXLY_CORRECT") {


Could you explain this change. As far as I know GNU checks whether the env var is defined not whether it's 1. Is echo an exception?

Wasn't sure about this. I checked in the test files and there it was set to be equal to one.

If the POSIXLY_CORRECT environment variable is set found this on gnu.org on echo man pages. So i just need to check if its set at all, not whether it is set to any true value like 1 or true?

reverted the change to now only check if it is defined at all

github-actions · 2025-03-29T12:16:11Z

GNU testsuite comparison:

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

blyxxyz

Nice! I think this behavior is exactly correct.

blyxxyz · 2025-03-30T19:29:06Z

src/uu/echo/src/echo.rs

+    if bytes.first() == Some(&b'-') {
+        let mut flags = EchoFlags {
+            disable_newline: false,
+            escape: false,
+            is_single_hyphen: false,
+        };
+        // Single hyphen case: "-" (stops flag processing, no other effect)
+        if arg.len() == 1 {
+            flags.is_single_hyphen = true;
+            return Some(flags);
+        }


Suggested change

if bytes.first() == Some(&b'-') {

let mut flags = EchoFlags {

disable_newline: false,

escape: false,

is_single_hyphen: false,

};

// Single hyphen case: "-" (stops flag processing, no other effect)

if arg.len() == 1 {

flags.is_single_hyphen = true;

return Some(flags);

}

if bytes.first() == Some(&b'-') && arg != "-" {

let mut flags = EchoFlags {

disable_newline: false,

escape: false,

};

Would this work? I think the is_single_hyphen case is identical to the None case

Also correct. Changed this part too. Thanks!

blyxxyz · 2025-03-30T19:35:41Z

src/uu/echo/src/echo.rs

+            if echo_flags.disable_newline {
+                trailing_newline = false;
+            }
+            escape = echo_flags.escape;


This is too aggressive since it means -e -n disables escape. Maybe is_echo_flag can mutably borrow a single EchoFlags value that's reused and returned?

Great observation. Indeed this was an issue. I fixed this and also added some test as there was no test in the test suite so far to check for this behaviour.

github-actions · 2025-03-31T10:00:37Z

GNU testsuite comparison:

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

src/uu/echo/src/echo.rs

Enabling POSIXLY_CORRECT flag if value is not UTF-8 Co-authored-by: Jan Verbeek <jan.verbeek@posteo.nl>

github-actions · 2025-03-31T11:08:45Z

GNU testsuite comparison:

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

BenWiederhake

LGTM! I can't check right now whether this is truly equal to the GNU behavior, but it seems sound to me. Feel free to address the nitpick or not :)

BenWiederhake · 2025-04-18T13:55:21Z

src/uu/echo/src/echo.rs

 }

 #[uucore::main]
 pub fn uumain(args: impl uucore::Args) -> UResult<()> {
-    let is_posixly_correct = env::var("POSIXLY_CORRECT").is_ok();
+    // Check POSIX compatibility mode
+    let is_posixly_correct = env::var_os("POSIXLY_CORRECT").is_some();


There is a subtle change in behavior here: POSIXLY_CORRECT=<non-utf8-sequence> is now interpreted as true, whereas before it was interpreted as false. I assume this is a correct change. (I can't test GNU easily right now.)

$ POSIXLY_CORRECT=$'\377asd' cargo run echo 'asd\nqwe' # current main asd\nqwe $ POSIXLY_CORRECT=$'\377asd' cargo run echo 'asd\nqwe' # your PR asd qwe

Nitpick: If you fix subtleties like this, it would be extra awesome if you also add a test, so that we don't lose it later :)

I suggested that change upthread when I saw this code at the edge of a diff 😅

The following test (in test_echo.rs inside mod posixly_correct {}) should do the trick:

#[cfg(unix)] #[test] fn non_utf8_variable() { use std::{ffi::OsStr, os::unix::ffi::OsStrExt}; new_ucmd!() .env("POSIXLY_CORRECT", OsStr::from_bytes(b"\xFF")) .arg("-e") .succeeds() .stdout_only("-e\n"); }

(I can confirm that this matches GNU, GNU activates POSIX mode regardless of whether the variable's value is 1 or 0 or empty string or \xFF.)

Awesome! But I would argue that if you change the behavior, then an old test cannot possibly correctly test for the new behavior :D

In particular, this test only tests whether -e is interpreted or considered as part of the output. It does not test whether escape sequences are interpreted or taken literally, i.e. whether "escape mode" is initially set or not.

Like I said, it's not a major thing, and doesn't block this PR.

It's not an old test, I meant that was where I'd put it. It fails if env::var(...).is_ok() is used.

tertsdiepraam

Great work!

@BenWiederhake since I baked this behaviour into uutils-args, this is a good candidate to port next. But nevertheless, it's great to have this implementation (and tests!) first.

We could also decide to take the behaviour out of uutils-args to simplify it.

BenWiederhake · 2025-04-22T21:09:00Z

@BenWiederhake since I baked this behaviour into uutils-args, this is a good candidate to port next. But nevertheless, it's great to have this implementation (and tests!) first.

I agree! The reason I tackled du -B first was that I didn't want to put this PR into cross-fire.

Btw, can't be merged currently due to merge conflicts.

* Parsing echo flags manually without clap as clap introduced various problematic interactions with hyphens * fixed error where multiple flags would parse wrong * Spelling & formatting fixes * docu for EchoFlag struct * more extensive comment/documentation * revert POSIXLY_CORRECT check to only check if it is set * Fixed problem of overwriting flags. Added test for same issue * cargo fmt * cspell * Update src/uu/echo/src/echo.rs Enabling POSIXLY_CORRECT flag if value is not UTF-8 Co-authored-by: Jan Verbeek <jan.verbeek@posteo.nl> --------- Co-authored-by: Jan Verbeek <jan.verbeek@posteo.nl>

cerdelen added 2 commits March 28, 2025 16:20

Parsing echo flags manually without clap as clap introduced various p…

ca07881

…roblematic interactions with hyphens

fixed error where multiple flags would parse wrong

2ead244

Spelling & formatting fixes

2f1a9d1

sylvestre requested a review from Copilot March 28, 2025 21:18

Copilot AI reviewed Mar 28, 2025

View reviewed changes

sylvestre reviewed Mar 28, 2025

View reviewed changes

cerdelen and others added 2 commits March 29, 2025 07:43

docu for EchoFlag struct

c1c127e

Merge branch 'main' into remove_clap_for_echo

79a90fd

more extensive comment/documentation

ab95cf8

tertsdiepraam reviewed Mar 29, 2025

View reviewed changes

revert POSIXLY_CORRECT check to only check if it is set

fcca66a

cerdelen requested a review from tertsdiepraam March 29, 2025 12:37

blyxxyz reviewed Mar 30, 2025

View reviewed changes

cerdelen added 3 commits March 31, 2025 11:19

Fixed problem of overwriting flags. Added test for same issue

5fc2c34

cargo fmt

bb16513

cspell

17c6c21

blyxxyz approved these changes Mar 31, 2025

View reviewed changes

src/uu/echo/src/echo.rs Outdated Show resolved Hide resolved

Update src/uu/echo/src/echo.rs

e8c3e42

Enabling POSIXLY_CORRECT flag if value is not UTF-8 Co-authored-by: Jan Verbeek <jan.verbeek@posteo.nl>

sylvestre requested a review from BenWiederhake April 17, 2025 19:48

BenWiederhake approved these changes Apr 18, 2025

View reviewed changes

tertsdiepraam approved these changes Apr 20, 2025

View reviewed changes

sylvestre merged commit 13c0a81 into uutils:main May 4, 2025
68 checks passed

BrewTestBot mentioned this pull request May 24, 2025

uutils-coreutils 0.1.0 Homebrew/homebrew-core#224645

Merged

moonfruit mentioned this pull request May 26, 2025

uutils-selected 0.1.0 moonfruit/homebrew-tap#243

Closed

Uh oh!

Remove clap for echo #7603

Remove clap for echo #7603

Uh oh!

Conversation

cerdelen commented Mar 28, 2025

Uh oh!

github-actions bot commented Mar 28, 2025

Uh oh!

github-actions bot commented Mar 28, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 29, 2025

Uh oh!

github-actions bot commented Mar 29, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 29, 2025

Uh oh!

blyxxyz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blyxxyz Mar 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 31, 2025

Uh oh!

Uh oh!

github-actions bot commented Mar 31, 2025

Uh oh!

BenWiederhake left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tertsdiepraam left a comment

Choose a reason for hiding this comment

Uh oh!

BenWiederhake commented Apr 22, 2025

Uh oh!

Uh oh!

Uh oh!

blyxxyz Mar 30, 2025 •

edited

Loading