Re: [KVM-AUTOTEST PATCH 1/2] KVM test: optionally convert PPM files to PNG format after test

[Lucas Meneghel Rodrigues <lmr@redhat.com>]

On Thu, 2009-06-18 at 06:59 -0400, Michael Goldish wrote:

> The call should never fail because we currently don't raise exceptions.
> It's actually easier to skip the check, because in case ImageMagick
> isn't installed, you'll just get a nice message like:
> 
> 'convert_ppm_files_to_png' specified; converting PPM files to PNG format...
> (mogrify) bash: mogrify: command not found
> (mogrify) (Process terminated with status 127)
> 
> Personally I am fond of these messages because they indicate to me that
> kvm_subprocess is functioning properly, and I also think they're quite
> readable because they contain "bash's own words", but since we might
> raise exceptions in run_bg()/run_fg() in the future, I see your point.

Yes, that was the point. I am not particularly against leaving it as it
is, since we still haven't decided about the behavior of these utility
functions.

> I suppose I should do something like:
> 
> logging.debug("converting ...")
> found = True
> try:
>   os_dep.command("mogrify")
>   found = True
> except:
>   logging.error("'mogrify' not found; ImageMagick is probably not installed")
>   found = False
> if found:
>   mogrify_cmd = ...
>   kvm_subprocess.run_fg(...)

Yes, something along these lines

> Do you see a shorter/more elegant syntax to do this?

Nope, since os_dep() already trhows exceptions by default

> (We can put the conversion code inside the 'try' clause to avoid using
> that 'found' variable, but it's generally recommended to make the code
> inside a 'try' clause as specific as possible.)

Yes, because if something else unexpected happens inside the try clause
we will capture the exception and tell the user generically that the
problem is ImageMagick not installed. So I am for keeping the conversion
out of the try clause.

> In any case, I don't want to fail the test just because mogrify is missing,
> so we do need to catch the exception raised by os_dep.command().

Yes, sounds good to me. I am leaning towards implementing this check
instead of leaving the patch on its original form, so let me know what
is your decision.

-- 
Lucas Meneghel Rodrigues
Software Engineer (QE)
Red Hat - Emerging Technologies