[2/5] dts: skip first line of send_command output
Checks
Commit Message
The first line of the InteractiveShell send_command method is generally
the command input field. This sometimes is unwanted, therefore this
commit enables the possibility of omitting the first line from the
returned output.
Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
Reviewed-by: Jack Bond-Preston <jack.bond-preston@arm.com>
---
dts/framework/remote_session/interactive_shell.py | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
Comments
On Fri, Apr 12, 2024 at 1:11 PM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
>
> The first line of the InteractiveShell send_command method is generally
> the command input field. This sometimes is unwanted, therefore this
> commit enables the possibility of omitting the first line from the
> returned output.
>
Oh, the first commit message was confusing. It said leading prompt
which I understood to be the first prompt (the one with the command).
I see that this commit actually addresses what I thought the first
commit was trying to do.
> Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
> Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
> Reviewed-by: Jack Bond-Preston <jack.bond-preston@arm.com>
> ---
> dts/framework/remote_session/interactive_shell.py | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py
> index 8a9bf96ea9..e290a083e9 100644
> --- a/dts/framework/remote_session/interactive_shell.py
> +++ b/dts/framework/remote_session/interactive_shell.py
> @@ -105,7 +105,9 @@ def _start_application(self, get_privileged_command: Callable[[str], str] | None
> start_command = get_privileged_command(start_command)
> self.send_command(start_command)
>
> - def send_command(self, command: str, prompt: str | None = None) -> str:
> + def send_command(
> + self, command: str, prompt: str | None = None, skip_first_line: bool = False
Do we generally want or don't want to include the first line? When do
we absolutely not want to include it?
> + ) -> str:
> """Send `command` and get all output before the expected ending string.
>
> Lines that expect input are not included in the stdout buffer, so they cannot
> @@ -121,6 +123,7 @@ def send_command(self, command: str, prompt: str | None = None) -> str:
> command: The command to send.
> prompt: After sending the command, `send_command` will be expecting this string.
> If :data:`None`, will use the class's default prompt.
> + skip_first_line: Skip the first line when capturing the output.
>
> Returns:
> All output in the buffer before expected string.
> @@ -132,6 +135,9 @@ def send_command(self, command: str, prompt: str | None = None) -> str:
> self._stdin.flush()
> out: str = ""
> for line in self._stdout:
> + if skip_first_line:
> + skip_first_line = False
> + continue
Is there ever a reason to distinguish between the first line and the
line with the command on it?
> if prompt in line and not line.rstrip().endswith(
> command.rstrip()
> ): # ignore line that sent command
> --
> 2.34.1
>
On 16/04/2024 09:48, Juraj Linkeš wrote:
> Oh, the first commit message was confusing. It said leading prompt
> which I understood to be the first prompt (the one with the command).
> I see that this commit actually addresses what I thought the first
> commit was trying to do.
Yes, my bad!
>> - def send_command(self, command: str, prompt: str | None = None) -> str:
>> + def send_command(
>> + self, command: str, prompt: str | None = None, skip_first_line: bool = False
>
> Do we generally want or don't want to include the first line? When do
> we absolutely not want to include it?
In the case of `show port info/stats {x}` if the provided port is
invalid, then the first message starts with `Invalid port`. By providing
an output that skips the command prompt, this is easily checked with
output.startswith("Invalid port") as you may have noticed in the next
commit. Otherwise it'd be a bit more complicated. Personally, I am not
sure whether we care about the first line. With my limited knowledge I
don't see a reason to include it (just as much as the trailing prompt).
>> + ) -> str:
>> """Send `command` and get all output before the expected ending string.
>>
>> Lines that expect input are not included in the stdout buffer, so they cannot
>> @@ -121,6 +123,7 @@ def send_command(self, command: str, prompt: str | None = None) -> str:
>> command: The command to send.
>> prompt: After sending the command, `send_command` will be expecting this string.
>> If :data:`None`, will use the class's default prompt.
>> + skip_first_line: Skip the first line when capturing the output.
>>
>> Returns:
>> All output in the buffer before expected string.
>> @@ -132,6 +135,9 @@ def send_command(self, command: str, prompt: str | None = None) -> str:
>> self._stdin.flush()
>> out: str = ""
>> for line in self._stdout:
>> + if skip_first_line:
>> + skip_first_line = False
>> + continue
>
> Is there ever a reason to distinguish between the first line and the
> line with the command on it?
As above, not really sure. Would this always be a command prompt? The
doubt arises only because I don't understand why we'd need the command
prompt fed back.
>
>> if prompt in line and not line.rstrip().endswith(
>> command.rstrip()
>> ): # ignore line that sent command
>> --
>> 2.34.1
>>
On Tue, Apr 16, 2024 at 2:15 PM Luca Vizzarro <Luca.Vizzarro@arm.com> wrote:
>
> On 16/04/2024 09:48, Juraj Linkeš wrote:
> > Oh, the first commit message was confusing. It said leading prompt
> > which I understood to be the first prompt (the one with the command).
> > I see that this commit actually addresses what I thought the first
> > commit was trying to do.
>
> Yes, my bad!
>
> >> - def send_command(self, command: str, prompt: str | None = None) -> str:
> >> + def send_command(
> >> + self, command: str, prompt: str | None = None, skip_first_line: bool = False
> >
> > Do we generally want or don't want to include the first line? When do
> > we absolutely not want to include it?
>
> In the case of `show port info/stats {x}` if the provided port is
> invalid, then the first message starts with `Invalid port`. By providing
> an output that skips the command prompt, this is easily checked with
> output.startswith("Invalid port") as you may have noticed in the next
> commit. Otherwise it'd be a bit more complicated. Personally, I am not
> sure whether we care about the first line. With my limited knowledge I
> don't see a reason to include it (just as much as the trailing prompt).
>
> >> + ) -> str:
> >> """Send `command` and get all output before the expected ending string.
> >>
> >> Lines that expect input are not included in the stdout buffer, so they cannot
> >> @@ -121,6 +123,7 @@ def send_command(self, command: str, prompt: str | None = None) -> str:
> >> command: The command to send.
> >> prompt: After sending the command, `send_command` will be expecting this string.
> >> If :data:`None`, will use the class's default prompt.
> >> + skip_first_line: Skip the first line when capturing the output.
> >>
> >> Returns:
> >> All output in the buffer before expected string.
> >> @@ -132,6 +135,9 @@ def send_command(self, command: str, prompt: str | None = None) -> str:
> >> self._stdin.flush()
> >> out: str = ""
> >> for line in self._stdout:
> >> + if skip_first_line:
> >> + skip_first_line = False
> >> + continue
> >
> > Is there ever a reason to distinguish between the first line and the
> > line with the command on it?
>
> As above, not really sure. Would this always be a command prompt? The
> doubt arises only because I don't understand why we'd need the command
> prompt fed back.
>
The only thing I could think of is debugging. Maybe it could offer
some extra insight in some corner cases.
> >
> >> if prompt in line and not line.rstrip().endswith(
> >> command.rstrip()
> >> ): # ignore line that sent command
> >> --
> >> 2.34.1
> >>
>
Apologies for the complications that this interactive shell provides
here. These problems didn't arise previously primarily because the
interactive shells were designed to receive commands, give you the raw
output, and then the developer extract specifically what they want
from the output and ignore the things they don't. I understand however
that in your case it might be beneficial to just consume everything.
Some of these changes seem universally good and overall not harmful to
include (like removing the trailing prompt, I can't really see a use
for it) but some others come with caveats, so if it is too complicated
this might be better to handle as something testpmd specific, and
leave the generic interactive shell to always just give you raw
output.
On Wed, Apr 17, 2024 at 9:18 AM Juraj Linkeš <juraj.linkes@pantheon.tech> wrote:
<snip>
> > >> @@ -132,6 +135,9 @@ def send_command(self, command: str, prompt: str | None = None) -> str:
> > >> self._stdin.flush()
> > >> out: str = ""
> > >> for line in self._stdout:
> > >> + if skip_first_line:
> > >> + skip_first_line = False
> > >> + continue
> > >
> > > Is there ever a reason to distinguish between the first line and the
> > > line with the command on it?
> >
> > As above, not really sure. Would this always be a command prompt? The
Whether this first line is always the command prompt or not is
specific to the shell unfortunately. In "bash-like" shells where
commands you send are echoed into stdout for easy-of-use for the
developer (like testpmd), this first line will always be the command
you sent to it. It technically isn't always required for this to
happen however, so we could make this assumption, but it could be
slightly more limiting down the line.
> > doubt arises only because I don't understand why we'd need the command
> > prompt fed back.
> >
>
> The only thing I could think of is debugging. Maybe it could offer
> some extra insight in some corner cases.
I agree that it is useful for debugging, but we do also log it
separately before sending the command. I don't think the command could
change by simply being sent into the shell unless something strange
happens like the shell breaks it across multiple lines. I think it
would be fine to exclude it, but as mentioned, it isn't always safe to
do so.
>
> > >
> > >> if prompt in line and not line.rstrip().endswith(
> > >> command.rstrip()
> > >> ): # ignore line that sent command
> > >> --
> > >> 2.34.1
> > >>
> >
@@ -105,7 +105,9 @@ def _start_application(self, get_privileged_command: Callable[[str], str] | None
start_command = get_privileged_command(start_command)
self.send_command(start_command)
- def send_command(self, command: str, prompt: str | None = None) -> str:
+ def send_command(
+ self, command: str, prompt: str | None = None, skip_first_line: bool = False
+ ) -> str:
"""Send `command` and get all output before the expected ending string.
Lines that expect input are not included in the stdout buffer, so they cannot
@@ -121,6 +123,7 @@ def send_command(self, command: str, prompt: str | None = None) -> str:
command: The command to send.
prompt: After sending the command, `send_command` will be expecting this string.
If :data:`None`, will use the class's default prompt.
+ skip_first_line: Skip the first line when capturing the output.
Returns:
All output in the buffer before expected string.
@@ -132,6 +135,9 @@ def send_command(self, command: str, prompt: str | None = None) -> str:
self._stdin.flush()
out: str = ""
for line in self._stdout:
+ if skip_first_line:
+ skip_first_line = False
+ continue
if prompt in line and not line.rstrip().endswith(
command.rstrip()
): # ignore line that sent command