Re: [OE-core] [PATCH 1/2] image.bbclass: Correct chaining compression support
On Tue, Sep 19, 2017 at 11:23:33AM +0200, Martin Hundebøll wrote: > Hi Tom, > > On 2017-07-22 00:06, Tom Rini wrote: > >diff --git a/meta/classes/image.bbclass b/meta/classes/image.bbclass > >index de535ce6fcff..bd6a5b7b810a 100644 > >--- a/meta/classes/image.bbclass > >+++ b/meta/classes/image.bbclass > >@@ -453,7 +453,7 @@ python () { > > rm_tmp_images = set() > > def gen_conversion_cmds(bt): > > for ctype in ctypes: > >-if bt[bt.find('.') + 1:] == ctype: > >+if bt.endswith("." + ctype): > > type = bt[0:-len(ctype) - 1] > > if type.startswith("debugfs_"): > > type = type[8:] > > This is the change that in fact messes with our cpio.gz.u-boot image > types, causing base hash changes. > > I suspect the changed if-check to be too permissive. In our case, it > now matches any ctype that ends with ".u-boot", and not just > "gz.u-boot" as set in IMAGE_FSTYPES. > > Wouldn't it be correct to match on the entire bt variable? I.e. > > diff --git a/meta/classes/image.bbclass b/meta/classes/image.bbclass > > index ef2b38aeaf..818932f7f1 100644 > --- a/meta/classes/image.bbclass > +++ b/meta/classes/image.bbclass > @@ -454,7 +454,7 @@ python () { > rm_tmp_images = set() > def gen_conversion_cmds(bt): > for ctype in ctypes: > -if bt.endswith("." + ctype): > +if bt.split('.', 1) == [t, ctype]: > type = bt[0:-len(ctype) - 1] > if type.startswith("debugfs_"): > type = type[8:] > > This works for me, preliminary tested with the following fs types > configured: > > IMAGE_FSTYPES = "cpio.gz.u-boot" > IMAGE_FSTYPES = "tar cpio cpio.gz cpio.gz.u-boot" > IMAGE_FSTYPES = "tar cpio cpio.gz cpio.gz.u-boot tar.bz2 jffs2 wic > wic.bmap" Good catch. Does it also work with say cpio.u-boot.gz ? And does oe-selftest still pass for the image related parts? I think those are spelled out in the thread too (I've forgotten the incantation myself now, sorry). -- Tom signature.asc Description: Digital signature -- ___ Openembedded-core mailing list Openembedded-core@lists.openembedded.org http://lists.openembedded.org/mailman/listinfo/openembedded-core
Re: [OE-core] [PATCH 1/2] image.bbclass: Correct chaining compression support
Hi Tom, On 2017-07-22 00:06, Tom Rini wrote: diff --git a/meta/classes/image.bbclass b/meta/classes/image.bbclass index de535ce6fcff..bd6a5b7b810a 100644 --- a/meta/classes/image.bbclass +++ b/meta/classes/image.bbclass @@ -453,7 +453,7 @@ python () { rm_tmp_images = set() def gen_conversion_cmds(bt): for ctype in ctypes: -if bt[bt.find('.') + 1:] == ctype: +if bt.endswith("." + ctype): type = bt[0:-len(ctype) - 1] if type.startswith("debugfs_"): type = type[8:] This is the change that in fact messes with our cpio.gz.u-boot image types, causing base hash changes. I suspect the changed if-check to be too permissive. In our case, it now matches any ctype that ends with ".u-boot", and not just "gz.u-boot" as set in IMAGE_FSTYPES. Wouldn't it be correct to match on the entire bt variable? I.e. diff --git a/meta/classes/image.bbclass b/meta/classes/image.bbclass index ef2b38aeaf..818932f7f1 100644 --- a/meta/classes/image.bbclass +++ b/meta/classes/image.bbclass @@ -454,7 +454,7 @@ python () { rm_tmp_images = set() def gen_conversion_cmds(bt): for ctype in ctypes: -if bt.endswith("." + ctype): +if bt.split('.', 1) == [t, ctype]: type = bt[0:-len(ctype) - 1] if type.startswith("debugfs_"): type = type[8:] This works for me, preliminary tested with the following fs types configured: IMAGE_FSTYPES = "cpio.gz.u-boot" IMAGE_FSTYPES = "tar cpio cpio.gz cpio.gz.u-boot" IMAGE_FSTYPES = "tar cpio cpio.gz cpio.gz.u-boot tar.bz2 jffs2 wic wic.bmap" // Martin -- ___ Openembedded-core mailing list Openembedded-core@lists.openembedded.org http://lists.openembedded.org/mailman/listinfo/openembedded-core
Re: [OE-core] [PATCH 1/2] image.bbclass: Correct chaining compression support
On Wed, Jul 26, 2017 at 10:16:39AM +0300, Ed Bartosh wrote: > On Fri, Jul 21, 2017 at 06:06:33PM -0400, Tom Rini wrote: > > When chaining of compression/conversion types was added, we had a new > > way to handle doing things like "ext4.bz2.sha256sum" or > > "ext2.gz.u-boot". However, because the U-Boot image class isn't > > included normally, it wasn't properly converted at the time. After the > > support was added the "clean" argument that the .u-boot code uses no > > longer functions. The fix for this inadvertently broke chaining > > compression/conversion. First, correct the u-boot conversion code. > > > > Fixes: 46bc438374de ("image.bbclass: do exact match for rootfs type") > > Cc: Zhenhua Luo> > Cc: Richard Purdie > > Cc: Patrick Ohly > > Signed-off-by: Tom Rini > > Acked-by: Ed Bartosh > > Any chance to have this functionality covered by oe-selftest? > That would help to ensure it will not be broken again. Give me a pointer to adding stuff to oe-selftest and I'll take a look. Thanks! -- Tom signature.asc Description: Digital signature -- ___ Openembedded-core mailing list Openembedded-core@lists.openembedded.org http://lists.openembedded.org/mailman/listinfo/openembedded-core
Re: [OE-core] [PATCH 1/2] image.bbclass: Correct chaining compression support
On Fri, Jul 21, 2017 at 06:06:33PM -0400, Tom Rini wrote: > When chaining of compression/conversion types was added, we had a new > way to handle doing things like "ext4.bz2.sha256sum" or > "ext2.gz.u-boot". However, because the U-Boot image class isn't > included normally, it wasn't properly converted at the time. After the > support was added the "clean" argument that the .u-boot code uses no > longer functions. The fix for this inadvertently broke chaining > compression/conversion. First, correct the u-boot conversion code. > > Fixes: 46bc438374de ("image.bbclass: do exact match for rootfs type") > Cc: Zhenhua Luo> Cc: Richard Purdie > Cc: Patrick Ohly > Signed-off-by: Tom Rini Acked-by: Ed Bartosh Any chance to have this functionality covered by oe-selftest? That would help to ensure it will not be broken again. > --- > This change is fairly important and, imho, innocuous and should be > populated to pyro/etc, once merged to master. The next part of the > series is clean-up and while with my U-Boot hat on, I would say should > be pushed more widely, I am biased. > --- > meta/classes/image.bbclass | 2 +- > meta/classes/image_types_uboot.bbclass | 13 + > 2 files changed, 6 insertions(+), 9 deletions(-) > > diff --git a/meta/classes/image.bbclass b/meta/classes/image.bbclass > index de535ce6fcff..bd6a5b7b810a 100644 > --- a/meta/classes/image.bbclass > +++ b/meta/classes/image.bbclass > @@ -453,7 +453,7 @@ python () { > rm_tmp_images = set() > def gen_conversion_cmds(bt): > for ctype in ctypes: > -if bt[bt.find('.') + 1:] == ctype: > +if bt.endswith("." + ctype): > type = bt[0:-len(ctype) - 1] > if type.startswith("debugfs_"): > type = type[8:] > diff --git a/meta/classes/image_types_uboot.bbclass > b/meta/classes/image_types_uboot.bbclass > index 5dfa39287dab..8db436efb14b 100644 > --- a/meta/classes/image_types_uboot.bbclass > +++ b/meta/classes/image_types_uboot.bbclass > @@ -3,9 +3,6 @@ inherit image_types kernel-arch > oe_mkimage () { > mkimage -A ${UBOOT_ARCH} -O linux -T ramdisk -C $2 -n ${IMAGE_NAME} \ > -d ${IMGDEPLOYDIR}/$1 ${IMGDEPLOYDIR}/$1.u-boot > -if [ x$3 = x"clean" ]; then > -rm $1 > -fi > } > > CONVERSIONTYPES += "gz.u-boot bz2.u-boot lz4.u-boot lzma.u-boot lzo.u-boot > u-boot" > @@ -14,19 +11,19 @@ CONVERSION_DEPENDS_u-boot = "u-boot-mkimage-native" > CONVERSION_CMD_u-boot = "oe_mkimage ${IMAGE_NAME}.rootfs.${type} none" > > CONVERSION_DEPENDS_gz.u-boot = "u-boot-mkimage-native" > -CONVERSION_CMD_gz.u-boot = "${CONVERSION_CMD_gz}; oe_mkimage > ${IMAGE_NAME}.rootfs.${type}.gz gzip clean" > +CONVERSION_CMD_gz.u-boot = "${CONVERSION_CMD_gz}; oe_mkimage > ${IMAGE_NAME}.rootfs.${type}.gz gzip" > > CONVERSION_DEPENDS_bz2.u-boot = "u-boot-mkimage-native" > -CONVERSION_CMD_bz2.u-boot = "${CONVERSION_CMD_bz2}; oe_mkimage > ${IMAGE_NAME}.rootfs.${type}.bz2 bzip2 clean" > +CONVERSION_CMD_bz2.u-boot = "${CONVERSION_CMD_bz2}; oe_mkimage > ${IMAGE_NAME}.rootfs.${type}.bz2 bzip2" > > CONVERSION_DEPENDS_lz4.u-boot = "u-boot-mkimage-native" > -CONVERSION_CMD_lz4.u-boot = "${CONVERSION_CMD_lz4_legacy}; oe_mkimage > ${IMAGE_NAME}.rootfs.${type}.lz4 lz4 clean" > +CONVERSION_CMD_lz4.u-boot = "${CONVERSION_CMD_lz4_legacy}; oe_mkimage > ${IMAGE_NAME}.rootfs.${type}.lz4 lz4" > > CONVERSION_DEPENDS_lzma.u-boot = "u-boot-mkimage-native" > -CONVERSION_CMD_lzma.u-boot = "${CONVERSION_CMD_lzma}; oe_mkimage > ${IMAGE_NAME}.rootfs.${type}.lzma lzma clean" > +CONVERSION_CMD_lzma.u-boot = "${CONVERSION_CMD_lzma}; oe_mkimage > ${IMAGE_NAME}.rootfs.${type}.lzma lzma" > > CONVERSION_DEPENDS_lzo.u-boot = "u-boot-mkimage-native" > -CONVERSION_CMD_lzo.u-boot = "${CONVERSION_CMD_lzo}; oe_mkimage > ${IMAGE_NAME}.rootfs.${type}.lzo lzo clean" > +CONVERSION_CMD_lzo.u-boot = "${CONVERSION_CMD_lzo}; oe_mkimage > ${IMAGE_NAME}.rootfs.${type}.lzo lzo" > > IMAGE_TYPES += "ext2.u-boot ext2.gz.u-boot ext2.bz2.u-boot ext2.lzma.u-boot > ext3.gz.u-boot ext4.gz.u-boot cpio.gz.u-boot" > > -- > 1.9.1 > > -- > ___ > Openembedded-core mailing list > Openembedded-core@lists.openembedded.org > http://lists.openembedded.org/mailman/listinfo/openembedded-core -- Regards, Ed -- ___ Openembedded-core mailing list Openembedded-core@lists.openembedded.org http://lists.openembedded.org/mailman/listinfo/openembedded-core
Re: [OE-core] [PATCH 1/2] image.bbclass: Correct chaining compression support
On Mon, 2017-07-24 at 07:19 -0400, Tom Rini wrote: > On Mon, Jul 24, 2017 at 10:35:37AM +0200, Patrick Ohly wrote: > > On Fri, 2017-07-21 at 18:06 -0400, Tom Rini wrote: > > > The fix for this inadvertently broke chaining > > > compression/conversion. First, correct the u-boot conversion > > > code. > > > > > > Fixes: 46bc438374de ("image.bbclass: do exact > > > match for rootfs type") > > > Cc: Zhenhua Luo> > > Cc: Richard Purdie > > > Cc: Patrick Ohly > > > Signed-off-by: Tom Rini > > > --- > > > This change is fairly important and, imho, innocuous and should > > > be > > > populated to pyro/etc, once merged to master. The next part of > > > the > > > series is clean-up and while with my U-Boot hat on, I would say > > > should > > > be pushed more widely, I am biased. > > > --- > > > meta/classes/image.bbclass | 2 +- > > > meta/classes/image_types_uboot.bbclass | 13 + > > > 2 files changed, 6 insertions(+), 9 deletions(-) > > > > > > diff --git a/meta/classes/image.bbclass > > > b/meta/classes/image.bbclass > > > index de535ce6fcff..bd6a5b7b810a 100644 > > > --- a/meta/classes/image.bbclass > > > +++ b/meta/classes/image.bbclass > > > @@ -453,7 +453,7 @@ python () { > > > rm_tmp_images = set() > > > def gen_conversion_cmds(bt): > > > for ctype in ctypes: > > > - if bt[bt.find('.') + 1:] == ctype: > > > + if bt.endswith("." + ctype) > > > > This reverts 46bc438374de and thus restores the code as I had > > originally written it. > > > > Looking at 46bc438374de, it's not clear to me how the commit > > message > > matches the code, i.e. I don't understand the commit. So it was an > > incorrect fix for the problem described in that commit message, and > > the > > right one are your changes to the u-boot conversion command? > > Ah, so the important bit is the other half of this patch, which > addresses the problem 46bc438374de was intended to deal with, > correctly. > Prior to the chaining compression/conversion support, the "u-boot" > targets would clean up their intermediate files. With your patch > those > files get cleaned up automatically and that the mkimage calling > function > was also doing it lead to build failures. But since we no longer > need > to have a manual cleaning step, we can just drop it. Makes sense. Acked-by: Patrick Ohly -- Best Regards, Patrick Ohly The content of this message is my personal opinion only and although I am an employee of Intel, the statements I make here in no way represent Intel's position on the issue, nor am I authorized to speak on behalf of Intel on this matter. -- ___ Openembedded-core mailing list Openembedded-core@lists.openembedded.org http://lists.openembedded.org/mailman/listinfo/openembedded-core
Re: [OE-core] [PATCH 1/2] image.bbclass: Correct chaining compression support
On Mon, Jul 24, 2017 at 10:35:37AM +0200, Patrick Ohly wrote: > On Fri, 2017-07-21 at 18:06 -0400, Tom Rini wrote: > > The fix for this inadvertently broke chaining > > compression/conversion. First, correct the u-boot conversion code. > > > > Fixes: 46bc438374de ("image.bbclass: do exact > > match for rootfs type") > > Cc: Zhenhua Luo> > Cc: Richard Purdie > > Cc: Patrick Ohly > > Signed-off-by: Tom Rini > > --- > > This change is fairly important and, imho, innocuous and should be > > populated to pyro/etc, once merged to master. The next part of the > > series is clean-up and while with my U-Boot hat on, I would say > > should > > be pushed more widely, I am biased. > > --- > > meta/classes/image.bbclass | 2 +- > > meta/classes/image_types_uboot.bbclass | 13 + > > 2 files changed, 6 insertions(+), 9 deletions(-) > > > > diff --git a/meta/classes/image.bbclass b/meta/classes/image.bbclass > > index de535ce6fcff..bd6a5b7b810a 100644 > > --- a/meta/classes/image.bbclass > > +++ b/meta/classes/image.bbclass > > @@ -453,7 +453,7 @@ python () { > > rm_tmp_images = set() > > def gen_conversion_cmds(bt): > > for ctype in ctypes: > > - if bt[bt.find('.') + 1:] == ctype: > > + if bt.endswith("." + ctype) > > This reverts 46bc438374de and thus restores the code as I had > originally written it. > > Looking at 46bc438374de, it's not clear to me how the commit message > matches the code, i.e. I don't understand the commit. So it was an > incorrect fix for the problem described in that commit message, and the > right one are your changes to the u-boot conversion command? Ah, so the important bit is the other half of this patch, which addresses the problem 46bc438374de was intended to deal with, correctly. Prior to the chaining compression/conversion support, the "u-boot" targets would clean up their intermediate files. With your patch those files get cleaned up automatically and that the mkimage calling function was also doing it lead to build failures. But since we no longer need to have a manual cleaning step, we can just drop it. -- Tom signature.asc Description: Digital signature -- ___ Openembedded-core mailing list Openembedded-core@lists.openembedded.org http://lists.openembedded.org/mailman/listinfo/openembedded-core
Re: [OE-core] [PATCH 1/2] image.bbclass: Correct chaining compression support
On Fri, 2017-07-21 at 18:06 -0400, Tom Rini wrote: > The fix for this inadvertently broke chaining > compression/conversion. First, correct the u-boot conversion code. > > Fixes: 46bc438374de ("image.bbclass: do exact > match for rootfs type") > Cc: Zhenhua Luo> Cc: Richard Purdie > Cc: Patrick Ohly > Signed-off-by: Tom Rini > --- > This change is fairly important and, imho, innocuous and should be > populated to pyro/etc, once merged to master. The next part of the > series is clean-up and while with my U-Boot hat on, I would say > should > be pushed more widely, I am biased. > --- > meta/classes/image.bbclass | 2 +- > meta/classes/image_types_uboot.bbclass | 13 + > 2 files changed, 6 insertions(+), 9 deletions(-) > > diff --git a/meta/classes/image.bbclass b/meta/classes/image.bbclass > index de535ce6fcff..bd6a5b7b810a 100644 > --- a/meta/classes/image.bbclass > +++ b/meta/classes/image.bbclass > @@ -453,7 +453,7 @@ python () { > rm_tmp_images = set() > def gen_conversion_cmds(bt): > for ctype in ctypes: > - if bt[bt.find('.') + 1:] == ctype: > + if bt.endswith("." + ctype) This reverts 46bc438374de and thus restores the code as I had originally written it. Looking at 46bc438374de, it's not clear to me how the commit message matches the code, i.e. I don't understand the commit. So it was an incorrect fix for the problem described in that commit message, and the right one are your changes to the u-boot conversion command? -- Best Regards, Patrick Ohly The content of this message is my personal opinion only and although I am an employee of Intel, the statements I make here in no way represent Intel's position on the issue, nor am I authorized to speak on behalf of Intel on this matter. -- ___ Openembedded-core mailing list Openembedded-core@lists.openembedded.org http://lists.openembedded.org/mailman/listinfo/openembedded-core
[OE-core] [PATCH 1/2] image.bbclass: Correct chaining compression support
When chaining of compression/conversion types was added, we had a new way to handle doing things like "ext4.bz2.sha256sum" or "ext2.gz.u-boot". However, because the U-Boot image class isn't included normally, it wasn't properly converted at the time. After the support was added the "clean" argument that the .u-boot code uses no longer functions. The fix for this inadvertently broke chaining compression/conversion. First, correct the u-boot conversion code. Fixes: 46bc438374de ("image.bbclass: do exact match for rootfs type") Cc: Zhenhua LuoCc: Richard Purdie Cc: Patrick Ohly Signed-off-by: Tom Rini --- This change is fairly important and, imho, innocuous and should be populated to pyro/etc, once merged to master. The next part of the series is clean-up and while with my U-Boot hat on, I would say should be pushed more widely, I am biased. --- meta/classes/image.bbclass | 2 +- meta/classes/image_types_uboot.bbclass | 13 + 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/meta/classes/image.bbclass b/meta/classes/image.bbclass index de535ce6fcff..bd6a5b7b810a 100644 --- a/meta/classes/image.bbclass +++ b/meta/classes/image.bbclass @@ -453,7 +453,7 @@ python () { rm_tmp_images = set() def gen_conversion_cmds(bt): for ctype in ctypes: -if bt[bt.find('.') + 1:] == ctype: +if bt.endswith("." + ctype): type = bt[0:-len(ctype) - 1] if type.startswith("debugfs_"): type = type[8:] diff --git a/meta/classes/image_types_uboot.bbclass b/meta/classes/image_types_uboot.bbclass index 5dfa39287dab..8db436efb14b 100644 --- a/meta/classes/image_types_uboot.bbclass +++ b/meta/classes/image_types_uboot.bbclass @@ -3,9 +3,6 @@ inherit image_types kernel-arch oe_mkimage () { mkimage -A ${UBOOT_ARCH} -O linux -T ramdisk -C $2 -n ${IMAGE_NAME} \ -d ${IMGDEPLOYDIR}/$1 ${IMGDEPLOYDIR}/$1.u-boot -if [ x$3 = x"clean" ]; then -rm $1 -fi } CONVERSIONTYPES += "gz.u-boot bz2.u-boot lz4.u-boot lzma.u-boot lzo.u-boot u-boot" @@ -14,19 +11,19 @@ CONVERSION_DEPENDS_u-boot = "u-boot-mkimage-native" CONVERSION_CMD_u-boot = "oe_mkimage ${IMAGE_NAME}.rootfs.${type} none" CONVERSION_DEPENDS_gz.u-boot = "u-boot-mkimage-native" -CONVERSION_CMD_gz.u-boot = "${CONVERSION_CMD_gz}; oe_mkimage ${IMAGE_NAME}.rootfs.${type}.gz gzip clean" +CONVERSION_CMD_gz.u-boot = "${CONVERSION_CMD_gz}; oe_mkimage ${IMAGE_NAME}.rootfs.${type}.gz gzip" CONVERSION_DEPENDS_bz2.u-boot = "u-boot-mkimage-native" -CONVERSION_CMD_bz2.u-boot = "${CONVERSION_CMD_bz2}; oe_mkimage ${IMAGE_NAME}.rootfs.${type}.bz2 bzip2 clean" +CONVERSION_CMD_bz2.u-boot = "${CONVERSION_CMD_bz2}; oe_mkimage ${IMAGE_NAME}.rootfs.${type}.bz2 bzip2" CONVERSION_DEPENDS_lz4.u-boot = "u-boot-mkimage-native" -CONVERSION_CMD_lz4.u-boot = "${CONVERSION_CMD_lz4_legacy}; oe_mkimage ${IMAGE_NAME}.rootfs.${type}.lz4 lz4 clean" +CONVERSION_CMD_lz4.u-boot = "${CONVERSION_CMD_lz4_legacy}; oe_mkimage ${IMAGE_NAME}.rootfs.${type}.lz4 lz4" CONVERSION_DEPENDS_lzma.u-boot = "u-boot-mkimage-native" -CONVERSION_CMD_lzma.u-boot = "${CONVERSION_CMD_lzma}; oe_mkimage ${IMAGE_NAME}.rootfs.${type}.lzma lzma clean" +CONVERSION_CMD_lzma.u-boot = "${CONVERSION_CMD_lzma}; oe_mkimage ${IMAGE_NAME}.rootfs.${type}.lzma lzma" CONVERSION_DEPENDS_lzo.u-boot = "u-boot-mkimage-native" -CONVERSION_CMD_lzo.u-boot = "${CONVERSION_CMD_lzo}; oe_mkimage ${IMAGE_NAME}.rootfs.${type}.lzo lzo clean" +CONVERSION_CMD_lzo.u-boot = "${CONVERSION_CMD_lzo}; oe_mkimage ${IMAGE_NAME}.rootfs.${type}.lzo lzo" IMAGE_TYPES += "ext2.u-boot ext2.gz.u-boot ext2.bz2.u-boot ext2.lzma.u-boot ext3.gz.u-boot ext4.gz.u-boot cpio.gz.u-boot" -- 1.9.1 -- ___ Openembedded-core mailing list Openembedded-core@lists.openembedded.org http://lists.openembedded.org/mailman/listinfo/openembedded-core