Skip to content

Conversation

@jonaswood01
Copy link
Contributor

Description

Per request of @igorpecovnik, this switches TI debian packages to be added to the image package list instead of being cached in the rootfs. Our previous use of add_packages_to_rootfs had caused immediate miss on any cached/standard rootfs, which this commit fixes.

How Has This Been Tested?

Note: the git status changes in the logs are proxy-related only.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • My changes generate no new warnings (as far as I can tell)

This switches TI deb pkgs to be added to the image package list,
instead of being cached in the rootfs.

Signed-off-by: Jonas Wood <[email protected]>
@github-actions github-actions bot added 11 Milestone: Fourth quarter release size/small PR with less then 50 lines Needs review Seeking for review Framework Framework components labels Nov 4, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 4, 2025

Walkthrough

The change modifies the Texas Instruments Debian packages extension by replacing the add_packages_to_rootfs function call with add_packages_to_image in the extension_prepare_config__add_packages function when TI_PACKAGES is non-empty. This redirects the target for package installation from the root filesystem to the image.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Single file modification in extensions/ti-debpkgs.sh
  • One function call replaced with an alternative
  • Consider verifying the functional implications of changing the package installation target from rootfs to image

Possibly related PRs

Suggested labels

size/medium, BSP, Hardware

Suggested reviewers

  • igorpecovnik
  • glneo

Pre-merge checks and finishing touches

✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: switching TI Debian package installation from rootfs to image in the ti-debpkgs extension.
Description check ✅ Passed The description is directly related to the changeset, explaining the rationale (fixing rootfs cache misses), testing performed across multiple boards, and confirming no new warnings.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 896afe2 and fa24a9b.

📒 Files selected for processing (1)
  • extensions/ti-debpkgs.sh (1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: EvilOlaf
Repo: armbian/build PR: 8428
File: config/boards/lckfb-taishanpi.csc:5-9
Timestamp: 2025-07-25T03:51:50.830Z
Learning: When reviewing PRs in the Armbian build system, U-Boot defconfig files and patches may be added as part of the PR changes but might not be visible in the current repository clone state during review. It's important to check the actual PR file changes directly via GitHub API (https://api.github.com/repos/armbian/build/pulls/{pr_number}/files) to get the complete picture of what files are being added or modified, especially for U-Boot patches that will be applied during the build process.
Learnt from: EvilOlaf
Repo: armbian/build PR: 8428
File: config/boards/lckfb-taishanpi.csc:5-9
Timestamp: 2025-07-25T03:51:50.830Z
Learning: When reviewing PRs in the Armbian build system, U-Boot defconfig files and patches may be added as part of the PR changes but might not be visible in the current repository clone state during review. It's important to check the actual PR file changes directly via GitHub or the PR API to get the complete picture of what files are being added or modified.
Learnt from: leggewie
Repo: armbian/build PR: 8502
File: config/desktop/trixie/environments/i3-wm/config_base/packages:44-44
Timestamp: 2025-08-14T17:19:39.693Z
Learning: When a PR author provides clear context about package transitions in the commit message and the changes are scoped to specific release pockets, focus the review on validating those specific changes rather than suggesting broader investigations across other releases or additional dependency verifications.
📚 Learning: 2025-06-22T00:45:26.182Z
Learnt from: djurny
Repo: armbian/build PR: 8315
File: lib/functions/rootfs/distro-agnostic.sh:54-68
Timestamp: 2025-06-22T00:45:26.182Z
Learning: In Armbian build system, external command dependencies like envsubst are handled by adding the required packages (e.g., gettext for envsubst) to host_dependencies in adaptative_prepare_host_dependencies function in lib/functions/host/prepare-host.sh, rather than checking command availability at runtime.

Applied to files:

  • extensions/ti-debpkgs.sh
📚 Learning: 2025-09-24T09:54:07.968Z
Learnt from: amazingfate
Repo: armbian/build PR: 8668
File: extensions/ufs.sh:2-2
Timestamp: 2025-09-24T09:54:07.968Z
Learning: In Armbian's build system, DOCKER_ARMBIAN_BASE_IMAGE must be set globally at file load time in extensions, not inside extension_prepare_config functions, because docker operations happen early in the build process and would default to ubuntu:noble if not set before docker_cli_prepare() runs.

Applied to files:

  • extensions/ti-debpkgs.sh
🧬 Code graph analysis (1)
extensions/ti-debpkgs.sh (1)
.github/generate_CODEOWNERS.sh (1)
  • add_packages_to_image (8-8)
🔇 Additional comments (1)
extensions/ti-debpkgs.sh (1)

1-5: Function call change is correct and safe. No issues found.

The replacement of add_packages_to_rootfs with add_packages_to_image in line 3 is properly isolated and verified:

  • add_packages_to_image function exists and is defined at lib/functions/configuration/package-lists.sh:32
  • No other extensions reference add_packages_to_rootfs for TI packages
  • This is the only TI-specific extension file in the codebase

The change correctly addresses the cache miss issue as intended.


Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai bot requested a review from glneo November 4, 2025 23:07
@jonaswood01
Copy link
Contributor Author

@Grippy98

@igorpecovnik
Copy link
Member

igorpecovnik commented Nov 4, 2025

Currently all caches exists, so this is not good test.

Verify if hashes from your build:
INFO: rootfs version [ packages_hash: 'c8930678dbeb-Heb7160-B850b0f' cache_type: 'minimal' rootfs_cache_id: 'c8930678dbeb-Heb7160-B850b0f' ]
are identical to some other(rockhip or amlogic) aarch64 board.

p.s.
When you need assistance for build framework related questions, ping @tabrisnet .

@jonaswood01
Copy link
Contributor Author

Currently all caches exists, so this is not good test.

Verify if hashes from your build: INFO: rootfs version [ packages_hash: 'c8930678dbeb-Heb7160-B850b0f' cache_type: 'minimal' rootfs_cache_id: 'c8930678dbeb-Heb7160-B850b0f' ] are identical to some other(rockhip or amlogic) aarch64 board.

p.s. When you need assistance for build framework related questions, ping @tabrisnet .

@tabrisnet @igorpecovnik @Grippy98 I re-ran builds on main HEAD (05511627a6ba6eaae55dae6bc94032e95e373770) to compare hashes (again only with modifications for proxy):

  • rootfs version [ packages_hash: 'c8930678dbeb-H7f4843-B850b0f' cache_type: 'minimal' rootfs_cache_id: 'c8930678dbeb-H7f4843-B850b0f' ]
    • orangepi5: log
    • odroidhc4: log
    • odroidxu4: log
  • rootfs version [ packages_hash: 'bdfdeb419398-Heb7160-B850b0f' cache_type: 'minimal' rootfs_cache_id: 'bdfdeb419398-Heb7160-B850b0f' ]
  • rootfs version [ packages_hash: 'c8930678dbeb-Heb7160-B850b0f' cache_type: 'minimal' rootfs_cache_id: 'c8930678dbeb-Heb7160-B850b0f' ]

Then I added change from this PR (add_packages_to_rootfs -> add_packages_to_image) and re-ran builds:

  • rootfs version [ packages_hash: 'c8930678dbeb-Heb7160-B850b0f' cache_type: 'minimal' rootfs_cache_id: 'c8930678dbeb-Heb7160-B850b0f' ]

@glneo
Copy link
Collaborator

glneo commented Nov 5, 2025

I see you have build logs for all the platforms, have you also boot tested on any of these platforms to verify the packages are being installed correctly?

@igorpecovnik
Copy link
Member

I think packages lists per image are generated in the output/logs. Worth looking into that too.

@jonaswood01
Copy link
Contributor Author

jonaswood01 commented Nov 6, 2025

I see you have build logs for all the platforms, have you also boot tested on any of these platforms to verify the packages are being installed correctly?

From what I've tested, building from armbian upstream on TI boards don't boot on several TI boards before/after this PR change anyway. TI/armbian-build downstream fork has more changes to config/sources/families/k3.conf and several TI board .conf's that have been thoroughly tested to work (especially on tagged releases), which are still pending to be sent upstream. This PR change has been tested on downstream fork, packages installing/working correctly. My thought is to merge this first, which doesn't affect functionality, and then send more PRs to fix config/sources/families/k3.conf and TI board .conf's.

@jonaswood01
Copy link
Contributor Author

but here are boot logs from this PR's changes:

  • sk-am62b: boot log
  • sk-am62p: boot log: needs further PR to fix boot (issue not caused by this PR)
  • sk-am64b: boot log: needs further PR to fix boot (issue not caused by this PR)
  • sk-am68: boot log: needs further PR to fix boot (issue not caused by this PR)
  • sk-am69: don't currently have board to test
  • sk-tda4vm: don't currently have board to test

@jonaswood01
Copy link
Contributor Author

I see you have build logs for all the platforms, have you also boot tested on any of these platforms to verify the packages are being installed correctly?

From what I've tested, building from armbian upstream on TI boards don't boot on several TI boards before/after this PR change anyway. TI/armbian-build downstream fork has more changes to config/sources/families/k3.conf and several TI board .conf's that have been thoroughly tested to work (especially on tagged releases), which are still pending to be sent upstream. This PR change has been tested on downstream fork, packages installing/working correctly. My thought is to merge this first, which doesn't affect functionality, and then send more PRs to fix config/sources/families/k3.conf and TI board .conf's.

The logs in this separate PR verifies that (that several don't boot before this PR anyway)

@igorpecovnik
Copy link
Member

From what I've tested, building from armbian upstream on TI boards don't boot on several TI boards before/after this PR change anyway

Yeah, quite possible.

My thought is to merge this first, which doesn't affect functionality

We can do that, yes.

@github-actions github-actions bot added the Ready to merge Reviewed, tested and ready for merge label Nov 6, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Nov 6, 2025

✅ This PR has been reviewed and approved — all set for merge!

@github-actions github-actions bot removed the Needs review Seeking for review label Nov 6, 2025
@igorpecovnik igorpecovnik merged commit 9cc6e3a into armbian:main Nov 6, 2025
11 checks passed
@jonaswood01 jonaswood01 deleted the upstream-PR branch November 6, 2025 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

11 Milestone: Fourth quarter release Framework Framework components Ready to merge Reviewed, tested and ready for merge size/small PR with less then 50 lines

Development

Successfully merging this pull request may close these issues.

3 participants