TI: Add TI's custom debian repository and install a few packages by-default #8305
Conversation
|
""" WalkthroughThis change updates the SK-AM62B board configuration by adding the Suggested labels
Suggested reviewers
✨ Finishing Touches
🧪 Generate Unit Tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
size/small
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
config/boards/sk-am62b.conf (1)
18-18: Add a short comment explaining the newCC33XX_SUPPORTswitchFuture maintainers unfamiliar with TI’s Wi-Fi stack won’t know what turning this flag on/off implies for the image size or driver set. A one-liner right above the variable would help.
extensions/ti-debpkgs.sh (2)
17-18: Potential sed delimiter collisionIf
${RELEASE}ever contains a “/” (unlikely but possible, e.g.testing/rolling), the substitution breaks. Considersed -i "s|bookworm|${RELEASE}|g" …to switch to a safer delimiter.
25-26: Copy runs outside chroot – ownership & uid may differ
cp … $SDCARD/etc/apt/preferences.d/happens on the host, so the file ends up owned by the invoking user (often root:root). If the rootfs later enforces different ownership (e.g._apt), move the copy insidechroot_sdcardto inherit correct permissions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
config/boards/sk-am62b.conf(1 hunks)config/sources/families/k3.conf(2 hunks)extensions/ti-debpkgs.sh(1 hunks)packages/bsp/ti/ti-debpkgs/ti-debpkgs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
extensions/ti-debpkgs.sh (1)
.github/generate_CODEOWNERS.sh (1)
display_alert(6-6)
🪛 Shellcheck (0.10.0)
extensions/ti-debpkgs.sh
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
[warning] 9-9: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
[error] 10-10: Argument mixes string and array. Use * or separate argument.
(SC2145)
[error] 12-12: Arrays implicitly concatenate in [[ ]]. Use a loop (or explicit * instead of @).
(SC2199)
[warning] 12-12: Remove quotes from right-hand side of =~ to match as a regex rather than literally.
(SC2076)
[error] 31-31: Argument mixes string and array. Use * or separate argument.
(SC2145)
🔇 Additional comments (2)
packages/bsp/ti/ti-debpkgs/ti-debpkgs (1)
1-3: Pin-priority1001may unintentionally override security updatesA priority above 1000 forces TI packages to win every dependency race – even over Debian security/point-release updates from official mirrors. Confirm this is the desired policy for all packages, or consider lowering to
990and use per-package pins where absolutely required.config/sources/families/k3.conf (1)
22-23: Unconditionalenable_extension "ti-debpkgs"can break non-TI derivativesIf a downstream board reuses this family file but does not host the TI repo key,
apt updatewill warn/fail on the missing source. Consider gating the call behind[[ "${TI_PACKAGES[*]}" ]]or another feature flag.
jsuhaas22
force-pushed
the
add-ti-debpkgs
branch
from
59b66e4 to
7fafbda
Compare
github-actions
bot
added
size/medium
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
extensions/ti-debpkgs.sh (1)
17-18: Quote the output path inwget
You correctly added the temp-dir creation but still need to quote the target file to handle paths with spaces (SC2086):- run_host_command_logged "wget -qO $SDCARD/tmp/ti-debpkgs.sources https://…" + run_host_command_logged "wget -qO \"$SDCARD/tmp/ti-debpkgs.sources\" https://…"
🧹 Nitpick comments (3)
extensions/ti-debpkgs.sh (3)
1-1: Add a shebang or shell directive
The script has no interpreter declaration (SC2148). Include a shebang and/or a ShellCheck directive to specify Bash explicitly.+#!/usr/bin/env bash +# shellcheck shell=bash
13-13: Avoid mixing array expansion inside quoted strings
ShellCheck SC2145 warns about mixing strings and arrays. Consider using${valid_suites[*]}or separate arguments:- display_alert "TI Repo has the following valid suites - ${valid_suites[@]}..." + display_alert "TI Repo has the following valid suites - ${valid_suites[*]}..."- display_alert "Valid Options Would Have Been: ${valid_suites[@]}" + display_alert "Valid Options Would Have Been: ${valid_suites[*]}"Also applies to: 35-35
32-36: Error-path alert formatting
Same array-expansion mix ondisplay_alertas above—use${valid_suites[*]}for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
config/boards/sk-am62b.conf(1 hunks)config/sources/families/k3.conf(2 hunks)extensions/ti-debpkgs.sh(1 hunks)packages/bsp/ti/ti-debpkgs/ti-debpkgs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/bsp/ti/ti-debpkgs/ti-debpkgs
- config/boards/sk-am62b.conf
- config/sources/families/k3.conf
🧰 Additional context used
🧬 Code Graph Analysis (1)
extensions/ti-debpkgs.sh (1)
.github/generate_CODEOWNERS.sh (1)
display_alert(6-6)
🪛 Shellcheck (0.10.0)
extensions/ti-debpkgs.sh
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
[error] 13-13: Argument mixes string and array. Use * or separate argument.
(SC2145)
[error] 35-35: Argument mixes string and array. Use * or separate argument.
(SC2145)
🔇 Additional comments (4)
extensions/ti-debpkgs.sh (4)
1-5: Theextension_prepare_config__add_packagesfunction correctly checks and expandsTI_PACKAGESintoadd_packages_to_rootfs. No changes needed here.
9-12: Good use ofmapfilefor JSON parsing
Usingmapfile -tto populatevalid_suitesavoids word-splitting issues and addresses earlier ShellCheck concerns.
15-16: Suite membership check is robust
Usingprintf … | grep -qxsafely tests for exact matches.
21-27: Sed replacement and chroot copy are sound
The in-placesedsubstitution for${RELEASE}and subsequent chroot operations look correct.
Texas Instruments maintains a custom apt repository [0] that contains:
* tools like k3conf, which run on K3 devices
* TI's versions of upstream packages (such as mesa)
* out-of-tree drivers and firmware for graphics, wifi etc
Therefore, add TI's custom repository as the highest priority repository
in the filesystem. Doing this ensures that if apt finds a version of a
package that exists in both upstream Debian and the TI repository, it
picks the latter.
Additionally, introduce K3_PACKAGES variable to store a list of packages
that should be installed by-default in a K3 image. Initialize it to hold
TI's CC33xx packages.
Also set EXTRAWIFI to "no" in `current` image.
[0] https://github.com/TexasInstruments/ti-debpkgs
Co-authored-by: Suhaas Joshi <s-joshi@ti.com>
Signed-off-by: Suhaas Joshi <s-joshi@ti.com>
Change U-Boot repository link from git.ti.com to Github. In the past, there have been CI issues caused due to git.ti.com failing. Signed-off-by: Suhaas Joshi <s-joshi@ti.com>
jsuhaas22
force-pushed
the
add-ti-debpkgs
branch
from
7fafbda to
43fb4a8
Compare
github-actions
bot
removed
the
Ready to merge
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
extensions/ti-debpkgs.sh (1)
17-17: Ensure temporary directory creationThe addition of
mkdir -p "$SDCARD/tmp"addresses the missing-directory issue raised previously.
🧹 Nitpick comments (4)
extensions/ti-debpkgs.sh (4)
1-1: Specify shell interpreter & enable strict modeAdd a shebang (
#!/usr/bin/env bash) and considerset -euo pipefailat the top to ensure the script runs under Bash and fails fast on errors.
13-13: Quote array expansion indisplay_alert
display_alert "TI Repo has the following valid suites - ${valid_suites[@]}..."mixes string and array. Consider using${valid_suites[*]}or wrapping the expansion in quotes to avoid unintended word-splitting.
18-18: Quote wget output path
wget -qO $SDCARD/tmp/ti-debpkgs.sources …is unquoted; wrap the path in quotes to handle potential spaces in$SDCARD/tmp.-run_host_command_logged "wget -qO $SDCARD/tmp/ti-debpkgs.sources https://raw.githubusercontent.com/TexasInstruments/ti-debpkgs/main/ti-debpkgs.sources" +run_host_command_logged "wget -qO \"$SDCARD/tmp/ti-debpkgs.sources\" https://raw.githubusercontent.com/TexasInstruments/ti-debpkgs/main/ti-debpkgs.sources"
35-35: Quote array expansion in error messageSame as above in the failure path: use
${valid_suites[*]}or quote the expansion to prevent word-splitting.-display_alert "Valid Options Would Have Been: ${valid_suites[@]}" +display_alert "Valid Options Would Have Been: ${valid_suites[*]}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
config/boards/sk-am62b.conf(1 hunks)config/sources/families/k3.conf(2 hunks)extensions/ti-debpkgs.sh(1 hunks)packages/bsp/ti/ti-debpkgs/ti-debpkgs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- config/boards/sk-am62b.conf
- packages/bsp/ti/ti-debpkgs/ti-debpkgs
- config/sources/families/k3.conf
🧰 Additional context used
🪛 Shellcheck (0.10.0)
extensions/ti-debpkgs.sh
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
[error] 13-13: Argument mixes string and array. Use * or separate argument.
(SC2145)
[error] 35-35: Argument mixes string and array. Use * or separate argument.
(SC2145)
🔇 Additional comments (3)
extensions/ti-debpkgs.sh (3)
1-5: ValidateTI_PACKAGEShandling
extension_prepare_config__add_packagescorrectly checks for populatedTI_PACKAGESand invokesadd_packages_to_rootfs. Implementation looks solid.
8-12: Use ofmapfileis correctThe JSON-to-array conversion via
mapfile -t valid_suitesavoids word-splitting and is a robust replacement for the previous$(...)approach.
21-21: Fixsedquoting so${RELEASE}is expandedUsing single quotes around the substitution prevents
${RELEASE}from being interpolated. Switch to a quoting style that allows host-level expansion:
[sraise_critical_issue]-chroot_sdcard "sed -i 's/bookworm/${RELEASE}/g' /tmp/ti-debpkgs.sources" +chroot_sdcard "sed -i \"s/bookworm/${RELEASE}/g\" /tmp/ti-debpkgs.sources"
|
I think it would be beneficial to move this to APA |
Also there is alternative - auto mirroring to our main download infrastructure.
Yes, that too. |
|
This APA thing seems interesting. We host our deb packages on Github as we wanted a quick and easy way to host them, but it wasn't meant to be a long term solution. These packages are built for our Debian offering, which is now Armbian, so if there is a way to put them on the Armbian CDN we would be all for that. Any plans for a build service for these (like Launchpad or Open Build Service)? Or is this supposed to only be a mirror service? For background, we currently build our source and binary deb packages from this repo[0] which simply fetches the source, adds the |
At this point, daily / hourly sync to
I think this will be possible trough APA, which will eventually be a part of main repository |
|
As of now, I didn't envision APA to be a build service. But hosting TI packages on APA, I'd be all for that. @glneo The stuff in main should be dead-easy to bring over as from what I can see you already are providing the .dsc files etc. That stuff should be directly buildable in APA. As of now, APA is basically only there for package dependency logic, no truly compiled code is shipped and only very few files. But there's nothing to say APA could not do that in the future, it simply uses regular debian build tools same as you. The stuff in non-free is probably there because of NDA, I assume and as such source cannot be provided. You mention that you build everything with regular debian tooling so I believe it should be easy enough to merge your packages into APA (or something similar). By the way, I see you are using reprepro for assembling the repository and that you do publish the conf and db directories. Those directories are generally not necessary for the published repository to function and it is generally discouraged to publish them for security reasons. |
|
APA as a build service would be interesting, long term might even be a good way to offload the kernel/u-boot/etc package building done currently by build. Which could allow for a cleaner split between image building and package building at some point. The packages in
Thanks for this info! @jsuhaas22 we should fix that |
There was a problem hiding this comment.
I am not trying to make anyone's life difficult, but this PR is essentially the role model of a change that we are trying to keep out of core and move into APA, so at this point in time, I'd like to ask for changes and I'm happy to help with those. I'm not fundamentally objecting to this PR but I wonder if we have a chance here to move the necessary changes into the existing apa extension instead.
I will shortly push a change to APA to declare an armbian-ti-common package where we can define packaging logic. This package would need to be added to $TI_PACKAGES, ideally it would end up being the ONLY package in $TI_PACKAGES. In APA, I try to avoid special-casing distribution suites and I hope this would be no exception. It needs to be verified if that remains feasible. armbian-ti-common would ship sources.list.d snippets, the pinning preferences.d snippet and the TI GPG key used for signing to make apt happy. This way, you already get the added benefit of a managed and upgradeable GPG key and sources.list definition, not stale ones requiring future user intervention. Instead of defining a new extension ti-debpkgs, we would attempt to leverage the existing apa extension from config/sources/families/k3.conf.
Please have a look at my wip-texasinstruments branch in APA for what I have in mind. I added your cx33* packages in Recommends but possibly Depends might be more appropriate. |
|
So if I understand correctly, the following is the list of TODO's right now, right?:
Is this correct?
The cc33* packages are meant only for Trixie. Our current main offering (even in past 2-3 releases) is Trixie, since TI's custom mesa packages have dependencies only found in Trixie, not Bookworm.
Thanks for doing this, @leggewie. Just a few comments:
Anyway, I do get what you're saying, and I can take this up from here if you like. |
|
With regards to TODOs, I'd say moving your main packages to the armbian CDN is a possibility, not a necessity, certainly with the benefit of better China coverage. It can also happen at a later date, though, not connected to this PR. @igorpecovnik is the person to discuss that with. To add to the list, I'd say there are some changes to this PR in case you are happy with my suggestion to leverage APA and its extension instead of creating a new one. I've created a commit with what I believe are the appropriate changes on top of your PR. I haven't had the chance to test this, yet. After all, you may not like the idea or certain details. The philosophy is that armbian-ti-common pulls in the necessary packages for you and it would do so liberally as long as it doesn't create problems. To help me understand; What happens when somebody tries to install the cc33* packages on an older release? On Ubuntu? On an SBC that doesn't have the necessary hardware? The way things currently stand in my proposal, the cc33* packages are in Recommends and there is no issue if they are unavailable or uninstallable for example due to unsatisfied libraries on an older release. You would need to make sure appropriate versioning of the dependencies is in place in your cc33* packages. They can be easily installed and uninstalled after flashing by the user and they can be pulled in via Depending on the situation, it might make sense to rename the armbian-ti-common package to armbian-bsp-k3 or even armbian-bsp-family-ti-k3 instead if that is more in line with what it would do. I'll change the proposed sources.list.d snippet to trixie. https://raw.githubusercontent.com/TexasInstruments/ti-debpkgs/main/ti-debpkgs.sources as of now still points to bookworm which is where I copied that info. |
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
TI maintains its own repository of debian packages here.
This PR adds this repository in TI images by-default through a new extension. So when the user performs
apt update,apt installetc in Armbian images for TI boards, even TI's packages can be installed. Further, the PR also installs a few of TI's packages by-default in its images.Additionally: it also changes TI's U-boot link from git.ti.com to Github.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Please also note any relevant details for your test configuration.
This has been tested on SK-AM62B trixie image:
apt updateandapt installtestsChecklist:
Please delete options that are not relevant.