[PATCH 0/9] Add more signature cachability tests to the testsuite

[PATCH 0/9] Add more signature cachability tests to the testsuite

[kergoth]

From: Christopher Larson <chris.larson@siemens.com>

This series improves isar-sstate's lint command to support checking sigdata in tmp/stamps and to check for absolute paths in SRC_URI, and adds an additional test to the testsuite to exercise this capability. The new test checks for signature cachability issues much more quickly, as it does so without a full build, so it checks more target configurations. This uses bitbake's `-S none` option to avoid building anything, and then runs isar-sstate lint on the resulting sigdata in tmp/stamps.

This test will accept a verbose parameter on the command-line to pass --verbose to isar-sstate lint, which will show the "other" absolute paths found, not just source and build directory. This series also fixes two of the failures which were identified by the new test.

Here we can see the issues identified by the new test, which are fixed in this series:
```
$ avocado run testsuite/citest.py -t signatures --max-parallel-tasks=1
JOB ID     : 3a3308967946663d9b239f638b030502fa80ef0a
JOB LOG    : /builder/avocado/job-results/job-2024-03-29T19.24-3a33089/job.log
 (1/1) testsuite/citest.py:SignatureTest.test_signature_lint: STARTED
 (1/1) testsuite/citest.py:SignatureTest.test_signature_lint: FAIL: Detected cachability issues (42.93 s)
RESULTS    : PASS 0 | ERROR 0 | FAIL 1 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 0
JOB TIME   : 44.63 s

Test summary:
1-testsuite/citest.py:SignatureTest.test_signature_lint: FAIL
```

Applicable section of the avocado full.log:
```
1-testsuite/citest.py:SignatureTest.test_signature_lint: 2024-03-29 19:25:10,818 avocado.test cibase           L0142 ERROR| ==== issues found in ubuntu-focal-amd64:isar-bootstrap-target:unpack (95af4581) ====
1-testsuite/citest.py:SignatureTest.test_signature_lint: 2024-03-29 19:25:10,818 avocado.test cibase           L0142 ERROR| -> path in sources-dir: SRC_URI entry "file:///home/kergoth/Code/industrial/signatures/isar/meta-isar/conf/distro/ubuntu.public.key;sha256sum=36a38199a4bf4eae1e7f574891f7dfcb79b91b87a33a499383265e1224b5e989"
1-testsuite/citest.py:SignatureTest.test_signature_lint: 2024-03-29 19:25:10,818 avocado.test cibase           L0142 ERROR| ==== issues found in ubuntu-focal-amd64:isar-bootstrap-target:fetch (e8249cb2) ====
1-testsuite/citest.py:SignatureTest.test_signature_lint: 2024-03-29 19:25:10,818 avocado.test cibase           L0142 ERROR| -> path in sources-dir: SRC_URI entry "file:///home/kergoth/Code/industrial/signatures/isar/meta-isar/conf/distro/ubuntu.public.key;sha256sum=36a38199a4bf4eae1e7f574891f7dfcb79b91b87a33a499383265e1224b5e989"
1-testsuite/citest.py:SignatureTest.test_signature_lint: 2024-03-29 19:25:10,818 avocado.test cibase           L0142 ERROR| ==== issues found in debian-bullseye-amd64:isar-ci-ssh-setup:install (1c0a8b21) ====
1-testsuite/citest.py:SignatureTest.test_signature_lint: 2024-03-29 19:25:10,818 avocado.test cibase           L0142 ERROR| -> path in sources-dir: TESTSUITEDIR = "/home/kergoth/Code/industrial/signatures/isar/testsuite"
1-testsuite/citest.py:SignatureTest.test_signature_lint: 2024-03-29 19:25:10,818 avocado.test cibase           L0142 ERROR| found cachability issues (scanned 602 signatures)
1-testsuite/citest.py:SignatureTest.test_signature_lint: 2024-03-29 19:25:10,818 avocado.test cibase           L0142 ERROR| -> absolute paths: sources-dir 3, build-dir 0, other 184
```

If we strip off the prefix for legibility:
```
ERROR| ==== issues found in ubuntu-focal-amd64:isar-bootstrap-target:unpack (95af4581) ====
ERROR| -> path in sources-dir: SRC_URI entry "file:///home/kergoth/Code/industrial/signatures/isar/meta-isar/conf/distro/ubuntu.public.key;sha256sum=36a38199a4bf4eae1e7f574891f7dfcb79b91b87a33a499383265e1224b5e989"
ERROR| ==== issues found in ubuntu-focal-amd64:isar-bootstrap-target:fetch (e8249cb2) ====
ERROR| -> path in sources-dir: SRC_URI entry "file:///home/kergoth/Code/industrial/signatures/isar/meta-isar/conf/distro/ubuntu.public.key;sha256sum=36a38199a4bf4eae1e7f574891f7dfcb79b91b87a33a499383265e1224b5e989"
ERROR| ==== issues found in debian-bullseye-amd64:isar-ci-ssh-setup:install (1c0a8b21) ====
ERROR| -> path in sources-dir: TESTSUITEDIR = "/home/kergoth/Code/industrial/signatures/isar/testsuite"
ERROR| found cachability issues (scanned 602 signatures)
ERROR| -> absolute paths: sources-dir 3, build-dir 0, other 184
```

The sstate/signatures tests were run for each commit in this series using `git rebase -x` to ensure it does not break a bisect.

I welcome any and all feedback on this. In particular I don't love extending the hardcoded list of image tasks being ignored in isar-sstate lint, but it's a start. I'm open to suggestions.

Christopher Larson (9):
  isar-bootstrap: avoid forced early expansion of key vars
  isar-ci-ssh-setup: avoid abs path in signatures
  isar-sstate: lint: check for absolute paths in SRC_URI
  isar-sstate: lint: add support for checking stamps
  isar-sstate: lint: ignore more image tasks
  isar-sstate: add --excluded-tasks argument
  cibuilder.py: add -S support to the bitbake method
  testsuite: add perform_signature_lint method
  testsuite: add signature cachability checks

 .../isar-ci-ssh-setup_0.1.bb                  |  3 +
 .../isar-bootstrap/isar-bootstrap.inc         |  8 ++-
 scripts/isar-sstate                           | 60 +++++++++++++++----
 testsuite/cibase.py                           | 19 ++++++
 testsuite/cibuilder.py                        |  5 +-
 testsuite/citest.py                           | 22 ++++++-
 6 files changed, 100 insertions(+), 17 deletions(-)

-- 
2.39.2

[PATCH 1/9] isar-bootstrap: avoid forced early expansion of key vars

[kergoth]

From: Christopher Larson <chris.larson@seimens.com>

Rather than appending the items from the expanded key variables into
SRC_URI individually, which means there's no way to use tools like
vardepvalue or vardepexclude to control signature generation, append the
unexpanded variables to the SRC_URI directly. This avoids issues with
shared state reuse for the isar-bootstrap packages.

Signed-off-by: Christopher Larson <chris.larson@siemens.com>
---
 meta/recipes-core/isar-bootstrap/isar-bootstrap.inc | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc b/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc
index 17f19fd8..de14e946 100644
--- a/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc
+++ b/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc
@@ -10,7 +10,9 @@ LIC_FILES_CHKSUM = "file://${LAYERDIR_core}/licenses/COPYING.GPLv2;md5=751419260
 FILESPATH:prepend := "${THISDIR}/files:"
 SRC_URI = " \
     file://locale \
-    file://chroot-setup.sh"
+    file://chroot-setup.sh \
+    ${DISTRO_BOOTSTRAP_KEYS} \
+    ${THIRD_PARTY_APT_KEYS}"
 PV = "1.0"
 
 BOOTSTRAP_FOR_HOST ?= "0"
@@ -22,6 +24,8 @@ APTSRCS = "${WORKDIR}/apt-sources"
 APTSRCS_INIT = "${WORKDIR}/apt-sources-init"
 DISTRO_BOOTSTRAP_KEYFILES = ""
 THIRD_PARTY_APT_KEYFILES = ""
+DISTRO_BOOTSTRAP_KEYS ?= ""
+THIRD_PARTY_APT_KEYS ?= ""
 DEPLOY_ISAR_BOOTSTRAP ?= ""
 DISTRO_BOOTSTRAP_BASE_PACKAGES = "locales"
 DISTRO_BOOTSTRAP_BASE_PACKAGES:append:gnupg = ",gnupg"
@@ -48,13 +52,11 @@ python () {
             distro_bootstrap_keys += own_pub_key.split()
 
     for key in distro_bootstrap_keys:
-        d.appendVar("SRC_URI", " %s" % key)
         fetcher = bb.fetch2.Fetch([key], d)
         filename = os.path.relpath(fetcher.localpath(key), topdir)
         d.appendVar("DISTRO_BOOTSTRAP_KEYFILES", " ${TOPDIR}/%s" % filename)
 
     for key in third_party_apt_keys:
-        d.appendVar("SRC_URI", " %s" % key)
         fetcher = bb.fetch2.Fetch([key], d)
         filename = os.path.relpath(fetcher.localpath(key), topdir)
         d.appendVar("THIRD_PARTY_APT_KEYFILES", " ${TOPDIR}/%s" % filename)
-- 
2.39.2

[PATCH 2/9] isar-ci-ssh-setup: avoid abs path in signatures

[kergoth]

From: Christopher Larson <chris.larson@seimens.com>

TESTSUITEDIR is a full absolute path to the testsuite directory in isar,
as set in the environment by the build setup scripts. This is referenced
in the install task, which prevents shared state reuse for this package.
While this is predominently used in CI, it's still a good idea to avoid
absolute paths in signatures, so we can reuse shared state for this
package in other contexts.

Rather than excluding the TESTSUITEDIR from signatures entirely with
vardepsexclude, we can retain some information about the path by using
os.path.relpath to make it relative to the top directory of the build.
This is the same approach used by isar-bootstrap for the keys, and the
vardepvalue approach is also used elsewhere for layer paths.

Signed-off-by: Christopher Larson <chris.larson@siemens.com>
---
 .../recipes-ci/isar-ci-ssh-setup/isar-ci-ssh-setup_0.1.bb      | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/meta-test/recipes-ci/isar-ci-ssh-setup/isar-ci-ssh-setup_0.1.bb b/meta-test/recipes-ci/isar-ci-ssh-setup/isar-ci-ssh-setup_0.1.bb
index 4693f647..89100444 100644
--- a/meta-test/recipes-ci/isar-ci-ssh-setup/isar-ci-ssh-setup_0.1.bb
+++ b/meta-test/recipes-ci/isar-ci-ssh-setup/isar-ci-ssh-setup_0.1.bb
@@ -13,6 +13,9 @@ DEBIAN_DEPENDS = "adduser, apt (>= 0.4.2), network-manager, sshd-regen-keys"
 
 inherit dpkg-raw
 
+# Avoid absolute paths in signatures which prevent shared state reuse
+TESTSUITEDIR[vardepvalue] = "${@os.path.relpath('${TESTSUITEDIR}', '${TOPDIR}')}"
+
 do_install() {
     # Install authorized SSH keys
     install -v -d ${D}/var/lib/isar-ci/.ssh/
-- 
2.39.2

[PATCH 3/9] isar-sstate: lint: check for absolute paths in SRC_URI

[kergoth]

From: Christopher Larson <chris.larson@seimens.com>

In addition to the current checks for variables starting with an
absolute path, particularly those within the build or sources
directories, we should also check for absolute paths in SRC_URI
file entries.

Signed-off-by: Christopher Larson <chris.larson@siemens.com>
---
 scripts/isar-sstate | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/scripts/isar-sstate b/scripts/isar-sstate
index c14c2843..9b20cb8e 100755
--- a/scripts/isar-sstate
+++ b/scripts/isar-sstate
@@ -839,6 +839,23 @@ def sstate_lint(target, verbose, sources_dir, build_dir, exit_code, pedantic, **
                 continue
             # remove leading whitespaces possibly added by appending
             val = val.lstrip()
+            if name == 'SRC_URI':
+                src_uri = val.split()
+                for entry in src_uri:
+                    if entry.startswith('file:///'):
+                        entry_path = entry[7:]
+                        if entry_path.startswith(build_dir):
+                            pn_issues.append(f'\033[0;31m-> path in build-dir:   SRC_URI entry "{entry}"\033[0m')
+                            hits_builddir += 1
+                        elif entry_path.startswith(sources_dir):
+                            pn_issues.append(f'\033[0;31m-> path in sources-dir: SRC_URI entry "{entry}"\033[0m')
+                            hits_srcdir += 1
+                        else:
+                            hits_other += 1
+                            if verbose:
+                                pn_issues.append(f'\033[0;34m-> other absolute path: SRC_URI entry "{entry}"\033[0m')
+                continue
+
             if not val[0] == '/':
                 continue
             if val.startswith(build_dir):
-- 
2.39.2

[PATCH 4/9] isar-sstate: lint: add support for checking stamps

[kergoth]

From: Christopher Larson <chris.larson@seimens.com>

Bitbake supports writing signature data directly to the stamps directory
without having to build, so we should add the ability to lint this
signature data as well. This is useful for checking for cachability
issues without having to complete a build.

Signed-off-by: Christopher Larson <chris.larson@siemens.com>
---
 scripts/isar-sstate | 34 +++++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 11 deletions(-)

diff --git a/scripts/isar-sstate b/scripts/isar-sstate
index 9b20cb8e..b77f73eb 100755
--- a/scripts/isar-sstate
+++ b/scripts/isar-sstate
@@ -154,10 +154,12 @@ SstateCacheEntry = namedtuple(
 #                    "${PV}:${PR}:${SSTATE_PKGARCH}:${SSTATE_VERSION}:"
 
 # This regex extracts relevant fields:
-SstateRegex = re.compile(r'sstate:(?P<pn>[^:]*):[^:]*:[^:]*:[^:]*:'
+SstateRegex = re.compile(r'(.*/)?sstate:(?P<pn>[^:]*):[^:]*:[^:]*:[^:]*:'
                          r'(?P<arch>[^:]*):[^:]*:(?P<hash>[0-9a-f]*)_'
                          r'(?P<task>[^\.]*)\.(?P<suffix>.*)')
-
+StampsRegex = re.compile(
+    r"(.*/)?(?P<arch>[^/]+)/(?P<pn>[^/]+)/([^/]+)\.do_(?P<task>[^/]+)\.(?P<suffix>sigdata)\.(?P<hash>[0-9a-f]{64})"
+)
 
 class SstateTargetBase(object):
     def __init__(self, path, cached=False):
@@ -288,12 +290,13 @@ class SstateTargetBase(object):
 
 
 class SstateFileTarget(SstateTargetBase):
-    def __init__(self, path, **kwargs):
+    def __init__(self, path, regex=SstateRegex, **kwargs):
         super().__init__(path, **kwargs)
         if path.startswith('file://'):
             path = path[len('file://'):]
         self.path = path
         self.basepath = os.path.abspath(path)
+        self.regex = regex
 
     def __repr__(self):
         return f"file://{self.path}"
@@ -334,12 +337,13 @@ class SstateFileTarget(SstateTargetBase):
         for subdir, dirs, files in os.walk(self.basepath):
             reldir = subdir[(len(self.basepath)+1):]
             for f in files:
-                m = SstateRegex.match(f)
+                relative = os.path.join(reldir, f)
+                m = self.regex.match(relative)
                 if m is not None:
                     islink = os.path.islink(os.path.join(subdir, f))
                     age = int(now - os.path.getmtime(os.path.join(subdir, f)))
                     all_files.append(SstateCacheEntry(
-                        path=os.path.join(reldir, f),
+                        path=relative,
                         size=os.path.getsize(os.path.join(subdir, f)),
                         islink=islink,
                         age=age,
@@ -592,6 +596,9 @@ def arguments():
     parser.add_argument(
         '--exit-code', type=int, default=None,
         help="lint: return this instead of number of found issues")
+    parser.add_argument(
+        '--lint-stamps', default=False, action='store_true',
+        help="lint: assume target is a stamps directory (target must be a local path)")
 
     args = parser.parse_args()
     if args.command in 'upload analyze'.split() and args.source is None:
@@ -798,7 +805,7 @@ def sstate_analyze(source, target, **kwargs):
             print('\n'.join(out))
 
 
-def sstate_lint(target, verbose, sources_dir, build_dir, exit_code, pedantic, **kwargs):
+def sstate_lint(target, verbose, sources_dir, build_dir, exit_code, pedantic, lint_stamps, **kwargs):
     ADDITIONAL_IGNORED_VARNAMES = 'PP'.split()
     # only list non-cacheable tasks here
     # note that these still can break caching of other tasks that depend on these.
@@ -809,7 +816,10 @@ def sstate_lint(target, verbose, sources_dir, build_dir, exit_code, pedantic, **
         print(f"WARNING: target {target} does not exist. Nothing to analyze.")
         return 0
 
-    cache_sigs = {s.hash: s for s in target.list_all() if s.suffix.endswith('.siginfo')}
+    if lint_stamps:
+        cache_sigs = {s.hash: s for s in target.list_all()}
+    else:
+        cache_sigs = {s.hash: s for s in target.list_all() if s.suffix.endswith('.siginfo')}
 
     hits_srcdir = 0
     hits_builddir = 0
@@ -891,10 +901,12 @@ def main():
         target = SstateDavTarget(args.target)
     elif args.target.startswith('s3://'):
         target = SstateS3Target(args.target)
-    elif args.target.startswith('file://'):
-        target = SstateFileTarget(args.target)
-    else:  # no protocol given, assume file://
-        target = SstateFileTarget(args.target)
+    else:  # Either file://, or no protocol given, assume file://
+        target = SstateFileTarget(args.target, StampsRegex if args.lint_stamps else SstateRegex)
+
+    if args.lint_stamps and not isinstance(target, SstateFileTarget):
+        print("ERROR: --lint-stamps only works with local file targets")
+        return 1
 
     args.target = target
     return globals()[f'sstate_{args.command}'](**vars(args))
-- 
2.39.2

[PATCH 5/9] isar-sstate: lint: ignore more image tasks

[kergoth]

From: Christopher Larson <chris.larson@seimens.com>

Currently isar-sstate ignores image_wic, but not the other image tasks.
Add additional image tasks to align with this behavior.

Signed-off-by: Christopher Larson <chris.larson@siemens.com>
---
 scripts/isar-sstate | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/scripts/isar-sstate b/scripts/isar-sstate
index b77f73eb..5270f944 100755
--- a/scripts/isar-sstate
+++ b/scripts/isar-sstate
@@ -146,6 +146,8 @@ try:
 except ModuleNotFoundError:
     s3_supported = False
 
+DEFAULT_IGNORED_TASKS = "rootfs_wicenv image_wic image_cpio image_tar image_ext4"
+
 SstateCacheEntry = namedtuple(
         'SstateCacheEntry', 'hash path arch pn task suffix islink age size'.split())
 
@@ -811,7 +813,7 @@ def sstate_lint(target, verbose, sources_dir, build_dir, exit_code, pedantic, li
     # note that these still can break caching of other tasks that depend on these.
     # Run in pedantic mode to also look for these issues (e.g. in image-in-image builds)
     # To make a build fully cacheable, avoid absolute paths in BBLAYERS
-    ADDITIONAL_IGNORED_TASKS = list() if pedantic else 'rootfs_wicenv image_wic'.split()
+    ADDITIONAL_IGNORED_TASKS = list() if pedantic else DEFAULT_IGNORED_TASKS.split()
     if not target.exists():
         print(f"WARNING: target {target} does not exist. Nothing to analyze.")
         return 0
-- 
2.39.2

[PATCH 6/9] isar-sstate: add --excluded-tasks argument

[kergoth]

From: Christopher Larson <chris.larson@seimens.com>

This allows the user to override the default lists of tasks to ignore
when linting the sstate cache.

Signed-off-by: Christopher Larson <chris.larson@siemens.com>
---
 scripts/isar-sstate | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/scripts/isar-sstate b/scripts/isar-sstate
index 5270f944..dddfafcb 100755
--- a/scripts/isar-sstate
+++ b/scripts/isar-sstate
@@ -146,7 +146,7 @@ try:
 except ModuleNotFoundError:
     s3_supported = False
 
-DEFAULT_IGNORED_TASKS = "rootfs_wicenv image_wic image_cpio image_tar image_ext4"
+DEFAULT_IGNORED_TASKS = "rootfs_wicenv,image_wic,image_cpio,image_tar,image_ext4"
 
 SstateCacheEntry = namedtuple(
         'SstateCacheEntry', 'hash path arch pn task suffix islink age size'.split())
@@ -601,6 +601,9 @@ def arguments():
     parser.add_argument(
         '--lint-stamps', default=False, action='store_true',
         help="lint: assume target is a stamps directory (target must be a local path)")
+    parser.add_argument(
+        '--excluded-tasks', type=str, default=DEFAULT_IGNORED_TASKS,
+        help="lint: comma-separated list of tasks to ignore (default: %(default)s)")
 
     args = parser.parse_args()
     if args.command in 'upload analyze'.split() and args.source is None:
@@ -609,6 +612,7 @@ def arguments():
     elif args.command in 'info clean'.split() and args.source is not None:
         print(f"ERROR: '{args.command}' must not have a source (only a target)")
         sys.exit(1)
+    args.excluded_tasks = args.excluded_tasks.split(',')
     return args
 
 
@@ -807,13 +811,14 @@ def sstate_analyze(source, target, **kwargs):
             print('\n'.join(out))
 
 
-def sstate_lint(target, verbose, sources_dir, build_dir, exit_code, pedantic, lint_stamps, **kwargs):
+def sstate_lint(target, verbose, sources_dir, build_dir, exit_code, pedantic, lint_stamps,
+                excluded_tasks, **kwargs):
     ADDITIONAL_IGNORED_VARNAMES = 'PP'.split()
     # only list non-cacheable tasks here
     # note that these still can break caching of other tasks that depend on these.
     # Run in pedantic mode to also look for these issues (e.g. in image-in-image builds)
     # To make a build fully cacheable, avoid absolute paths in BBLAYERS
-    ADDITIONAL_IGNORED_TASKS = list() if pedantic else DEFAULT_IGNORED_TASKS.split()
+    ADDITIONAL_IGNORED_TASKS = list() if pedantic else excluded_tasks
     if not target.exists():
         print(f"WARNING: target {target} does not exist. Nothing to analyze.")
         return 0
-- 
2.39.2

[PATCH 7/9] cibuilder.py: add -S support to the bitbake method

[kergoth]

From: Christopher Larson <chris.larson@seimens.com>

This allows a test writer to call `bitbake -S none`, to generate
signature data in tmp/stamps.

Signed-off-by: Christopher Larson <chris.larson@siemens.com>
---
 testsuite/cibuilder.py | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/testsuite/cibuilder.py b/testsuite/cibuilder.py
index fa30c2f5..e968d14d 100755
--- a/testsuite/cibuilder.py
+++ b/testsuite/cibuilder.py
@@ -188,7 +188,7 @@ def move_in_build_dir(self, src, dst):
         if os.path.exists(self.build_dir + '/' + src):
             shutil.move(self.build_dir + '/' + src, self.build_dir + '/' + dst)
 
-    def bitbake(self, target, bitbake_cmd=None, **kwargs):
+    def bitbake(self, target, bitbake_cmd=None, sig_handler=None, **kwargs):
         self.check_init()
         self.log.info('===================================================')
         self.log.info('Building ' + str(target))
@@ -200,6 +200,9 @@ def bitbake(self, target, bitbake_cmd=None, **kwargs):
         if bitbake_cmd:
             cmdline.append('-c')
             cmdline.append(bitbake_cmd)
+        if sig_handler:
+            cmdline.append('-S')
+            cmdline.append(sig_handler)
         if isinstance(target, list):
             cmdline.extend(target)
         else:
-- 
2.39.2

[PATCH 8/9] testsuite: add perform_signature_lint method

[kergoth]

From: Christopher Larson <chris.larson@seimens.com>

This method is provided to generate signature data for specified target
or targets and check for cachability issues without having to complete a
build.

Signed-off-by: Christopher Larson <chris.larson@siemens.com>
---
 testsuite/cibase.py | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/testsuite/cibase.py b/testsuite/cibase.py
index 90591f32..349a79f0 100755
--- a/testsuite/cibase.py
+++ b/testsuite/cibase.py
@@ -125,6 +125,25 @@ def perform_sstate_populate(self, image_target, **kwargs):
         # Remove isar configuration so the the following test creates a new one
         self.delete_from_build_dir('conf')
 
+    def perform_signature_lint(self, targets, verbose=False, sources_dir=isar_root,
+                               excluded_tasks=None, **kwargs):
+        """Generate signature data for target(s) and check for cachability issues."""
+        self.configure(**kwargs)
+        self.move_in_build_dir("tmp", "tmp_before_sstate")
+        self.bitbake(targets, sig_handler="none")
+
+        verbose_arg = "--verbose" if verbose else ""
+        excluded_arg = f"--excluded-tasks {','.join(excluded_tasks)}" if excluded_tasks else ""
+        cmd = f"{isar_root}/scripts/isar-sstate lint --lint-stamps {self.build_dir}/tmp/stamps " \
+              f"--build-dir {self.build_dir} --sources-dir {sources_dir} {verbose_arg} {excluded_arg}"
+        self.log.info(f"Running: {cmd}")
+        exit_status, output = process.getstatusoutput(cmd, ignore_status=True)
+        if exit_status > 0:
+            ansi_escape = re.compile(r'\x1B(?:[@-Z\\-_]|\[[0-?]*[ -/]*[@-~])')
+            for line in output.splitlines():
+                self.log.error(ansi_escape.sub('', line))
+            self.fail("Detected cachability issues")
+
     def perform_sstate_test(self, image_target, package_target, **kwargs):
         def check_executed_tasks(target, expected):
             taskorder_file = glob.glob(f'{self.build_dir}/tmp/work/*/{target}/*/temp/log.task_order')
-- 
2.39.2

[PATCH 9/9] testsuite: add signature cachability checks

[kergoth]

From: Christopher Larson <chris.larson@seimens.com>

The current sstate tests do call `isar-sstate lint`` after populating
the shared state cache, but those tests take some time to do so, and do
not check for cachability issues for other targets, so add a new test
which does so.

Signed-off-by: Christopher Larson <chris.larson@siemens.com>
---
 testsuite/citest.py | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/testsuite/citest.py b/testsuite/citest.py
index 7e24c498..b1c0b760 100755
--- a/testsuite/citest.py
+++ b/testsuite/citest.py
@@ -290,8 +290,28 @@ def test_container_sdk(self):
         self.init()
         self.perform_build_test(targets, bitbake_cmd='do_populate_sdk', container=True)
 
-class SstateTest(CIBaseTest):
+class SignatureTest(CIBaseTest):
+    """
+    Test for signature cachability issues which prevent shared state reuse.
 
+    SstateTest also checks for these, but this test is faster and will check more cases.
+
+    :avocado: tags=signatures,sstate
+    """
+    def test_signature_lint(self):
+        verbose = bool(int(self.params.get("verbose", default=0)))
+        targets = [
+            'mc:qemuamd64-bullseye:isar-image-ci',
+            'mc:qemuarm-bullseye:isar-image-base',
+            'mc:qemuarm-bullseye:isar-image-base:do_populate_sdk',
+            'mc:qemuarm64-bullseye:isar-image-base',
+            'mc:qemuamd64-focal:isar-image-base'
+                  ]
+
+        self.init()
+        self.perform_signature_lint(targets, verbose=verbose)
+
+class SstateTest(CIBaseTest):
     """
     Test builds with artifacts taken from sstate cache
 
-- 
2.39.2

Re: [PATCH 1/9] isar-bootstrap: avoid forced early expansion of key vars

[MOESSBAUER, Felix]

On Tue, 2024-04-02 at 17:28 +0000, kergoth@gmail.com wrote:
> From: Christopher Larson <chris.larson@seimens.com>
------------------------------------------^

Hi, please resend the whole series with a correct "From:".

Felix

> 
> Rather than appending the items from the expanded key variables into
> SRC_URI individually, which means there's no way to use tools like
> vardepvalue or vardepexclude to control signature generation, append
> the
> unexpanded variables to the SRC_URI directly. This avoids issues with
> shared state reuse for the isar-bootstrap packages.
> 
> Signed-off-by: Christopher Larson <chris.larson@siemens.com>
> ---
>  meta/recipes-core/isar-bootstrap/isar-bootstrap.inc | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc
> b/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc
> index 17f19fd8..de14e946 100644
> --- a/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc
> +++ b/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc
> @@ -10,7 +10,9 @@ LIC_FILES_CHKSUM =
> "file://${LAYERDIR_core}/licenses/COPYING.GPLv2;md5=751419260
>  FILESPATH:prepend := "${THISDIR}/files:"
>  SRC_URI = " \
>      file://locale \
> -    file://chroot-setup.sh"
> +    file://chroot-setup.sh \
> +    ${DISTRO_BOOTSTRAP_KEYS} \
> +    ${THIRD_PARTY_APT_KEYS}"
>  PV = "1.0"
>  
>  BOOTSTRAP_FOR_HOST ?= "0"
> @@ -22,6 +24,8 @@ APTSRCS = "${WORKDIR}/apt-sources"
>  APTSRCS_INIT = "${WORKDIR}/apt-sources-init"
>  DISTRO_BOOTSTRAP_KEYFILES = ""
>  THIRD_PARTY_APT_KEYFILES = ""
> +DISTRO_BOOTSTRAP_KEYS ?= ""
> +THIRD_PARTY_APT_KEYS ?= ""
>  DEPLOY_ISAR_BOOTSTRAP ?= ""
>  DISTRO_BOOTSTRAP_BASE_PACKAGES = "locales"
>  DISTRO_BOOTSTRAP_BASE_PACKAGES:append:gnupg = ",gnupg"
> @@ -48,13 +52,11 @@ python () {
>              distro_bootstrap_keys += own_pub_key.split()
>  
>      for key in distro_bootstrap_keys:
> -        d.appendVar("SRC_URI", " %s" % key)
>          fetcher = bb.fetch2.Fetch([key], d)
>          filename = os.path.relpath(fetcher.localpath(key), topdir)
>          d.appendVar("DISTRO_BOOTSTRAP_KEYFILES", " ${TOPDIR}/%s" %
> filename)
>  
>      for key in third_party_apt_keys:
> -        d.appendVar("SRC_URI", " %s" % key)
>          fetcher = bb.fetch2.Fetch([key], d)
>          filename = os.path.relpath(fetcher.localpath(key), topdir)
>          d.appendVar("THIRD_PARTY_APT_KEYFILES", " ${TOPDIR}/%s" %
> filename)
> -- 
> 2.39.2
> 

-- 
Siemens AG, Technology
Linux Expert Center

Re: [PATCH 3/9] isar-sstate: lint: check for absolute paths in SRC_URI

[MOESSBAUER, Felix]

On Tue, 2024-04-02 at 17:28 +0000, kergoth@gmail.com wrote:
> From: Christopher Larson <chris.larson@seimens.com>
> 
> In addition to the current checks for variables starting with an
> absolute path, particularly those within the build or sources
> directories, we should also check for absolute paths in SRC_URI
> file entries.

Good idea. Thanks for adding.

Acked-by: Felix Moessbauer <felix.moessbauer@siemens.com>

> 
> Signed-off-by: Christopher Larson <chris.larson@siemens.com>
> ---
>  scripts/isar-sstate | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/scripts/isar-sstate b/scripts/isar-sstate
> index c14c2843..9b20cb8e 100755
> --- a/scripts/isar-sstate
> +++ b/scripts/isar-sstate
> @@ -839,6 +839,23 @@ def sstate_lint(target, verbose, sources_dir,
> build_dir, exit_code, pedantic, **
>                  continue
>              # remove leading whitespaces possibly added by appending
>              val = val.lstrip()
> +            if name == 'SRC_URI':
> +                src_uri = val.split()
> +                for entry in src_uri:
> +                    if entry.startswith('file:///'):
> +                        entry_path = entry[7:]
> +                        if entry_path.startswith(build_dir):
> +                            pn_issues.append(f'\033[0;31m-> path in
> build-dir:   SRC_URI entry "{entry}"\033[0m')
> +                            hits_builddir += 1
> +                        elif entry_path.startswith(sources_dir):
> +                            pn_issues.append(f'\033[0;31m-> path in
> sources-dir: SRC_URI entry "{entry}"\033[0m')
> +                            hits_srcdir += 1
> +                        else:
> +                            hits_other += 1
> +                            if verbose:
> +                                pn_issues.append(f'\033[0;34m->
> other absolute path: SRC_URI entry "{entry}"\033[0m')
> +                continue
> +
>              if not val[0] == '/':
>                  continue
>              if val.startswith(build_dir):
> -- 
> 2.39.2
> 

-- 
Siemens AG, Technology
Linux Expert Center

Re: [PATCH 4/9] isar-sstate: lint: add support for checking stamps

[MOESSBAUER, Felix]

On Tue, 2024-04-02 at 17:28 +0000, kergoth@gmail.com wrote:
> From: Christopher Larson <chris.larson@seimens.com>
> 
> Bitbake supports writing signature data directly to the stamps
> directory
> without having to build, so we should add the ability to lint this
> signature data as well. This is useful for checking for cachability
> issues without having to complete a build.

Hi,

I guess that is enabled with the '--dump-signatures=' parameter. Can we
please add a brief example (one-line) to the linter script for this
case.

> 
> Signed-off-by: Christopher Larson <chris.larson@siemens.com>
> ---
>  scripts/isar-sstate | 34 +++++++++++++++++++++++-----------
>  1 file changed, 23 insertions(+), 11 deletions(-)
> 
> diff --git a/scripts/isar-sstate b/scripts/isar-sstate
> index 9b20cb8e..b77f73eb 100755
> --- a/scripts/isar-sstate
> +++ b/scripts/isar-sstate
> @@ -154,10 +154,12 @@ SstateCacheEntry = namedtuple(
>  #                   
> "${PV}:${PR}:${SSTATE_PKGARCH}:${SSTATE_VERSION}:"
>  
>  # This regex extracts relevant fields:
> -SstateRegex = re.compile(r'sstate:(?P<pn>[^:]*):[^:]*:[^:]*:[^:]*:'
> +SstateRegex =
> re.compile(r'(.*/)?sstate:(?P<pn>[^:]*):[^:]*:[^:]*:[^:]*:'
>                           r'(?P<arch>[^:]*):[^:]*:(?P<hash>[0-9a-
> f]*)_'
>                           r'(?P<task>[^\.]*)\.(?P<suffix>.*)')
> -
> +StampsRegex = re.compile(
> + 

Is this regex hand-crafted or can it be found somewhere in bitbake as
well? Is the format documented in bitbake? I'm a bit worried that this
might change across versions. If so, please add a note that this might
need to be adjusted on bitbake updates.

Felix

>   
> r"(.*/)?(?P<arch>[^/]+)/(?P<pn>[^/]+)/([^/]+)\.do_(?P<task>[^/]+)\.(?
> P<suffix>sigdata)\.(?P<hash>[0-9a-f]{64})"
> +)
>  
>  class SstateTargetBase(object):
>      def __init__(self, path, cached=False):
> @@ -288,12 +290,13 @@ class SstateTargetBase(object):
>  
>  
>  class SstateFileTarget(SstateTargetBase):
> -    def __init__(self, path, **kwargs):
> +    def __init__(self, path, regex=SstateRegex, **kwargs):
>          super().__init__(path, **kwargs)
>          if path.startswith('file://'):
>              path = path[len('file://'):]
>          self.path = path
>          self.basepath = os.path.abspath(path)
> +        self.regex = regex
>  
>      def __repr__(self):
>          return f"file://{self.path}"
> @@ -334,12 +337,13 @@ class SstateFileTarget(SstateTargetBase):
>          for subdir, dirs, files in os.walk(self.basepath):
>              reldir = subdir[(len(self.basepath)+1):]
>              for f in files:
> -                m = SstateRegex.match(f)
> +                relative = os.path.join(reldir, f)
> +                m = self.regex.match(relative)
>                  if m is not None:
>                      islink = os.path.islink(os.path.join(subdir, f))
>                      age = int(now -
> os.path.getmtime(os.path.join(subdir, f)))
>                      all_files.append(SstateCacheEntry(
> -                        path=os.path.join(reldir, f),
> +                        path=relative,
>                          size=os.path.getsize(os.path.join(subdir,
> f)),
>                          islink=islink,
>                          age=age,
> @@ -592,6 +596,9 @@ def arguments():
>      parser.add_argument(
>          '--exit-code', type=int, default=None,
>          help="lint: return this instead of number of found issues")
> +    parser.add_argument(
> +        '--lint-stamps', default=False, action='store_true',
> +        help="lint: assume target is a stamps directory (target must
> be a local path)")
>  
>      args = parser.parse_args()
>      if args.command in 'upload analyze'.split() and args.source is
> None:
> @@ -798,7 +805,7 @@ def sstate_analyze(source, target, **kwargs):
>              print('\n'.join(out))
>  
>  
> -def sstate_lint(target, verbose, sources_dir, build_dir, exit_code,
> pedantic, **kwargs):
> +def sstate_lint(target, verbose, sources_dir, build_dir, exit_code,
> pedantic, lint_stamps, **kwargs):
>      ADDITIONAL_IGNORED_VARNAMES = 'PP'.split()
>      # only list non-cacheable tasks here
>      # note that these still can break caching of other tasks that
> depend on these.
> @@ -809,7 +816,10 @@ def sstate_lint(target, verbose, sources_dir,
> build_dir, exit_code, pedantic, **
>          print(f"WARNING: target {target} does not exist. Nothing to
> analyze.")
>          return 0
>  
> -    cache_sigs = {s.hash: s for s in target.list_all() if
> s.suffix.endswith('.siginfo')}
> +    if lint_stamps:
> +        cache_sigs = {s.hash: s for s in target.list_all()}
> +    else:
> +        cache_sigs = {s.hash: s for s in target.list_all() if
> s.suffix.endswith('.siginfo')}
>  
>      hits_srcdir = 0
>      hits_builddir = 0
> @@ -891,10 +901,12 @@ def main():
>          target = SstateDavTarget(args.target)
>      elif args.target.startswith('s3://'):
>          target = SstateS3Target(args.target)
> -    elif args.target.startswith('file://'):
> -        target = SstateFileTarget(args.target)
> -    else:  # no protocol given, assume file://
> -        target = SstateFileTarget(args.target)
> +    else:  # Either file://, or no protocol given, assume file://
> +        target = SstateFileTarget(args.target, StampsRegex if
> args.lint_stamps else SstateRegex)
> +
> +    if args.lint_stamps and not isinstance(target,
> SstateFileTarget):
> +        print("ERROR: --lint-stamps only works with local file
> targets")
> +        return 1
>  
>      args.target = target
>      return globals()[f'sstate_{args.command}'](**vars(args))
> -- 
> 2.39.2
> 

-- 
Siemens AG, Technology
Linux Expert Center

Re: [PATCH 5/9] isar-sstate: lint: ignore more image tasks

[MOESSBAUER, Felix]

On Tue, 2024-04-02 at 17:28 +0000, kergoth@gmail.com wrote:
> From: Christopher Larson <chris.larson@seimens.com>
> 
> Currently isar-sstate ignores image_wic, but not the other image
> tasks.
> Add additional image tasks to align with this behavior.
> 
> Signed-off-by: Christopher Larson <chris.larson@siemens.com>
> ---
>  scripts/isar-sstate | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/scripts/isar-sstate b/scripts/isar-sstate
> index b77f73eb..5270f944 100755
> --- a/scripts/isar-sstate
> +++ b/scripts/isar-sstate
> @@ -146,6 +146,8 @@ try:
>  except ModuleNotFoundError:
>      s3_supported = False
>  
> +DEFAULT_IGNORED_TASKS = "rootfs_wicenv image_wic image_cpio
> image_tar image_ext4"

It might be better to ignore all image tasks, like "image_*". But as we
have more and more image typedeps chains (e.g. image_squashfs ->
image_verity -> image_wic), I'm wondering if ignoring this part still
makes sense. Maybe Adriaan can comment on this.

Felix

> +
>  SstateCacheEntry = namedtuple(
>          'SstateCacheEntry', 'hash path arch pn task suffix islink
> age size'.split())
>  
> @@ -811,7 +813,7 @@ def sstate_lint(target, verbose, sources_dir,
> build_dir, exit_code, pedantic, li
>      # note that these still can break caching of other tasks that
> depend on these.
>      # Run in pedantic mode to also look for these issues (e.g. in
> image-in-image builds)
>      # To make a build fully cacheable, avoid absolute paths in
> BBLAYERS
> -    ADDITIONAL_IGNORED_TASKS = list() if pedantic else
> 'rootfs_wicenv image_wic'.split()
> +    ADDITIONAL_IGNORED_TASKS = list() if pedantic else
> DEFAULT_IGNORED_TASKS.split()
>      if not target.exists():
>          print(f"WARNING: target {target} does not exist. Nothing to
> analyze.")
>          return 0
> -- 
> 2.39.2
> 

-- 
Siemens AG, Technology
Linux Expert Center

Re: [PATCH 6/9] isar-sstate: add --excluded-tasks argument

[MOESSBAUER, Felix]

On Tue, 2024-04-02 at 17:28 +0000, kergoth@gmail.com wrote:
> From: Christopher Larson <chris.larson@seimens.com>
> 
> This allows the user to override the default lists of tasks to ignore
> when linting the sstate cache.
> 
> Signed-off-by: Christopher Larson <chris.larson@siemens.com>
> ---
>  scripts/isar-sstate | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/scripts/isar-sstate b/scripts/isar-sstate
> index 5270f944..dddfafcb 100755
> --- a/scripts/isar-sstate
> +++ b/scripts/isar-sstate
> @@ -146,7 +146,7 @@ try:
>  except ModuleNotFoundError:
>      s3_supported = False
>  
> -DEFAULT_IGNORED_TASKS = "rootfs_wicenv image_wic image_cpio
> image_tar image_ext4"
> +DEFAULT_IGNORED_TASKS =
> "rootfs_wicenv,image_wic,image_cpio,image_tar,image_ext4"

Also here, it might be good to support wildcards, e.g. 'image_*'.
Apart from that minor detail, this is a valuable addition.

Acked-by: Felix Moessbauer <felix.moessbauer@siemens.com>

Felix

>  
>  SstateCacheEntry = namedtuple(
>          'SstateCacheEntry', 'hash path arch pn task suffix islink
> age size'.split())
> @@ -601,6 +601,9 @@ def arguments():
>      parser.add_argument(
>          '--lint-stamps', default=False, action='store_true',
>          help="lint: assume target is a stamps directory (target must
> be a local path)")
> +    parser.add_argument(
> +        '--excluded-tasks', type=str, default=DEFAULT_IGNORED_TASKS,
> +        help="lint: comma-separated list of tasks to ignore
> (default: %(default)s)")
>  
>      args = parser.parse_args()
>      if args.command in 'upload analyze'.split() and args.source is
> None:
> @@ -609,6 +612,7 @@ def arguments():
>      elif args.command in 'info clean'.split() and args.source is not
> None:
>          print(f"ERROR: '{args.command}' must not have a source (only
> a target)")
>          sys.exit(1)
> +    args.excluded_tasks = args.excluded_tasks.split(',')
>      return args
>  
>  
> @@ -807,13 +811,14 @@ def sstate_analyze(source, target, **kwargs):
>              print('\n'.join(out))
>  
>  
> -def sstate_lint(target, verbose, sources_dir, build_dir, exit_code,
> pedantic, lint_stamps, **kwargs):
> +def sstate_lint(target, verbose, sources_dir, build_dir, exit_code,
> pedantic, lint_stamps,
> +                excluded_tasks, **kwargs):
>      ADDITIONAL_IGNORED_VARNAMES = 'PP'.split()
>      # only list non-cacheable tasks here
>      # note that these still can break caching of other tasks that
> depend on these.
>      # Run in pedantic mode to also look for these issues (e.g. in
> image-in-image builds)
>      # To make a build fully cacheable, avoid absolute paths in
> BBLAYERS
> -    ADDITIONAL_IGNORED_TASKS = list() if pedantic else
> DEFAULT_IGNORED_TASKS.split()
> +    ADDITIONAL_IGNORED_TASKS = list() if pedantic else
> excluded_tasks
>      if not target.exists():
>          print(f"WARNING: target {target} does not exist. Nothing to
> analyze.")
>          return 0
> -- 
> 2.39.2
> 

-- 
Siemens AG, Technology
Linux Expert Center

Re: [PATCH 7/9] cibuilder.py: add -S support to the bitbake method

[MOESSBAUER, Felix]

On Tue, 2024-04-02 at 17:28 +0000, kergoth@gmail.com wrote:
> From: Christopher Larson <chris.larson@seimens.com>
> 
> This allows a test writer to call `bitbake -S none`, to generate
> signature data in tmp/stamps.

Is there a particular reason why we don't use the long version of the
parameters? In case this parameter ever changes, it is much easier to
find all locations if the long form is used.

Felix

> 
> Signed-off-by: Christopher Larson <chris.larson@siemens.com>
> ---
>  testsuite/cibuilder.py | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/testsuite/cibuilder.py b/testsuite/cibuilder.py
> index fa30c2f5..e968d14d 100755
> --- a/testsuite/cibuilder.py
> +++ b/testsuite/cibuilder.py
> @@ -188,7 +188,7 @@ def move_in_build_dir(self, src, dst):
>          if os.path.exists(self.build_dir + '/' + src):
>              shutil.move(self.build_dir + '/' + src, self.build_dir +
> '/' + dst)
>  
> -    def bitbake(self, target, bitbake_cmd=None, **kwargs):
> +    def bitbake(self, target, bitbake_cmd=None, sig_handler=None,
> **kwargs):
>          self.check_init()
>         
> self.log.info('===================================================')
>          self.log.info('Building ' + str(target))
> @@ -200,6 +200,9 @@ def bitbake(self, target, bitbake_cmd=None,
> **kwargs):
>          if bitbake_cmd:
>              cmdline.append('-c')
>              cmdline.append(bitbake_cmd)
> +        if sig_handler:
> +            cmdline.append('-S')
> +            cmdline.append(sig_handler)
>          if isinstance(target, list):
>              cmdline.extend(target)
>          else:
> -- 
> 2.39.2
> 

-- 
Siemens AG, Technology
Linux Expert Center

RE: [PATCH 7/9] cibuilder.py: add -S support to the bitbake method

[Larson, Chris]

No real reason, I'll update it. Thanks!

-----Original Message-----
From: Moessbauer, Felix (T CED OES-DE) <felix.moessbauer@siemens.com> 
Sent: Wednesday, April 3, 2024 12:13 AM
To: kergoth@gmail.com; isar-users@googlegroups.com
Cc: Larson, Chris (DI CTO FDS CES LX MEL) <chris.larson@siemens.com>; chris.larson@seimens.com
Subject: Re: [PATCH 7/9] cibuilder.py: add -S support to the bitbake method

On Tue, 2024-04-02 at 17:28 +0000, kergoth@gmail.com wrote:
> From: Christopher Larson <chris.larson@seimens.com>
> 
> This allows a test writer to call `bitbake -S none`, to generate 
> signature data in tmp/stamps.

Is there a particular reason why we don't use the long version of the parameters? In case this parameter ever changes, it is much easier to find all locations if the long form is used.

Felix

> 
> Signed-off-by: Christopher Larson <chris.larson@siemens.com>
> ---
>  testsuite/cibuilder.py | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/testsuite/cibuilder.py b/testsuite/cibuilder.py index 
> fa30c2f5..e968d14d 100755
> --- a/testsuite/cibuilder.py
> +++ b/testsuite/cibuilder.py
> @@ -188,7 +188,7 @@ def move_in_build_dir(self, src, dst):
>          if os.path.exists(self.build_dir + '/' + src):
>              shutil.move(self.build_dir + '/' + src, self.build_dir + 
> '/' + dst)
>  
> -    def bitbake(self, target, bitbake_cmd=None, **kwargs):
> +    def bitbake(self, target, bitbake_cmd=None, sig_handler=None,
> **kwargs):
>          self.check_init()
>         
> self.log.info('===================================================')
>          self.log.info('Building ' + str(target)) @@ -200,6 +200,9 @@ 
> def bitbake(self, target, bitbake_cmd=None,
> **kwargs):
>          if bitbake_cmd:
>              cmdline.append('-c')
>              cmdline.append(bitbake_cmd)
> +        if sig_handler:
> +            cmdline.append('-S')
> +            cmdline.append(sig_handler)
>          if isinstance(target, list):
>              cmdline.extend(target)
>          else:
> --
> 2.39.2
> 

-- 
Siemens AG, Technology
Linux Expert Center

RE: [PATCH 6/9] isar-sstate: add --excluded-tasks argument

[Larson, Chris]

Good idea, I can include that in the v2 if there's no objection.

-----Original Message-----
From: Moessbauer, Felix (T CED OES-DE) <felix.moessbauer@siemens.com> 
Sent: Wednesday, April 3, 2024 12:11 AM
To: kergoth@gmail.com; isar-users@googlegroups.com
Cc: Larson, Chris (DI CTO FDS CES LX MEL) <chris.larson@siemens.com>
Subject: Re: [PATCH 6/9] isar-sstate: add --excluded-tasks argument

On Tue, 2024-04-02 at 17:28 +0000, kergoth@gmail.com wrote:
> From: Christopher Larson <chris.larson@seimens.com>
> 
> This allows the user to override the default lists of tasks to ignore 
> when linting the sstate cache.
> 
> Signed-off-by: Christopher Larson <chris.larson@siemens.com>
> ---
>  scripts/isar-sstate | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/scripts/isar-sstate b/scripts/isar-sstate index 
> 5270f944..dddfafcb 100755
> --- a/scripts/isar-sstate
> +++ b/scripts/isar-sstate
> @@ -146,7 +146,7 @@ try:
>  except ModuleNotFoundError:
>      s3_supported = False
>  
> -DEFAULT_IGNORED_TASKS = "rootfs_wicenv image_wic image_cpio image_tar 
> image_ext4"
> +DEFAULT_IGNORED_TASKS =
> "rootfs_wicenv,image_wic,image_cpio,image_tar,image_ext4"

Also here, it might be good to support wildcards, e.g. 'image_*'.
Apart from that minor detail, this is a valuable addition.

Acked-by: Felix Moessbauer <felix.moessbauer@siemens.com>

Felix

>  
>  SstateCacheEntry = namedtuple(
>          'SstateCacheEntry', 'hash path arch pn task suffix islink age 
> size'.split()) @@ -601,6 +601,9 @@ def arguments():
>      parser.add_argument(
>          '--lint-stamps', default=False, action='store_true',
>          help="lint: assume target is a stamps directory (target must 
> be a local path)")
> +    parser.add_argument(
> +        '--excluded-tasks', type=str, default=DEFAULT_IGNORED_TASKS,
> +        help="lint: comma-separated list of tasks to ignore
> (default: %(default)s)")
>  
>      args = parser.parse_args()
>      if args.command in 'upload analyze'.split() and args.source is
> None:
> @@ -609,6 +612,7 @@ def arguments():
>      elif args.command in 'info clean'.split() and args.source is not
> None:
>          print(f"ERROR: '{args.command}' must not have a source (only 
> a target)")
>          sys.exit(1)
> +    args.excluded_tasks = args.excluded_tasks.split(',')
>      return args
>  
>  
> @@ -807,13 +811,14 @@ def sstate_analyze(source, target, **kwargs):
>              print('\n'.join(out))
>  
>  
> -def sstate_lint(target, verbose, sources_dir, build_dir, exit_code, 
> pedantic, lint_stamps, **kwargs):
> +def sstate_lint(target, verbose, sources_dir, build_dir, exit_code,
> pedantic, lint_stamps,
> +                excluded_tasks, **kwargs):
>      ADDITIONAL_IGNORED_VARNAMES = 'PP'.split()
>      # only list non-cacheable tasks here
>      # note that these still can break caching of other tasks that 
> depend on these.
>      # Run in pedantic mode to also look for these issues (e.g. in 
> image-in-image builds)
>      # To make a build fully cacheable, avoid absolute paths in 
> BBLAYERS
> -    ADDITIONAL_IGNORED_TASKS = list() if pedantic else
> DEFAULT_IGNORED_TASKS.split()
> +    ADDITIONAL_IGNORED_TASKS = list() if pedantic else
> excluded_tasks
>      if not target.exists():
>          print(f"WARNING: target {target} does not exist. Nothing to
> analyze.")
>          return 0
> --
> 2.39.2
> 

-- 
Siemens AG, Technology
Linux Expert Center

RE: [PATCH 4/9] isar-sstate: lint: add support for checking stamps

[Larson, Chris]

Will do, thanks. Your feedback has been quite helpful.

-----Original Message-----
From: Moessbauer, Felix (T CED OES-DE) <felix.moessbauer@siemens.com> 
Sent: Wednesday, April 3, 2024 12:03 AM
To: kergoth@gmail.com; isar-users@googlegroups.com
Cc: Larson, Chris (DI CTO FDS CES LX MEL) <chris.larson@siemens.com>
Subject: Re: [PATCH 4/9] isar-sstate: lint: add support for checking stamps

On Tue, 2024-04-02 at 17:28 +0000, kergoth@gmail.com wrote:
> From: Christopher Larson <chris.larson@seimens.com>
> 
> Bitbake supports writing signature data directly to the stamps 
> directory without having to build, so we should add the ability to 
> lint this signature data as well. This is useful for checking for 
> cachability issues without having to complete a build.

Hi,

I guess that is enabled with the '--dump-signatures=' parameter. Can we please add a brief example (one-line) to the linter script for this case.

> 
> Signed-off-by: Christopher Larson <chris.larson@siemens.com>
> ---
>  scripts/isar-sstate | 34 +++++++++++++++++++++++-----------
>  1 file changed, 23 insertions(+), 11 deletions(-)
> 
> diff --git a/scripts/isar-sstate b/scripts/isar-sstate index 
> 9b20cb8e..b77f73eb 100755
> --- a/scripts/isar-sstate
> +++ b/scripts/isar-sstate
> @@ -154,10 +154,12 @@ SstateCacheEntry = namedtuple(
>  #
> "${PV}:${PR}:${SSTATE_PKGARCH}:${SSTATE_VERSION}:"
>  
>  # This regex extracts relevant fields:
> -SstateRegex = re.compile(r'sstate:(?P<pn>[^:]*):[^:]*:[^:]*:[^:]*:'
> +SstateRegex =
> re.compile(r'(.*/)?sstate:(?P<pn>[^:]*):[^:]*:[^:]*:[^:]*:'
>                           r'(?P<arch>[^:]*):[^:]*:(?P<hash>[0-9a-
> f]*)_'
>                           r'(?P<task>[^\.]*)\.(?P<suffix>.*)')
> -
> +StampsRegex = re.compile(
> + 

Is this regex hand-crafted or can it be found somewhere in bitbake as well? Is the format documented in bitbake? I'm a bit worried that this might change across versions. If so, please add a note that this might need to be adjusted on bitbake updates.

Felix

>   
> r"(.*/)?(?P<arch>[^/]+)/(?P<pn>[^/]+)/([^/]+)\.do_(?P<task>[^/]+)\.(?
> P<suffix>sigdata)\.(?P<hash>[0-9a-f]{64})"
> +)
>  
>  class SstateTargetBase(object):
>      def __init__(self, path, cached=False):
> @@ -288,12 +290,13 @@ class SstateTargetBase(object):
>  
>  
>  class SstateFileTarget(SstateTargetBase):
> -    def __init__(self, path, **kwargs):
> +    def __init__(self, path, regex=SstateRegex, **kwargs):
>          super().__init__(path, **kwargs)
>          if path.startswith('file://'):
>              path = path[len('file://'):]
>          self.path = path
>          self.basepath = os.path.abspath(path)
> +        self.regex = regex
>  
>      def __repr__(self):
>          return f"file://{self.path}"
> @@ -334,12 +337,13 @@ class SstateFileTarget(SstateTargetBase):
>          for subdir, dirs, files in os.walk(self.basepath):
>              reldir = subdir[(len(self.basepath)+1):]
>              for f in files:
> -                m = SstateRegex.match(f)
> +                relative = os.path.join(reldir, f)
> +                m = self.regex.match(relative)
>                  if m is not None:
>                      islink = os.path.islink(os.path.join(subdir, f))
>                      age = int(now -
> os.path.getmtime(os.path.join(subdir, f)))
>                      all_files.append(SstateCacheEntry(
> -                        path=os.path.join(reldir, f),
> +                        path=relative,
>                          size=os.path.getsize(os.path.join(subdir,
> f)),
>                          islink=islink,
>                          age=age,
> @@ -592,6 +596,9 @@ def arguments():
>      parser.add_argument(
>          '--exit-code', type=int, default=None,
>          help="lint: return this instead of number of found issues")
> +    parser.add_argument(
> +        '--lint-stamps', default=False, action='store_true',
> +        help="lint: assume target is a stamps directory (target must
> be a local path)")
>  
>      args = parser.parse_args()
>      if args.command in 'upload analyze'.split() and args.source is
> None:
> @@ -798,7 +805,7 @@ def sstate_analyze(source, target, **kwargs):
>              print('\n'.join(out))
>  
>  
> -def sstate_lint(target, verbose, sources_dir, build_dir, exit_code, 
> pedantic, **kwargs):
> +def sstate_lint(target, verbose, sources_dir, build_dir, exit_code,
> pedantic, lint_stamps, **kwargs):
>      ADDITIONAL_IGNORED_VARNAMES = 'PP'.split()
>      # only list non-cacheable tasks here
>      # note that these still can break caching of other tasks that 
> depend on these.
> @@ -809,7 +816,10 @@ def sstate_lint(target, verbose, sources_dir, 
> build_dir, exit_code, pedantic, **
>          print(f"WARNING: target {target} does not exist. Nothing to
> analyze.")
>          return 0
>  
> -    cache_sigs = {s.hash: s for s in target.list_all() if 
> s.suffix.endswith('.siginfo')}
> +    if lint_stamps:
> +        cache_sigs = {s.hash: s for s in target.list_all()}
> +    else:
> +        cache_sigs = {s.hash: s for s in target.list_all() if
> s.suffix.endswith('.siginfo')}
>  
>      hits_srcdir = 0
>      hits_builddir = 0
> @@ -891,10 +901,12 @@ def main():
>          target = SstateDavTarget(args.target)
>      elif args.target.startswith('s3://'):
>          target = SstateS3Target(args.target)
> -    elif args.target.startswith('file://'):
> -        target = SstateFileTarget(args.target)
> -    else:  # no protocol given, assume file://
> -        target = SstateFileTarget(args.target)
> +    else:  # Either file://, or no protocol given, assume file://
> +        target = SstateFileTarget(args.target, StampsRegex if
> args.lint_stamps else SstateRegex)
> +
> +    if args.lint_stamps and not isinstance(target,
> SstateFileTarget):
> +        print("ERROR: --lint-stamps only works with local file
> targets")
> +        return 1
>  
>      args.target = target
>      return globals()[f'sstate_{args.command}'](**vars(args))
> --
> 2.39.2
> 

-- 
Siemens AG, Technology
Linux Expert Center

RE: [PATCH 1/9] isar-bootstrap: avoid forced early expansion of key vars

[Larson, Chris]

Actually the From was correct, as I really should be using my work address, I just need to actually send the emails from there instead to match. I'll do that for the v2. Thanks.

-----Original Message-----
From: Moessbauer, Felix (T CED OES-DE) <felix.moessbauer@siemens.com> 
Sent: Tuesday, April 2, 2024 11:55 PM
To: kergoth@gmail.com; isar-users@googlegroups.com
Cc: Larson, Chris (DI CTO FDS CES LX MEL) <chris.larson@siemens.com>; chris.larson@seimens.com
Subject: Re: [PATCH 1/9] isar-bootstrap: avoid forced early expansion of key vars

On Tue, 2024-04-02 at 17:28 +0000, kergoth@gmail.com wrote:
> From: Christopher Larson <chris.larson@seimens.com>
------------------------------------------^

Hi, please resend the whole series with a correct "From:".

Felix

> 
> Rather than appending the items from the expanded key variables into 
> SRC_URI individually, which means there's no way to use tools like 
> vardepvalue or vardepexclude to control signature generation, append 
> the unexpanded variables to the SRC_URI directly. This avoids issues 
> with shared state reuse for the isar-bootstrap packages.
> 
> Signed-off-by: Christopher Larson <chris.larson@siemens.com>
> ---
>  meta/recipes-core/isar-bootstrap/isar-bootstrap.inc | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc
> b/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc
> index 17f19fd8..de14e946 100644
> --- a/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc
> +++ b/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc
> @@ -10,7 +10,9 @@ LIC_FILES_CHKSUM =
> "file://${LAYERDIR_core}/licenses/COPYING.GPLv2;md5=751419260
>  FILESPATH:prepend := "${THISDIR}/files:"
>  SRC_URI = " \
>      file://locale \
> -    file://chroot-setup.sh"
> +    file://chroot-setup.sh \
> +    ${DISTRO_BOOTSTRAP_KEYS} \
> +    ${THIRD_PARTY_APT_KEYS}"
>  PV = "1.0"
>  
>  BOOTSTRAP_FOR_HOST ?= "0"
> @@ -22,6 +24,8 @@ APTSRCS = "${WORKDIR}/apt-sources"
>  APTSRCS_INIT = "${WORKDIR}/apt-sources-init"
>  DISTRO_BOOTSTRAP_KEYFILES = ""
>  THIRD_PARTY_APT_KEYFILES = ""
> +DISTRO_BOOTSTRAP_KEYS ?= ""
> +THIRD_PARTY_APT_KEYS ?= ""
>  DEPLOY_ISAR_BOOTSTRAP ?= ""
>  DISTRO_BOOTSTRAP_BASE_PACKAGES = "locales"
>  DISTRO_BOOTSTRAP_BASE_PACKAGES:append:gnupg = ",gnupg"
> @@ -48,13 +52,11 @@ python () {
>              distro_bootstrap_keys += own_pub_key.split()
>  
>      for key in distro_bootstrap_keys:
> -        d.appendVar("SRC_URI", " %s" % key)
>          fetcher = bb.fetch2.Fetch([key], d)
>          filename = os.path.relpath(fetcher.localpath(key), topdir)
>          d.appendVar("DISTRO_BOOTSTRAP_KEYFILES", " ${TOPDIR}/%s" %
> filename)
>  
>      for key in third_party_apt_keys:
> -        d.appendVar("SRC_URI", " %s" % key)
>          fetcher = bb.fetch2.Fetch([key], d)
>          filename = os.path.relpath(fetcher.localpath(key), topdir)
>          d.appendVar("THIRD_PARTY_APT_KEYFILES", " ${TOPDIR}/%s" %
> filename)
> --
> 2.39.2
> 

-- 
Siemens AG, Technology
Linux Expert Center

RE: [PATCH 6/9] isar-sstate: add --excluded-tasks argument

[Larson, Chris]

I have a quick question about this. In the code, there are constants using the "ignored" wording rather than excluded, so my adding the argument using excluded seems like it's probably not consistent. I should probably rename the argument to match, or adjust the variable naming for consistency. Any thoughts?

-----Original Message-----
From: Moessbauer, Felix (T CED OES-DE) <felix.moessbauer@siemens.com> 
Sent: Wednesday, April 3, 2024 12:11 AM
To: kergoth@gmail.com; isar-users@googlegroups.com
Cc: Larson, Chris (DI CTO FDS CES LX MEL) <chris.larson@siemens.com>
Subject: Re: [PATCH 6/9] isar-sstate: add --excluded-tasks argument

On Tue, 2024-04-02 at 17:28 +0000, kergoth@gmail.com wrote:
> From: Christopher Larson <chris.larson@seimens.com>
> 
> This allows the user to override the default lists of tasks to ignore 
> when linting the sstate cache.
> 
> Signed-off-by: Christopher Larson <chris.larson@siemens.com>
> ---
>  scripts/isar-sstate | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/scripts/isar-sstate b/scripts/isar-sstate index 
> 5270f944..dddfafcb 100755
> --- a/scripts/isar-sstate
> +++ b/scripts/isar-sstate
> @@ -146,7 +146,7 @@ try:
>  except ModuleNotFoundError:
>      s3_supported = False
>  
> -DEFAULT_IGNORED_TASKS = "rootfs_wicenv image_wic image_cpio image_tar 
> image_ext4"
> +DEFAULT_IGNORED_TASKS =
> "rootfs_wicenv,image_wic,image_cpio,image_tar,image_ext4"

Also here, it might be good to support wildcards, e.g. 'image_*'.
Apart from that minor detail, this is a valuable addition.

Acked-by: Felix Moessbauer <felix.moessbauer@siemens.com>

Felix

>  
>  SstateCacheEntry = namedtuple(
>          'SstateCacheEntry', 'hash path arch pn task suffix islink age 
> size'.split()) @@ -601,6 +601,9 @@ def arguments():
>      parser.add_argument(
>          '--lint-stamps', default=False, action='store_true',
>          help="lint: assume target is a stamps directory (target must 
> be a local path)")
> +    parser.add_argument(
> +        '--excluded-tasks', type=str, default=DEFAULT_IGNORED_TASKS,
> +        help="lint: comma-separated list of tasks to ignore
> (default: %(default)s)")
>  
>      args = parser.parse_args()
>      if args.command in 'upload analyze'.split() and args.source is
> None:
> @@ -609,6 +612,7 @@ def arguments():
>      elif args.command in 'info clean'.split() and args.source is not
> None:
>          print(f"ERROR: '{args.command}' must not have a source (only 
> a target)")
>          sys.exit(1)
> +    args.excluded_tasks = args.excluded_tasks.split(',')
>      return args
>  
>  
> @@ -807,13 +811,14 @@ def sstate_analyze(source, target, **kwargs):
>              print('\n'.join(out))
>  
>  
> -def sstate_lint(target, verbose, sources_dir, build_dir, exit_code, 
> pedantic, lint_stamps, **kwargs):
> +def sstate_lint(target, verbose, sources_dir, build_dir, exit_code,
> pedantic, lint_stamps,
> +                excluded_tasks, **kwargs):
>      ADDITIONAL_IGNORED_VARNAMES = 'PP'.split()
>      # only list non-cacheable tasks here
>      # note that these still can break caching of other tasks that 
> depend on these.
>      # Run in pedantic mode to also look for these issues (e.g. in 
> image-in-image builds)
>      # To make a build fully cacheable, avoid absolute paths in 
> BBLAYERS
> -    ADDITIONAL_IGNORED_TASKS = list() if pedantic else
> DEFAULT_IGNORED_TASKS.split()
> +    ADDITIONAL_IGNORED_TASKS = list() if pedantic else
> excluded_tasks
>      if not target.exists():
>          print(f"WARNING: target {target} does not exist. Nothing to
> analyze.")
>          return 0
> --
> 2.39.2
> 

-- 
Siemens AG, Technology
Linux Expert Center

Re: [PATCH 1/9] isar-bootstrap: avoid forced early expansion of key vars

[MOESSBAUER, Felix]

On Wed, 2024-04-03 at 21:42 +0000, Larson, Chris (DI CTO FDS CES LX
MEL) wrote:
> Actually the From was correct, as I really should be using my work
> address, I just need to actually send the emails from there instead
> to match. I'll do that for the v2. Thanks.

Sending from a different address is fine, but there is a typo in your
email: <chris.larson@seimens.com> != <chris.larson@siemens.com>
(SEIMENS vs. SIEMENS).

Felix

> 
> -----Original Message-----
> From: Moessbauer, Felix (T CED OES-DE) <felix.moessbauer@siemens.com>
> Sent: Tuesday, April 2, 2024 11:55 PM
> To: kergoth@gmail.com; isar-users@googlegroups.com
> Cc: Larson, Chris (DI CTO FDS CES LX MEL) <chris.larson@siemens.com>;
> chris.larson@seimens.com
> Subject: Re: [PATCH 1/9] isar-bootstrap: avoid forced early expansion
> of key vars
> 
> On Tue, 2024-04-02 at 17:28 +0000, kergoth@gmail.com wrote:
> > From: Christopher Larson <chris.larson@seimens.com>
> ------------------------------------------^
> 
> Hi, please resend the whole series with a correct "From:".
> 
> Felix
> 
> > 
> > Rather than appending the items from the expanded key variables
> > into 
> > SRC_URI individually, which means there's no way to use tools like 
> > vardepvalue or vardepexclude to control signature generation,
> > append 
> > the unexpanded variables to the SRC_URI directly. This avoids
> > issues 
> > with shared state reuse for the isar-bootstrap packages.
> > 
> > Signed-off-by: Christopher Larson <chris.larson@siemens.com>
> > ---
> >  meta/recipes-core/isar-bootstrap/isar-bootstrap.inc | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc
> > b/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc
> > index 17f19fd8..de14e946 100644
> > --- a/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc
> > +++ b/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc
> > @@ -10,7 +10,9 @@ LIC_FILES_CHKSUM =
> > "file://${LAYERDIR_core}/licenses/COPYING.GPLv2;md5=751419260
> >  FILESPATH:prepend := "${THISDIR}/files:"
> >  SRC_URI = " \
> >      file://locale \
> > -    file://chroot-setup.sh"
> > +    file://chroot-setup.sh \
> > +    ${DISTRO_BOOTSTRAP_KEYS} \
> > +    ${THIRD_PARTY_APT_KEYS}"
> >  PV = "1.0"
> >  
> >  BOOTSTRAP_FOR_HOST ?= "0"
> > @@ -22,6 +24,8 @@ APTSRCS = "${WORKDIR}/apt-sources"
> >  APTSRCS_INIT = "${WORKDIR}/apt-sources-init"
> >  DISTRO_BOOTSTRAP_KEYFILES = ""
> >  THIRD_PARTY_APT_KEYFILES = ""
> > +DISTRO_BOOTSTRAP_KEYS ?= ""
> > +THIRD_PARTY_APT_KEYS ?= ""
> >  DEPLOY_ISAR_BOOTSTRAP ?= ""
> >  DISTRO_BOOTSTRAP_BASE_PACKAGES = "locales"
> >  DISTRO_BOOTSTRAP_BASE_PACKAGES:append:gnupg = ",gnupg"
> > @@ -48,13 +52,11 @@ python () {
> >              distro_bootstrap_keys += own_pub_key.split()
> >  
> >      for key in distro_bootstrap_keys:
> > -        d.appendVar("SRC_URI", " %s" % key)
> >          fetcher = bb.fetch2.Fetch([key], d)
> >          filename = os.path.relpath(fetcher.localpath(key), topdir)
> >          d.appendVar("DISTRO_BOOTSTRAP_KEYFILES", " ${TOPDIR}/%s" %
> > filename)
> >  
> >      for key in third_party_apt_keys:
> > -        d.appendVar("SRC_URI", " %s" % key)
> >          fetcher = bb.fetch2.Fetch([key], d)
> >          filename = os.path.relpath(fetcher.localpath(key), topdir)
> >          d.appendVar("THIRD_PARTY_APT_KEYFILES", " ${TOPDIR}/%s" %
> > filename)
> > --
> > 2.39.2
> > 
> 
> -- 
> Siemens AG, Technology
> Linux Expert Center
> 
> 

-- 
Siemens AG, Technology
Linux Expert Center

Re: [PATCH 6/9] isar-sstate: add --excluded-tasks argument

[MOESSBAUER, Felix]

On Wed, 2024-04-03 at 21:44 +0000, Larson, Chris (DI CTO FDS CES LX
MEL) wrote:
> I have a quick question about this. In the code, there are constants
> using the "ignored" wording rather than excluded, so my adding the
> argument using excluded seems like it's probably not consistent. I
> should probably rename the argument to match, or adjust the variable
> naming for consistency. Any thoughts?

We should at least keep it consistent. Renaming the internal variables
is fine, but please try to not change the external interface (the
parameters).

Felix

> 
> -----Original Message-----
> From: Moessbauer, Felix (T CED OES-DE) <felix.moessbauer@siemens.com>
> Sent: Wednesday, April 3, 2024 12:11 AM
> To: kergoth@gmail.com; isar-users@googlegroups.com
> Cc: Larson, Chris (DI CTO FDS CES LX MEL) <chris.larson@siemens.com>
> Subject: Re: [PATCH 6/9] isar-sstate: add --excluded-tasks argument
> 
> On Tue, 2024-04-02 at 17:28 +0000, kergoth@gmail.com wrote:
> > From: Christopher Larson <chris.larson@seimens.com>
> > 
> > This allows the user to override the default lists of tasks to
> > ignore 
> > when linting the sstate cache.
> > 
> > Signed-off-by: Christopher Larson <chris.larson@siemens.com>
> > ---
> >  scripts/isar-sstate | 11 ++++++++---
> >  1 file changed, 8 insertions(+), 3 deletions(-)
> > 
> > diff --git a/scripts/isar-sstate b/scripts/isar-sstate index 
> > 5270f944..dddfafcb 100755
> > --- a/scripts/isar-sstate
> > +++ b/scripts/isar-sstate
> > @@ -146,7 +146,7 @@ try:
> >  except ModuleNotFoundError:
> >      s3_supported = False
> >  
> > -DEFAULT_IGNORED_TASKS = "rootfs_wicenv image_wic image_cpio
> > image_tar 
> > image_ext4"
> > +DEFAULT_IGNORED_TASKS =
> > "rootfs_wicenv,image_wic,image_cpio,image_tar,image_ext4"
> 
> Also here, it might be good to support wildcards, e.g. 'image_*'.
> Apart from that minor detail, this is a valuable addition.
> 
> Acked-by: Felix Moessbauer <felix.moessbauer@siemens.com>
> 
> Felix
> 
> >  
> >  SstateCacheEntry = namedtuple(
> >          'SstateCacheEntry', 'hash path arch pn task suffix islink
> > age 
> > size'.split()) @@ -601,6 +601,9 @@ def arguments():
> >      parser.add_argument(
> >          '--lint-stamps', default=False, action='store_true',
> >          help="lint: assume target is a stamps directory (target
> > must 
> > be a local path)")
> > +    parser.add_argument(
> > +        '--excluded-tasks', type=str,
> > default=DEFAULT_IGNORED_TASKS,
> > +        help="lint: comma-separated list of tasks to ignore
> > (default: %(default)s)")
> >  
> >      args = parser.parse_args()
> >      if args.command in 'upload analyze'.split() and args.source is
> > None:
> > @@ -609,6 +612,7 @@ def arguments():
> >      elif args.command in 'info clean'.split() and args.source is
> > not
> > None:
> >          print(f"ERROR: '{args.command}' must not have a source
> > (only 
> > a target)")
> >          sys.exit(1)
> > +    args.excluded_tasks = args.excluded_tasks.split(',')
> >      return args
> >  
> >  
> > @@ -807,13 +811,14 @@ def sstate_analyze(source, target, **kwargs):
> >              print('\n'.join(out))
> >  
> >  
> > -def sstate_lint(target, verbose, sources_dir, build_dir,
> > exit_code, 
> > pedantic, lint_stamps, **kwargs):
> > +def sstate_lint(target, verbose, sources_dir, build_dir,
> > exit_code,
> > pedantic, lint_stamps,
> > +                excluded_tasks, **kwargs):
> >      ADDITIONAL_IGNORED_VARNAMES = 'PP'.split()
> >      # only list non-cacheable tasks here
> >      # note that these still can break caching of other tasks that 
> > depend on these.
> >      # Run in pedantic mode to also look for these issues (e.g. in 
> > image-in-image builds)
> >      # To make a build fully cacheable, avoid absolute paths in 
> > BBLAYERS
> > -    ADDITIONAL_IGNORED_TASKS = list() if pedantic else
> > DEFAULT_IGNORED_TASKS.split()
> > +    ADDITIONAL_IGNORED_TASKS = list() if pedantic else
> > excluded_tasks
> >      if not target.exists():
> >          print(f"WARNING: target {target} does not exist. Nothing
> > to
> > analyze.")
> >          return 0
> > --
> > 2.39.2
> > 
> 
> -- 
> Siemens AG, Technology
> Linux Expert Center
> 
> 

-- 
Siemens AG, Technology
Linux Expert Center

[PATCH 8/9] testsuite: add perform_signature_lint method

[chris.larson]

From: Christopher Larson <chris.larson@siemens.com>

This method is provided to generate signature data for specified target
or targets and check for cachability issues without having to complete a
build.

Submitted at https://groups.google.com/g/isar-users/c/2NB-PXyswq8/m/42BMya5TAQAJ.

Signed-off-by: Christopher Larson <chris.larson@siemens.com>
---
 testsuite/cibase.py | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/testsuite/cibase.py b/testsuite/cibase.py
index 90591f32..349a79f0 100755
--- a/testsuite/cibase.py
+++ b/testsuite/cibase.py
@@ -125,6 +125,25 @@ def perform_sstate_populate(self, image_target, **kwargs):
         # Remove isar configuration so the the following test creates a new one
         self.delete_from_build_dir('conf')
 
+    def perform_signature_lint(self, targets, verbose=False, sources_dir=isar_root,
+                               excluded_tasks=None, **kwargs):
+        """Generate signature data for target(s) and check for cachability issues."""
+        self.configure(**kwargs)
+        self.move_in_build_dir("tmp", "tmp_before_sstate")
+        self.bitbake(targets, sig_handler="none")
+
+        verbose_arg = "--verbose" if verbose else ""
+        excluded_arg = f"--excluded-tasks {','.join(excluded_tasks)}" if excluded_tasks else ""
+        cmd = f"{isar_root}/scripts/isar-sstate lint --lint-stamps {self.build_dir}/tmp/stamps " \
+              f"--build-dir {self.build_dir} --sources-dir {sources_dir} {verbose_arg} {excluded_arg}"
+        self.log.info(f"Running: {cmd}")
+        exit_status, output = process.getstatusoutput(cmd, ignore_status=True)
+        if exit_status > 0:
+            ansi_escape = re.compile(r'\x1B(?:[@-Z\\-_]|\[[0-?]*[ -/]*[@-~])')
+            for line in output.splitlines():
+                self.log.error(ansi_escape.sub('', line))
+            self.fail("Detected cachability issues")
+
     def perform_sstate_test(self, image_target, package_target, **kwargs):
         def check_executed_tasks(target, expected):
             taskorder_file = glob.glob(f'{self.build_dir}/tmp/work/*/{target}/*/temp/log.task_order')
-- 
2.39.2