assert_script_run needlessly(?) re-implements script_run

[AdamWill]

Maybe there's something I'm missing here, but why do assert_script_run and distribution::script_run have more or less identical code for running the command, directing the exit code plus a kinda 'salt' to the serial output, and then deciding if the script passed or failed?

distribution::script_run:

if ($wait > 0) {
    my $str = testapi::hashed_string("SR$cmd$wait");
    if (testapi::is_serial_terminal) {
        testapi::type_string " ; echo $str-\$?-\n";
    }
    else {
        testapi::type_string " ; echo $str-\$?- > /dev/$testapi::serialdev\n";
    }
    my $res = testapi::wait_serial(qr/$str-\d+-/, $wait);
    return unless $res;
    return ($res =~ /$str-(\d+)-/)[0];

assert_script_run:

my $str = hashed_string("ASR$cmd");
# call script_run with idle_timeout 0 so we don't wait twice
if (is_serial_terminal) {
    script_run("$cmd; echo $str-\$?-", 0);
}
else {
    script_run("$cmd; echo $str-\$?- > /dev/$serialdev", 0);
}
my $ret = wait_serial("$str-\\d+-", $args{timeout});
my $die_msg = "command '$cmd' failed";
$die_msg .= ": $args{fail_message}" if $args{fail_message};
croak $die_msg unless (defined $ret && $ret =~ /$str-0-/);

in fact, assert_script_run has to go out of its way to call script_run with a 0 timeout to avoid hitting the code that does more or less the same thing it's about to do itself!

Why doesn't assert_script_run just call script_run with whatever timeout it was given, take the exit code that script_run returns, and then decide whether to return or die based on that? Am I missing something?

[coolo]

this is historic - as script_run did nothing of this at the beginning.

[AdamWill]

sorry, this was meant to be on os-autoinst, dunno how it wound up here. so you'd accept a PR to remove the duplication?

[okurz]

sounds reasonable

[AdamWill]

rgr, will do it if I get a minute. the documentation of script_run could also explain that it returns the exit code, then I wouldn't keep forgetting. :P

[coolo]

thanks for reading our documentation every evening :)

[AdamWill]

also while I'm asking 'why'...why is assert_script_run entirely implemented at the testapi level but testapi::script_run is just a thin wrapper around distribution::script_run...which then has to read stuff from testapi? while I'm fiddling, should I just move script_run fully up into testapi, or no?

[coolo]

well, last time I touched this everyone was protesting that testapi should not contain linux specific stuff like serial handling and that windows distributions would have a different way to run scripts.

I don't really care for that use cases - and SUSE no longer has its own script_run in our distribution (from what grep says), so I would opt for easy design.

[AdamWill]

well, distribution::script_run uses is_serial_terminal and serialdev and wait_serial from testapi anyway, so even though the function is in distribution it's getting all the serial bits from testapi already.

[coolo]

yes, but distribution you can overload in your test distri - testapi not. So either we have everything with serial to distribution or we just drop the idea of being portable.

[okurz]

yes, I guess the architecture should be improved and we should make sure layers are not mixed. I am for keeping the thing portable and improve on that. Also, I tried the idea to extract the "GNU/Linux" specific parts but failed so far on circular dependencies. I guess moving more into the "distribution" for that matter would help there.

[AdamWill]

Yeah, I do see. I'll keep the split. So in theory, if assert_script_run still had platform-specific bits in it we'd want to move it to distribution (rather than moving script_run to testapi)...but I think an assert_script_run that's just a thin wrapper around script_run would be portable anyway.

[okurz]

yes, true.

also see os-autoinst/os-autoinst#690 for some related doc updates

[coolo]

I don't think there is a problem with making assert_script_run a wrapper and avoid more unportable bits in testapi

[AdamWill]

So os-autoinst/os-autoinst#694 does that. I had a more ambitious version that also harmonized script_sudo and assert_script_sudo, but I bailed on that when I realized it's a bit problematic: in particular, I don't think testapi::assert_script_sudo is very likely to work with the script_sudo implementation in distribution.pm, because it'll only run the actual command through sudo, the echo $str-\$?- > /dev/$serialdev part will run with user privileges, which (at least on Fedora) is not allowed, regular users can't write to the serial devices. So I kinda didn't want to fiddle with that stuff if I couldn't make it work. I may send a follow-up based on the idea of changing the default script_sudo to work like the one from susedistribution.pm (which is more of a script_su-c and ensures the echo is done as root as well as the command).

[coolo]

this looks resolved