Add an alternative, enhanceable form of rpm.execute() with io redirect capabilities #3236
Conversation
The problem with the original rpm.execute() syntax is that it cannot be enhanced in any way, because all the arguments are treated as options to be passed to the command. Add an alternative form where the command and it's options are passed as a single Lua table, thus allowing other types of (optional) arguments to be added later.
|
This is basically a pre-requisite for #3192, I'm just wondering if this should be an entirely new function instead to make it possible to detect whether rpm supports it. But then, at least in Fedora there are very few users of rpm.execute() to begin with - exactly one. |
pmatilai
changed the title
|
The second commit adds basic stdout/stderr redirect capability. The API feels a little off though, comments welcome. Converted into a draft as this should be considered work-in-progress atm. |
The status returned by waitpid() is not a simple integer, one needs the various W* macros to examine it.
|
|
||
| Redirect standard error to `/dev/null`: | ||
| ``` | ||
| rpm.execute({'systemctl', 'restart', 'httpd'}, nil, '/dev/null') |
There was a problem hiding this comment.
I don't want to bikeshead this, but I would have expected it the other way round, like this:
rpm.execute({redirect_stderr='/dev/null'}, 'systemctl', 'restart', 'httpd')There was a problem hiding this comment.
I don't think that's bikeshedding at all when I said I wasn't too happy with the API I came up with and specifically asked for feedback 😄
Putting the redirects into a table, as the first argument, would indeed allow keeping the rest of the existing calling convention. I was probably so obsessed with the idea that optional arguments must come last that I just dismissed the whole possibility to do it this way. And, whether at the front or end, named entries in a table are nicer than positional optional arguments for the caller (just more work to implement). I'll have another look at this, the PR is currently a draft for more reasons than one.
So, thanks for the feedback!
There was a problem hiding this comment.
I posted this to the commit before seeing this conversation:
For me as the API consumer, the need to pass nil to stdout in order to change the stderr feel cumbersome.
It would feel more natural for me to call it like this:
rpm.execute({'systemctl', 'restart', 'httpd'}, {stderr='/dev/null'})That would even allow to extend the supported range of arguments indefinitely, as originally suggested in the issue.
I now see the conversation here which proposes a similar syntax, but perhaps reversed in order to allow passing positional arguments for the command itself.
May I suggest:
rpm.execute('systemctl', 'restart', 'httpd', {stderr='/dev/null'})...instead? Checking if the last argument is a table should be doable. It feels more natural to me to pass the "keyword arguments" after the "positional arguments" (quotes to indicate the separation is faked).
There was a problem hiding this comment.
Yup, I noticed myself on Friday that having to pass nil for just stderr control is ugly. So I think we all agree here that the redirect control needs to be a table, really, and that's also best for expandability.
I too instinctively place such optional arguments to the tail rather than front. It's possible to detect a table as the last thing in the variable argument list and behave differently, it just seems clunky.
I also dislike the existing rpm.execute() calling style, manipulating an array is much more convenient for the caller. So with all that, we basically get what @hroncok suggested above:
rpm.execute({'systemctl', 'restart', 'httpd'}, {stderr='/dev/null'})
But there's the fact that any change to rpm.execute() breaks it for older rpm versions, and the above no longer much resembles the original one either. So perhaps the sane thing to do is to just add a different function whose presence you can then actually test. Bikeshed time! Too bad rpm.execute() is taken 😆 rpm.spawn()?
Oh and, thanks again for the constructive feedback!
There was a problem hiding this comment.
But there's the fact that any change to rpm.execute() breaks it for older rpm versions...
Not necessarily. It can support the old (possibly deprecated) API and the new, based on the types of arguments.
There was a problem hiding this comment.
But if you don't like that, I consider spawn a good name for this function. It spawns a process, the name resembles posix_spwan but at the same time it isn't exactly posix_spwan, so I would not expect the same API as posix_spwan.
There was a problem hiding this comment.
What I mean by breakage is that an older rpm encountering the new variant will fail with a maybe not-so-obvious "not a string", whereas if it's a different function it's quite obvious that this is something new that is just not supported by that rpm. The bigger problem of course being that there's no good way (actually no way at all) to track the Lua API compatibility in packages.
But, I think rpm.spawn() it is then.
|
Closing this in favor of #3241. Thanks again for the suggestions! |
The problem with the original rpm.execute() syntax is that it cannot be enhanced in any way, because all the arguments are treated as options to be passed to the command.
Add an alternative form where the command and it's options are passed as a single Lua table, thus allowing other types of (optional) arguments to be added later.