From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lucas Meneghel Rodrigues Subject: Re: [PATCH KVM-AUTOTEST 2/2] Convert images to JPEG using PIL instead of an external program Date: Wed, 12 Aug 2009 10:26:19 -0300 Message-ID: <1250083579.2921.36.camel@localhost.localdomain> References: <1250069685-17727-1-git-send-email-avi@redhat.com> <1250069685-17727-3-git-send-email-avi@redhat.com> <1250081076.2921.2.camel@localhost.localdomain> <4A82BCE6.9050703@redhat.com> Mime-Version: 1.0 Content-Type: text/plain Content-Transfer-Encoding: 7bit Cc: Lucas Meneghel Rodrigues , kvm@vger.kernel.org To: Avi Kivity Return-path: Received: from mx2.redhat.com ([66.187.237.31]:40021 "EHLO mx2.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752431AbZHLN0W (ORCPT ); Wed, 12 Aug 2009 09:26:22 -0400 Received: from int-mx2.corp.redhat.com (int-mx2.corp.redhat.com [172.16.27.26]) by mx2.redhat.com (8.13.8/8.13.8) with ESMTP id n7CDQNnQ006304 for ; Wed, 12 Aug 2009 09:26:23 -0400 In-Reply-To: <4A82BCE6.9050703@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Wed, 2009-08-12 at 16:00 +0300, Avi Kivity wrote: > On 08/12/2009 03:44 PM, Lucas Meneghel Rodrigues wrote: > >> @@ -110,9 +111,8 @@ def barrier_2(vm, words, params, debug_dir, data_scrdump_filename, > >> history_scrdump_filename = os.path.join(history_dir, > >> "scrdump-step_%s-%s.jpg" % (current_step_num, > >> time.strftime("%Y%m%d-%H%M%S"))) > >> - kvm_subprocess.run_fg("convert -quality 30 %s %s" % > >> - (scrdump_filename, history_scrdump_filename), > >> - logging.debug, "(convert) ", timeout=30) > >> + image = PIL.Image.open(scrdump_filename) > >> + image.save(history_scrdump_filename, format = 'JPEG', quality = 30) > >> > > Utilities to perform automatic guest installation using step files. > > > > Looks great, but since the python imaging library is an external > > library, we need to handle import failures. We can't guarantee that it > > will allways be installed, so we just degrade functionality gracefully > > in the case is not present. > Why not require it unconditionally? It's not new and installing it is > trivial. Just general autotest policy I try to follow. For client side testing, we try to make as few assumptions as possible about the machine where we are going to run the client, except to have a python install > 2.4 and a working toolchain (to build C/C++ programs). We try to not rely on anything but those 2 items. For the autotest server machine it's OK to depend on external libraries. I also agree that this is a special case: * PIL is almost ubiquitous across linux distributions * It comes installed by default in most setups * It's not like we are going to test kvm on dramatically old linux systems But I prefer to follow the project policy when possible. The reason why I accepted the original code that Michael wrote to perform the conversion was graceful degradation of functionality (if you don't have ImageMagick installed, the test will not abort). We could handle import failures in a way that supports graceful degradation and prints a warning message saying that the user don't have PIL installed, and that the conversion functionality only works when having PIL installed. What do you think? If you still think it's not worth the effort of doing this import handling I am going to think a little bit more, and eventually apply this.