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: support string args by "-S", "-vS" or "--split-strings" #5767

Closed
wants to merge 27 commits into from

Conversation

cre4ture
Copy link
Contributor

@cre4ture cre4ture commented Jan 1, 2024

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>

Therefor I was trying different existing rust implementations of sh/bash-like shells. The first one that was working for me to a large extent was the nsh: https://github.com/nuta/nsh
Its licenced as "MIT or CC0". So we can use it without licence issues.

Sadly, it was not yet perfectly suited for the env split-string feature. Therefor I had to do modifications. I pushed them to a feature branch on my fork: https://github.com/cre4ture/nsh/tree/feature/coreutils_env_split_string
Then I added a git submodule pointing to that feature branch.
EDIT: Apparently, the submodule has the issue that style checks and clippy tests are run on it. This causes red CI/CD builds.

I guess that these points needs to be discussed, agreed at and fixed before this pull request can be approved:

  • is nsh the right "library" for us to use?
    • bring changes into the main branch of nsh
    • decide on sticking with submodule
  • is the patching mechanism ok?

@sylvestre
Copy link
Contributor

Why do you need nsh ?
Seems that we don't need to add a dependency on a shell for this feature?

@cre4ture
Copy link
Contributor Author

cre4ture commented Jan 1, 2024

Why do you need nsh ? Seems that we don't need to add a dependency on a shell for this feature?

Let me help you getting the right picture.
Citation: "What shells do when splitting a command-line string into arguments is far from trivial, especially when you want to handle things like quoting." (Source: stack-overflow )
Because of this it is then recommend to use shell-words or shlex to avoid spending a lot of time on it.
The parser of shell-words has 500 lines of code. The shlexha 420. Both are missing the part that expands $VARIABLES.
And both implementations fail at many of the cases from the GNU test-suite, such they can't be used without adaptions. I had the impression that the nsh parser was doing the best in the test-suite. And therefor I started to adapt it from there.

I also see that its not ideal, as we need to do adaptions to nsh as well. My hope was, that these adaptions could make into the official main branch of nsh. But they might conflict with the goals of nsh.

If we consider that we anyway need to do adaptions, we could try to go for shell-words and a other library for variable expansion like shellexpand or subst. Fork them, and modify them as much as needed to fit for env-S.

But I think this will also be quite some work.
Please tell me what expectations you have regarding the implementation.

@sylvestre
Copy link
Contributor

yeah, sorry, it is no go for add a dependency to a shell for this kind of feature

Instead, you should contribute to nsh to move this feature into a new crate so that both us and nsh can use it.

Really sorry but we are not going to add a fork of shell as a new dep :(

Note: stackoverflow isn't a good source for me, sorry :(

@cre4ture cre4ture closed this Jan 7, 2024
@cre4ture
Copy link
Contributor Author

cre4ture commented Jan 7, 2024

will create new one

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.

2 participants