Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged

Conversation

cre4ture
Copy link
Contributor

@cre4ture cre4ture commented Mar 30, 2024

as discussed with @sylvestre here: #5801

this makes the gnu-test tests/env/env.sh green again.

changes explained:

  • accept new parameter and provide it at program start
  • adapt debug logmessages to fit to gnu ones
  • introduce a second debug-level (-vv) that allowes more detailed, non-gnu conform logs.
  • add own test that uses --argv0 argument to coreutils executable to execute specific tools without placing seperate argument for tool-name
  • UPDATE: patch gnu test to seperately test "-a" and "--argv0"

@cre4ture cre4ture force-pushed the feature/env_argv0_overwrite_unix branch from 45e2574 to f3e2b2e Compare March 30, 2024 20:30
Copy link

GNU testsuite comparison:

Skipping an intermittent issue tests/tail/inotify-dir-recreate (passes in this run but fails in the 'main' branch)

@cre4ture cre4ture force-pushed the feature/env_argv0_overwrite_unix branch 2 times, most recently from fbb5f79 to dfb629b Compare March 30, 2024 22:53
Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/dd/stats is no longer failing!

@sylvestre
Copy link
Contributor

GNU testsuite comparison:

Congrats! The gnu test tests/dd/stats is no longer failing!

are you sure you fixed it ? :)

@cre4ture
Copy link
Contributor Author

are you sure you fixed it ? :)

sorry, I was apparently a bit quick there. I forgot to add the patch for the test.
And here I need your opinion.
The GNU-test is kind of crappy due to two reasons:

  1. it prepares the expected output but doesn't actually compare it with the real output.
  2. it tests both option-variants at the same time. It provides the short form -a and the long form --argv0 at the same time expecting that the last one provided precedes. uutils/clap by default raises an error if one is doing this, and I think this makes actually more sence. Thats why I provided the patch.

What do you think. Is it more important to replicate the GNU behaviour in this case?

@sylvestre
Copy link
Contributor

We could report bugs upstream to get this adjusted.

Copy link

GNU testsuite comparison:

Skip an intermittent issue tests/tail/inotify-dir-recreate (fails in this run but passes in the 'main' branch)

@cre4ture cre4ture force-pushed the feature/env_argv0_overwrite_unix branch from 364f98b to e8346fb Compare March 31, 2024 11:46
Copy link

GNU testsuite comparison:

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

@BenWiederhake
Copy link
Collaborator

I might be missing some context, but:

it tests both option-variants at the same time. It provides the short form -a and the long form --argv0 at the same time expecting that the last one provided precedes.

That's actually the case for the vast majority of options (but not all of them, sigh), and there's a huge chunk of programs where we still do it wrong: #5998

In this case, simply doing Arg::new("argv0").overrides_with("argv0") should do the trick.

@cre4ture
Copy link
Contributor Author

@BenWiederhake thanks for pointing this out.
As our reference is gnu, I will change it even though I do still think that our initial implementation (self-conflict) is more intuitive. Why would someone intentionally provide twice the same option?

@cre4ture cre4ture force-pushed the feature/env_argv0_overwrite_unix branch from e8346fb to 6608a0d Compare March 31, 2024 13:56
Copy link

GNU testsuite comparison:

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

@cre4ture cre4ture force-pushed the feature/env_argv0_overwrite_unix branch from 6608a0d to 0b5f098 Compare April 1, 2024 12:26
Copy link

github-actions bot commented Apr 1, 2024

GNU testsuite comparison:

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

@cre4ture cre4ture force-pushed the feature/env_argv0_overwrite_unix branch 2 times, most recently from f24d34f to e4216b3 Compare April 1, 2024 16:48
Copy link

github-actions bot commented Apr 1, 2024

GNU testsuite comparison:

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

@cre4ture cre4ture force-pushed the feature/env_argv0_overwrite_unix branch from b076824 to e4216b3 Compare April 1, 2024 17:45
Copy link

github-actions bot commented Apr 1, 2024

GNU testsuite comparison:

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

@sylvestre sylvestre force-pushed the feature/env_argv0_overwrite_unix branch from e4216b3 to 92919cb Compare April 1, 2024 18:41
Copy link

github-actions bot commented Apr 1, 2024

GNU testsuite comparison:

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

@cre4ture cre4ture force-pushed the feature/env_argv0_overwrite_unix branch from aee650a to e0a5878 Compare April 2, 2024 21:14
Copy link

github-actions bot commented Apr 2, 2024

GNU testsuite comparison:

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

Copy link
Collaborator

@BenWiederhake BenWiederhake left a comment

Choose a reason for hiding this comment

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

Looks mostly good, just a few nitpicks about testing.

tests/by-util/test_env.rs Outdated Show resolved Hide resolved
src/uu/env/src/env.rs Show resolved Hide resolved
Copy link
Collaborator

@BenWiederhake BenWiederhake left a comment

Choose a reason for hiding this comment

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

Fantastic! I didn't realize that argument overrides across -S already works.

I'm a bit uncertain how to add this to the repo; squashing it all seems like a bad idea (e.g. the changes to GNUmakefile should be in a different commit than the actual argv0 changes), but the commits are also not atomic, so I can't just rebase it. I'll discuss this with the others.

In the meantime, please consider doing git-rebase -i origin/main :)

Copy link

github-actions bot commented Apr 6, 2024

GNU testsuite comparison:

GNU test failed: tests/timeout/timeout. tests/timeout/timeout is passing on 'main'. Maybe you have to rebase?
Congrats! The gnu test tests/env/env is no longer failing!

@cre4ture cre4ture force-pushed the feature/env_argv0_overwrite_unix branch 2 times, most recently from 3463e1c to 1999902 Compare April 6, 2024 22:54
@cre4ture cre4ture force-pushed the feature/env_argv0_overwrite_unix branch from 1999902 to 3b8647c Compare April 6, 2024 22:56
Copy link

github-actions bot commented Apr 6, 2024

GNU testsuite comparison:

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

Copy link
Collaborator

@BenWiederhake BenWiederhake left a comment

Choose a reason for hiding this comment

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

Awesome work, and thanks for the rebase! :)

@BenWiederhake BenWiederhake merged commit abdeead into uutils:main Apr 6, 2024
64 checks passed
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.

3 participants