env: support string args by "-S", "-vS" or "--split-strings" #5801

cre4ture · 2024-01-07T12:41:38Z

addresses #5710

This implements all the functionality tested by gnu/tests/env/env-S.pl.
I had to patch the test due to more powerfull implementation and differencences in how parser errors are reported.
I Introduced a mechanism to patch the test with patch-files as the sed-stuff I could not make run for me.

The feature is quite complex, as the tool needs do parsing and evaluation (quoting, escape sequences, environmental variables) of the string on a similar level as a shell:
env -S<single full featured shell command>

I used "shell-words" create (MIT license) as a basis. I did heavy modifications, such that I directly commit the sources to this repo. EDIT: Additionally I extended shell-words with a variable expanstion functionality as needed by env.
The idea to use subst was rejected after discussions with the subst maintainer.

sylvestre · 2024-01-07T12:59:11Z

I really don't like forking. Did you try to contact upstream to propose this behavior?

Also, please squash your changes before you push them. Thanks

cre4ture · 2024-01-07T13:59:03Z

I think the changes for subst are small and probably acceptable by upstream. I'll do a pull request for this and try to get it in.
Regarding shell-words I'm not so optimistic. The needed changes where really against many of the existing tests there. And then I started to do refactorings and integrated the variable substitution directly... I hope your fine with doing a "fork" there. I think this is less of an issue as the original implementation in only one single file with ~500 lines of code. What do you think?

github-actions · 2024-01-07T14:43:28Z

GNU testsuite comparison:

Skip an intermittent issue tests/tail/inotify-dir-recreate (fails in this run but passes in the 'main' branch)
Congrats! The gnu test tests/env/env-S is no longer failing!

sylvestre · 2024-01-07T16:33:08Z

you forked this file, right ?
https://github.com/tmiasko/shell-words/blob/master/src/lib.rs

cre4ture · 2024-01-08T18:01:11Z

yes. its content is partly in new file split_iterator.rs and the tests I moved into the test_env.rs test suite

github-actions · 2024-01-08T18:38:22Z

GNU testsuite comparison:

Congrats! The gnu test tests/env/env-S is no longer failing!

github-actions · 2024-01-10T21:47:48Z

GNU testsuite comparison:

Congrats! The gnu test tests/env/env-S is no longer failing!
Skipping an intermittent issue tests/tail/inotify-dir-recreate (passes in this run but fails in the 'main' branch)

github-actions · 2024-01-10T22:35:08Z

GNU testsuite comparison:

Congrats! The gnu test tests/env/env-S is no longer failing!

src/uu/env/Cargo.toml

github-actions · 2024-01-12T21:08:35Z

GNU testsuite comparison:

Congrats! The gnu test tests/env/env-S is no longer failing!
Skipping an intermittent issue tests/tail/inotify-dir-recreate (passes in this run but fails in the 'main' branch)

github-actions · 2024-01-12T21:57:13Z

GNU testsuite comparison:

Congrats! The gnu test tests/env/env-S is no longer failing!
Skipping an intermittent issue tests/tail/inotify-dir-recreate (passes in this run but fails in the 'main' branch)

github-actions · 2024-01-12T22:47:37Z

GNU testsuite comparison:

Congrats! The gnu test tests/env/env-S is no longer failing!

github-actions · 2024-01-14T11:45:47Z

GNU testsuite comparison:

Congrats! The gnu test tests/env/env-S is no longer failing!

github-actions · 2024-01-14T12:38:39Z

GNU testsuite comparison:

Skip an intermittent issue tests/tail/inotify-dir-recreate (fails in this run but passes in the 'main' branch)
Congrats! The gnu test tests/env/env-S is no longer failing!

github-actions · 2024-01-14T13:35:38Z

GNU testsuite comparison:

Skip an intermittent issue tests/tail/inotify-dir-recreate (fails in this run but passes in the 'main' branch)
Congrats! The gnu test tests/env/env-S is no longer failing!

github-actions · 2024-01-15T15:09:19Z

GNU testsuite comparison:

Congrats! The gnu test tests/env/env-S is no longer failing!

github-actions · 2024-01-15T16:07:12Z

GNU testsuite comparison:

Congrats! The gnu test tests/env/env-S is no longer failing!

github-actions · 2024-01-15T17:40:16Z

GNU testsuite comparison:

Congrats! The gnu test tests/env/env-S is no longer failing!

github-actions · 2024-01-15T20:09:33Z

GNU testsuite comparison:

Skip an intermittent issue tests/tail/inotify-dir-recreate (fails in this run but passes in the 'main' branch)
Congrats! The gnu test tests/env/env-S is no longer failing!

github-actions · 2024-02-25T21:36:45Z

GNU testsuite comparison:

Congrats! The gnu test tests/env/env-S is no longer failing!

github-actions · 2024-02-26T22:36:06Z

GNU testsuite comparison:

Congrats! The gnu test tests/env/env-S is no longer failing!

github-actions · 2024-02-27T16:21:18Z

GNU testsuite comparison:

Congrats! The gnu test tests/env/env-S is no longer failing!

github-actions · 2024-03-08T23:13:57Z

GNU testsuite comparison:

Skip an intermittent issue tests/tail/inotify-dir-recreate (fails in this run but passes in the 'main' branch)
Congrats! The gnu test tests/env/env-S is no longer failing!

cre4ture · 2024-03-14T18:41:07Z

I implemented it now by using u8 byte arrays on linux and u16 word arrays on windows.
This way, it should be fully compatible with non-utf encoded sequences on both operating system types.

github-actions · 2024-03-14T19:07:32Z

GNU testsuite comparison:

Congrats! The gnu test tests/env/env-S is no longer failing!
Congrats! The gnu test tests/mv/backup-dir is no longer failing!

sylvestre · 2024-03-18T16:02:10Z

is it ready for review ? :)
thanks

github-actions · 2024-03-19T00:37:15Z

GNU testsuite comparison:

Congrats! The gnu test tests/env/env-S is no longer failing!

cre4ture · 2024-03-19T18:03:58Z

@sylvestre yes its ready. thanks

sylvestre · 2024-03-19T18:08:38Z

src/uu/env/src/env.rs

    }

-    // NOTE: we manually set and unset the env vars below rather than using Command::env() to more
-    //       easily handle the case where no command is given
+    #[allow(clippy::cognitive_complexity)]


for a future PR, it would be nice to address this warning

this is moved code. But I'll address it anyway.

sylvestre · 2024-03-19T18:09:00Z

src/uu/env/src/env.rs

+            program: vec![],
+        };
+
+        // change directory


explain the why :)

this is moved code. But I'll address it anyway by extraction to function

sylvestre · 2024-03-19T18:09:48Z

src/uu/env/src/env.rs

+        // load .env-style config file prior to those given on the command-line
+        load_config_file(&mut opts)?;
+
+        // unset specified env vars


same, explain why we unset these variables.
Security?

this is moved code. But I'll address it anyway by extraction to function.
the variables are unset because the corresponding command line argument to unset the variables is provided.

sylvestre · 2024-03-19T18:10:23Z

src/uu/env/src/env.rs

+                        // `exit.code()` returns `None` on Unix when the process is terminated by a signal.
+                        // See std::os::unix::process::ExitStatusExt for more information. This prints out
+                        // the interrupted process and the signal it received.
+                        let signal_code = exit.signal().unwrap();


should we handle the potential errors ? (same question for the next line)

this is moved code.
I validated that the unwraps are save. So I think there is nothing to address.

sylvestre · 2024-03-19T18:15:20Z

src/uu/env/src/native_int_str.rs

+// For the full copyright and license information, please view the LICENSE
+// file that was distributed with this source code.
+
+// This module contains classes and functions for dealing with the differences


this can't be used elsewhere in the coreutils ?

if so, maybe it could be moved into uucore
cc @cakebaker @tertsdiepraam

I was planning to move it as soon as I found some other usecase for it.
But If you wish, I move it also in advance.

sylvestre · 2024-03-19T18:17:15Z

Some minor comments, it is impressive work btw :)

sylvestre · 2024-03-19T18:32:18Z

given the size of the work, let's land it now and fix my comments later :)
well done again

cre4ture · 2024-03-22T18:50:57Z

given the size of the work, let's land it now and fix my comments later :) well done again

thanks. :-)
I'll create a new PR for the findings then.

sylvestre · 2024-03-29T14:34:29Z

@cre4ture seems that v9.5 of coreutils made this env-S test fails
#6139

cre4ture · 2024-03-30T18:48:36Z

its not related to "-S" as its in the env.sh.
But I investigated it already. It seems that there was a new feature added to env:

‘-a arg’
‘--argv0=arg’

    Override the zeroth argument passed to the command being executed. Without this option a default value of command is used.

if rust std-library allows to override this then it should be an easy thing. If not, it's difficult.

cre4ture force-pushed the feature/env_string_args_try_with_shell_words branch from 3efb40a to 52077cb Compare January 7, 2024 14:09

cre4ture mentioned this pull request Jan 9, 2024

substitute_one_step: stop after first variable has been substituted fizyr/subst#11

Closed

cre4ture force-pushed the feature/env_string_args_try_with_shell_words branch 2 times, most recently from 80a3291 to d76ee6b Compare January 10, 2024 21:11

sylvestre reviewed Jan 11, 2024

View reviewed changes

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

tertsdiepraam mentioned this pull request Jan 11, 2024

env: split-string implementation #1326

Closed

cakebaker linked an issue Jan 11, 2024 that may be closed by this pull request

env: split-string implementation #1326

Closed

cre4ture force-pushed the feature/env_string_args_try_with_shell_words branch from 06d603a to a4a3ba1 Compare January 12, 2024 20:29

cre4ture force-pushed the feature/env_string_args_try_with_shell_words branch from 84448b4 to 299decf Compare January 12, 2024 21:23

cre4ture force-pushed the feature/env_string_args_try_with_shell_words branch from b995a0d to c5eebe6 Compare January 14, 2024 12:47

cre4ture force-pushed the feature/env_string_args_try_with_shell_words branch from 514a489 to ab50934 Compare January 15, 2024 21:29

cre4ture requested a review from sylvestre January 15, 2024 21:30

cre4ture force-pushed the feature/env_string_args_try_with_shell_words branch from ab5b5b8 to b980a88 Compare February 27, 2024 15:49

cre4ture force-pushed the feature/env_string_args_try_with_shell_words branch from b980a88 to 2bd2e58 Compare March 8, 2024 22:04

cre4ture force-pushed the feature/env_string_args_try_with_shell_words branch from 2bd2e58 to 7271b35 Compare March 14, 2024 18:38

env: support string args by "-S", "-vS" or "--split-strings"

c62ba55

cre4ture force-pushed the feature/env_string_args_try_with_shell_words branch from 7271b35 to c62ba55 Compare March 18, 2024 23:45

sylvestre reviewed Mar 19, 2024

View reviewed changes

sylvestre merged commit 08172c2 into uutils:main Mar 19, 2024
64 checks passed

cre4ture mentioned this pull request Mar 22, 2024

Fix/findings for env string args #6105

Merged

chenrui333 mentioned this pull request Mar 24, 2024

uutils-coreutils 0.0.25 Homebrew/homebrew-core#167053

Merged

moonfruit mentioned this pull request Mar 28, 2024

uutils-selected 0.0.25 moonfruit/homebrew-tap#92

Merged

cre4ture mentioned this pull request Mar 30, 2024

env argv0 overwrite possibility (unix only) - fixes new gnu test version #6154

Merged

BenWiederhake mentioned this pull request Mar 31, 2024

env: implement -S/--split-string #5710

Closed

env: support string args by "-S", "-vS" or "--split-strings" #5801

env: support string args by "-S", "-vS" or "--split-strings" #5801

Conversation

cre4ture commented Jan 7, 2024 • edited Loading

sylvestre commented Jan 7, 2024

cre4ture commented Jan 7, 2024

github-actions bot commented Jan 7, 2024

sylvestre commented Jan 7, 2024

cre4ture commented Jan 8, 2024

github-actions bot commented Jan 8, 2024

github-actions bot commented Jan 10, 2024

github-actions bot commented Jan 10, 2024

github-actions bot commented Jan 12, 2024

github-actions bot commented Jan 12, 2024

github-actions bot commented Jan 12, 2024

github-actions bot commented Jan 14, 2024

github-actions bot commented Jan 14, 2024

github-actions bot commented Jan 14, 2024

github-actions bot commented Jan 15, 2024

github-actions bot commented Jan 15, 2024

github-actions bot commented Jan 15, 2024

github-actions bot commented Jan 15, 2024

github-actions bot commented Feb 25, 2024

github-actions bot commented Feb 26, 2024

github-actions bot commented Feb 27, 2024

github-actions bot commented Mar 8, 2024

cre4ture commented Mar 14, 2024

github-actions bot commented Mar 14, 2024

sylvestre commented Mar 18, 2024

github-actions bot commented Mar 19, 2024

cre4ture commented Mar 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cre4ture Mar 22, 2024 • 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

Choose a reason for hiding this comment

sylvestre commented Mar 19, 2024

sylvestre commented Mar 19, 2024

cre4ture commented Mar 22, 2024

sylvestre commented Mar 29, 2024

cre4ture commented Mar 30, 2024

cre4ture commented Jan 7, 2024 •

edited

Loading

cre4ture Mar 22, 2024 •

edited

Loading