Merge lp://qastaging/~dooferlad/linaro-image-tools/copy-files-new-syntax into lp://qastaging/linaro-image-tools/11.11

Just for the record, I don't think such extreme refactoring was required, and it being started, is not complete - instead of moving entire file processing to be package based and happen on the level of linaro-media-create, this still leaves bootloader.file, bootloader.spl_file as it was, just flips copy_files handling - instead of being done consistent with how bootloader.file/bootloader.spl_file works, it is now package based, but that means we have file-extraction code spread around both linaro-hwpack-create and linaro-media-create.

But for the purpose of having it done, it's ok.

(more comments to come.)

It seems that EXTRACT_FILES dict is no longer used by this code, then it should be removed, possibly propagating that recursively (i.e. if it used symbol defined elsewhere, and those symbols are not used either, they should be removed too).

It's not really clear what .get_last_used_keys() does, suggested to add more details docstring.

call_for_all_boards_and_bootloaders(self, function) - somehow I got used to such functions be called foreach_board_and_bootloader() , which hints that it implements enumeration loop, though that's of course just a preference.

[package for package in copy_files] - that looks a bit strange. Was "copy_files[:]" meant instead (make a copy of list)?

> [package for package in copy_files] - that looks a bit strange. Was "copy_files[:]" meant instead (make a copy of list)?

Ah, I see, that's unobvious way to do copy_files.keys(). Btw, I missed moment when they allowed iteration on dict directly, and that started to mean iteration over its keys. I still think that doing it explicitly ("for i in a.iterkeys()") is more clear.

> is more clear

And here's example why:

http://stackoverflow.com/questions/3294889/iterating-over-dictionaries-for-loops-in-python

> if not dest_path.startswith("/boot"):

Great, thanks for implementing this handling.

342 - for expected_file, contents in expected_files:
343 - self.assertThat(
344 - tf, TarfileHasFile(expected_file, content=contents))
345 + stored_package_names = [p.name for p in builder.packages]
346 + for package_name in package_names:
347 + self.assertIn(package_name, stored_package_names)

I'm again not sure about this change - old version did check that file comes from the expected package and not some else, I don't the new does.

On 11 September 2012 10:37, Paul Sokolovsky <email address hidden> wrote:
01234567890123456789012345678901234567890123456789012345678901234567890123456789
> Just for the record, I don't think such extreme refactoring was required,
> and it being started, is not complete - instead of moving entire file
> processing to be package based and happen on the level of
> linaro-media-create, this still leaves bootloader.file, bootloader.spl_file
> as it was, just flips copy_files handling - instead of being done consistent
> with how bootloader.file/bootloader.spl_file works, it is now package based,
> but that means we have file-extraction code spread around both
> linaro-hwpack-create and linaro-media-create.
>
> But for the purpose of having it done, it's ok.

I agree that we could move all file extraction to the hardware pack
install stage, and it is something I considered. I think it is worth
doing as part of a separate update because it would change existing
functionality (well, change it more than has already been done as part
of supporting multiple boards and bootloaders). I don't know if anyone
is replacing bootloader binaries in a hardware pack after they have
created it with linaro-hwpack-create. If they are, we need to discuss
changes to the hardware pack format with them.

I didn't make the decision to start refactoring lightly. I only did
when it became clear that I was going to have to write some rather
ugly code to support the new copy_files format as well as
re-implementing storing files that exist in a package - it seemed that
just using packages would be a lot cleaner!

I expect that the reason for U-Boot and the second stage bootloader
binaries being in a separate folder in the hardware pack it is
historical - I could not see any mention of the reasons in the code!
It adds unnecessary complication to the hardware pack create and
install stages and now that we support multiple bootloaders and boards
in a single hardware pack, we have already missed some bugs around it,
which you fixed (thanks!)

Thanks for the other comments. I will address them now and get an
updated branch ready.

--
James Tunnicliffe