Add rpm.spawn() Lua API #3241
Add rpm.spawn() Lua API #3241
Conversation
docs/manual/lua.md
Outdated
| | `stdout`| path | Redirect standard output to path | ||
| | `stderr`| path | Redirect standard error to path | ||
|
|
||
| Returns the command exit status. |
There was a problem hiding this comment.
I don't see this tested. (But the test framework is a bit hard to read for me.)
There was a problem hiding this comment.
I almost agreed 😅 but it is actually tested by this:
RPMTEST_CHECK([
runroot_other rpmlua -e "print(rpm.spawn({'cat'}, {stdin='aaa'}))"
],
[0],
[nil No such file or directory 2.0
],
[])
There was a problem hiding this comment.
What is missing is the detailed description of the return code... (shared by rpm.execute())
There was a problem hiding this comment.
I see:
> rpm.execute('true')
0,0
> rpm.execute('false')
nil "exit code" 256,0There was a problem hiding this comment.
A brief description of the return status added.
There was a problem hiding this comment.
Hmm, this is strange:
> print(rpm.execute('ls blah'))
nil No such file or directory 2.0
> print(rpm.spawn({'ls', 'blah'}))
ls: cannot access 'blah': No such file or directory
nil exit code 2
Shouldn't the second invocation's error message be the same as that of the first one?
There was a problem hiding this comment.
Interesting. The difference comes from executing posix_spawn_file_actions_init(), even if it's empty. And rpm.spawn() always initializes it to keep things simple, under the assumption that an empty set doesn't make any difference, right? 😄 Good spotting, thanks.
There was a problem hiding this comment.
Except ... nope 😄 For a moment there I thought I was losing my mind when I couldn't reproduce this in the test-suite, but those two are not equivalent at all. rpm.execute('ls blah') is trying to execute a binary ls blah with no arguments, and because there isn't such an executable on most systems, you get a "No such file or directory". Whereas the latter is succesfully executing ls and with a blah argument, and the "ls: cannot.." is from ls, like it says in the message. The spawn equivalent is print(rpm.spawn({'ls blah'}) and with that, you get the same result.
The rpm.spawn({'ls', 'blah'}) equivalent is rpm.execute('ls', 'blah') and that too has identical behavior.
There was a problem hiding this comment.
Oh, damn... Of course, rpm.execute() takes a list of arguments (much like similar spawn functions in e.g. Python), not a string. My bad 😅.
pmatilai
force-pushed
the
rpmspawn
branch
3 times, most recently
from
6e8d0ed
to
52938a9
Compare
|
Rebased to sort out the conflict. |
Turns out real-world usage needs more control than rpm.execute() delivers. This could be crammed into rpm.execute() but it'd be forward incompatible in a somewhat non-obvious way, we might just as well add a separate function for it. Supports redirecting stdin, stdout and stderr to a given path, support for file descriptors, other actions and spawn attributes can be added later. Fixes: rpm-software-management#3192
|
...and changed to only initialize file actions if actually there, it's not that much more code and somehow seems more right anyhow. |
|
LGTM, just one question: Why did the old |
|
Good spotting again. It's a bug I noticed while looking at this. In some early version I had it fixed, but then it got in the way and then I planned on merging the rpm.execute() implementation with this one so it'd be taken care of, but that had other complications, so ... I'll add that as a separate commit to this PR 😄 |
The status returned from waitpid() is not your exit code, it's encoded and needs processing the W*() macros, like we do in the new rpm.spawn() function. These two should really be refactored to use common code, but leaving that to later because one is in C and the other one in C++...
|
Added one more commit to fix the rpm.execute() return code handling too. It makes all these "these should be refactored to use common code" alarms in my head go wild but we don't want to do that just now, because one is in C and the other in C++ and it all just gets messy. So, we'll refactor in git master, after the dust settles a bit. |
| } else { | ||
| return pusherror(L, WEXITSTATUS(status), "exit code"); | ||
| } | ||
| } | ||
|
|
||
| return pushresult(L, status); |
There was a problem hiding this comment.
Oops, seems like this one should also be fixed to use WEXITSTATUS(status) instead 😄
There was a problem hiding this comment.
That will only be reached when status is zero, so it doesn't make any difference.
There was a problem hiding this comment.
We could of course make the zero explicit to make it clearer.
There was a problem hiding this comment.
Oh right, of course... nevermind then. We'll be refactoring this code anyway.
Turns out real-world usage needs more control than rpm.execute() delivers. This could be crammed into rpm.execute() but it'd be forward incompatible in a somewhat non-obvious way, we might just as well add a separate function for it.
Supports redirecting stdin, stdout and stderr to a given path, support for file descriptors, other actions and spawn attributes can be added later.
Fixes: #3192