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

Improve error message of commands consisting of multiple arguments #99

Merged
merged 1 commit into from
Sep 10, 2024

Conversation

agoscinski
Copy link
Contributor

@agoscinski agoscinski commented Jul 25, 2024

Commands like git diff, verdi status or python script.py will fail with different opaque error messages. We add a check that the command consists only out of one argument to give the user a clearer error message.

src/aiida_shell/launch.py Outdated Show resolved Hide resolved
tests/test_launch.py Outdated Show resolved Hide resolved
@sphuber
Copy link
Owner

sphuber commented Aug 28, 2024

Thanks @agoscinski . I guess this makes sense, but I am also wondering if there may be valid use cases where the command would contain a space. In principle it accepts a full path to the executable as well, and having a space in that path should be fine. And there are other checks that could be done, such as checking there are no new lines perhaps?

In #100 there is another discussion where validation that we are doing that is supposed to be helping is actually backfiring a bit. I was wondering if a better approach is simply to improve the error reporting instead of preemptively trying to validate. But I just checked and the behavior can be quite different based on the command, e.g.:

$ which git rtghget
/usr/bin/git
$ which git diff
/usr/bin/git
/usr/bin/diff

In the first case, the second argument just gets swallowed as which returns nothing since it cannot find the command. But in the second case, the command is expanded to two lines, since each argument happens to correspond to an existing command.

This leads me to what I think is the actual problem. The code is calling:

status, stdout, stderr = transport.exec_command_wait(f'which {command}')

i.e the command is not quoted. If we quote it then which "git diff" would return a non-zero exit code and report the problem. I think this is the proper solution as it would also still support the use case of an actual command containing a space in the path.

Thoughts?

@agoscinski
Copy link
Contributor Author

Yes that was the actual problem, I did not think about this solution. I think that works better.

@agoscinski agoscinski force-pushed the fix-multi-commands branch 2 times, most recently from 0948b83 to a02c524 Compare September 10, 2024 10:16
…message

Commands like `git diff`, `verdi status` or `python script.py` will fail
with different opaque error messages, because only the first argument of
the command is resolved. With this PR the whole command is resolved
improving the error messages.
Copy link
Owner

@sphuber sphuber left a comment

Choose a reason for hiding this comment

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

Thanks @agoscinski

@sphuber sphuber merged commit 104d03b into sphuber:master Sep 10, 2024
6 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.

2 participants