Merge lp:~milo/linaro-image-tools/method-refactor into lp:linaro-image-tools/11.11

Proposed by Milo Casagrande
Status: Merged
Merged at revision: 579
Proposed branch: lp:~milo/linaro-image-tools/method-refactor
Merge into: lp:linaro-image-tools/11.11
Diff against target: 283 lines (+154/-113)
1 file modified
linaro_image_tools/hwpack/builder.py (+154/-113)
To merge this branch: bzr merge lp:~milo/linaro-image-tools/method-refactor
Reviewer Review Type Date Requested Status
James Tunnicliffe (community) Approve
Paul Sokolovsky Pending
Linaro Infrastructure Pending
Review via email: mp+130123@code.launchpad.net

Description of the change

Branch with some code refactoring for the build() method in builder.py.
The method was starting to be very big and dificult to read.

To post a comment you must log in.
Revision history for this message
James Tunnicliffe (dooferlad) wrote :

Thanks for tidying this up, but since it is a tidy-up branch, I am
going to be extra picky :-)

154 + def _old_format_extract_files(self):
155 + """
156 + Extract files for hwpack versions < 3.0.
157 + """

I think you have space to put the docstring on one line.
http://www.python.org/dev/peps/pep-0257/ likes docstrings that start
on the same line as the opening quote and is OK with the closing quote
on the same line.

There are several new functions without docstrings. Since this is a
re-factor, should probably include them.

161 + bootloader_package = \
162 + self.find_fetched_package(self.packages,
163 + self.config.bootloader_package)

I personally find the use of \ to continue a line ugly and think the
following form is easier on the eyes:

            bootloader_package = self.find_fetched_package(
                                        self.packages,
                                        self.config.bootloader_package)

Just personal preference though. There are several instances of this
type of line continuation that I think are only there because it was
how the old code was formatted.

Fix those two and it is fine (I assume you have done some test builds
and the unit tests pass).

579. By Milo Casagrande

Fixed docstring and code indent.

580. By Milo Casagrande

Added docstrings.

Revision history for this message
Milo Casagrande (milo) wrote :

Hey James!

Thanks for the review.

On Wed, Oct 17, 2012 at 5:42 PM, James Tunnicliffe
<email address hidden> wrote:
> Thanks for tidying this up, but since it is a tidy-up branch, I am
> going to be extra picky :-)
>
> 154 + def _old_format_extract_files(self):
> 155 + """
> 156 + Extract files for hwpack versions < 3.0.
> 157 + """
>
> I think you have space to put the docstring on one line.
> http://www.python.org/dev/peps/pep-0257/ likes docstrings that start
> on the same line as the opening quote and is OK with the closing quote
> on the same line.
>
> There are several new functions without docstrings. Since this is a
> re-factor, should probably include them.

Fixed them both, at least for the newly introduced functions.

>
> 161 + bootloader_package = \
> 162 + self.find_fetched_package(self.packages,
> 163 + self.config.bootloader_package)
>
> I personally find the use of \ to continue a line ugly and think the
> following form is easier on the eyes:
>
> bootloader_package = self.find_fetched_package(
> self.packages,
> self.config.bootloader_package)
>
> Just personal preference though. There are several instances of this
> type of line continuation that I think are only there because it was
> how the old code was formatted.

I do not like using \ either, removed and used a better formatting.
Thanks again!

--
Milo Casagrande
Infrastructure Engineer
Linaro.org <www.linaro.org> │ Open source software for ARM SoCs

Revision history for this message
Milo Casagrande (milo) wrote :

Forgot about this last:

On Wed, Oct 17, 2012 at 5:42 PM, James Tunnicliffe
<email address hidden> wrote:
> Fix those two and it is fine (I assume you have done some test builds
> and the unit tests pass).

Yes, tests pass (apart the one from bug 1067786).
I created also two hwpacks with the new code, and compared with two
done using old code: they seems to work to the point where l-m-c can
create an image.

--
Milo Casagrande
Infrastructure Engineer
Linaro.org <www.linaro.org> │ Open source software for ARM SoCs

Revision history for this message
James Tunnicliffe (dooferlad) wrote :

Great, thanks!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'linaro_image_tools/hwpack/builder.py'
--- linaro_image_tools/hwpack/builder.py 2012-10-12 12:31:22 +0000
+++ linaro_image_tools/hwpack/builder.py 2012-10-17 16:06:24 +0000
@@ -270,125 +270,166 @@
270 self.packages,270 self.packages,
271 download_content=self.config.include_debs)271 download_content=self.config.include_debs)
272272
273 # On a v3 hwpack, all the values we need to check are273 if self.format.format_as_string == '3.0':
274 # in the bootloaders and boards section, so we loop
275 # through both of them changing what is necessary.
276
277 if self.config.format.format_as_string == '3.0':
278 self.extract_files()274 self.extract_files()
279 else:275 else:
280 bootloader_package = None276 self._old_format_extract_files()
281 if self.config.bootloader_file is not None:277
282 assert(self.config.bootloader_package278 self._add_packages_to_hwpack(local_packages)
283 is not None)279
284 bootloader_package = self.find_fetched_package(
285 self.packages,
286 self.config.bootloader_package)
287 self.hwpack.metadata.u_boot = \
288 self.add_file_to_hwpack(
289 bootloader_package,
290 self.config.bootloader_file,
291 self.hwpack.U_BOOT_DIR)
292
293 spl_package = None
294 if self.config.spl_file is not None:
295 assert self.config.spl_package is not None
296 spl_package = self.find_fetched_package(
297 self.packages,
298 self.config.spl_package)
299 self.hwpack.metadata.spl = \
300 self.add_file_to_hwpack(
301 spl_package,
302 self.config.spl_file,
303 self.hwpack.SPL_DIR)
304
305 # bootloader_package and spl_package can be
306 # identical
307 if (bootloader_package is not None and
308 bootloader_package in self.packages):
309 self.packages.remove(bootloader_package)
310 if (spl_package is not None and
311 spl_package in self.packages):
312 self.packages.remove(spl_package)
313
314 logger.debug("Adding packages to hwpack")
315 self.hwpack.add_packages(self.packages)
316 for local_package in local_packages:
317 if local_package not in self.packages:
318 logger.warning(
319 "Local package '%s' not included",
320 local_package.name)
321 self.hwpack.add_dependency_package(
322 self.config.packages)
323 out_name = self.out_name280 out_name = self.out_name
324 if not out_name:281 if not out_name:
325 out_name = self.hwpack.filename()282 out_name = self.hwpack.filename()
326 with open(out_name, 'w') as f:283
327 self.hwpack.to_file(f)
328 logger.info("Wrote %s" % out_name)
329 manifest_name = os.path.splitext(out_name)[0]284 manifest_name = os.path.splitext(out_name)[0]
330 if manifest_name.endswith('.tar'):285 if manifest_name.endswith('.tar'):
331 manifest_name = os.path.splitext(manifest_name)[0]286 manifest_name = os.path.splitext(manifest_name)[0]
332 manifest_name += '.manifest.txt'287 manifest_name += '.manifest.txt'
333 with open(manifest_name, 'w') as f:288
334 f.write(self.hwpack.manifest_text())289 self._write_hwpack_and_manifest(out_name,
335290 manifest_name)
336 logger.debug("Extracting build-info")291
337 build_info_dir = os.path.join(fetcher.cache.tempdir,292 cache_dir = fetcher.cache.tempdir
338 'build-info')293 self._extract_build_info(cache_dir, out_name,
339 build_info_available = 0294 manifest_name)
340 for deb_pkg in self.packages:295
341 deb_pkg_file_path = deb_pkg.filepath296 def _write_hwpack_and_manifest(self, out_name, manifest_name):
342 if os.path.islink(deb_pkg_file_path):297 """Write the real hwpack file and its manifest file.
343 # Skip symlink-ed debian package file298
344 # e.g. fetched package with dummy information299 :param out_name: The name of the file to write.
345 continue300 :type out_name: str
346 try:301 :param manifest_name: The name of the manifest file.
347 # Extract Build-Info attribute from debian302 :type manifest_name: str
348 # control303 """
349 deb_control = \304 logger.debug("Writing hwpack file")
350 DebFile(deb_pkg_file_path).control.debcontrol()305 with open(out_name, 'w') as f:
351 build_info = deb_control.get('Build-Info')306 self.hwpack.to_file(f)
352 except ArError:307 logger.info("Wrote %s" % out_name)
353 # Skip invalid debian package file308
354 # e.g. fetched package with dummy information309 logger.debug("Writing manifest file content")
355 continue310 with open(manifest_name, 'w') as f:
356 if build_info is not None:311 f.write(self.hwpack.manifest_text())
357 build_info_available += 1312
358 # Extract debian packages with build313 def _old_format_extract_files(self):
359 # information314 """Extract files for hwpack versions < 3.0."""
360 env = os.environ315 bootloader_package = None
361 env['LC_ALL'] = 'C'316 if self.config.bootloader_file is not None:
362 env['NO_PKG_MANGLE'] = '1'317 assert(self.config.bootloader_package is not None)
363 proc = cmd_runner.Popen(['dpkg-deb', '-x',318 bootloader_package = self.find_fetched_package(
364 deb_pkg_file_path, build_info_dir],319 self.packages,
365 env=env, stdout=subprocess.PIPE,320 self.config.bootloader_package)
366 stderr=subprocess.PIPE)321 self.hwpack.metadata.u_boot = self.add_file_to_hwpack(
367 (stdoutdata, stderrdata) = proc.communicate()322 bootloader_package,
368 if proc.returncode:323 self.config.bootloader_file,
369 raise ValueError('dpkg-deb extract failed!'324 self.hwpack.U_BOOT_DIR)
370 '\n%s' % stderrdata)325
371 if stderrdata:326 spl_package = None
372 raise ValueError('dpkg-deb extract had '327 if self.config.spl_file is not None:
373 'warnings:\n%s' % stderrdata)328 assert self.config.spl_package is not None
374329 spl_package = self.find_fetched_package(self.packages,
375 # Concatenate BUILD-INFO.txt files330 self.config.spl_package)
376 dst_file = open('BUILD-INFO.txt', 'wb')331 self.hwpack.metadata.spl = self.add_file_to_hwpack(
377 if build_info_available > 0:332 spl_package,
378 build_info_path = \333 self.config.spl_file,
379 r'%s/usr/share/doc/*/BUILD-INFO.txt' % \334 self.hwpack.SPL_DIR)
380 build_info_dir335
381 for src_file in iglob(build_info_path):336 # bootloader_package and spl_package can be identical
382 with open(src_file, 'rb') as f:337 if (bootloader_package is not None and
383 dst_file.write('\nFiles-Pattern: %s\n' % \338 bootloader_package in self.packages):
384 out_name)339 self.packages.remove(bootloader_package)
385 shutil.copyfileobj(f, dst_file)340 if (spl_package is not None and spl_package in self.packages):
386 dst_file.write('\nFiles-Pattern: %s\n'341 self.packages.remove(spl_package)
387 'License-Type: open\n' %\342
388 manifest_name)343 def _add_packages_to_hwpack(self, local_packages):
389 else:344 """Adds the packages to the hwpack.
390 dst_file.write('Format-Version: 0.1\n'345
391 'Files-Pattern: %s, %s\n'346 :param local_packages: The packages to add.
392 'License-Type: open\n' % \347 :type local_packages: list
393 (out_name, manifest_name))348 """
394 dst_file.close()349 logger.debug("Adding packages to hwpack")
350 self.hwpack.add_packages(self.packages)
351 for local_package in local_packages:
352 if local_package not in self.packages:
353 logger.warning("Local package '%s' not included",
354 local_package.name)
355 self.hwpack.add_dependency_package(self.config.packages)
356
357 def _extract_build_info(self, cache_dir, out_name, manifest_name):
358 """Extracts build-info from the packages.
359
360 :param cache_dir: The cache directory where build-info should be
361 located.
362 :type cache_dir: str
363 :param out_name: The name of the hwpack file.
364 :type out_name: str
365 :param manifest_name: The name of the manifest file.
366 :type manifest_name: str
367 """
368 logger.debug("Extracting build-info")
369 build_info_dir = os.path.join(cache_dir, 'build-info')
370 build_info_available = 0
371 for deb_pkg in self.packages:
372 deb_pkg_file_path = deb_pkg.filepath
373 if os.path.islink(deb_pkg_file_path):
374 # Skip symlink-ed debian package file
375 # e.g. fetched package with dummy information
376 continue
377 try:
378 # Extract Build-Info attribute from debian control
379 deb_control = DebFile(deb_pkg_file_path).control.debcontrol()
380 build_info = deb_control.get('Build-Info')
381 except ArError:
382 # Skip invalid debian package file
383 # e.g. fetched package with dummy information
384 continue
385 if build_info is not None:
386 build_info_available += 1
387 # Extract debian packages with build information
388 env = os.environ
389 env['LC_ALL'] = 'C'
390 env['NO_PKG_MANGLE'] = '1'
391 proc = cmd_runner.Popen(['dpkg-deb', '-x',
392 deb_pkg_file_path, build_info_dir],
393 env=env, stdout=subprocess.PIPE,
394 stderr=subprocess.PIPE)
395
396 (stdoutdata, stderrdata) = proc.communicate()
397 if proc.returncode:
398 raise ValueError('dpkg-deb extract failed!\n%s' %
399 stderrdata)
400 if stderrdata:
401 raise ValueError('dpkg-deb extract had warnings:\n%s' %
402 stderrdata)
403
404 self._concatenate_build_info(build_info_available, build_info_dir,
405 out_name, manifest_name)
406
407 def _concatenate_build_info(self, build_info_available, build_info_dir,
408 out_name, manifest_name):
409 """Concatenates the build-info text if more than one is available.
410
411 :param build_info_available: The number of available build-info.
412 :type build_info_available: int
413 :param build_info_dir: Where build-info files should be.
414 :type build_info_dir: str
415 :param out_name: The name of the hwpack file.
416 :type out_name: str
417 :param manifest_name: The name of the manifest file.
418 :type manifest_name: str
419 """
420 logger.debug("Concatenating build-info files")
421 dst_file = open('BUILD-INFO.txt', 'wb')
422 if build_info_available > 0:
423 build_info_path = (r'%s/usr/share/doc/*/BUILD-INFO.txt' %
424 build_info_dir)
425 for src_file in iglob(build_info_path):
426 with open(src_file, 'rb') as f:
427 dst_file.write('\nFiles-Pattern: %s\n' % out_name)
428 shutil.copyfileobj(f, dst_file)
429 dst_file.write('\nFiles-Pattern: %s\nLicense-Type: open\n' %
430 manifest_name)
431 else:
432 dst_file.write('Format-Version: 0.1\n'
433 'Files-Pattern: %s, %s\n'
434 'License-Type: open\n' % (out_name, manifest_name))
435 dst_file.close()

Subscribers

People subscribed via source and target branches