* [PATCH] Revert "featimage: refactor style"
@ 2022-01-29 0:29 Marek Vasut
2022-01-29 1:06 ` [OE-core] " Peter Kjellerstedt
0 siblings, 1 reply; 10+ messages in thread
From: Marek Vasut @ 2022-01-29 0:29 UTC (permalink / raw)
To: openembedded-core; +Cc: Marek Vasut, Andrej Valek, Richard Purdie
This reverts commit f44bb458884da64356ee188917094b5515d3b159.
The reverted patch attempted to perform some sort of clean up, however
it only brought in style inconsistencies like this:
```
conf_desc="$conf_desc${sep}setup"
```
The curly brackets around variables were placed in the kernel-fitimage
bbclass deliberately, since when assembling the fitimage ITS there are
multiple variables where it is difficult to identify where the variable
ends and some sort of follow up string starts.
Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Andrej Valek <andrej.valek@siemens.com>
Cc: Richard Purdie <richard.purdie@linuxfoundation.org>
---
meta/classes/kernel-fitimage.bbclass | 289 +++++++++++++--------------
meta/classes/uboot-sign.bbclass | 56 +++---
2 files changed, 171 insertions(+), 174 deletions(-)
diff --git a/meta/classes/kernel-fitimage.bbclass b/meta/classes/kernel-fitimage.bbclass
index b0c971b0eb..1e3bc21f1f 100644
--- a/meta/classes/kernel-fitimage.bbclass
+++ b/meta/classes/kernel-fitimage.bbclass
@@ -73,7 +73,7 @@ FIT_SIGN_INDIVIDUAL ?= "0"
#
# $1 ... .its filename
fitimage_emit_fit_header() {
- cat << EOF >> $1
+ cat << EOF >> ${1}
/dts-v1/;
/ {
@@ -94,24 +94,24 @@ EOF
fitimage_emit_section_maint() {
case $2 in
imagestart)
- cat << EOF >> $1
+ cat << EOF >> ${1}
images {
EOF
;;
confstart)
- cat << EOF >> $1
+ cat << EOF >> ${1}
configurations {
EOF
;;
sectend)
- cat << EOF >> $1
+ cat << EOF >> ${1}
};
EOF
;;
fitend)
- cat << EOF >> $1
+ cat << EOF >> ${1}
};
EOF
;;
@@ -137,28 +137,28 @@ fitimage_emit_section_kernel() {
awk '$3=="${UBOOT_ENTRYSYMBOL}" {print "0x"$1;exit}'`
fi
- cat << EOF >> $1
- kernel-$2 {
+ cat << EOF >> ${1}
+ kernel-${2} {
description = "Linux kernel";
- data = /incbin/("$3");
+ data = /incbin/("${3}");
type = "kernel";
arch = "${UBOOT_ARCH}";
os = "linux";
- compression = "$4";
+ compression = "${4}";
load = <${UBOOT_LOADADDRESS}>;
- entry = <$ENTRYPOINT>;
+ entry = <${ENTRYPOINT}>;
hash-1 {
- algo = "$kernel_csum";
+ algo = "${kernel_csum}";
};
};
EOF
- if [ "${UBOOT_SIGN_ENABLE}" = "1" -a "${FIT_SIGN_INDIVIDUAL}" = "1" -a -n "$kernel_sign_keyname" ] ; then
- sed -i '$ d' $1
- cat << EOF >> $1
+ if [ "${UBOOT_SIGN_ENABLE}" = "1" -a "${FIT_SIGN_INDIVIDUAL}" = "1" -a -n "${kernel_sign_keyname}" ] ; then
+ sed -i '$ d' ${1}
+ cat << EOF >> ${1}
signature-1 {
- algo = "$kernel_csum,$kernel_sign_algo";
- key-name-hint = "$kernel_sign_keyname";
+ algo = "${kernel_csum},${kernel_sign_algo}";
+ key-name-hint = "${kernel_sign_keyname}";
};
};
EOF
@@ -186,26 +186,26 @@ fitimage_emit_section_dtb() {
elif [ -n "${UBOOT_DTB_LOADADDRESS}" ]; then
dtb_loadline="load = <${UBOOT_DTB_LOADADDRESS}>;"
fi
- cat << EOF >> $1
- fdt-$2 {
+ cat << EOF >> ${1}
+ fdt-${2} {
description = "Flattened Device Tree blob";
- data = /incbin/("$3");
+ data = /incbin/("${3}");
type = "flat_dt";
arch = "${UBOOT_ARCH}";
compression = "none";
- $dtb_loadline
+ ${dtb_loadline}
hash-1 {
- algo = "$dtb_csum";
+ algo = "${dtb_csum}";
};
};
EOF
- if [ "${UBOOT_SIGN_ENABLE}" = "1" -a "${FIT_SIGN_INDIVIDUAL}" = "1" -a -n "$dtb_sign_keyname" ] ; then
- sed -i '$ d' $1
- cat << EOF >> $1
+ if [ "${UBOOT_SIGN_ENABLE}" = "1" -a "${FIT_SIGN_INDIVIDUAL}" = "1" -a -n "${dtb_sign_keyname}" ] ; then
+ sed -i '$ d' ${1}
+ cat << EOF >> ${1}
signature-1 {
- algo = "$dtb_csum,$dtb_sign_algo";
- key-name-hint = "$dtb_sign_keyname";
+ algo = "${dtb_csum},${dtb_sign_algo}";
+ key-name-hint = "${dtb_sign_keyname}";
};
};
EOF
@@ -220,29 +220,29 @@ EOF
# $3 ... Path to boot script image
fitimage_emit_section_boot_script() {
- bootscr_csum="${FIT_HASH_ALG}"
+ bootscr_csum="${FIT_HASH_ALG}"
bootscr_sign_algo="${FIT_SIGN_ALG}"
bootscr_sign_keyname="${UBOOT_SIGN_IMG_KEYNAME}"
- cat << EOF >> $1
- bootscr-$2 {
+ cat << EOF >> ${1}
+ bootscr-${2} {
description = "U-boot script";
- data = /incbin/("$3");
+ data = /incbin/("${3}");
type = "script";
arch = "${UBOOT_ARCH}";
compression = "none";
hash-1 {
- algo = "$bootscr_csum";
+ algo = "${bootscr_csum}";
};
};
EOF
- if [ "${UBOOT_SIGN_ENABLE}" = "1" -a "${FIT_SIGN_INDIVIDUAL}" = "1" -a -n "$bootscr_sign_keyname" ] ; then
- sed -i '$ d' $1
- cat << EOF >> $1
+ if [ "${UBOOT_SIGN_ENABLE}" = "1" -a "${FIT_SIGN_INDIVIDUAL}" = "1" -a -n "${bootscr_sign_keyname}" ] ; then
+ sed -i '$ d' ${1}
+ cat << EOF >> ${1}
signature-1 {
- algo = "$bootscr_csum,$bootscr_sign_algo";
- key-name-hint = "$bootscr_sign_keyname";
+ algo = "${bootscr_csum},${bootscr_sign_algo}";
+ key-name-hint = "${bootscr_sign_keyname}";
};
};
EOF
@@ -259,10 +259,10 @@ fitimage_emit_section_setup() {
setup_csum="${FIT_HASH_ALG}"
- cat << EOF >> $1
- setup-$2 {
+ cat << EOF >> ${1}
+ setup-${2} {
description = "Linux setup.bin";
- data = /incbin/("$3");
+ data = /incbin/("${3}");
type = "x86_setup";
arch = "${UBOOT_ARCH}";
os = "linux";
@@ -270,7 +270,7 @@ fitimage_emit_section_setup() {
load = <0x00090000>;
entry = <0x00090000>;
hash-1 {
- algo = "$setup_csum";
+ algo = "${setup_csum}";
};
};
EOF
@@ -297,28 +297,28 @@ fitimage_emit_section_ramdisk() {
ramdisk_entryline="entry = <${UBOOT_RD_ENTRYPOINT}>;"
fi
- cat << EOF >> $1
- ramdisk-$2 {
+ cat << EOF >> ${1}
+ ramdisk-${2} {
description = "${INITRAMFS_IMAGE}";
- data = /incbin/("$3");
+ data = /incbin/("${3}");
type = "ramdisk";
arch = "${UBOOT_ARCH}";
os = "linux";
compression = "none";
- $ramdisk_loadline
- $ramdisk_entryline
+ ${ramdisk_loadline}
+ ${ramdisk_entryline}
hash-1 {
- algo = "$ramdisk_csum";
+ algo = "${ramdisk_csum}";
};
};
EOF
- if [ "${UBOOT_SIGN_ENABLE}" = "1" -a "${FIT_SIGN_INDIVIDUAL}" = "1" -a -n "$ramdisk_sign_keyname" ] ; then
- sed -i '$ d' $1
- cat << EOF >> $1
+ if [ "${UBOOT_SIGN_ENABLE}" = "1" -a "${FIT_SIGN_INDIVIDUAL}" = "1" -a -n "${ramdisk_sign_keyname}" ] ; then
+ sed -i '$ d' ${1}
+ cat << EOF >> ${1}
signature-1 {
- algo = "$ramdisk_csum,$ramdisk_sign_algo";
- key-name-hint = "$ramdisk_sign_keyname";
+ algo = "${ramdisk_csum},${ramdisk_sign_algo}";
+ key-name-hint = "${ramdisk_sign_keyname}";
};
};
EOF
@@ -343,13 +343,13 @@ fitimage_emit_section_config() {
conf_sign_keyname="${UBOOT_SIGN_KEYNAME}"
fi
- its_file="$1"
- kernel_id="$2"
- dtb_image="$3"
- ramdisk_id="$4"
- bootscr_id="$5"
- config_id="$6"
- default_flag="$7"
+ its_file="${1}"
+ kernel_id="${2}"
+ dtb_image="${3}"
+ ramdisk_id="${4}"
+ bootscr_id="${5}"
+ config_id="${6}"
+ default_flag="${7}"
# Test if we have any DTBs at all
sep=""
@@ -364,106 +364,106 @@ fitimage_emit_section_config() {
# conf node name is selected based on dtb ID if it is present,
# otherwise its selected based on kernel ID
- if [ -n "$dtb_image" ]; then
- conf_node=$conf_node$dtb_image
+ if [ -n "${dtb_image}" ]; then
+ conf_node=$conf_node${dtb_image}
else
- conf_node=$conf_node$kernel_id
+ conf_node=$conf_node${kernel_id}
fi
- if [ -n "$kernel_id" ]; then
+ if [ -n "${kernel_id}" ]; then
conf_desc="Linux kernel"
sep=", "
- kernel_line="kernel = \"kernel-$kernel_id\";"
+ kernel_line="kernel = \"kernel-${kernel_id}\";"
fi
- if [ -n "$dtb_image" ]; then
- conf_desc="$conf_desc${sep}FDT blob"
+ if [ -n "${dtb_image}" ]; then
+ conf_desc="${conf_desc}${sep}FDT blob"
sep=", "
- fdt_line="fdt = \"fdt-$dtb_image\";"
+ fdt_line="fdt = \"fdt-${dtb_image}\";"
fi
- if [ -n "$ramdisk_id" ]; then
- conf_desc="$conf_desc${sep}ramdisk"
+ if [ -n "${ramdisk_id}" ]; then
+ conf_desc="${conf_desc}${sep}ramdisk"
sep=", "
- ramdisk_line="ramdisk = \"ramdisk-$ramdisk_id\";"
+ ramdisk_line="ramdisk = \"ramdisk-${ramdisk_id}\";"
fi
- if [ -n "$bootscr_id" ]; then
- conf_desc="$conf_desc${sep}u-boot script"
+ if [ -n "${bootscr_id}" ]; then
+ conf_desc="${conf_desc}${sep}u-boot script"
sep=", "
- bootscr_line="bootscr = \"bootscr-$bootscr_id\";"
+ bootscr_line="bootscr = \"bootscr-${bootscr_id}\";"
fi
- if [ -n "$config_id" ]; then
- conf_desc="$conf_desc${sep}setup"
- setup_line="setup = \"setup-$config_id\";"
+ if [ -n "${config_id}" ]; then
+ conf_desc="${conf_desc}${sep}setup"
+ setup_line="setup = \"setup-${config_id}\";"
fi
- if [ "$default_flag" = "1" ]; then
+ if [ "${default_flag}" = "1" ]; then
# default node is selected based on dtb ID if it is present,
# otherwise its selected based on kernel ID
- if [ -n "$dtb_image" ]; then
- default_line="default = \"conf-$dtb_image\";"
+ if [ -n "${dtb_image}" ]; then
+ default_line="default = \"conf-${dtb_image}\";"
else
- default_line="default = \"conf-$kernel_id\";"
+ default_line="default = \"conf-${kernel_id}\";"
fi
fi
- cat << EOF >> $its_file
- $default_line
+ cat << EOF >> ${its_file}
+ ${default_line}
$conf_node {
- description = "$default_flag $conf_desc";
- $kernel_line
- $fdt_line
- $ramdisk_line
- $bootscr_line
- $setup_line
+ description = "${default_flag} ${conf_desc}";
+ ${kernel_line}
+ ${fdt_line}
+ ${ramdisk_line}
+ ${bootscr_line}
+ ${setup_line}
hash-1 {
- algo = "$conf_csum";
+ algo = "${conf_csum}";
};
EOF
- if [ -n "$conf_sign_keyname" ] ; then
+ if [ ! -z "${conf_sign_keyname}" ] ; then
sign_line="sign-images = "
sep=""
- if [ -n "$kernel_id" ]; then
- sign_line="$sign_line${sep}\"kernel\""
+ if [ -n "${kernel_id}" ]; then
+ sign_line="${sign_line}${sep}\"kernel\""
sep=", "
fi
- if [ -n "$dtb_image" ]; then
- sign_line="$sign_line${sep}\"fdt\""
+ if [ -n "${dtb_image}" ]; then
+ sign_line="${sign_line}${sep}\"fdt\""
sep=", "
fi
- if [ -n "$ramdisk_id" ]; then
- sign_line="$sign_line${sep}\"ramdisk\""
+ if [ -n "${ramdisk_id}" ]; then
+ sign_line="${sign_line}${sep}\"ramdisk\""
sep=", "
fi
- if [ -n "$bootscr_id" ]; then
- sign_line="$sign_line${sep}\"bootscr\""
+ if [ -n "${bootscr_id}" ]; then
+ sign_line="${sign_line}${sep}\"bootscr\""
sep=", "
fi
- if [ -n "$config_id" ]; then
- sign_line="$sign_line${sep}\"setup\""
+ if [ -n "${config_id}" ]; then
+ sign_line="${sign_line}${sep}\"setup\""
fi
- sign_line="$sign_line;"
+ sign_line="${sign_line};"
- cat << EOF >> $its_file
+ cat << EOF >> ${its_file}
signature-1 {
- algo = "$conf_csum,$conf_sign_algo";
- key-name-hint = "$conf_sign_keyname";
- $sign_line
+ algo = "${conf_csum},${conf_sign_algo}";
+ key-name-hint = "${conf_sign_keyname}";
+ ${sign_line}
};
EOF
fi
- cat << EOF >> $its_file
+ cat << EOF >> ${its_file}
};
EOF
}
@@ -478,21 +478,21 @@ fitimage_assemble() {
kernelcount=1
dtbcount=""
DTBS=""
- ramdiskcount=$3
+ ramdiskcount=${3}
setupcount=""
bootscr_id=""
- rm -f $1 arch/${ARCH}/boot/$2
+ rm -f ${1} arch/${ARCH}/boot/${2}
- if [ -n "${UBOOT_SIGN_IMG_KEYNAME}" -a "${UBOOT_SIGN_KEYNAME}" = "${UBOOT_SIGN_IMG_KEYNAME}" ]; then
+ if [ ! -z "${UBOOT_SIGN_IMG_KEYNAME}" -a "${UBOOT_SIGN_KEYNAME}" = "${UBOOT_SIGN_IMG_KEYNAME}" ]; then
bbfatal "Keys used to sign images and configuration nodes must be different."
fi
- fitimage_emit_fit_header $1
+ fitimage_emit_fit_header ${1}
#
# Step 1: Prepare a kernel image section.
#
- fitimage_emit_section_maint $1 imagestart
+ fitimage_emit_section_maint ${1} imagestart
uboot_prep_kimage
fitimage_emit_section_kernel $1 $kernelcount linux.bin "$linux_comp"
@@ -504,9 +504,9 @@ fitimage_assemble() {
if [ -n "${KERNEL_DEVICETREE}" ]; then
dtbcount=1
for DTB in ${KERNEL_DEVICETREE}; do
- if echo $DTB | grep -q '/dts/'; then
- bbwarn "$DTB contains the full path to the the dts file, but only the dtb name should be used."
- DTB=`basename $DTB | sed 's,\.dts$,.dtb,g'`
+ if echo ${DTB} | grep -q '/dts/'; then
+ bbwarn "${DTB} contains the full path to the the dts file, but only the dtb name should be used."
+ DTB=`basename ${DTB} | sed 's,\.dts$,.dtb,g'`
fi
# Skip ${DTB} if it's also provided in ${EXTERNAL_KERNEL_DEVICETREE}
@@ -514,23 +514,23 @@ fitimage_assemble() {
continue
fi
- DTB_PATH="arch/${ARCH}/boot/dts/$DTB"
- if [ ! -e "$DTB_PATH" ]; then
- DTB_PATH="arch/${ARCH}/boot/$DTB"
+ DTB_PATH="arch/${ARCH}/boot/dts/${DTB}"
+ if [ ! -e "${DTB_PATH}" ]; then
+ DTB_PATH="arch/${ARCH}/boot/${DTB}"
fi
- DTB=$(echo "$DTB" | tr '/' '_')
- DTBS="$DTBS $DTB"
- fitimage_emit_section_dtb $1 $DTB $DTB_PATH
+ DTB=$(echo "${DTB}" | tr '/' '_')
+ DTBS="${DTBS} ${DTB}"
+ fitimage_emit_section_dtb ${1} ${DTB} ${DTB_PATH}
done
fi
if [ -n "${EXTERNAL_KERNEL_DEVICETREE}" ]; then
dtbcount=1
for DTB in $(find "${EXTERNAL_KERNEL_DEVICETREE}" \( -name '*.dtb' -o -name '*.dtbo' \) -printf '%P\n' | sort); do
- DTB=$(echo "$DTB" | tr '/' '_')
- DTBS="$DTBS $DTB"
- fitimage_emit_section_dtb $1 $DTB "${EXTERNAL_KERNEL_DEVICETREE}/$DTB"
+ DTB=$(echo "${DTB}" | tr '/' '_')
+ DTBS="${DTBS} ${DTB}"
+ fitimage_emit_section_dtb ${1} ${DTB} "${EXTERNAL_KERNEL_DEVICETREE}/${DTB}"
done
fi
@@ -542,7 +542,7 @@ fitimage_assemble() {
if [ -e "${STAGING_DIR_HOST}/boot/${UBOOT_ENV_BINARY}" ]; then
cp ${STAGING_DIR_HOST}/boot/${UBOOT_ENV_BINARY} ${B}
bootscr_id="${UBOOT_ENV_BINARY}"
- fitimage_emit_section_boot_script $1 "$bootscr_id" ${UBOOT_ENV_BINARY}
+ fitimage_emit_section_boot_script ${1} "${bootscr_id}" ${UBOOT_ENV_BINARY}
else
bbwarn "${STAGING_DIR_HOST}/boot/${UBOOT_ENV_BINARY} not found."
fi
@@ -553,7 +553,7 @@ fitimage_assemble() {
#
if [ -e arch/${ARCH}/boot/setup.bin ]; then
setupcount=1
- fitimage_emit_section_setup $1 $setupcount arch/${ARCH}/boot/setup.bin
+ fitimage_emit_section_setup ${1} "${setupcount}" arch/${ARCH}/boot/setup.bin
fi
#
@@ -562,30 +562,27 @@ fitimage_assemble() {
if [ "x${ramdiskcount}" = "x1" ] && [ "${INITRAMFS_IMAGE_BUNDLE}" != "1" ]; then
# Find and use the first initramfs image archive type we find
for img in cpio.lz4 cpio.lzo cpio.lzma cpio.xz cpio.zst cpio.gz ext2.gz cpio; do
- initramfs_path="${DEPLOY_DIR_IMAGE}/${INITRAMFS_IMAGE_NAME}.$img"
- echo -n "Searching for $initramfs_path..."
- if [ -e "$initramfs_path" ]; then
- echo "found"
- fitimage_emit_section_ramdisk $1 "$ramdiskcount" "$initramfs_path"
+ initramfs_path="${DEPLOY_DIR_IMAGE}/${INITRAMFS_IMAGE_NAME}.${img}"
+ echo "Using $initramfs_path"
+ if [ -e "${initramfs_path}" ]; then
+ fitimage_emit_section_ramdisk ${1} "${ramdiskcount}" "${initramfs_path}"
break
- else
- echo "not found"
fi
done
fi
- fitimage_emit_section_maint $1 sectend
+ fitimage_emit_section_maint ${1} sectend
# Force the first Kernel and DTB in the default config
kernelcount=1
- if [ -n "$dtbcount" ]; then
+ if [ -n "${dtbcount}" ]; then
dtbcount=1
fi
#
# Step 6: Prepare a configurations section
#
- fitimage_emit_section_maint $1 confstart
+ fitimage_emit_section_maint ${1} confstart
# kernel-fitimage.bbclass currently only supports a single kernel (no less or
# more) to be added to the FIT image along with 0 or more device trees and
@@ -596,33 +593,33 @@ fitimage_assemble() {
# the default configuration to be used is based on the dtbcount. If there is
# no dtb present than select the default configuation to be based on
# the kernelcount.
- if [ -n "$DTBS" ]; then
+ if [ -n "${DTBS}" ]; then
i=1
for DTB in ${DTBS}; do
dtb_ext=${DTB##*.}
- if [ "$dtb_ext" = "dtbo" ]; then
- fitimage_emit_section_config $1 "" "$DTB" "" "$bootscr_id" "" "`expr $i = $dtbcount`"
+ if [ "${dtb_ext}" = "dtbo" ]; then
+ fitimage_emit_section_config ${1} "" "${DTB}" "" "${bootscr_id}" "" "`expr ${i} = ${dtbcount}`"
else
- fitimage_emit_section_config $1 $kernelcount "$DTB" "$ramdiskcount" "$bootscr_id" "$setupcount" "`expr $i = $dtbcount`"
+ fitimage_emit_section_config ${1} "${kernelcount}" "${DTB}" "${ramdiskcount}" "${bootscr_id}" "${setupcount}" "`expr ${i} = ${dtbcount}`"
fi
- i=`expr $i + 1`
+ i=`expr ${i} + 1`
done
else
defaultconfigcount=1
- fitimage_emit_section_config $1 $kernelcount "" "$ramdiskcount" "$bootscr_id" "$setupcount" $defaultconfigcount
+ fitimage_emit_section_config ${1} "${kernelcount}" "" "${ramdiskcount}" "${bootscr_id}" "${setupcount}" "${defaultconfigcount}"
fi
- fitimage_emit_section_maint $1 sectend
+ fitimage_emit_section_maint ${1} sectend
- fitimage_emit_section_maint $1 fitend
+ fitimage_emit_section_maint ${1} fitend
#
# Step 7: Assemble the image
#
${UBOOT_MKIMAGE} \
${@'-D "${UBOOT_MKIMAGE_DTCOPTS}"' if len('${UBOOT_MKIMAGE_DTCOPTS}') else ''} \
- -f $1 \
- arch/${ARCH}/boot/$2
+ -f ${1} \
+ arch/${ARCH}/boot/${2}
#
# Step 8: Sign the image and add public key to U-Boot dtb
@@ -639,7 +636,7 @@ fitimage_assemble() {
${@'-D "${UBOOT_MKIMAGE_DTCOPTS}"' if len('${UBOOT_MKIMAGE_DTCOPTS}') else ''} \
-F -k "${UBOOT_SIGN_KEYDIR}" \
$add_key_to_u_boot \
- -r arch/${ARCH}/boot/$2 \
+ -r arch/${ARCH}/boot/${2} \
${UBOOT_MKIMAGE_SIGN_ARGS}
fi
}
@@ -647,7 +644,7 @@ fitimage_assemble() {
do_assemble_fitimage() {
if echo ${KERNEL_IMAGETYPES} | grep -wq "fitImage"; then
cd ${B}
- fitimage_assemble fit-image.its fitImage ""
+ fitimage_assemble fit-image.its fitImage
fi
}
diff --git a/meta/classes/uboot-sign.bbclass b/meta/classes/uboot-sign.bbclass
index 8d136e9405..c39b30f43b 100644
--- a/meta/classes/uboot-sign.bbclass
+++ b/meta/classes/uboot-sign.bbclass
@@ -176,8 +176,8 @@ concat_dtb() {
mkdir -p ${DEPLOYDIR}
if [ -n "${UBOOT_CONFIG}" ]; then
for config in ${UBOOT_MACHINE}; do
- CONFIG_B_PATH="$config"
- cd ${B}/$config
+ CONFIG_B_PATH="${config}"
+ cd ${B}/${config}
concat_dtb_helper
done
else
@@ -193,8 +193,8 @@ concat_spl_dtb() {
mkdir -p ${DEPLOYDIR}
if [ -n "${UBOOT_CONFIG}" ]; then
for config in ${UBOOT_MACHINE}; do
- CONFIG_B_PATH="$config"
- cd ${B}/$config
+ CONFIG_B_PATH="${config}"
+ cd ${B}/${config}
concat_spl_dtb_helper
done
else
@@ -245,7 +245,7 @@ do_install:append() {
if [ "${PN}" = "${UBOOT_PN}" ]; then
if [ -n "${UBOOT_CONFIG}" ]; then
for config in ${UBOOT_MACHINE}; do
- cd ${B}/$config
+ cd ${B}/${config}
if [ "${UBOOT_SIGN_ENABLE}" = "1" -o "${UBOOT_FITIMAGE_ENABLE}" = "1" ] && \
[ -n "${UBOOT_DTB_BINARY}" ]; then
install_helper
@@ -300,19 +300,19 @@ addtask uboot_generate_rsa_keys before do_uboot_assemble_fitimage after do_compi
# Create a ITS file for the U-boot FIT, for use when
# we want to sign it so that the SPL can verify it
uboot_fitimage_assemble() {
- uboot_its="$1"
- uboot_nodtb_bin="$2"
- uboot_dtb="$3"
- uboot_bin="$4"
- spl_dtb="$5"
+ uboot_its="${1}"
+ uboot_nodtb_bin="${2}"
+ uboot_dtb="${3}"
+ uboot_bin="${4}"
+ spl_dtb="${5}"
uboot_csum="${UBOOT_FIT_HASH_ALG}"
uboot_sign_algo="${UBOOT_FIT_SIGN_ALG}"
uboot_sign_keyname="${SPL_SIGN_KEYNAME}"
- rm -f $uboot_its $uboot_bin
+ rm -f ${uboot_its} ${uboot_bin}
# First we create the ITS script
- cat << EOF >> $uboot_its
+ cat << EOF >> ${uboot_its}
/dts-v1/;
/ {
@@ -322,7 +322,7 @@ uboot_fitimage_assemble() {
images {
uboot {
description = "U-Boot image";
- data = /incbin/("$uboot_nodtb_bin");
+ data = /incbin/("${uboot_nodtb_bin}");
type = "standalone";
os = "u-boot";
arch = "${UBOOT_ARCH}";
@@ -332,34 +332,34 @@ uboot_fitimage_assemble() {
EOF
if [ "${SPL_SIGN_ENABLE}" = "1" ] ; then
- cat << EOF >> $uboot_its
+ cat << EOF >> ${uboot_its}
signature {
- algo = "$uboot_csum,$uboot_sign_algo";
- key-name-hint = "$uboot_sign_keyname";
+ algo = "${uboot_csum},${uboot_sign_algo}";
+ key-name-hint = "${uboot_sign_keyname}";
};
EOF
fi
- cat << EOF >> $uboot_its
+ cat << EOF >> ${uboot_its}
};
fdt {
description = "U-Boot FDT";
- data = /incbin/("$uboot_dtb");
+ data = /incbin/("${uboot_dtb}");
type = "flat_dt";
arch = "${UBOOT_ARCH}";
compression = "none";
EOF
if [ "${SPL_SIGN_ENABLE}" = "1" ] ; then
- cat << EOF >> $uboot_its
+ cat << EOF >> ${uboot_its}
signature {
- algo = "$uboot_csum,$uboot_sign_algo";
- key-name-hint = "$uboot_sign_keyname";
+ algo = "${uboot_csum},${uboot_sign_algo}";
+ key-name-hint = "${uboot_sign_keyname}";
};
EOF
fi
- cat << EOF >> $uboot_its
+ cat << EOF >> ${uboot_its}
};
};
@@ -379,8 +379,8 @@ EOF
#
${UBOOT_MKIMAGE} \
${@'-D "${SPL_MKIMAGE_DTCOPTS}"' if len('${SPL_MKIMAGE_DTCOPTS}') else ''} \
- -f $uboot_its \
- $uboot_bin
+ -f ${uboot_its} \
+ ${uboot_bin}
if [ "${SPL_SIGN_ENABLE}" = "1" ] ; then
#
@@ -389,8 +389,8 @@ EOF
${UBOOT_MKIMAGE_SIGN} \
${@'-D "${SPL_MKIMAGE_DTCOPTS}"' if len('${SPL_MKIMAGE_DTCOPTS}') else ''} \
-F -k "${SPL_SIGN_KEYDIR}" \
- -K "$spl_dtb" \
- -r $uboot_bin \
+ -K "${spl_dtb}" \
+ -r ${uboot_bin} \
${SPL_MKIMAGE_SIGN_ARGS}
fi
@@ -422,8 +422,8 @@ do_uboot_assemble_fitimage() {
kernel_uboot_fitimage_name=`basename ${STAGING_DATADIR}/u-boot-fitImage-*`
kernel_uboot_its_name=`basename ${STAGING_DATADIR}/u-boot-its-*`
cd ${B}
- uboot_fitimage_assemble $kernel_uboot_its_name ${UBOOT_NODTB_BINARY} \
- ${UBOOT_DTB_BINARY} $kernel_uboot_fitimage_name \
+ uboot_fitimage_assemble ${kernel_uboot_its_name} ${UBOOT_NODTB_BINARY} \
+ ${UBOOT_DTB_BINARY} ${kernel_uboot_fitimage_name} \
${SPL_DTB_BINARY}
fi
}
--
2.34.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* RE: [OE-core] [PATCH] Revert "featimage: refactor style"
2022-01-29 0:29 [PATCH] Revert "featimage: refactor style" Marek Vasut
@ 2022-01-29 1:06 ` Peter Kjellerstedt
2022-01-29 1:38 ` Marek Vasut
0 siblings, 1 reply; 10+ messages in thread
From: Peter Kjellerstedt @ 2022-01-29 1:06 UTC (permalink / raw)
To: Marek Vasut, openembedded-core; +Cc: Andrej Valek, Richard Purdie
> -----Original Message-----
> From: openembedded-core@lists.openembedded.org <openembedded-
> core@lists.openembedded.org> On Behalf Of Marek Vasut
> Sent: den 29 januari 2022 01:29
> To: openembedded-core@lists.openembedded.org
> Cc: Marek Vasut <marex@denx.de>; Andrej Valek <andrej.valek@siemens.com>;
> Richard Purdie <richard.purdie@linuxfoundation.org>
> Subject: [OE-core] [PATCH] Revert "featimage: refactor style"
>
> This reverts commit f44bb458884da64356ee188917094b5515d3b159.
>
> The reverted patch attempted to perform some sort of clean up, however
> it only brought in style inconsistencies like this:
>
> ```
> conf_desc="$conf_desc${sep}setup"
> ```
>
> The curly brackets around variables were placed in the kernel-fitimage
> bbclass deliberately, since when assembling the fitimage ITS there are
> multiple variables where it is difficult to identify where the variable
> ends and some sort of follow up string starts.
There is actually a technical reason to not use ${foo} for shell
variables unless necessary in bitbake files and it is because
bitbake will treat them all as potential bitbake variables. This
means they are unnecessarily included in the taskhashes that
bitbake calculates.
//Peter
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Andrej Valek <andrej.valek@siemens.com>
> Cc: Richard Purdie <richard.purdie@linuxfoundation.org>
> ---
> meta/classes/kernel-fitimage.bbclass | 289 +++++++++++++--------------
> meta/classes/uboot-sign.bbclass | 56 +++---
> 2 files changed, 171 insertions(+), 174 deletions(-)
>
> diff --git a/meta/classes/kernel-fitimage.bbclass b/meta/classes/kernel-
> fitimage.bbclass
> index b0c971b0eb..1e3bc21f1f 100644
> --- a/meta/classes/kernel-fitimage.bbclass
> +++ b/meta/classes/kernel-fitimage.bbclass
> @@ -73,7 +73,7 @@ FIT_SIGN_INDIVIDUAL ?= "0"
> #
> # $1 ... .its filename
> fitimage_emit_fit_header() {
> - cat << EOF >> $1
> + cat << EOF >> ${1}
> /dts-v1/;
>
> / {
> @@ -94,24 +94,24 @@ EOF
> fitimage_emit_section_maint() {
> case $2 in
> imagestart)
> - cat << EOF >> $1
> + cat << EOF >> ${1}
>
> images {
> EOF
> ;;
> confstart)
> - cat << EOF >> $1
> + cat << EOF >> ${1}
>
> configurations {
> EOF
> ;;
> sectend)
> - cat << EOF >> $1
> + cat << EOF >> ${1}
> };
> EOF
> ;;
> fitend)
> - cat << EOF >> $1
> + cat << EOF >> ${1}
> };
> EOF
> ;;
> @@ -137,28 +137,28 @@ fitimage_emit_section_kernel() {
> awk '$3=="${UBOOT_ENTRYSYMBOL}" {print
> "0x"$1;exit}'`
> fi
>
> - cat << EOF >> $1
> - kernel-$2 {
> + cat << EOF >> ${1}
> + kernel-${2} {
> description = "Linux kernel";
> - data = /incbin/("$3");
> + data = /incbin/("${3}");
> type = "kernel";
> arch = "${UBOOT_ARCH}";
> os = "linux";
> - compression = "$4";
> + compression = "${4}";
> load = <${UBOOT_LOADADDRESS}>;
> - entry = <$ENTRYPOINT>;
> + entry = <${ENTRYPOINT}>;
> hash-1 {
> - algo = "$kernel_csum";
> + algo = "${kernel_csum}";
> };
> };
> EOF
>
> - if [ "${UBOOT_SIGN_ENABLE}" = "1" -a "${FIT_SIGN_INDIVIDUAL}" =
> "1" -a -n "$kernel_sign_keyname" ] ; then
> - sed -i '$ d' $1
> - cat << EOF >> $1
> + if [ "${UBOOT_SIGN_ENABLE}" = "1" -a "${FIT_SIGN_INDIVIDUAL}" =
> "1" -a -n "${kernel_sign_keyname}" ] ; then
> + sed -i '$ d' ${1}
> + cat << EOF >> ${1}
> signature-1 {
> - algo = "$kernel_csum,$kernel_sign_algo";
> - key-name-hint = "$kernel_sign_keyname";
> + algo =
> "${kernel_csum},${kernel_sign_algo}";
> + key-name-hint = "${kernel_sign_keyname}";
> };
> };
> EOF
> @@ -186,26 +186,26 @@ fitimage_emit_section_dtb() {
> elif [ -n "${UBOOT_DTB_LOADADDRESS}" ]; then
> dtb_loadline="load = <${UBOOT_DTB_LOADADDRESS}>;"
> fi
> - cat << EOF >> $1
> - fdt-$2 {
> + cat << EOF >> ${1}
> + fdt-${2} {
> description = "Flattened Device Tree blob";
> - data = /incbin/("$3");
> + data = /incbin/("${3}");
> type = "flat_dt";
> arch = "${UBOOT_ARCH}";
> compression = "none";
> - $dtb_loadline
> + ${dtb_loadline}
> hash-1 {
> - algo = "$dtb_csum";
> + algo = "${dtb_csum}";
> };
> };
> EOF
>
> - if [ "${UBOOT_SIGN_ENABLE}" = "1" -a "${FIT_SIGN_INDIVIDUAL}" =
> "1" -a -n "$dtb_sign_keyname" ] ; then
> - sed -i '$ d' $1
> - cat << EOF >> $1
> + if [ "${UBOOT_SIGN_ENABLE}" = "1" -a "${FIT_SIGN_INDIVIDUAL}" =
> "1" -a -n "${dtb_sign_keyname}" ] ; then
> + sed -i '$ d' ${1}
> + cat << EOF >> ${1}
> signature-1 {
> - algo = "$dtb_csum,$dtb_sign_algo";
> - key-name-hint = "$dtb_sign_keyname";
> + algo = "${dtb_csum},${dtb_sign_algo}";
> + key-name-hint = "${dtb_sign_keyname}";
> };
> };
> EOF
> @@ -220,29 +220,29 @@ EOF
> # $3 ... Path to boot script image
> fitimage_emit_section_boot_script() {
>
> - bootscr_csum="${FIT_HASH_ALG}"
> + bootscr_csum="${FIT_HASH_ALG}"
> bootscr_sign_algo="${FIT_SIGN_ALG}"
> bootscr_sign_keyname="${UBOOT_SIGN_IMG_KEYNAME}"
>
> - cat << EOF >> $1
> - bootscr-$2 {
> + cat << EOF >> ${1}
> + bootscr-${2} {
> description = "U-boot script";
> - data = /incbin/("$3");
> + data = /incbin/("${3}");
> type = "script";
> arch = "${UBOOT_ARCH}";
> compression = "none";
> hash-1 {
> - algo = "$bootscr_csum";
> + algo = "${bootscr_csum}";
> };
> };
> EOF
>
> - if [ "${UBOOT_SIGN_ENABLE}" = "1" -a "${FIT_SIGN_INDIVIDUAL}" =
> "1" -a -n "$bootscr_sign_keyname" ] ; then
> - sed -i '$ d' $1
> - cat << EOF >> $1
> + if [ "${UBOOT_SIGN_ENABLE}" = "1" -a "${FIT_SIGN_INDIVIDUAL}" =
> "1" -a -n "${bootscr_sign_keyname}" ] ; then
> + sed -i '$ d' ${1}
> + cat << EOF >> ${1}
> signature-1 {
> - algo =
> "$bootscr_csum,$bootscr_sign_algo";
> - key-name-hint = "$bootscr_sign_keyname";
> + algo =
> "${bootscr_csum},${bootscr_sign_algo}";
> + key-name-hint =
> "${bootscr_sign_keyname}";
> };
> };
> EOF
> @@ -259,10 +259,10 @@ fitimage_emit_section_setup() {
>
> setup_csum="${FIT_HASH_ALG}"
>
> - cat << EOF >> $1
> - setup-$2 {
> + cat << EOF >> ${1}
> + setup-${2} {
> description = "Linux setup.bin";
> - data = /incbin/("$3");
> + data = /incbin/("${3}");
> type = "x86_setup";
> arch = "${UBOOT_ARCH}";
> os = "linux";
> @@ -270,7 +270,7 @@ fitimage_emit_section_setup() {
> load = <0x00090000>;
> entry = <0x00090000>;
> hash-1 {
> - algo = "$setup_csum";
> + algo = "${setup_csum}";
> };
> };
> EOF
> @@ -297,28 +297,28 @@ fitimage_emit_section_ramdisk() {
> ramdisk_entryline="entry =
> <${UBOOT_RD_ENTRYPOINT}>;"
> fi
>
> - cat << EOF >> $1
> - ramdisk-$2 {
> + cat << EOF >> ${1}
> + ramdisk-${2} {
> description = "${INITRAMFS_IMAGE}";
> - data = /incbin/("$3");
> + data = /incbin/("${3}");
> type = "ramdisk";
> arch = "${UBOOT_ARCH}";
> os = "linux";
> compression = "none";
> - $ramdisk_loadline
> - $ramdisk_entryline
> + ${ramdisk_loadline}
> + ${ramdisk_entryline}
> hash-1 {
> - algo = "$ramdisk_csum";
> + algo = "${ramdisk_csum}";
> };
> };
> EOF
>
> - if [ "${UBOOT_SIGN_ENABLE}" = "1" -a "${FIT_SIGN_INDIVIDUAL}" =
> "1" -a -n "$ramdisk_sign_keyname" ] ; then
> - sed -i '$ d' $1
> - cat << EOF >> $1
> + if [ "${UBOOT_SIGN_ENABLE}" = "1" -a "${FIT_SIGN_INDIVIDUAL}" =
> "1" -a -n "${ramdisk_sign_keyname}" ] ; then
> + sed -i '$ d' ${1}
> + cat << EOF >> ${1}
> signature-1 {
> - algo =
> "$ramdisk_csum,$ramdisk_sign_algo";
> - key-name-hint = "$ramdisk_sign_keyname";
> + algo =
> "${ramdisk_csum},${ramdisk_sign_algo}";
> + key-name-hint =
> "${ramdisk_sign_keyname}";
> };
> };
> EOF
> @@ -343,13 +343,13 @@ fitimage_emit_section_config() {
> conf_sign_keyname="${UBOOT_SIGN_KEYNAME}"
> fi
>
> - its_file="$1"
> - kernel_id="$2"
> - dtb_image="$3"
> - ramdisk_id="$4"
> - bootscr_id="$5"
> - config_id="$6"
> - default_flag="$7"
> + its_file="${1}"
> + kernel_id="${2}"
> + dtb_image="${3}"
> + ramdisk_id="${4}"
> + bootscr_id="${5}"
> + config_id="${6}"
> + default_flag="${7}"
>
> # Test if we have any DTBs at all
> sep=""
> @@ -364,106 +364,106 @@ fitimage_emit_section_config() {
>
> # conf node name is selected based on dtb ID if it is present,
> # otherwise its selected based on kernel ID
> - if [ -n "$dtb_image" ]; then
> - conf_node=$conf_node$dtb_image
> + if [ -n "${dtb_image}" ]; then
> + conf_node=$conf_node${dtb_image}
> else
> - conf_node=$conf_node$kernel_id
> + conf_node=$conf_node${kernel_id}
> fi
>
> - if [ -n "$kernel_id" ]; then
> + if [ -n "${kernel_id}" ]; then
> conf_desc="Linux kernel"
> sep=", "
> - kernel_line="kernel = \"kernel-$kernel_id\";"
> + kernel_line="kernel = \"kernel-${kernel_id}\";"
> fi
>
> - if [ -n "$dtb_image" ]; then
> - conf_desc="$conf_desc${sep}FDT blob"
> + if [ -n "${dtb_image}" ]; then
> + conf_desc="${conf_desc}${sep}FDT blob"
> sep=", "
> - fdt_line="fdt = \"fdt-$dtb_image\";"
> + fdt_line="fdt = \"fdt-${dtb_image}\";"
> fi
>
> - if [ -n "$ramdisk_id" ]; then
> - conf_desc="$conf_desc${sep}ramdisk"
> + if [ -n "${ramdisk_id}" ]; then
> + conf_desc="${conf_desc}${sep}ramdisk"
> sep=", "
> - ramdisk_line="ramdisk = \"ramdisk-$ramdisk_id\";"
> + ramdisk_line="ramdisk = \"ramdisk-${ramdisk_id}\";"
> fi
>
> - if [ -n "$bootscr_id" ]; then
> - conf_desc="$conf_desc${sep}u-boot script"
> + if [ -n "${bootscr_id}" ]; then
> + conf_desc="${conf_desc}${sep}u-boot script"
> sep=", "
> - bootscr_line="bootscr = \"bootscr-$bootscr_id\";"
> + bootscr_line="bootscr = \"bootscr-${bootscr_id}\";"
> fi
>
> - if [ -n "$config_id" ]; then
> - conf_desc="$conf_desc${sep}setup"
> - setup_line="setup = \"setup-$config_id\";"
> + if [ -n "${config_id}" ]; then
> + conf_desc="${conf_desc}${sep}setup"
> + setup_line="setup = \"setup-${config_id}\";"
> fi
>
> - if [ "$default_flag" = "1" ]; then
> + if [ "${default_flag}" = "1" ]; then
> # default node is selected based on dtb ID if it is
> present,
> # otherwise its selected based on kernel ID
> - if [ -n "$dtb_image" ]; then
> - default_line="default = \"conf-
> $dtb_image\";"
> + if [ -n "${dtb_image}" ]; then
> + default_line="default = \"conf-
> ${dtb_image}\";"
> else
> - default_line="default = \"conf-
> $kernel_id\";"
> + default_line="default = \"conf-
> ${kernel_id}\";"
> fi
> fi
>
> - cat << EOF >> $its_file
> - $default_line
> + cat << EOF >> ${its_file}
> + ${default_line}
> $conf_node {
> - description = "$default_flag $conf_desc";
> - $kernel_line
> - $fdt_line
> - $ramdisk_line
> - $bootscr_line
> - $setup_line
> + description = "${default_flag}
> ${conf_desc}";
> + ${kernel_line}
> + ${fdt_line}
> + ${ramdisk_line}
> + ${bootscr_line}
> + ${setup_line}
> hash-1 {
> - algo = "$conf_csum";
> + algo = "${conf_csum}";
> };
> EOF
>
> - if [ -n "$conf_sign_keyname" ] ; then
> + if [ ! -z "${conf_sign_keyname}" ] ; then
>
> sign_line="sign-images = "
> sep=""
>
> - if [ -n "$kernel_id" ]; then
> - sign_line="$sign_line${sep}\"kernel\""
> + if [ -n "${kernel_id}" ]; then
> + sign_line="${sign_line}${sep}\"kernel\""
> sep=", "
> fi
>
> - if [ -n "$dtb_image" ]; then
> - sign_line="$sign_line${sep}\"fdt\""
> + if [ -n "${dtb_image}" ]; then
> + sign_line="${sign_line}${sep}\"fdt\""
> sep=", "
> fi
>
> - if [ -n "$ramdisk_id" ]; then
> - sign_line="$sign_line${sep}\"ramdisk\""
> + if [ -n "${ramdisk_id}" ]; then
> + sign_line="${sign_line}${sep}\"ramdisk\""
> sep=", "
> fi
>
> - if [ -n "$bootscr_id" ]; then
> - sign_line="$sign_line${sep}\"bootscr\""
> + if [ -n "${bootscr_id}" ]; then
> + sign_line="${sign_line}${sep}\"bootscr\""
> sep=", "
> fi
>
> - if [ -n "$config_id" ]; then
> - sign_line="$sign_line${sep}\"setup\""
> + if [ -n "${config_id}" ]; then
> + sign_line="${sign_line}${sep}\"setup\""
> fi
>
> - sign_line="$sign_line;"
> + sign_line="${sign_line};"
>
> - cat << EOF >> $its_file
> + cat << EOF >> ${its_file}
> signature-1 {
> - algo = "$conf_csum,$conf_sign_algo";
> - key-name-hint = "$conf_sign_keyname";
> - $sign_line
> + algo = "${conf_csum},${conf_sign_algo}";
> + key-name-hint = "${conf_sign_keyname}";
> + ${sign_line}
> };
> EOF
> fi
>
> - cat << EOF >> $its_file
> + cat << EOF >> ${its_file}
> };
> EOF
> }
> @@ -478,21 +478,21 @@ fitimage_assemble() {
> kernelcount=1
> dtbcount=""
> DTBS=""
> - ramdiskcount=$3
> + ramdiskcount=${3}
> setupcount=""
> bootscr_id=""
> - rm -f $1 arch/${ARCH}/boot/$2
> + rm -f ${1} arch/${ARCH}/boot/${2}
>
> - if [ -n "${UBOOT_SIGN_IMG_KEYNAME}" -a "${UBOOT_SIGN_KEYNAME}"
> = "${UBOOT_SIGN_IMG_KEYNAME}" ]; then
> + if [ ! -z "${UBOOT_SIGN_IMG_KEYNAME}" -a
> "${UBOOT_SIGN_KEYNAME}" = "${UBOOT_SIGN_IMG_KEYNAME}" ]; then
> bbfatal "Keys used to sign images and configuration
> nodes must be different."
> fi
>
> - fitimage_emit_fit_header $1
> + fitimage_emit_fit_header ${1}
>
> #
> # Step 1: Prepare a kernel image section.
> #
> - fitimage_emit_section_maint $1 imagestart
> + fitimage_emit_section_maint ${1} imagestart
>
> uboot_prep_kimage
> fitimage_emit_section_kernel $1 $kernelcount linux.bin
> "$linux_comp"
> @@ -504,9 +504,9 @@ fitimage_assemble() {
> if [ -n "${KERNEL_DEVICETREE}" ]; then
> dtbcount=1
> for DTB in ${KERNEL_DEVICETREE}; do
> - if echo $DTB | grep -q '/dts/'; then
> - bbwarn "$DTB contains the full
> path to the the dts file, but only the dtb name should be used."
> - DTB=`basename $DTB | sed
> 's,\.dts$,.dtb,g'`
> + if echo ${DTB} | grep -q '/dts/'; then
> + bbwarn "${DTB} contains the
> full path to the the dts file, but only the dtb name should be used."
> + DTB=`basename ${DTB} | sed
> 's,\.dts$,.dtb,g'`
> fi
>
> # Skip ${DTB} if it's also provided in
> ${EXTERNAL_KERNEL_DEVICETREE}
> @@ -514,23 +514,23 @@ fitimage_assemble() {
> continue
> fi
>
> - DTB_PATH="arch/${ARCH}/boot/dts/$DTB"
> - if [ ! -e "$DTB_PATH" ]; then
> -
> DTB_PATH="arch/${ARCH}/boot/$DTB"
> + DTB_PATH="arch/${ARCH}/boot/dts/${DTB}"
> + if [ ! -e "${DTB_PATH}" ]; then
> +
> DTB_PATH="arch/${ARCH}/boot/${DTB}"
> fi
>
> - DTB=$(echo "$DTB" | tr '/' '_')
> - DTBS="$DTBS $DTB"
> - fitimage_emit_section_dtb $1 $DTB
> $DTB_PATH
> + DTB=$(echo "${DTB}" | tr '/' '_')
> + DTBS="${DTBS} ${DTB}"
> + fitimage_emit_section_dtb ${1} ${DTB}
> ${DTB_PATH}
> done
> fi
>
> if [ -n "${EXTERNAL_KERNEL_DEVICETREE}" ]; then
> dtbcount=1
> for DTB in $(find "${EXTERNAL_KERNEL_DEVICETREE}" \(
> -name '*.dtb' -o -name '*.dtbo' \) -printf '%P\n' | sort); do
> - DTB=$(echo "$DTB" | tr '/' '_')
> - DTBS="$DTBS $DTB"
> - fitimage_emit_section_dtb $1 $DTB
> "${EXTERNAL_KERNEL_DEVICETREE}/$DTB"
> + DTB=$(echo "${DTB}" | tr '/' '_')
> + DTBS="${DTBS} ${DTB}"
> + fitimage_emit_section_dtb ${1} ${DTB}
> "${EXTERNAL_KERNEL_DEVICETREE}/${DTB}"
> done
> fi
>
> @@ -542,7 +542,7 @@ fitimage_assemble() {
> if [ -e
> "${STAGING_DIR_HOST}/boot/${UBOOT_ENV_BINARY}" ]; then
> cp
> ${STAGING_DIR_HOST}/boot/${UBOOT_ENV_BINARY} ${B}
> bootscr_id="${UBOOT_ENV_BINARY}"
> - fitimage_emit_section_boot_script $1
> "$bootscr_id" ${UBOOT_ENV_BINARY}
> + fitimage_emit_section_boot_script ${1}
> "${bootscr_id}" ${UBOOT_ENV_BINARY}
> else
> bbwarn
> "${STAGING_DIR_HOST}/boot/${UBOOT_ENV_BINARY} not found."
> fi
> @@ -553,7 +553,7 @@ fitimage_assemble() {
> #
> if [ -e arch/${ARCH}/boot/setup.bin ]; then
> setupcount=1
> - fitimage_emit_section_setup $1 $setupcount
> arch/${ARCH}/boot/setup.bin
> + fitimage_emit_section_setup ${1} "${setupcount}"
> arch/${ARCH}/boot/setup.bin
> fi
>
> #
> @@ -562,30 +562,27 @@ fitimage_assemble() {
> if [ "x${ramdiskcount}" = "x1" ] && [
> "${INITRAMFS_IMAGE_BUNDLE}" != "1" ]; then
> # Find and use the first initramfs image archive
> type we find
> for img in cpio.lz4 cpio.lzo cpio.lzma cpio.xz
> cpio.zst cpio.gz ext2.gz cpio; do
> -
> initramfs_path="${DEPLOY_DIR_IMAGE}/${INITRAMFS_IMAGE_NAME}.$im
> g"
> - echo -n "Searching for $initramfs_path..."
> - if [ -e "$initramfs_path" ]; then
> - echo "found"
> - fitimage_emit_section_ramdisk
> $1 "$ramdiskcount" "$initramfs_path"
> +
> initramfs_path="${DEPLOY_DIR_IMAGE}/${INITRAMFS_IMAGE_NAME}.${i
> mg}"
> + echo "Using $initramfs_path"
> + if [ -e "${initramfs_path}" ]; then
> + fitimage_emit_section_ramdisk
> ${1} "${ramdiskcount}" "${initramfs_path}"
> break
> - else
> - echo "not found"
> fi
> done
> fi
>
> - fitimage_emit_section_maint $1 sectend
> + fitimage_emit_section_maint ${1} sectend
>
> # Force the first Kernel and DTB in the default config
> kernelcount=1
> - if [ -n "$dtbcount" ]; then
> + if [ -n "${dtbcount}" ]; then
> dtbcount=1
> fi
>
> #
> # Step 6: Prepare a configurations section
> #
> - fitimage_emit_section_maint $1 confstart
> + fitimage_emit_section_maint ${1} confstart
>
> # kernel-fitimage.bbclass currently only supports a single
> kernel (no less or
> # more) to be added to the FIT image along with 0 or more
> device trees and
> @@ -596,33 +593,33 @@ fitimage_assemble() {
> # the default configuration to be used is based on the
> dtbcount. If there is
> # no dtb present than select the default configuation to be
> based on
> # the kernelcount.
> - if [ -n "$DTBS" ]; then
> + if [ -n "${DTBS}" ]; then
> i=1
> for DTB in ${DTBS}; do
> dtb_ext=${DTB##*.}
> - if [ "$dtb_ext" = "dtbo" ]; then
> - fitimage_emit_section_config $1
> "" "$DTB" "" "$bootscr_id" "" "`expr $i = $dtbcount`"
> + if [ "${dtb_ext}" = "dtbo" ]; then
> + fitimage_emit_section_config
> ${1} "" "${DTB}" "" "${bootscr_id}" "" "`expr ${i} = ${dtbcount}`"
> else
> - fitimage_emit_section_config $1
> $kernelcount "$DTB" "$ramdiskcount" "$bootscr_id" "$setupcount" "`expr $i
> = $dtbcount`"
> + fitimage_emit_section_config
> ${1} "${kernelcount}" "${DTB}" "${ramdiskcount}" "${bootscr_id}"
> "${setupcount}" "`expr ${i} = ${dtbcount}`"
> fi
> - i=`expr $i + 1`
> + i=`expr ${i} + 1`
> done
> else
> defaultconfigcount=1
> - fitimage_emit_section_config $1 $kernelcount ""
> "$ramdiskcount" "$bootscr_id" "$setupcount" $defaultconfigcount
> + fitimage_emit_section_config ${1} "${kernelcount}"
> "" "${ramdiskcount}" "${bootscr_id}" "${setupcount}"
> "${defaultconfigcount}"
> fi
>
> - fitimage_emit_section_maint $1 sectend
> + fitimage_emit_section_maint ${1} sectend
>
> - fitimage_emit_section_maint $1 fitend
> + fitimage_emit_section_maint ${1} fitend
>
> #
> # Step 7: Assemble the image
> #
> ${UBOOT_MKIMAGE} \
> ${@'-D "${UBOOT_MKIMAGE_DTCOPTS}"' if
> len('${UBOOT_MKIMAGE_DTCOPTS}') else ''} \
> - -f $1 \
> - arch/${ARCH}/boot/$2
> + -f ${1} \
> + arch/${ARCH}/boot/${2}
>
> #
> # Step 8: Sign the image and add public key to U-Boot dtb
> @@ -639,7 +636,7 @@ fitimage_assemble() {
> ${@'-D "${UBOOT_MKIMAGE_DTCOPTS}"' if
> len('${UBOOT_MKIMAGE_DTCOPTS}') else ''} \
> -F -k "${UBOOT_SIGN_KEYDIR}" \
> $add_key_to_u_boot \
> - -r arch/${ARCH}/boot/$2 \
> + -r arch/${ARCH}/boot/${2} \
> ${UBOOT_MKIMAGE_SIGN_ARGS}
> fi
> }
> @@ -647,7 +644,7 @@ fitimage_assemble() {
> do_assemble_fitimage() {
> if echo ${KERNEL_IMAGETYPES} | grep -wq "fitImage"; then
> cd ${B}
> - fitimage_assemble fit-image.its fitImage ""
> + fitimage_assemble fit-image.its fitImage
> fi
> }
>
> diff --git a/meta/classes/uboot-sign.bbclass b/meta/classes/uboot-
> sign.bbclass
> index 8d136e9405..c39b30f43b 100644
> --- a/meta/classes/uboot-sign.bbclass
> +++ b/meta/classes/uboot-sign.bbclass
> @@ -176,8 +176,8 @@ concat_dtb() {
> mkdir -p ${DEPLOYDIR}
> if [ -n "${UBOOT_CONFIG}" ]; then
> for config in ${UBOOT_MACHINE}; do
> - CONFIG_B_PATH="$config"
> - cd ${B}/$config
> + CONFIG_B_PATH="${config}"
> + cd ${B}/${config}
> concat_dtb_helper
> done
> else
> @@ -193,8 +193,8 @@ concat_spl_dtb() {
> mkdir -p ${DEPLOYDIR}
> if [ -n "${UBOOT_CONFIG}" ]; then
> for config in ${UBOOT_MACHINE}; do
> - CONFIG_B_PATH="$config"
> - cd ${B}/$config
> + CONFIG_B_PATH="${config}"
> + cd ${B}/${config}
> concat_spl_dtb_helper
> done
> else
> @@ -245,7 +245,7 @@ do_install:append() {
> if [ "${PN}" = "${UBOOT_PN}" ]; then
> if [ -n "${UBOOT_CONFIG}" ]; then
> for config in ${UBOOT_MACHINE}; do
> - cd ${B}/$config
> + cd ${B}/${config}
> if [ "${UBOOT_SIGN_ENABLE}" =
> "1" -o "${UBOOT_FITIMAGE_ENABLE}" = "1" ] && \
> [ -n
> "${UBOOT_DTB_BINARY}" ]; then
> install_helper
> @@ -300,19 +300,19 @@ addtask uboot_generate_rsa_keys before
> do_uboot_assemble_fitimage after do_compi
> # Create a ITS file for the U-boot FIT, for use when
> # we want to sign it so that the SPL can verify it
> uboot_fitimage_assemble() {
> - uboot_its="$1"
> - uboot_nodtb_bin="$2"
> - uboot_dtb="$3"
> - uboot_bin="$4"
> - spl_dtb="$5"
> + uboot_its="${1}"
> + uboot_nodtb_bin="${2}"
> + uboot_dtb="${3}"
> + uboot_bin="${4}"
> + spl_dtb="${5}"
> uboot_csum="${UBOOT_FIT_HASH_ALG}"
> uboot_sign_algo="${UBOOT_FIT_SIGN_ALG}"
> uboot_sign_keyname="${SPL_SIGN_KEYNAME}"
>
> - rm -f $uboot_its $uboot_bin
> + rm -f ${uboot_its} ${uboot_bin}
>
> # First we create the ITS script
> - cat << EOF >> $uboot_its
> + cat << EOF >> ${uboot_its}
> /dts-v1/;
>
> / {
> @@ -322,7 +322,7 @@ uboot_fitimage_assemble() {
> images {
> uboot {
> description = "U-Boot image";
> - data = /incbin/("$uboot_nodtb_bin");
> + data = /incbin/("${uboot_nodtb_bin}");
> type = "standalone";
> os = "u-boot";
> arch = "${UBOOT_ARCH}";
> @@ -332,34 +332,34 @@ uboot_fitimage_assemble() {
> EOF
>
> if [ "${SPL_SIGN_ENABLE}" = "1" ] ; then
> - cat << EOF >> $uboot_its
> + cat << EOF >> ${uboot_its}
> signature {
> - algo = "$uboot_csum,$uboot_sign_algo";
> - key-name-hint = "$uboot_sign_keyname";
> + algo = "${uboot_csum},${uboot_sign_algo}";
> + key-name-hint = "${uboot_sign_keyname}";
> };
> EOF
> fi
>
> - cat << EOF >> $uboot_its
> + cat << EOF >> ${uboot_its}
> };
> fdt {
> description = "U-Boot FDT";
> - data = /incbin/("$uboot_dtb");
> + data = /incbin/("${uboot_dtb}");
> type = "flat_dt";
> arch = "${UBOOT_ARCH}";
> compression = "none";
> EOF
>
> if [ "${SPL_SIGN_ENABLE}" = "1" ] ; then
> - cat << EOF >> $uboot_its
> + cat << EOF >> ${uboot_its}
> signature {
> - algo = "$uboot_csum,$uboot_sign_algo";
> - key-name-hint = "$uboot_sign_keyname";
> + algo = "${uboot_csum},${uboot_sign_algo}";
> + key-name-hint = "${uboot_sign_keyname}";
> };
> EOF
> fi
>
> - cat << EOF >> $uboot_its
> + cat << EOF >> ${uboot_its}
> };
> };
>
> @@ -379,8 +379,8 @@ EOF
> #
> ${UBOOT_MKIMAGE} \
> ${@'-D "${SPL_MKIMAGE_DTCOPTS}"' if
> len('${SPL_MKIMAGE_DTCOPTS}') else ''} \
> - -f $uboot_its \
> - $uboot_bin
> + -f ${uboot_its} \
> + ${uboot_bin}
>
> if [ "${SPL_SIGN_ENABLE}" = "1" ] ; then
> #
> @@ -389,8 +389,8 @@ EOF
> ${UBOOT_MKIMAGE_SIGN} \
> ${@'-D "${SPL_MKIMAGE_DTCOPTS}"' if
> len('${SPL_MKIMAGE_DTCOPTS}') else ''} \
> -F -k "${SPL_SIGN_KEYDIR}" \
> - -K "$spl_dtb" \
> - -r $uboot_bin \
> + -K "${spl_dtb}" \
> + -r ${uboot_bin} \
> ${SPL_MKIMAGE_SIGN_ARGS}
> fi
>
> @@ -422,8 +422,8 @@ do_uboot_assemble_fitimage() {
> kernel_uboot_fitimage_name=`basename
> ${STAGING_DATADIR}/u-boot-fitImage-*`
> kernel_uboot_its_name=`basename
> ${STAGING_DATADIR}/u-boot-its-*`
> cd ${B}
> - uboot_fitimage_assemble $kernel_uboot_its_name
> ${UBOOT_NODTB_BINARY} \
> - ${UBOOT_DTB_BINARY}
> $kernel_uboot_fitimage_name \
> + uboot_fitimage_assemble ${kernel_uboot_its_name}
> ${UBOOT_NODTB_BINARY} \
> + ${UBOOT_DTB_BINARY}
> ${kernel_uboot_fitimage_name} \
> ${SPL_DTB_BINARY}
> fi
> }
> --
> 2.34.1
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [OE-core] [PATCH] Revert "featimage: refactor style"
2022-01-29 1:06 ` [OE-core] " Peter Kjellerstedt
@ 2022-01-29 1:38 ` Marek Vasut
2022-01-29 2:01 ` Peter Kjellerstedt
0 siblings, 1 reply; 10+ messages in thread
From: Marek Vasut @ 2022-01-29 1:38 UTC (permalink / raw)
To: Peter Kjellerstedt, openembedded-core; +Cc: Andrej Valek, Richard Purdie
On 1/29/22 02:06, Peter Kjellerstedt wrote:
>> -----Original Message-----
>> From: openembedded-core@lists.openembedded.org <openembedded-
>> core@lists.openembedded.org> On Behalf Of Marek Vasut
>> Sent: den 29 januari 2022 01:29
>> To: openembedded-core@lists.openembedded.org
>> Cc: Marek Vasut <marex@denx.de>; Andrej Valek <andrej.valek@siemens.com>;
>> Richard Purdie <richard.purdie@linuxfoundation.org>
>> Subject: [OE-core] [PATCH] Revert "featimage: refactor style"
>>
>> This reverts commit f44bb458884da64356ee188917094b5515d3b159.
>>
>> The reverted patch attempted to perform some sort of clean up, however
>> it only brought in style inconsistencies like this:
>>
>> ```
>> conf_desc="$conf_desc${sep}setup"
>> ```
>>
>> The curly brackets around variables were placed in the kernel-fitimage
>> bbclass deliberately, since when assembling the fitimage ITS there are
>> multiple variables where it is difficult to identify where the variable
>> ends and some sort of follow up string starts.
>
> There is actually a technical reason to not use ${foo} for shell
> variables unless necessary in bitbake files and it is because
> bitbake will treat them all as potential bitbake variables. This
> means they are unnecessarily included in the taskhashes that
> bitbake calculates.
Yikes. (it would be good to include this gem in the commit message)
So are we stuck with this inconsistent coding style change or is there a
third alternative ? I mean, besides rewriting the fitimage generation
into python, which might make it more flexible too.
^ permalink raw reply [flat|nested] 10+ messages in thread* RE: [OE-core] [PATCH] Revert "featimage: refactor style"
2022-01-29 1:38 ` Marek Vasut
@ 2022-01-29 2:01 ` Peter Kjellerstedt
2022-01-29 2:36 ` Marek Vasut
0 siblings, 1 reply; 10+ messages in thread
From: Peter Kjellerstedt @ 2022-01-29 2:01 UTC (permalink / raw)
To: Marek Vasut, openembedded-core; +Cc: Andrej Valek, Richard Purdie
> -----Original Message-----
> From: Marek Vasut <marex@denx.de>
> Sent: den 29 januari 2022 02:39
> To: Peter Kjellerstedt <peter.kjellerstedt@axis.com>; openembedded-
> core@lists.openembedded.org
> Cc: Andrej Valek <andrej.valek@siemens.com>; Richard Purdie
> <richard.purdie@linuxfoundation.org>
> Subject: Re: [OE-core] [PATCH] Revert "featimage: refactor style"
>
> On 1/29/22 02:06, Peter Kjellerstedt wrote:
> >> -----Original Message-----
> >> From: openembedded-core@lists.openembedded.org <openembedded-
> >> core@lists.openembedded.org> On Behalf Of Marek Vasut
> >> Sent: den 29 januari 2022 01:29
> >> To: openembedded-core@lists.openembedded.org
> >> Cc: Marek Vasut <marex@denx.de>; Andrej Valek
> <andrej.valek@siemens.com>;
> >> Richard Purdie <richard.purdie@linuxfoundation.org>
> >> Subject: [OE-core] [PATCH] Revert "featimage: refactor style"
> >>
> >> This reverts commit f44bb458884da64356ee188917094b5515d3b159.
> >>
> >> The reverted patch attempted to perform some sort of clean up, however
> >> it only brought in style inconsistencies like this:
> >>
> >> ```
> >> conf_desc="$conf_desc${sep}setup"
> >> ```
> >>
> >> The curly brackets around variables were placed in the kernel-fitimage
> >> bbclass deliberately, since when assembling the fitimage ITS there are
> >> multiple variables where it is difficult to identify where the variable
> >> ends and some sort of follow up string starts.
> >
> > There is actually a technical reason to not use ${foo} for shell
> > variables unless necessary in bitbake files and it is because
> > bitbake will treat them all as potential bitbake variables. This
> > means they are unnecessarily included in the taskhashes that
> > bitbake calculates.
>
> Yikes. (it would be good to include this gem in the commit message)
Well there was:
- use bash variable notation without {} where possible
- just to make sure it looks like bash variable not bitbake variable one
though I guess my explanation has a bit more technical merit
than that it improves the looks. ;)
> So are we stuck with this inconsistent coding style change
> or is there a
Personally I do not see it as inconsistent, it is just the way
shell handles variables. It is just something to get used to (I
also had a colleague who would review any shell code changes we
made and comment on every single unnecessary character so one
quickly learned to use $foo everywhere possible rather than
${foo}...) That said, I have never looked at this code so I
have no real idea of how bad it is or isn't.
> third alternative ? I mean, besides rewriting the fitimage
> generation into python, which might make it more flexible too.
Replacing shell code that has grown beyond a couple of hundred
(tens?) lines with something written in a better language is
almost always a good idea.
//Peter
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [OE-core] [PATCH] Revert "featimage: refactor style"
2022-01-29 2:01 ` Peter Kjellerstedt
@ 2022-01-29 2:36 ` Marek Vasut
2022-01-31 7:01 ` Valek, Andrej
0 siblings, 1 reply; 10+ messages in thread
From: Marek Vasut @ 2022-01-29 2:36 UTC (permalink / raw)
To: Peter Kjellerstedt, openembedded-core; +Cc: Andrej Valek, Richard Purdie
On 1/29/22 03:01, Peter Kjellerstedt wrote:
Hi,
[...]
> Personally I do not see it as inconsistent, it is just the way
> shell handles variables. It is just something to get used to (I
> also had a colleague who would review any shell code changes we
> made and comment on every single unnecessary character so one
> quickly learned to use $foo everywhere possible rather than
> ${foo}...)
On the other hand, once you get burnt ${foo}bar / $foobar , you might
quickly start putting the {} everywhere. That's my case.
(we should likely stop this ^ discussion thread before it turns into a
flamewar)
> That said, I have never looked at this code so I
> have no real idea of how bad it is or isn't.
This patch lists pretty much all the vars, so you can get a pretty
decent idea from just looking at that.
>> third alternative ? I mean, besides rewriting the fitimage
>> generation into python, which might make it more flexible too.
>
> Replacing shell code that has grown beyond a couple of hundred
> (tens?) lines with something written in a better language is
> almost always a good idea.
It's grown to almost 800 LoC, so maybe it is time to reevaluate the
python conversion then.
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [OE-core] [PATCH] Revert "featimage: refactor style"
2022-01-29 2:36 ` Marek Vasut
@ 2022-01-31 7:01 ` Valek, Andrej
2022-01-31 9:39 ` Marek Vasut
0 siblings, 1 reply; 10+ messages in thread
From: Valek, Andrej @ 2022-01-31 7:01 UTC (permalink / raw)
To: marex, openembedded-core, peter.kjellerstedt; +Cc: richard.purdie
Hi,
Sorry, but personally I don't like your idea. What's the benefit of
reverting this? I would keep the ${} for bitbake and $ for shell. The
{} has to be placed only for variables like $a${b}c.
We should respect the workflow on all recipes otherwise we're braking
the "unwritten" rules.
Regards,
Andrej
On Sat, 2022-01-29 at 03:36 +0100, Marek Vasut wrote:
> On 1/29/22 03:01, Peter Kjellerstedt wrote:
>
> Hi,
>
> [...]
>
> > Personally I do not see it as inconsistent, it is just the way
> > shell handles variables. It is just something to get used to (I
> > also had a colleague who would review any shell code changes we
> > made and comment on every single unnecessary character so one
> > quickly learned to use $foo everywhere possible rather than
> > ${foo}...)
>
> quickly start putting the {} everywhere. That's my case.
>
> (we should likely stop this ^ discussion thread before it turns into
> a
> flamewar)
>
> > That said, I have never looked at this code so I
> > have no real idea of how bad it is or isn't.
>
> This patch lists pretty much all the vars, so you can get a pretty
> decent idea from just looking at that.
>
> > > third alternative ? I mean, besides rewriting the fitimage
> > > generation into python, which might make it more flexible too.
> >
> > Replacing shell code that has grown beyond a couple of hundred
> > (tens?) lines with something written in a better language is
> > almost always a good idea.
>
> It's grown to almost 800 LoC, so maybe it is time to reevaluate the
> python conversion then.
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [OE-core] [PATCH] Revert "featimage: refactor style"
2022-01-31 7:01 ` Valek, Andrej
@ 2022-01-31 9:39 ` Marek Vasut
2022-02-02 6:51 ` Valek, Andrej
0 siblings, 1 reply; 10+ messages in thread
From: Marek Vasut @ 2022-01-31 9:39 UTC (permalink / raw)
To: Valek, Andrej, openembedded-core, peter.kjellerstedt; +Cc: richard.purdie
On 1/31/22 08:01, Valek, Andrej wrote:
> Hi,
Hello Andrej,
(please avoid top-posting)
> Sorry, but personally I don't like your idea. What's the benefit of
> reverting this? I would keep the ${} for bitbake and $ for shell. The
> {} has to be placed only for variables like $a${b}c.
That's exactly the benefit of using ${} in shell scripts consistently --
you don't have to worry about variable names being accidentally
conflated with surrounding strings, either due to your own mistake, or
some automated transformation that was applied incorrectly .
> We should respect the workflow on all recipes otherwise we're braking
> the "unwritten" rules.
The workflow on all recipes ? What does this mean ?
The "unwritten" rules ? If they are unwritten, of course they will be
broken by people. Better update the documentation.
There is one technical counter-argument to this revert from Peter, quote:
"
There is actually a technical reason to not use ${foo} for shell
variables unless necessary in bitbake files and it is because
bitbake will treat them all as potential bitbake variables. This
means they are unnecessarily included in the taskhashes that
bitbake calculates.
"
But the patch being reverted here addresses the problem only partly,
because it still contains remnants like this:
"
conf_desc="$conf_desc${sep}setup"
"
[...]
>>>> third alternative ? I mean, besides rewriting the fitimage
>>>> generation into python, which might make it more flexible too.
>>>
>>> Replacing shell code that has grown beyond a couple of hundred
>>> (tens?) lines with something written in a better language is
>>> almost always a good idea.
>>
>> It's grown to almost 800 LoC, so maybe it is time to reevaluate the
>> python conversion then.
So a rewrite into something more suitable might be a good idea ^ ,
probably python in this case.
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [OE-core] [PATCH] Revert "featimage: refactor style"
2022-01-31 9:39 ` Marek Vasut
@ 2022-02-02 6:51 ` Valek, Andrej
2022-02-02 8:17 ` Marek Vasut
0 siblings, 1 reply; 10+ messages in thread
From: Valek, Andrej @ 2022-02-02 6:51 UTC (permalink / raw)
To: marex, openembedded-core, peter.kjellerstedt; +Cc: richard.purdie
Marek,
Sorry, but these are still not an arguments, why to do that.
On Mon, 2022-01-31 at 10:39 +0100, Marek Vasut wrote:
> On 1/31/22 08:01, Valek, Andrej wrote:
> > Hi,
>
> Hello Andrej,
>
> (please avoid top-posting)
>
> > Sorry, but personally I don't like your idea. What's the benefit of
> > reverting this? I would keep the ${} for bitbake and $ for shell. The
> > {} has to be placed only for variables like $a${b}c.
>
> That's exactly the benefit of using ${} in shell scripts consistently -
> -
> you don't have to worry about variable names being accidentally
> conflated with surrounding strings, either due to your own mistake, or
> some automated transformation that was applied incorrectly .
>
> > We should respect the workflow on all recipes otherwise we're braking
> > the "unwritten" rules.
>
> The workflow on all recipes ? What does this mean ?
>
> broken by people. Better update the documentation.
>
> There is one technical counter-argument to this revert from Peter,
> quote:
> "
> There is actually a technical reason to not use ${foo} for shell
> variables unless necessary in bitbake files and it is because
> bitbake will treat them all as potential bitbake variables. This
> means they are unnecessarily included in the taskhashes that
> bitbake calculates.
> "
>
> But the patch being reverted here addresses the problem only partly,
> because it still contains remnants like this:
> "
> conf_desc="$conf_desc${sep}setup"
> "
Just for your information, this is not remnants, this is exactly the
right {} usage. If you didn't place the {}, it will be
conf_desc="$conf_desc$sepsetup", which doesn't make any sense.
>
> [...]
>
> > > > > third alternative ? I mean, besides rewriting the fitimage
> > > > > generation into python, which might make it more flexible too.
> > > >
> > > > Replacing shell code that has grown beyond a couple of hundred
> > > > (tens?) lines with something written in a better language is
> > > > almost always a good idea.
> > >
> > > It's grown to almost 800 LoC, so maybe it is time to reevaluate the
> > > python conversion then.
>
> So a rewrite into something more suitable might be a good idea ^ ,
> probably python in this case.
But anyway, if you want to do that, then do not forget for other kernel
classes, where the same commits have been applied by someone else.
So from my point of view, it has to stay like it is.
Cheers,
Andrej
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [OE-core] [PATCH] Revert "featimage: refactor style"
2022-02-02 6:51 ` Valek, Andrej
@ 2022-02-02 8:17 ` Marek Vasut
2022-02-02 9:03 ` Valek, Andrej
0 siblings, 1 reply; 10+ messages in thread
From: Marek Vasut @ 2022-02-02 8:17 UTC (permalink / raw)
To: Valek, Andrej, openembedded-core, peter.kjellerstedt; +Cc: richard.purdie
On 2/2/22 07:51, Valek, Andrej wrote:
> Marek,
Hello Andrej,
> Sorry, but these are still not an arguments, why to do that.
I'm sorry, I am lost and confused ... what part of the email are you
referring to ?
> On Mon, 2022-01-31 at 10:39 +0100, Marek Vasut wrote:
>> On 1/31/22 08:01, Valek, Andrej wrote:
>>> Hi,
>>
>> Hello Andrej,
>>
>> (please avoid top-posting)
>>
>>> Sorry, but personally I don't like your idea. What's the benefit of
>>> reverting this? I would keep the ${} for bitbake and $ for shell. The
>>> {} has to be placed only for variables like $a${b}c.
>>
>> That's exactly the benefit of using ${} in shell scripts consistently -
>> -
>> you don't have to worry about variable names being accidentally
>> conflated with surrounding strings, either due to your own mistake, or
>> some automated transformation that was applied incorrectly .
>>
>>> We should respect the workflow on all recipes otherwise we're braking
>>> the "unwritten" rules.
>>
>> The workflow on all recipes ? What does this mean ?
>>
>> broken by people. Better update the documentation.
>>
>> There is one technical counter-argument to this revert from Peter,
>> quote:
>> "
>> There is actually a technical reason to not use ${foo} for shell
>> variables unless necessary in bitbake files and it is because
>> bitbake will treat them all as potential bitbake variables. This
>> means they are unnecessarily included in the taskhashes that
>> bitbake calculates.
>> "
>>
>> But the patch being reverted here addresses the problem only partly,
>> because it still contains remnants like this:
>> "
>> conf_desc="$conf_desc${sep}setup"
>> "
> Just for your information, this is not remnants, this is exactly the
> right {} usage. If you didn't place the {}, it will be
> conf_desc="$conf_desc$sepsetup", which doesn't make any sense.
OK, one more time then.
Either your patch attempted to change the coding style of this script to
match your personal preference, and did it only partly, so the result is
inconsistent.
Or
You were fixing the aforementioned taskhash issue, in which case the
taskhash issue is also fixed only partly.
The commit message is not clear on what the intention was.
[...]
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [OE-core] [PATCH] Revert "featimage: refactor style"
2022-02-02 8:17 ` Marek Vasut
@ 2022-02-02 9:03 ` Valek, Andrej
0 siblings, 0 replies; 10+ messages in thread
From: Valek, Andrej @ 2022-02-02 9:03 UTC (permalink / raw)
To: marex, openembedded-core, peter.kjellerstedt; +Cc: richard.purdie
Hello Marek,
I think, we have to stop the discussion now, because it is not leading
into any conclusion. Anyway, both of us have a different opinion.
Maybe rewriting into python will solve it, I won't do that.
Cheers,
Andrej
On Wed, 2022-02-02 at 09:17 +0100, Marek Vasut wrote:
> On 2/2/22 07:51, Valek, Andrej wrote:
> > Marek,
>
> Hello Andrej,
>
> > Sorry, but these are still not an arguments, why to do that.
>
> I'm sorry, I am lost and confused ... what part of the email are you
> referring to ?
>
> > On Mon, 2022-01-31 at 10:39 +0100, Marek Vasut wrote:
> > > On 1/31/22 08:01, Valek, Andrej wrote:
> > > > Hi,
> > >
> > > Hello Andrej,
> > >
> > > (please avoid top-posting)
> > >
> > > > Sorry, but personally I don't like your idea. What's the
> > > > benefit of
> > > > reverting this? I would keep the ${} for bitbake and $ for
> > > > shell. The
> > > > {} has to be placed only for variables like $a${b}c.
> > >
> > > That's exactly the benefit of using ${} in shell scripts
> > > consistently -
> > > -
> > > you don't have to worry about variable names being accidentally
> > > conflated with surrounding strings, either due to your own
> > > mistake, or
> > > some automated transformation that was applied incorrectly .
> > >
> > > > We should respect the workflow on all recipes otherwise we're
> > > > braking
> > > > the "unwritten" rules.
> > >
> > > The workflow on all recipes ? What does this mean ?
> > >
> > > broken by people. Better update the documentation.
> > >
> > > There is one technical counter-argument to this revert from
> > > Peter,
> > > quote:
> > > "
> > > There is actually a technical reason to not use ${foo} for shell
> > > variables unless necessary in bitbake files and it is because
> > > bitbake will treat them all as potential bitbake variables. This
> > > means they are unnecessarily included in the taskhashes that
> > > bitbake calculates.
> > > "
> > >
> > > But the patch being reverted here addresses the problem only
> > > partly,
> > > because it still contains remnants like this:
> > > "
> > > conf_desc="$conf_desc${sep}setup"
> > > "
> > Just for your information, this is not remnants, this is exactly
> > the
> > right {} usage. If you didn't place the {}, it will be
> > conf_desc="$conf_desc$sepsetup", which doesn't make any sense.
>
> OK, one more time then.
>
> Either your patch attempted to change the coding style of this script
> to
> match your personal preference, and did it only partly, so the result
> is
> inconsistent.
>
> Or
>
> You were fixing the aforementioned taskhash issue, in which case the
> taskhash issue is also fixed only partly.
>
> The commit message is not clear on what the intention was.
>
> [...]
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-02-02 9:04 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-29 0:29 [PATCH] Revert "featimage: refactor style" Marek Vasut
2022-01-29 1:06 ` [OE-core] " Peter Kjellerstedt
2022-01-29 1:38 ` Marek Vasut
2022-01-29 2:01 ` Peter Kjellerstedt
2022-01-29 2:36 ` Marek Vasut
2022-01-31 7:01 ` Valek, Andrej
2022-01-31 9:39 ` Marek Vasut
2022-02-02 6:51 ` Valek, Andrej
2022-02-02 8:17 ` Marek Vasut
2022-02-02 9:03 ` Valek, Andrej
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.