| Summary: | The detection of the image type is incorrectly matching a wrong type | ||||||
|---|---|---|---|---|---|---|---|
| Product: | [Build System, Metadata & Runtime] OE-Core | Reporter: | Igor Stoppa <igor.stoppa> | ||||
| Component: | deployment | Assignee: | Patrick Ohly <patrick.ohly> | ||||
| Status: | RESOLVED FIXED | QA Contact: | |||||
| Severity: | normal | ||||||
| Priority: | Medium | CC: | patrick.ohly, sgw | ||||
| Version: | unspecified | ||||||
| Target Milestone: | 2.2 | ||||||
| Hardware: | x86 | ||||||
| OS: | Multiple | ||||||
| Whiteboard: | |||||||
| OS type for building Yocto: | --- | Type of Regression: | --- | ||||
| Verified: | Documentation change: | Yes (doc changes required) | |||||
| Bug Depends on: | |||||||
| Bug Blocks: | 9346 | ||||||
| Attachments: |
|
||||||
Created attachment 3019 [details] make .rootfs suffix configurable Our distro and the corresponding image creation class are now public: https://github.com/ostroproject/meta-ostro/blob/master/meta-ostro/classes/image-dsk.bbclass (In reply to comment #0) > I have extended the image types to support a .dsk format and this is > correctly supported. > > However when I tried to support also its xz-compressed version, represented > with the type dsk.xz, this triggers a bogus match and the build fails with > trying to compress a file that doesn't exist: {IMAGE_NAME}.rootfs.${type} > > The file doesn't exist because I am not using the boilerplate recipe for > creating the disk image and I have a different naming convention. There are two possible solutions: 1. adapt the base classes such that they also support our naming convention (${IMAGE_NAME}.dsk instead of ${IMAGE_NAME}.rootfs.dsk) and use the compression support from the base classes 2. disable compression support by overriding COMPRESSIONTYPES I believe solution 2 is already possible without modifying the core classes. However, I haven't tried it because I find solution 1 more attractive (no need to re-implement functionality in Ostro OS). I have tested solution 1 with the attached patch, the result worked for me. Would something like this be acceptable for OE-core? I would prefer a 3rd option: fix the improper use of the term "rootfs" by dropping it entirely, since the image is not always a single partition.
Attachment 309 [details] doesn't rectify the misnomer, which is the root cause of .dsk images implementing custom way of doing compression.
Said that, I think it's still wrong that the current regex associates "dsk.xz" to "xz", when they are obviously different.
The proposed attachment would simply hide the bug.
Note that https://bugzilla.yoctoproject.org/attachment.cgi?id=3019 just illustrates the general approach. After thinking about it some more, I'm not too happy about the variable names anymore and probably would update the patch to use: IMAGE_NAME_ROOTFS_SUFFIX_DEFAULT = ".rootfs" IMAGE_NAME_ROOTFS_SUFFIX ??= ".rootfs" IMAGE_NAME_ROOTFS = "${IMAGE_NAME}${IMAGE_NAME_ROOTFS_SUFFIX}" My gripe is that the entire class/recipe makes a mistake in generalizing too much what is instead a subcase (image with no partition table and only one partition). So any reference to rootfs should be discarded or at least limited to when it's really sure that it's a rootfs, iow when dealing with the rootfs *partition*. (In reply to comment #2) > I would prefer a 3rd option: fix the improper use of the term "rootfs" by > dropping it entirely, since the image is not always a single partition. I agree, and RP said on IRC that he also would prefer that. However, this cannot be done without some carefully managed transition. This patch is a step towards that goal. > Attachment 309 [details] doesn't rectify the misnomer, which is the root > cause of .dsk images implementing custom way of doing compression. The patch allows removing the misnomer. It just doesn't do it by default, which wouldn't be appropriate in the current phase of OE-core. > Said that, I think it's still wrong that the current regex associates > "dsk.xz" to "xz", when they are obviously different. > > The proposed attachment would simply hide the bug. One can also argue that the base class is working as intended and just doesn't get used as intended. IMHO it is pointless to discuss that further, one can argue either way. Instead let's focus on improving the base class. (In reply to comment #2) > Said that, I think it's still wrong that the current regex associates > "dsk.xz" to "xz", when they are obviously different. On chat Igor clarified that this comment is about supporting stacked conversion commands. Supporting the conversion of dsk into dsk.vdi and then into dsk.vdi.xz via COMPRESS_CMDs would address his concerns about the base class. (In reply to comment #6) > (In reply to comment #2) > > Said that, I think it's still wrong that the current regex associates > > "dsk.xz" to "xz", when they are obviously different. > > On chat Igor clarified that this comment is about supporting stacked > conversion commands. Supporting the conversion of dsk into dsk.vdi and then > into dsk.vdi.xz via COMPRESS_CMDs would address his concerns about the base > class. ... and there's some weirdness around applying compression to masked image types. In Ostro, we currently use that for generating .vdi images. We can avoid the problem my defining a conversion command and stacking conversions. Comment on attachment 3019 [details]
make .rootfs suffix configurable
Replaced by "[PATCH 1/2] image creation: allow overriding .rootfs suffix" on the OE-core mailing list.
I renamed IMAGE_NAME_ROOTFS to IMAGE_NAME_SUFFIX and removed the IMAGE_NAME_ROOTFS_DEFAULT in favor of a different mechanism in the Ostro class which overrides the default.
"[PATCH 2/2] image.bbclass: support chaining compression (aka conversion) commands" on the OE-core mailing list implements chaining of conversion commands. Regarding this issue here, there are several additional possible enhancements: 1. If there are both conversion commands and dedicated image commands that end up supporting the same IMAGE_FSTYPE, then use only the image command. Example: there is a) IMAGE_CMD_dsk b) IMAGE_TYPES_MASKED = "dsk.xz" c) COMPRESS_CMD_xz. In this case, dsk.xz should be run by executing IMAGE_CMD_dsk without involving COMPRESS_CMD_xz. 2. Rename COMPRESSION_METHODS and COMPRESS_CMD to something that reflects that conversion is also supported. Alternatively, document this disconnect between legacy names (which are an API and thus not easily changed) and the new, extended functionality. 3. Replace image-vm.bbclass and thus IMAGE_FSTYPES = "qcow2/vdi/vmdk" with conversion commands for these three types, and thus supporting IMAGE_FSTYPES = "hddimg.qcow2/hddimg.vdi/hddimg.vmdk" instead. In my opinion, point 3 should be done after the next OE-core release. For point 2 I would only update the documentation. While point 1 would be nice, I'm worried about the additional complexity. Perhaps someone needs to try implementing it to see how complex it will be. (In reply to comment #10) > Regarding this issue here, there are several additional possible > enhancements: > 1. If there are both conversion commands and dedicated image commands that > end > up supporting the same IMAGE_FSTYPE, then use only the image command. > Example: > there is a) IMAGE_CMD_dsk b) IMAGE_TYPES_MASKED = "dsk.xz" c) > COMPRESS_CMD_xz. > In this case, dsk.xz should be run by executing IMAGE_CMD_dsk without > involving COMPRESS_CMD_xz. > 2. Rename COMPRESSION_METHODS and COMPRESS_CMD to something that reflects > that conversion is also supported. Alternatively, document this disconnect > between legacy names (which are an API and thus not easily changed) and > the new, extended functionality. > 3. Replace image-vm.bbclass and thus IMAGE_FSTYPES = "qcow2/vdi/vmdk" with > conversion commands for these three types, and thus supporting > IMAGE_FSTYPES = "hddimg.qcow2/hddimg.vdi/hddimg.vmdk" instead. I have commits for all of these points in https://github.com/pohly/ostro-os/commits/image-compression Ed, if you think that any of them should really still go into 2.1, can you cherry-pick, test some more and then submit? Otherwise I'll do that after 2.1 is released. We discussed this patchset and came up with the following: - we'll land this patchet after 2.1 is released as amount of changes is quite big for 2.1 considering the time left for the release - I'll test the changes as much as I can before trying to land them Sent patchset for review: http://lists.openembedded.org/pipermail/openembedded-core/2016-May/121071.html One patch has been accepted. Sent v2 with the remaining 6 patches: http://lists.openembedded.org/pipermail/openembedded-core/2016-May/121696.html Two more patches have been accepted: https://git.yoctoproject.org/cgit/cgit.cgi/poky/commit/?id=e7b5bd2ba11118f8220b915a44bfff7c9c39ca95 https://git.yoctoproject.org/cgit/cgit.cgi/poky/commit/?id=1974624eb98d090556b23ece45271e1225c68031 4 patches are still pending. One more patch has been merged to master: https://git.yoctoproject.org/cgit/cgit.cgi/poky/commit/?id=ff3a455ee84b91e797052760c8d4618109b54c63 I'm going to close this bug as most of the changes have been accepted. The rest of the changes were raising concerns among maintainers and I'm not sure I'm ready to continue pushing them. http://lists.openembedded.org/pipermail/openembedded-core/2016-July/124472.html http://lists.openembedded.org/pipermail/openembedded-core/2016-July/124472.html Since not all the patches required are merged either in the current form or as rework, I don't think FIXED is the correct resolution. WONTFIX would seem more appropriate. Incidentally, the same url to the patch review is listed twice. Is it just a double paste or were there supposed to be 2 different links? OK then. Reassigning to the original author of changes. Feel free to continue working on this or close if you wish. Here are more links to the review posts: http://lists.openembedded.org/pipermail/openembedded-core/2016-July/124473.html http://lists.openembedded.org/pipermail/openembedded-core/2016-July/124516.html http://lists.openembedded.org/pipermail/openembedded-core/2016-July/124517.html Current status: from https://github.com/pohly/ostro-os/commits/image-compression the following three patches remained unmerged: be80d93 image.bbclass: prefer specialized image creation methods over chaining Uncertain whether anyone needs that. fdb302c image creation: support converting masked types and use that for hdddirect Useful code cleanup (and RP agreed, see http://lists.openembedded.org/pipermail/openembedded-core/2016-July/124517.html), but patch needs to be redone and explained better. Not a useful change while stabilizing OE-core for 2.2. 75e4b9e image creation: fix dependency calculation when using conversion chaining Was fixed independently. My conclusion is that the original issue is addressed for 2.2 and what's left are enhancements that build upon that. I've filed bug #10203 and bug #10204 and intend to work on that after the 2.2 release. |
I have extended the image types to support a .dsk format and this is correctly supported. However when I tried to support also its xz-compressed version, represented with the type dsk.xz, this triggers a bogus match and the build fails with trying to compress a file that doesn't exist: {IMAGE_NAME}.rootfs.${type} The file doesn't exist because I am not using the boilerplate recipe for creating the disk image and I have a different naming convention. Ideally the image_types class would be more flexible, but, lacking that, it should not try to gobble file types that are not its business. I suspect it's because the search for a match is too greedy.