fix(scripts/bundle): Pack NVMe-SPINOR tarballs separately by vishwasudupa · Pull Request #354 · qualcomm-linux/qcom-deb-images

vishwasudupa · 2026-04-06T06:11:14Z

Place SPINOR content under the NVME folder to align with the current content.xml layout.

Use _UFS and _NVME directories to distinguish storage types instead of inferring from partition conf.

github-actions · 2026-04-06T07:18:56Z

Test Results

2 files ±0 6 suites ±0 3m 19s ⏱️ ±0s
20 tests ±0 20 ✅ ±0 0 💤 ±0 0 ❌ ±0
64 runs ±0 64 ✅ ±0 0 💤 ±0 0 ❌ ±0

Results for commit 3f1b69f. ± Comparison against base commit f120944.

♻️ This comment has been updated with latest results.

github-actions · 2026-04-06T07:18:56Z

Test jobs for commit `00807f9`

lool

Hey, thanks for this!

Your changes look clean but they kind of go counter the current idea: we currently generate two tarballs, grouping platforms by the sector size they use. To do this, we're scanning generated files to see if the 512B or 4k sector size was used (disk-sdcard or -ufs).

Right now, nvme platforms end up in the emmc tarball because they use 512B sectors, and spinor ends up with them due to the default case – a bit of a hack.

What I would propose instead is to:

stop including spinor in emmc by default
for each _spinor dir, copy it to each of the corresponding _emmc, _ufs and _nvme directories with the same platform name

This avoids special casing NVMe and special cases spinor as a sign of a NHLOS storage.

I don't think you need to do it in this PR, but I'd like us to rename disk-sdcard/disk-ufs and flash-ufs/flash-emmc to better names related to the sector size.

lool · 2026-04-08T07:09:02Z

Hey, thanks for this!

Your changes look clean but they kind of go counter the current idea: we currently generate two tarballs, grouping platforms by the sector size they use. To do this, we're scanning generated files to see if the 512B or 4k sector size was used (disk-sdcard or -ufs).

Right now, nvme platforms end up in the emmc tarball because they use 512B sectors, and spinor ends up with them due to the default case – a bit of a hack.

What I would propose instead is to:

stop including spinor in emmc by default

for each _spinor dir, copy it to each of the corresponding _emmc, _ufs and _nvme directories with the same platform name

This avoids special casing NVMe and special cases spinor as a sign of a NHLOS storage.

I don't think you need to do it in this PR, but I'd like us to rename disk-sdcard/disk-ufs and flash-ufs/flash-emmc to better names related to the sector size.

I slept on this, and realized we probably don't want to change the contents of the flash dirs in the late bundling stage, so idea in 2) should probably be implemented in the logic which is currently in the flash recipe, but that Agathe started moving out in #334

github-actions · 2026-04-09T11:59:34Z

Test jobs for commit `b2ba056`

lool · 2026-04-09T14:22:56Z

We discussed this out of band with Vishwas:

I pointed out I want to keep the two tarballs coming out of CI (one per disk sector size)
perhaps we can add a yaml metadata entry to point out to the NHLOS ptool config to use (spinor for glymur-crd)
Vishwas pointed out ptool has the information in partitions.conf about the sector size; this is great news, and we can use it to route files to one or the other (probably testing rawprogram0.xml to see which sector size it refers to)
I haven't checked how the contents.xml.in in ptool currently expect things to be laid out, but I expect that it could look like:
- flash_glymur-crd_nvme/
  - files to flash on the NVMe (disk-media.img1, disk-media.img2, rawprogram0.xml, patch0.xml...)
  - spinor/
    - files to flash on spinor (cdt.bin, uefi.bin, rawprogram0.xml, patch0.xml...)
  - contents.xml listing both
and since it's likely going to use 512B sectors, I expect it will be zipped into flash-emmc.tgz for now

github-actions · 2026-04-09T17:47:15Z

Test jobs for commit `c871bee`

vishwasudupa · 2026-04-10T01:38:09Z

Hey, thanks for this!

Your changes look clean but they kind of go counter the current idea: we currently generate two tarballs, grouping platforms by the sector size they use. To do this, we're scanning generated files to see if the 512B or 4k sector size was used (disk-sdcard or -ufs).

Right now, nvme platforms end up in the emmc tarball because they use 512B sectors, and spinor ends up with them due to the default case – a bit of a hack.

What I would propose instead is to:

stop including spinor in emmc by default

for each _spinor dir, copy it to each of the corresponding _emmc, _ufs and _nvme directories with the same platform name

This avoids special casing NVMe and special cases spinor as a sign of a NHLOS storage.

I don't think you need to do it in this PR, but I'd like us to rename disk-sdcard/disk-ufs and flash-ufs/flash-emmc to better names related to the sector size.

I have enabled spinor to be going to its respective directory in latest patch

lool

Hey, this seems needlessly complicated, there's already a top loop on all directories earlier and a simpler way to write this

My main issue though is that I think we could do that in the flash recipe itself, so that this behavior is the result of image builds, not just the result in our CI artifacts (this script is used from the github workflow).

I would think the same logic should work from the main flash recipe; after the $platforms loop; something like:

for dir in flash_*; do
    # skip directories that are named _spinor
    if [ -z "${d%%*_spinor}" ]; then
        continue
    fi
    # strip _storage suffix, and check for a _spinor directory
    spinor_dir="${d%_*}_spinor"
    if [ -d "$spinor_dir" ]; then
        # embed a copy of spinor contents
        cp ...preserve args... "$spinor_dir" "$d"
    fi
done

lool · 2026-04-10T07:35:49Z

scripts/bundle-flash-dirs.sh


+# Organize spinor directories under their base directories
+for dir in $emmc_dirs $ufs_dirs; do
+    [ -d "$dir" ] || continue


I don't think this line is useful? we just assembled a list of directories, and you're checking if the directory is present?

ok will remove it and simplify

lool · 2026-04-10T07:48:55Z

scripts/bundle-flash-dirs.sh

+# Organize spinor directories under their base directories
+for dir in $emmc_dirs $ufs_dirs; do
+    [ -d "$dir" ] || continue
+    echo "$dir" | grep -q "_spinor$" || continue


So we loop through all dirs that mix emmc, ufs, nvme and spinor, just to spot the spinor ones; then we have to do another loop to see where to copy them to. That seems needlessly complicated.

You could either:

do this earlier

or check if the directory is not suffixed _spinor, and then see if there's a corresponding _spinor dir to copy into it

I have added in start and simplified code without && for better visibility

lool · 2026-04-10T07:50:07Z

scripts/bundle-flash-dirs.sh

+    echo "$dir" | grep -q "_spinor$" || continue
+    prefix=$(echo "$dir" | sed 's/_spinor$//')
+    for folder in $emmc_dirs $ufs_dirs; do
+        echo "$folder" | grep -q "^${prefix}_" && [ "$folder" != "$dir" ] && [ -d "$folder" ] && cp -r "${dir}"/* "${folder}/" && break


The && chained commands are hard to read; also, you seem to stop the loop after the first cp (I guess that's the point of break), but what if we support nvme and ufs, and we want spinor in both?

yes in those cases need to maintain metadata somewhere to see which HLOS once get spinor, i am thinking id that info can be got via any xml/conf from ptool

let me know if any place if this mapping can be found

I found a way to get the content.xml in spinor works with which other storage

I have used that logic in latest patch, this should resolve for case where there is both nvme/ufs

…ctory When a platform has a *_spinor directory, place it under the corresponding platform target storage (emmc/ufs/nvme). Signed-off-by: Vishwas Udupa <vudupa@qti.qualcomm.com>

vishwasudupa · 2026-04-13T06:55:29Z

Hey, this seems needlessly complicated, there's already a top loop on all directories earlier and a simpler way to write this

My main issue though is that I think we could do that in the flash recipe itself, so that this behavior is the result of image builds, not just the result in our CI artifacts (this script is used from the github workflow).

I would think the same logic should work from the main flash recipe; after the $platforms loop; something like:
for dir in flash_*; do
    # skip directories that are named _spinor
    if [ -z "${d%%*_spinor}" ]; then
        continue
    fi
    # strip _storage suffix, and check for a _spinor directory
    spinor_dir="${d%_*}_spinor"
    if [ -d "$spinor_dir" ]; then
        # embed a copy of spinor contentsd
        cp ...preserve args... "$spinor_dir" "$d"
    fi
done

I taught since this is bundling decision ,it was better to keep in bundle-flash-dirs.
alterations if needed, can it be done in future PR?

github-actions · 2026-04-13T07:52:48Z

Test jobs for commit `3f1b69f`

lool requested changes Apr 7, 2026

View reviewed changes

vishwasudupa force-pushed the flash-nvme-spinor branch from 00807f9 to b2ba056 Compare April 9, 2026 10:58

vishwasudupa force-pushed the flash-nvme-spinor branch from b2ba056 to c871bee Compare April 9, 2026 16:43

lool requested changes Apr 10, 2026

View reviewed changes

fix(scripts/bundle): Pack spinor under respective target-storage dire…

3f1b69f

…ctory When a platform has a *_spinor directory, place it under the corresponding platform target storage (emmc/ufs/nvme). Signed-off-by: Vishwas Udupa <vudupa@qti.qualcomm.com>

vishwasudupa force-pushed the flash-nvme-spinor branch from c871bee to 3f1b69f Compare April 13, 2026 06:50

Conversation

vishwasudupa commented Apr 6, 2026

Uh oh!

github-actions bot commented Apr 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

github-actions bot commented Apr 6, 2026

Test jobs for commit 00807f9

Uh oh!

lool left a comment

Choose a reason for hiding this comment

Uh oh!

lool commented Apr 8, 2026

Uh oh!

github-actions bot commented Apr 9, 2026

Test jobs for commit b2ba056

Uh oh!

lool commented Apr 9, 2026

Uh oh!

github-actions bot commented Apr 9, 2026

Test jobs for commit c871bee

Uh oh!

vishwasudupa commented Apr 10, 2026

Uh oh!

lool left a comment

Choose a reason for hiding this comment

Uh oh!

lool Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

vishwasudupa Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

lool Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

vishwasudupa Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

lool Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

vishwasudupa Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

vishwasudupa Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

vishwasudupa commented Apr 13, 2026

Uh oh!

github-actions bot commented Apr 13, 2026

Test jobs for commit 3f1b69f

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions bot commented Apr 6, 2026 •

edited

Loading

Test jobs for commit `00807f9`

Test jobs for commit `b2ba056`

Test jobs for commit `c871bee`

Test jobs for commit `3f1b69f`