Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Use the `role` parameter when creating images #100
Conversation
sil2100
added some commits
Nov 18, 2016
|
This is still WIP. As mentioned on IRC, this still needs the following:
Submitting this now as I want some eyeballs to say if this is the 'right way to go' - as for backwards compatibility I'm attaching a rootfs part in case it's not defined explicitly. |
|
Regarding this branch, I just got an idea I would need feedback on. Right now in this approach I check if the system-data role partition is present in the builder code and if not, append the partition manually then. Now that I think about it: maybe I could do that validation and 'legacy-support' in the parser instead? I could check during parse if there was a system-data part available and, if not, just append the new partition there instead. This seems like a cleaner way of doing it, as after the 'parse' step our structure list is cleanly defined. If there are no objections I'll give it a try. |
sil2100
added some commits
Dec 6, 2016
sil2100
changed the title from
WIP: use the `role` parameter when creating images
to
Use the `role` parameter when creating images
Dec 14, 2016
|
Ok, in theory this should now be good to land. It's been around for quite a while but it's mergable with trunk and tests are passing. |
warsaw
suggested changes
Dec 15, 2016
Looks pretty good! I have a couple of questions, but nothing major.
| @@ -208,7 +204,7 @@ def populate_bootfs_contents(self): | ||
| target_dir = os.path.join(self.workdir, 'part{}'.format(partnum)) | ||
| # XXX: Use file system label for the moment, until we get a proper | ||
| # way to identify the boot partition. | ||
| - if part.filesystem_label == 'system-boot': | ||
| + if part.role is StructureRole.system_boot: |
warsaw
Dec 15, 2016
Contributor
Please correct me if I'm wrong but we can get away with this because in the parser, you check to see if role is defined, and if not, you parse the file system label for fall backs, right?
sil2100
Dec 16, 2016
Contributor
Exactly. We don't have to do any label parsing anymore here in the builder code. I guess I need to update the comment.
| + # We defer creating the root file system image because we have | ||
| + # to populate it at the same time. See mkfs.ext4(8) for details | ||
| + with open(part_img, 'w'): | ||
| + pass |
warsaw
Dec 15, 2016
Contributor
I've been thinking we might want to switch to pathlib since it's a nicer API although the integration with other APIs will be much better in Python 3.6. I know this is just a refactor of existing code, but what do you think about using Path.touch()?
sil2100
Dec 16, 2016
Contributor
Ok, we could. When adding new features I tend to just re-use the existing code since this simplifies the diff and makes it easier to see how the parts moved. I can change it anyway.
warsaw
Dec 16, 2016
Contributor
Yep I realized later that this was just a code move. I forgot to come back and update this comment. That's cool, we can switch later.
| - new='{}M:+{}M'.format( | ||
| - part.offset // MiB(1), part.size // MiB(1)), | ||
| + new='{}M:+{}K'.format( | ||
| + part.offset // MiB(1), ceil(part.size / 1024)), |
sil2100
Dec 16, 2016
Contributor
If you look at the old code for the builder, you'll see that there were two image.partition() calls made - one for each partition and one for the rootfs, each of them having a different 'new' format ('{}M:+{}K' for rootfs, '{}M:+{}M' for normal). Now we just need one of them, so it was just a matter of deciding which of the two should stay. I decided on leaving the one that was used for the rootfs.
So in fact it's not that this 'changed', it's just about which of the two was removed. I can switch to the other one if you want though.
warsaw
Dec 16, 2016
Contributor
TL;DR: +1
I just saw LP: #1460715 today, so I think that's an opportune change, although we might also need to change the first 'M' (I haven't fully reviewed the impact of that bug on u-i yet). Thanks for the explanation.
Noting that there's a subtle behavioral difference here. If part.size is some fraction of a MiB bigger than 1MiB, the partition will only be the floor of the size in MiB under the old code. So I think the ceil() calculation is more right because the resulting partition may be slightly bigger than the part size, which is okay because it definitely won't be smaller!
| @@ -1833,6 +1863,7 @@ def test_mbr_structure_not_at_offset_zero_implicit(self): | ||
| bootloader: u-boot | ||
| structure: | ||
| - type: 00000000-0000-0000-0000-0000deadbeef | ||
| + |
| @@ -301,8 +300,7 @@ def prepare_filesystems(self): | ||
| part.size = self.rootfs_size | ||
| # We defer creating the root file system image because we have | ||
| # to populate it at the same time. See mkfs.ext4(8) for details | ||
| - with open(part_img, 'w'): | ||
| - pass | ||
| + Path(part_img).touch() |
|
Branch updated, I'll let CI run and then will merge. Thanks! |
|
I've noticed some problems in additional local testing; resulting images aren't bootable. Stand by. |
warsaw
and others
added some commits
Dec 16, 2016
|
I think I fixed the coverage issues. I also added the rootfs label test. Also, changed the rootfs label mismatch warning to a hard error as per discussion. Some random cleanups. If this passes CI, I will land it. |
sil2100 commentedNov 30, 2016
No description provided.