Use Verify to check required commands are present #283
Conversation
eds-collabora
force-pushed
the
eds/verify
branch
from
1ba357e to
0f36f38
Compare
| } | ||
|
|
||
| func (cmd Command) CheckExists(label string, cmdline ...string) error { | ||
| w := newCommandWrapper(label, false) |
There was a problem hiding this comment.
is it worth being simple and just using exec.LookPath to check the executable exists somewhere rather than running the help commadn and checking the output ?
There was a problem hiding this comment.
If you'd prefer, I can make that change. Actually running the command seemed safest and simplest to me; it catches things like broken installs, missing libraries, and so forth. Just the presence of an executable doesn't mean you can use it.
There was a problem hiding this comment.
Fwiw i'm fine either way; I'm not sure trying to run things add much given well, so many things could potentially be broken. What if the tools are slightly different versions? what if a tool isn't bulid with the right options? etc etc. So i think going beyond, does it exist in the path has minimal extra value..
That said as things run in fakemachine that may potentially break programs, but that case seems both extremely rare and questionable if you'd detect that with just running --help.
But again either way works for me as i don't think running help on a bunch of tools will have a negative impact either ;)
Output is suppressed for readability, unless the check fails.
Verify (which should be called Prepare) must run before the fakemachine is created, in order to setup the arguments for the fakemachine itself. It then has to run again in the fakemachine in order to rebuild any necessary internal state. Command checking can only occur in the same environment as run, as commands that are in the path when running may not be available to the regular user (e.g. Debian installs debootstrap in sbin).
sjoerdsimons
force-pushed
the
eds/verify
branch
from
0f36f38 to
9527ef3
Compare
|
|
||
| type commandWrapper struct { | ||
| label string | ||
| writeBeforeFlush bool |
There was a problem hiding this comment.
can we name this in a somewhat more meaningfull way? I had to go over this a few times before i understood what this name meant ;)
| } | ||
|
|
||
| func (cmd Command) CheckExists(label string, cmdline ...string) error { | ||
| w := newCommandWrapper(label, false) |
There was a problem hiding this comment.
Fwiw i'm fine either way; I'm not sure trying to run things add much given well, so many things could potentially be broken. What if the tools are slightly different versions? what if a tool isn't bulid with the right options? etc etc. So i think going beyond, does it exist in the path has minimal extra value..
That said as things run in fakemachine that may potentially break programs, but that case seems both extremely rare and questionable if you'd detect that with just running --help.
But again either way works for me as i don't think running help on a bunch of tools will have a negative impact either ;)
|
@daissi assigning to you to ideally take over & rework. |
|
continued in #598 ; closing this stale PR :-) |
github-project-automation
bot
moved this from Stalled
to Done
in Debos&Fakemachine development
4 participants
For each action, check that as many commands it needs are available as possible. This can't be meaningfully extended to commands that run in the chroot, or might run in the chroot.
This follows up from comments on #276 which were captured as issue #280.
Closes: #280