From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dan.rpsys.net (dan.rpsys.net [93.97.175.187]) by mail.openembedded.org (Postfix) with ESMTP id E655C6D13F for ; Tue, 29 Oct 2013 15:33:41 +0000 (UTC) Received: from localhost (dan.rpsys.net [127.0.0.1]) by dan.rpsys.net (8.14.4/8.14.4/Debian-2.1ubuntu1) with ESMTP id r9TFXakR013981; Tue, 29 Oct 2013 15:33:36 GMT X-Virus-Scanned: Debian amavisd-new at dan.rpsys.net Received: from dan.rpsys.net ([127.0.0.1]) by localhost (dan.rpsys.net [127.0.0.1]) (amavisd-new, port 10024) with LMTP id Xqo2X6kZ-45o; Tue, 29 Oct 2013 15:33:36 +0000 (GMT) Received: from [192.168.3.10] (rpvlan0 [192.168.3.10]) (authenticated bits=0) by dan.rpsys.net (8.14.4/8.14.4/Debian-2.1ubuntu1) with ESMTP id r9TFXXlm013960 (version=TLSv1/SSLv3 cipher=DHE-RSA-CAMELLIA256-SHA bits=256 verify=NOT); Tue, 29 Oct 2013 15:33:34 GMT Message-ID: <1383060808.25877.12.camel@ted> From: Richard Purdie To: Andrea Adami Date: Tue, 29 Oct 2013 15:33:28 +0000 In-Reply-To: References: <5739f47649bfb50369107ca603b747b4201c70b0.1382307169.git.andrea.adami@gmail.com> <1383044469.17867.8.camel@ted> <1383047127.25877.4.camel@ted> X-Mailer: Evolution 3.6.4-0ubuntu1 Mime-Version: 1.0 Cc: openembedded-core Subject: Re: [PATCH 3/5] image_types.bbclass: fix EXTRA_IMAGECMD_sum.jffs2 X-BeenThere: openembedded-core@lists.openembedded.org X-Mailman-Version: 2.1.12 Precedence: list List-Id: Patches and discussions about the oe-core layer List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 29 Oct 2013 15:33:42 -0000 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit On Tue, 2013-10-29 at 13:17 +0100, Andrea Adami wrote: > On Tue, Oct 29, 2013 at 12:45 PM, Richard Purdie > wrote: > > On Tue, 2013-10-29 at 12:17 +0100, Andrea Adami wrote: > >> On Tue, Oct 29, 2013 at 12:01 PM, Richard Purdie > >> wrote: > >> > On Mon, 2013-10-21 at 00:34 +0200, Andrea Adami wrote: > >> >> When overriding EXTRA_IMAGE_CMD_jffs2 = "--pad=foo ..." > >> >> we are passing a malformed option to sumtool: > >> >> > >> >> sumtool: option '--pad' doesn't allow an argument > >> >> > >> >> Fix this by declaring a separate variable for the purpose. > >> >> > >> >> Signed-off-by: Andrea Adami > >> >> --- > >> >> meta/classes/image_types.bbclass | 5 +++-- > >> >> 1 file changed, 3 insertions(+), 2 deletions(-) > >> >> > >> >> diff --git a/meta/classes/image_types.bbclass b/meta/classes/image_types.bbclass > >> >> index b8779e0..21391e8 100644 > >> >> --- a/meta/classes/image_types.bbclass > >> >> +++ b/meta/classes/image_types.bbclass > >> >> @@ -141,8 +141,8 @@ XZ_INTEGRITY_CHECK ?= "crc32" > >> >> XZ_THREADS ?= "-T 0" > >> >> > >> >> IMAGE_CMD_jffs2 = "mkfs.jffs2 --root=${IMAGE_ROOTFS} --faketime --output=${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.jffs2 ${EXTRA_IMAGECMD}" > >> >> -IMAGE_CMD_sum.jffs2 = "${IMAGE_CMD_jffs2} && sumtool -i ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.jffs2 \ > >> >> - -o ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.sum.jffs2 ${EXTRA_IMAGECMD}" > >> >> +IMAGE_CMD_sum.jffs2 = "mkfs.jffs2 --root=${IMAGE_ROOTFS} --faketime --output=${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.jffs2 ${EXTRA_IMAGECMD_jffs2} \ > >> >> + && sumtool -i ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.jffs2 -o ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.sum.jffs2 ${EXTRA_IMAGECMD}" > >> >> > >> >> IMAGE_CMD_cramfs = "mkfs.cramfs ${IMAGE_ROOTFS} ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.cramfs ${EXTRA_IMAGECMD}" > >> >> > >> >> @@ -212,6 +212,7 @@ inherit siteinfo > >> >> JFFS2_ENDIANNESS ?= "${@base_conditional('SITEINFO_ENDIANNESS', 'le', '-l', '-b', d)}" > >> >> JFFS2_ERASEBLOCK ?= "0x40000" > >> >> EXTRA_IMAGECMD_jffs2 ?= "--pad ${JFFS2_ENDIANNESS} --eraseblock=${JFFS2_ERASEBLOCK} --no-cleanmarkers" > >> >> +EXTRA_IMAGECMD_sum.jffs2 ?= "--pad ${JFFS2_ENDIANNESS} --eraseblock=${JFFS2_ERASEBLOCK} --no-cleanmarkers" > >> >> > >> >> # Change these if you want default mkfs behavior (i.e. create minimal inode number) > >> >> EXTRA_IMAGECMD_ext2 ?= "-i 8192" > >> > > >> > This patch is very confused. You say sumtool doesn't take a --pad > >> > option, yet "EXTRA_IMAGECMD_sum.jffs" which is presumably used with > >> > sumtool does have the option. > >> > > >> > >> After the commits you've done the creation of sum.jffs2 images is > >> broken on collie. > >> The error message is described clearly in the patch: > >> > >> sumtool: option '--pad' doesn't allow an argument > >> > >> Why that? Because collie needs to customize the padding. > >> EXTRA_IMAGECMD_jffs2 = "--pad=14680064 -l -e ${JFFS2_ERASEBLOCK}" > >> > >> This --pad=14680064 is then passed to IMAGE_CMD_sum.jffs2 and we get > >> build error. > >> > >> That's why we need to redefine both IMAGE_CMD_sum.jffs2 and > >> EXTRA_IMAGECMD_sum.jffs2 so to pass --pad=XY only to mkfs.jffs2 and > >> not to the sumtool part of IMAGE_CMD_sum.jffs2. > >> > >> --- > >> By the way this sum.jffs2 imagetype is used only in meta-handheld afaik. > >> It would be also ok if you'd remove IMAGE_CMD_sum.jffs2 so we can > >> append the whole stuff in a customized EXTRA_IMAGECMD_jffs2. > > > > I understand your problem however: > > > > a) Copy and pasting the entire IMAGE_CMD_jffs2 command into the second > > one is rather ugly. > Yes, but using the IMAGE_CMD_jffs2 we incorporate the > EXTRA_IMAGECMD_jffs2 = "--pad=xyz.." > > > b) You still end up passing "--pad ${JFFS2_ENDIANNESS} --eraseblock= > > ${JFFS2_ERASEBLOCK} --no-cleanmarkers" as parameters to sumtool, > > including a --pad option with no argument. Why/how? > > Because this padding is limited to the last eraseblock. > " -p, --pad Pad the OUTPUT with 0xFF to the end of the > final eraseblock" > > > > > You call: > > > > sumtool -i ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.jffs2 -o ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.sum.jffs2 ${EXTRA_IMAGECMD} > > > > and EXTRA_IMAGECMD is set to EXTRA_IMAGECMD_sum.jffs2 due to the use of > > overrides in the image generation code. > > We cannot reuse IMAGE_CMD_jffs2 because in that case > EXTRA_IMAGECMD_jffs2 is already evaluated and this contains the > infamous --pad==xy. > > > > > So whilst collie is broken, I do not think this patch makes readable > > code and I think we need to find something better. > > > > The thing I'm confused on is whether there should be any --pad option to > > sumtool or not. > > > > Yes, but without argument > > http://git.infradead.org/mtd-utils.git/blob/HEAD:/sumtool.c Ok, we need to make this clearer in the commit message. Looking and thinking about this further, how about we add some dependency ordering into this code, something like the patch below. This moves the jffs2 call into its own true namespace. You should then just be able to explicitly set EXTRA_IMGAGECMD_sum.jffs2 to the value you need (yet still have EXTRA_IMGAGECMD_jffs2 able to take a different value). See if you can build something on top of the patch below... Cheers, Richard >From 4ff883824221ce5f1b993239249b1ce1e3ab9b32 Mon Sep 17 00:00:00 2001 From: Richard Purdie Date: Tue, 29 Oct 2013 15:26:54 +0000 Subject: image_types: Improve dependency handling between types (and use for sum.jffs2) We're seeing a pattern of one image type needing to depend on another type. A good example is jffs2 and sum.jffs2. This patch makes sum.jffs2 depend on jffs2 which will then allow a EXTRA_IMGAGECMD to be set for sum.jffs2 individually without changing the jffs2 command. This allows the -pad option to be configured differently. Signed-off-by: Richard Purdie --- diff --git a/meta/classes/image_types.bbclass b/meta/classes/image_types.bbclass index b8779e0..5ce1ddb 100644 --- a/meta/classes/image_types.bbclass +++ b/meta/classes/image_types.bbclass @@ -7,14 +7,20 @@ def get_imagecmds(d): ctypes = d.getVar('COMPRESSIONTYPES', True).split() cimages = {} + # Image type b depends on a having been generated first + def addtypedepends(a, b): + if a in alltypes: + alltypes.remove(a): + if b not in alltypes: + alltypes.append(b) + alltypes.append(a) + # The elf image depends on the cpio.gz image already having # been created, so we add that explicit ordering here. + addtypedepends("elf", "cpio.gz") - if "elf" in alltypes: - alltypes.remove("elf") - if "cpio.gz" not in alltypes: - alltypes.append("cpio.gz") - alltypes.append("elf") + # jffs2 sumtool'd images need jffs2 + addtypedepends("sum.jffs2", "jffs2") # Filter out all the compressed images from alltypes for type in alltypes: @@ -141,8 +147,7 @@ XZ_INTEGRITY_CHECK ?= "crc32" XZ_THREADS ?= "-T 0" IMAGE_CMD_jffs2 = "mkfs.jffs2 --root=${IMAGE_ROOTFS} --faketime --output=${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.jffs2 ${EXTRA_IMAGECMD}" -IMAGE_CMD_sum.jffs2 = "${IMAGE_CMD_jffs2} && sumtool -i ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.jffs2 \ - -o ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.sum.jffs2 ${EXTRA_IMAGECMD}" +IMAGE_CMD_sum.jffs2 = "sumtool -i ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.jffs2 -o ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.sum.jffs2 ${EXTRA_IMAGECMD}" IMAGE_CMD_cramfs = "mkfs.cramfs ${IMAGE_ROOTFS} ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.cramfs ${EXTRA_IMAGECMD}"