action #88452
closedscript_run syntax error when & is at the end of command
Description
script_run and assert_script_run cause a preventable bash syntax error, when there is a '&' at the end of the command (run in background).
A solution would be to remove the ';' for the serial handling when the command terminates with ';' or '&'.
This is a (low priority) feature request to do so, to make writing tests in openQA more user friendly.
Observation¶
Let's assume I want to do the following
assert_script_run('sleep 30 &')
This fails with a bash syntax error, because we add the return code handling at the end of the command in the form
; echo BLABLA-$?- > /dev/serial
Which results in a command like the following:
$ sleep 30 & ; echo 12345-$?- > /dev/serial
Since '&' and ';' are terminators for bash, between the two is an empty command, which is not allowed.
Solution¶
If there is a terminator at the end of a script_run or assert_script_run command, omit the ';' before the echo '12345-$?- > /dev/serial'
$ sleep 30 & ; echo 12345-$?- > /dev/serial # Syntax error
$ sleep 30 & echo 12345-$?- > /dev/serial # OK!
Current workarounds¶
I know of two possible workarounds, which are done in openQA tests:
First, enclose the command within brackets. This works but it creates a new context for the commands within the brackets, so e.g. 'jobs' or 'wait' will not work here.
assert_script_run('( sleep 30 & )');
assert_script_run('wait'); # doesn't see sleep 30 and will thus not wait for it
Second, add a true at the end of the script. This would work but most people use the first solution without being aware of the consequences. Indeed, this is the whole reason why I write this feature request :-)
assert_script_run('( sleep 30 & true');
assert_script_run('wait'); # will work
Updated by ph03nix almost 4 years ago
The main goal of this feature request is to make commands like the following possible
assert_script_run('wget ... &'); # Or whatever other command you want
assert_script_run('wget ... &'); # Or whatever other command you want
assert_script_run('wait');
Updated by ph03nix almost 4 years ago
Btw. in the case anyone wonders "why on Earth would anyone ever want to do that": We use this to install and update our virtualization guests in parallel:
# Install guests
assert_script_run("virt-install ... & true ") foreach (keys %virt_autotest::common::guests);
assert_script_run("wait", timeout => 3000);
# [...]
# Update guests
foreach my $guest (keys %virt_autotest::common::guests)
# [...]
assert_script_run("ssh root\@$guest 'zypper ref && zypper -n dup' </dev/null >$guest.txt 2>&1 & true");
}
assert_script_run("wait", timeout => 1200);
Updated by okurz almost 4 years ago
It should be possible to use
assert_script_run('(wget ... &)');
right?
Updated by ph03nix almost 4 years ago
okurz wrote:
It should be possible to use
assert_script_run('(wget ... &)');
right?
That will work, but you will not be able to wait for those jobs. This means, while this the job is scheduled in the background, a 'wait' will fail:
assert_script_run('(wget ... &)'); # will work
assert_script_run('(wget ... &)'); # will work
assert_script_run('(wget ... &)'); # will work
assert_script_run('wait'); # will NOT wait for the previous wget
Perhaps the following snippet is more useful to illustrate the problem:
( sleep 30 & ); jobs # will not list anything
( sleep 30 & ); wait # will not wait for sleep
sleep 30 & jobs # works
sleep 30 & wait # lists sleep
Updated by tinita almost 4 years ago
Another idea (instead of looking for &
or ;
in the command) could be to add a parameter to assert_script_run
, e.g. background => 1
.
Updated by okurz almost 4 years ago
Well, I am open for the proposal, I just wonder if that is really making things more clear to users.
Two other things that I think will work, please confirm if it does do what you want or not:
- Use
type_string
instead ofscript_run
- Instead of
sleep 1&
, callsleep 1&:
Updated by ph03nix almost 4 years ago
okurz wrote:
Well, I am open for the proposal, I just wonder if that is really making things more clear to users.
Two other things that I think will work, please confirm if it does do what you want or not:
- Use
type_string
instead ofscript_run
- Instead of
sleep 1&
, callsleep 1&:
Both should work, and I am sure that all openQA devs have figured a way to deal with background jobs already.
The feature request aims at a different purpose: To make the behaviour of assert_script_run and script_run be closer to the behaviour of bash, or more specifically, to be able to terminate a command with & without getting a syntax error. I think that could lower the bar a bit for fresh users/devs, because this is an issue, where everyone stumbles across at some point.
Updated by livdywan almost 4 years ago
ph03nix wrote:
Update guests¶
foreach my $guest (keys %virt_autotest::common::guests)
# [...]
assert_script_run("ssh root\@$guest 'zypper ref && zypper -n dup' $guest.txt 2>&1 & true");
}
assert_script_run("wait", timeout => 1200);
I feel like what you may actually want here is for assert_script_run
to return an ID that you can wait on.
Updated by okurz almost 4 years ago
- Status changed from New to Feedback
- Assignee set to okurz
- Target version changed from future to Ready
ph03nix wrote:
The feature request aims at a different purpose: To make the behaviour of assert_script_run and script_run be closer to the behaviour of bash, or more specifically, to be able to terminate a command with & without getting a syntax error. I think that could lower the bar a bit for fresh users/devs, because this is an issue, where everyone stumbles across at some point.
Absolutely. I prepared a quick PR myself now: https://github.com/os-autoinst/os-autoinst/pull/1619
Updated by livdywan almost 4 years ago
okurz wrote:
Absolutely. I prepared a quick PR myself now: https://github.com/os-autoinst/os-autoinst/pull/1619
The PR modifies the internal handling of ;
in ... so that this specific case can work:
assert_script_run 'sleep 30 &'
Discussion in the comments led to a counter-proposal for a new function enter_string
, and updating of existing uses to prevent people from habitually trying to stick to assert_script_run even when it's not the best option.
Updated by okurz almost 4 years ago
Yeah I am still torn. To me enter_string
sounds highly ambiguous and easy to confuse with type_string
.
Updated by okurz almost 4 years ago
- Due date set to 2021-03-15
Updated https://github.com/os-autoinst/os-autoinst/pull/1619 after the initial discussion
Updated by okurz almost 4 years ago
- Status changed from Feedback to In Progress
Updated by okurz almost 4 years ago
PR merged. Now we (or I) can try to come up with a corresponding change for os-autoinst-distri-opensuse itself to show how the new function can be used:
https://github.com/os-autoinst/os-autoinst-distri-opensuse/pull/12086
Updated by okurz over 3 years ago
- Due date changed from 2021-03-15 to 2021-03-23
No feedback within a week. I provided a reminder within https://chat.suse.de/channel/testing?msg=QgM8WLg4WMRMfeJSZ and will wait roughly one more week.
Updated by livdywan over 3 years ago
The PR introducing enter_cmd
was merged. os-autoinst#os-autoinst#1632 proposes background_script_run
which takes a command and returns the PID as well
Updated by livdywan over 3 years ago
- Status changed from Feedback to Resolved
Both PRs got merged - I assume this is done
Updated by okurz over 3 years ago
- Due date changed from 2021-03-23 to 2021-04-01
- Status changed from Resolved to Workable
Please don't do too much for what assignees are responsible for doing themselves. Actually the work is not yet done as there is still https://github.com/os-autoinst/os-autoinst-distri-opensuse/pull/12086 open. As that PR involves a lot of changes I wanted to have more feedback but have not received it so I waited a bit longer. I set the due date myself as reminder. I think I can follow up in the next days.
Updated by okurz over 3 years ago
- Due date changed from 2021-04-01 to 2021-04-15
- Status changed from Workable to Feedback
Updated https://github.com/os-autoinst/os-autoinst-distri-opensuse/pull/12086 . Awaiting feedback from test maintainers, pinged @os-autoinst/tests-maintainer for review in github.
Updated by okurz over 3 years ago
- Status changed from Feedback to Resolved
PR was merged. two follow-up fixes:
- https://github.com/os-autoinst/os-autoinst-distri-opensuse/pull/12299
- https://github.com/os-autoinst/os-autoinst-distri-opensuse/pull/12297
both merged.
In my eyes this concludes the story :)