| Message ID | 20230223064359.4171845-3-felix.moessbauer@siemens.com |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | Fix data-race in deployment of initrd | expand |
23/02/2023 08:43, Felix Moessbauer wrote: > This patch changes how we deploy the boot files. > Instead of manually deploying, we use the sstate infrastructure for > that. By that, accidental overrides of the artifacts can be > automatically detected. On clean, the artifacts are also cleaned. > > Signed-off-by: Felix Moessbauer <felix.moessbauer@siemens.com> Hello Felix, This logic fails on fargets that deploy DTB files if building several distros inside one builddir. DTB filenames are usually the same in this case, so it is detected as copy of already exist files. Steps to reproduce: $ bitbake mc:imx6-sabrelite-bullseye:isar-image-base And right after then $ bitbake mc:imx6-sabrelite-buster:isar-image-base Will trigger: ERROR: mc:imx6-sabrelite-buster:isar-image-base-1.0-r0 do_copy_boot_files: The recipe isar-image-base is trying to install files into a shared area when those files already exist.
Am Freitag, dem 29.12.2023 um 09:29 +0200 schrieb Anton Mikanovich: > 23/02/2023 08:43, Felix Moessbauer wrote: > > This patch changes how we deploy the boot files. > > Instead of manually deploying, we use the sstate infrastructure for > > that. By that, accidental overrides of the artifacts can be > > automatically detected. On clean, the artifacts are also cleaned. > > > > Signed-off-by: Felix Moessbauer <felix.moessbauer@siemens.com> > > Hello Felix, > > This logic fails on fargets that deploy DTB files if building several > distros > inside one builddir. DTB filenames are usually the same in this case, > so > it is > detected as copy of already exist files. Hi, this is a bug in the initial deploy logic which is just uncovered by the deploy-via-sstate mechanism. In the past, we probably had data- races on these files (of course only if the targets were build in parallel). One possible fix would be to prefix the DTB files with the image+machine+distro name. How is this done in OE? Best regards, Felix > > Steps to reproduce: > > $ bitbake mc:imx6-sabrelite-bullseye:isar-image-base > > And right after then > > $ bitbake mc:imx6-sabrelite-buster:isar-image-base > > Will trigger: > > ERROR: mc:imx6-sabrelite-buster:isar-image-base-1.0-r0 > do_copy_boot_files: > The recipe isar-image-base is trying to install files into a shared > area > when those files already exist. >
On 09.01.24 13:46, Moessbauer, Felix (T CED OES-DE) wrote: > Am Freitag, dem 29.12.2023 um 09:29 +0200 schrieb Anton Mikanovich: >> 23/02/2023 08:43, Felix Moessbauer wrote: >>> This patch changes how we deploy the boot files. >>> Instead of manually deploying, we use the sstate infrastructure for >>> that. By that, accidental overrides of the artifacts can be >>> automatically detected. On clean, the artifacts are also cleaned. >>> >>> Signed-off-by: Felix Moessbauer <felix.moessbauer@siemens.com> >> >> Hello Felix, >> >> This logic fails on fargets that deploy DTB files if building several >> distros >> inside one builddir. DTB filenames are usually the same in this case, >> so >> it is >> detected as copy of already exist files. > > Hi, > > this is a bug in the initial deploy logic which is just uncovered by > the deploy-via-sstate mechanism. In the past, we probably had data- > races on these files (of course only if the targets were build in > parallel). > > One possible fix would be to prefix the DTB files with the > image+machine+distro name. How is this done in OE? Careful with renaming, I'm not sure of we aren't picking up the DTBs from there and also relying on the names as the firmware will need specific naming as well. This needs at least double-checking. Jan
09/01/2024 15:46, Jan Kiszka wrote: > Careful with renaming, I'm not sure of we aren't picking up the DTBs > from there and also relying on the names as the firmware will need > specific naming as well. This needs at least double-checking. > > Jan > It looks like there is no other solution, because different distros may have differencies in their dtb files. Downstreams are rely on Isar output paths, so we should at least create a item in RECIPE-API-CHANGELOG.md.
On 09.01.24 15:04, Anton Mikanovich wrote: > 09/01/2024 15:46, Jan Kiszka wrote: >> Careful with renaming, I'm not sure of we aren't picking up the DTBs >> from there and also relying on the names as the firmware will need >> specific naming as well. This needs at least double-checking. >> >> Jan >> > It looks like there is no other solution, because different distros may > have > differencies in their dtb files. > Downstreams are rely on Isar output paths, so we should at least create > a item > in RECIPE-API-CHANGELOG.md. > This is about naming for the kernel or the firmware, not for the layer. Again: Careful! Also, it would likely be better to enhance DEPLOY_DIR_IMAGE with the distro, rather than messing with the file names. Jan
On Wed, 2024-01-10 at 05:27 +0100, 'Jan Kiszka' via isar-users wrote: > On 09.01.24 15:04, Anton Mikanovich wrote: > > 09/01/2024 15:46, Jan Kiszka wrote: > > > Careful with renaming, I'm not sure of we aren't picking up the > > > DTBs > > > from there and also relying on the names as the firmware will > > > need > > > specific naming as well. This needs at least double-checking. > > > > > > Jan > > > > > It looks like there is no other solution, because different distros > > may > > have > > differencies in their dtb files. > > Downstreams are rely on Isar output paths, so we should at least > > create > > a item > > in RECIPE-API-CHANGELOG.md. > > > > This is about naming for the kernel or the firmware, not for the > layer. > Again: Careful! Also, it would likely be better to enhance > DEPLOY_DIR_IMAGE with the distro, rather than messing with the file > names. > > Jan > > -- > Siemens AG, Technology > Linux Expert Center > Currently we have an internal solution that uses a subdirectory named according to the image built (e.g., 'isar-image-base-bootfiles'). This saves original DTB namings, but we just have them in a separate dir. This, of course, also needs mentioning in RECIPE-API-CHANGELOG.md since imagers/CI may use the files for their purpose. Anyway, there is another issue in the whole patchset. We make sstate do things it is not supposed to do. And if the user wants to completely disable sstate, they at least won't find kernel/DTB files in deploy dir. At most, imager can fail if it looks for them here. A simple way to reproduce the issue with disabled sstate: 1. Run 'kas-container menu', configure stm32mp15x-bullseye target 2. Run 'kas-container shell' 3. Disable sstate ``` builder@25df5af8cc0a:/build$ echo 'INHERIT:remove = "sstate"' >> conf/local.conf ``` 4. Build the image ``` builder@25df5af8cc0a:/build$ bitbake isar-image-base ``` 5. Check `build/tmp/deploy/images` => there are no any kernel/initrd files, they exist only in `tmp/work/debian-bullseye-armhf/isar-image- base-stm32mp15x/1.0-r0/deploy/` Some other targets (like phyboard-mira) even fail in imager, since it needs the files to be in deploy dir. I guess, later we need to rework the functionality the following way: 1. Don't use sstate for copying files, they should be directly deployed; 2. Split boot files extracted from rootfses with different distro and targets (e.g., `-base`, `-debug`, etc) - but with no sstate help.
On Mon, 2024-02-26 at 16:32 +0300, Uladzimir Bely wrote: > On Wed, 2024-01-10 at 05:27 +0100, 'Jan Kiszka' via isar-users wrote: > > On 09.01.24 15:04, Anton Mikanovich wrote: > > > 09/01/2024 15:46, Jan Kiszka wrote: > > > > Careful with renaming, I'm not sure of we aren't picking up the > > > > DTBs > > > > from there and also relying on the names as the firmware will > > > > need > > > > specific naming as well. This needs at least double-checking. > > > > > > > > Jan > > > > > > > It looks like there is no other solution, because different > > > distros > > > may > > > have > > > differencies in their dtb files. > > > Downstreams are rely on Isar output paths, so we should at least > > > create > > > a item > > > in RECIPE-API-CHANGELOG.md. > > > > > > > This is about naming for the kernel or the firmware, not for the > > layer. > > Again: Careful! Also, it would likely be better to enhance > > DEPLOY_DIR_IMAGE with the distro, rather than messing with the file > > names. > > > > Jan > > > > -- > > Siemens AG, Technology > > Linux Expert Center > > > > Currently we have an internal solution that uses a subdirectory named > according to the image built (e.g., 'isar-image-base-bootfiles'). > This > saves original DTB namings, but we just have them in a separate dir. > This, of course, also needs mentioning in RECIPE-API-CHANGELOG.md > since > imagers/CI may use the files for their purpose. > > Anyway, there is another issue in the whole patchset. We make sstate > do > things it is not supposed to do. And if the user wants to completely > disable sstate, they at least won't find kernel/DTB files in deploy > dir. At most, imager can fail if it looks for them here. Is this really true? Just because we deploy through sstate (which is anyways proposed by yocto), does not mean that we MUST use the cache. Unfortunately, the concept documentation of yocto is a bit imprecise regarding this point: https://docs.yoctoproject.org/overview-manual/concepts.html?highlight=sstate#setscene-tasks-and-shared-state > > A simple way to reproduce the issue with disabled sstate: > > 1. Run 'kas-container menu', configure stm32mp15x-bullseye target > > 2. Run 'kas-container shell' > > 3. Disable sstate > > ``` > builder@25df5af8cc0a:/build$ echo 'INHERIT:remove = "sstate"' >> > conf/local.conf This is not the correct way to disable sstate. Instead, please use SSTATE_SKIP_CREATION="1". Also I'm unsure if disabling sstate cache is even supported globally (at least in yocto it is not). Felix > ``` > > 4. Build the image > > ``` > builder@25df5af8cc0a:/build$ bitbake isar-image-base > ``` > > 5. Check `build/tmp/deploy/images` => there are no any kernel/initrd > files, they exist only in `tmp/work/debian-bullseye-armhf/isar-image- > base-stm32mp15x/1.0-r0/deploy/` > > Some other targets (like phyboard-mira) even fail in imager, since it > needs the files to be in deploy dir. > > I guess, later we need to rework the functionality the following way: > > 1. Don't use sstate for copying files, they should be directly > deployed; > 2. Split boot files extracted from rootfses with different distro and > targets (e.g., `-base`, `-debug`, etc) - but with no sstate help. > > >
diff --git a/meta/classes/image.bbclass b/meta/classes/image.bbclass index ef7d5a2a..6277069f 100644 --- a/meta/classes/image.bbclass +++ b/meta/classes/image.bbclass @@ -3,11 +3,13 @@ # Make workdir and stamps machine-specific without changing common PN target WORKDIR = "${TMPDIR}/work/${DISTRO}-${DISTRO_ARCH}/${PN}-${MACHINE}/${PV}-${PR}" +DEPLOYDIR = "${WORKDIR}/deploy" STAMP = "${STAMPS_DIR}/${DISTRO}-${DISTRO_ARCH}/${PN}-${MACHINE}/${PV}-${PR}" STAMPCLEAN = "${STAMPS_DIR}/${DISTRO}-${DISTRO_ARCH}/${PN}-${MACHINE}/*-*" # Sstate also needs to be machine-specific SSTATE_MANIFESTS = "${TMPDIR}/sstate-control/${MACHINE}-${DISTRO}-${DISTRO_ARCH}" +SSTATETASKS += "do_copy_boot_files" IMAGE_INSTALL ?= "" IMAGE_FSTYPES ?= "${@ d.getVar("IMAGE_TYPE", True) if d.getVar("IMAGE_TYPE", True) else "ext4"}" @@ -355,8 +357,9 @@ INITRD_IMG = "${PP_DEPLOY}/${INITRD_IMAGE}" # only one dtb file supported, pick the first DTB_IMG = "${PP_DEPLOY}/${@(d.getVar('DTB_FILES').split() or [''])[0]}" -do_copy_boot_files[dirs] = "${DEPLOY_DIR_IMAGE}" -do_copy_boot_files[lockfiles] += "${DEPLOY_DIR_IMAGE}/isar.lock" +do_copy_boot_files[cleandirs] += "${DEPLOYDIR}" +do_copy_boot_files[sstate-inputdirs] = "${DEPLOYDIR}" +do_copy_boot_files[sstate-outputdirs] = "${DEPLOY_DIR_IMAGE}" do_copy_boot_files[network] = "${TASK_USE_SUDO}" do_copy_boot_files() { kernel="$(realpath -q '${IMAGE_ROOTFS}'/vmlinu[xz])" @@ -364,7 +367,7 @@ do_copy_boot_files() { kernel="$(realpath -q '${IMAGE_ROOTFS}'/boot/vmlinu[xz])" fi if [ -f "$kernel" ]; then - sudo cat "$kernel" > "${DEPLOY_DIR_IMAGE}/${KERNEL_IMAGE}" + sudo cat "$kernel" > "${DEPLOYDIR}/${KERNEL_IMAGE}" fi initrd="$(realpath -q '${IMAGE_ROOTFS}/initrd.img')" @@ -372,7 +375,7 @@ do_copy_boot_files() { initrd="$(realpath -q '${IMAGE_ROOTFS}/boot/initrd.img')" fi if [ -f "$initrd" ]; then - cp -f "$initrd" '${DEPLOY_DIR_IMAGE}/${INITRD_IMAGE}' + cp -f "$initrd" '${DEPLOYDIR}/${INITRD_IMAGE}' fi for file in ${DTB_FILES}; do @@ -383,11 +386,16 @@ do_copy_boot_files() { die "${file} not found" fi - cp -f "$dtb" "${DEPLOY_DIR_IMAGE}/" + cp -f "$dtb" "${DEPLOYDIR}/" done } addtask copy_boot_files before do_rootfs_postprocess after do_rootfs_install +python do_copy_boot_files_setscene () { + sstate_setscene(d) +} +addtask do_copy_boot_files_setscene + python do_image_tools() { """Virtual task""" pass
This patch changes how we deploy the boot files. Instead of manually deploying, we use the sstate infrastructure for that. By that, accidental overrides of the artifacts can be automatically detected. On clean, the artifacts are also cleaned. Signed-off-by: Felix Moessbauer <felix.moessbauer@siemens.com> --- meta/classes/image.bbclass | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-)