diff mbox series

[isar-cip-core] squashfs: Fix vardeps for SQUASHFS_THREADS and SQUASHFS_MEMLIMIT

Message ID 46e8b3a2-128f-c63a-cbfc-6d38a1792a35@siemens.com (mailing list archive)
State Accepted
Headers show
Series [isar-cip-core] squashfs: Fix vardeps for SQUASHFS_THREADS and SQUASHFS_MEMLIMIT | expand

Commit Message

Jan Kiszka May 27, 2023, 4:47 p.m. UTC
From: Jan Kiszka <jan.kiszka@siemens.com>

While SQUASHFS_DEFAULTS was decoupled from its potentially unstable
inputs, SQUASHFS_THREADS and SQUASHFS_MEMLIMIT, the consumer of
SQUASHFS_DEFAULTS was not. Rework this moving the vardepsexclude to
IMAGE_CMD:squashfs. This also obsoletes the related vardepvalue and
vardepsexclude for SQUASHFS_THREADS and SQUASHFS_MEMLIMIT.

As we are now expecting only generation limits via that ignored
variable, rename it to SQUASHFS_CREATION_LIMITS and deny overwriting.
Only SQUASHFS_THREADS and SQUASHFS_MEMLIMIT are supposed to be tuned
to the build environment - if at all.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 classes/squashfs.bbclass | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

Comments

Felix Moessbauer May 29, 2023, 10:24 a.m. UTC | #1
On Sat, 2023-05-27 at 18:47 +0200, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> While SQUASHFS_DEFAULTS was decoupled from its potentially unstable
> inputs, SQUASHFS_THREADS and SQUASHFS_MEMLIMIT, the consumer of
> SQUASHFS_DEFAULTS was not. Rework this moving the vardepsexclude to
> IMAGE_CMD:squashfs. This also obsoletes the related vardepvalue and
> vardepsexclude for SQUASHFS_THREADS and SQUASHFS_MEMLIMIT.

Why was this required? The consumer does not expand the variables and
by that changes of e.g SQUASHFS_THREADS should not affect the hashes.

> 
> As we are now expecting only generation limits via that ignored
> variable, rename it to SQUASHFS_CREATION_LIMITS and deny overwriting.
> Only SQUASHFS_THREADS and SQUASHFS_MEMLIMIT are supposed to be tuned
> to the build environment - if at all.

I'm not against this change, but I don't understand what issue is
solved here.

Felix

> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  classes/squashfs.bbclass | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/classes/squashfs.bbclass b/classes/squashfs.bbclass
> index b14768c2..1f2a7595 100644
> --- a/classes/squashfs.bbclass
> +++ b/classes/squashfs.bbclass
> @@ -1,7 +1,7 @@
>  #
>  # CIP Core, generic profile
>  #
> -# Copyright (c) Siemens AG, 2021-2022
> +# Copyright (c) Siemens AG, 2021-2023
>  #
>  # Authors:
>  #  Quirin Gylstorff <quirin.gylstorff@siemens.com>
> @@ -16,11 +16,9 @@ SQUASHFS_CONTENT ?= "${PP_ROOTFS}"
>  SQUASHFS_CREATION_ARGS ?= ""
>  
>  SQUASHFS_THREADS ?= "${@oe.utils.cpu_count(at_least=2)}"
> -SQUASHFS_THREADS[vardepvalue] = "1"
>  # default according to mksquasfs docs
>  SQUASHFS_MEMLIMIT ?= "7982M"
> -SQUASHFS_DEFAULTS ?= "-mem ${SQUASHFS_MEMLIMIT} -processors
> ${SQUASHFS_THREADS}"
> -SQUASHFS_DEFAULTS[vardepsexclude] += "SQUASHFS_MEMLIMIT
> SQUASHFS_THREADS"
> +SQUASHFS_CREATION_LIMITS = "-mem ${SQUASHFS_MEMLIMIT} -processors
> ${SQUASHFS_THREADS}"
>  
>  python __anonymous() {
>      exclude_directories = d.getVar('SQUASHFS_EXCLUDE_DIRS').split()
> @@ -35,8 +33,9 @@ python __anonymous() {
>  }
>  
>  IMAGE_CMD:squashfs[depends] = "${PN}:do_transform_template"
> +IMAGE_CMD:squashfs[vardepsexclude] += "SQUASHFS_CREATION_LIMITS"
>  IMAGE_CMD:squashfs() {
>      ${SUDO_CHROOT} /bin/mksquashfs \
>          '${SQUASHFS_CONTENT}' '${IMAGE_FILE_CHROOT}' \
> -        -noappend ${SQUASHFS_DEFAULTS} ${SQUASHFS_CREATION_ARGS}
> +        -noappend ${SQUASHFS_CREATION_LIMITS}
> ${SQUASHFS_CREATION_ARGS}
>  }
Jan Kiszka May 29, 2023, 10:52 a.m. UTC | #2
On 29.05.23 12:24, MOESSBAUER, Felix (T CED INW-CN) wrote:
> On Sat, 2023-05-27 at 18:47 +0200, Jan Kiszka wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> While SQUASHFS_DEFAULTS was decoupled from its potentially unstable
>> inputs, SQUASHFS_THREADS and SQUASHFS_MEMLIMIT, the consumer of
>> SQUASHFS_DEFAULTS was not. Rework this moving the vardepsexclude to
>> IMAGE_CMD:squashfs. This also obsoletes the related vardepvalue and
>> vardepsexclude for SQUASHFS_THREADS and SQUASHFS_MEMLIMIT.
> 
> Why was this required? The consumer does not expand the variables and
> by that changes of e.g SQUASHFS_THREADS should not affect the hashes.
> 
>>
>> As we are now expecting only generation limits via that ignored
>> variable, rename it to SQUASHFS_CREATION_LIMITS and deny overwriting.
>> Only SQUASHFS_THREADS and SQUASHFS_MEMLIMIT are supposed to be tuned
>> to the build environment - if at all.
> 
> I'm not against this change, but I don't understand what issue is
> solved here.

The current code broke as soon as patch 2 ("Calculate a smarter default
SQUASHFS_MEMLIMIT") got applied and actually brought variation into the
variables, and that already during runtime (changing free memory ->
unstable task hashes). I'm sure you can reproduce the issue as well by
varying the number of available CPUs between builds.

Jan
diff mbox series

Patch

diff --git a/classes/squashfs.bbclass b/classes/squashfs.bbclass
index b14768c2..1f2a7595 100644
--- a/classes/squashfs.bbclass
+++ b/classes/squashfs.bbclass
@@ -1,7 +1,7 @@ 
 #
 # CIP Core, generic profile
 #
-# Copyright (c) Siemens AG, 2021-2022
+# Copyright (c) Siemens AG, 2021-2023
 #
 # Authors:
 #  Quirin Gylstorff <quirin.gylstorff@siemens.com>
@@ -16,11 +16,9 @@  SQUASHFS_CONTENT ?= "${PP_ROOTFS}"
 SQUASHFS_CREATION_ARGS ?= ""
 
 SQUASHFS_THREADS ?= "${@oe.utils.cpu_count(at_least=2)}"
-SQUASHFS_THREADS[vardepvalue] = "1"
 # default according to mksquasfs docs
 SQUASHFS_MEMLIMIT ?= "7982M"
-SQUASHFS_DEFAULTS ?= "-mem ${SQUASHFS_MEMLIMIT} -processors ${SQUASHFS_THREADS}"
-SQUASHFS_DEFAULTS[vardepsexclude] += "SQUASHFS_MEMLIMIT SQUASHFS_THREADS"
+SQUASHFS_CREATION_LIMITS = "-mem ${SQUASHFS_MEMLIMIT} -processors ${SQUASHFS_THREADS}"
 
 python __anonymous() {
     exclude_directories = d.getVar('SQUASHFS_EXCLUDE_DIRS').split()
@@ -35,8 +33,9 @@  python __anonymous() {
 }
 
 IMAGE_CMD:squashfs[depends] = "${PN}:do_transform_template"
+IMAGE_CMD:squashfs[vardepsexclude] += "SQUASHFS_CREATION_LIMITS"
 IMAGE_CMD:squashfs() {
     ${SUDO_CHROOT} /bin/mksquashfs \
         '${SQUASHFS_CONTENT}' '${IMAGE_FILE_CHROOT}' \
-        -noappend ${SQUASHFS_DEFAULTS} ${SQUASHFS_CREATION_ARGS}
+        -noappend ${SQUASHFS_CREATION_LIMITS} ${SQUASHFS_CREATION_ARGS}
 }