Fix install_apt_sources failure detection

[antoinecellerier]

msg output going to stdout meant the = "0" check failed to appropriately detect failure to download the sources file (e.g. running on debian testing where VERSION_ID doesn't exist). msg output also was hidden from user visible output.

with this change return codes are unambiguous to test, and all msg output properly makes it to the user visible stdout

[coderabbitai]

📝 Walkthrough

Walkthrough

The PR refactors how install_apt_sources signals success or failure to its caller. Instead of printing return codes to stdout and comparing string output, the function now uses shell return codes, with the caller checking $? to decide whether to proceed with APT-based installation or fall back to local compilation.

Changes

Exit code refactoring in APT source installation

Layer / File(s)	Summary
Return code signaling in install_apt_sources and caller logic tools/libs/manage_apps.sh	install_apt_sources now returns exit codes (0 on fallback, 1 on success) instead of printing output; install_apps checks $? instead of comparing string values from command substitution. Fallback behavior (cleanup, warnings, local compilation) remains unchanged.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

A rabbit hops through scripts with glee,
Return codes dance, $?!
No echo strings in the exit flow,
Shell signals true, watch functions grow. 🐰

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: fixing failure detection in install_apt_sources by switching from string output comparison to return codes.
Description check	✅ Passed	The description is directly related to the changeset, explaining the problem with stdout interference and the solution of using return codes.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with 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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tools/libs/manage_apps.sh`:
- Around line 115-124: The branch in install_apt_sources currently returns 1 on
successful curl downloads and 0 on fallback, which is inverted; change it so
successful download returns 0 and the fallback/compile path returns non-zero
(e.g., 1), and update any callers (like install_apps) that rely on that return
so they invert their conditional or use the function directly in an if/then to
follow shell conventions; specifically, flip the return values in the curl
success/else block (referencing apt_source and key_path) and adjust
install_apps' logic to treat a 0 return as success and non-zero as failure.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 0815b7ef-0a4e-49ea-943c-465a399611de

📥 Commits

Reviewing files that changed from the base of the PR and between f606c69 and a36b2f1.

📒 Files selected for processing (1)

• tools/libs/manage_apps.sh

[mryel00]

Thanks for the PR, but there are a few things about this PR that should be checked before merging:
First of all the CodeRabbit already pointed out the inverted logic now. echo "0" is a failure, therefore if you actually change it to return codes, you should invert the return.

msg output going to stdout meant the = "0" check failed to appropriately detect failure to download the sources file (e.g. running on debian testing where VERSION_ID doesn't exist).

Sry, but I don't really understand that message 🙈
Did you mean to say that the $(install_apt_sources)" = "0" line fails because of the msg function printing something to the output?
I guess so and you are right. This somehow slipped through testing and review and your PR fixes this.

But I would like to stay consistent in the whole repo and we use the echo syntax everywhere. (I'm no bash wizzard, I just copied the existing syntax from the old installer)
So it makes more sense to just remove the msg inside the install_apt_sources and move them into install_apps. The whole printing is kinda doubled too, therefore moving this, would fix both issues.

If you like, you could apply the above mentioned change and create a second PR that refactors this whole echo style into proper returns.

[antoinecellerier]

Thanks for the quick review. I've pushed an updated version keeping prior style. I've confirmed the fix to be working locally. Let me know if I missed anything.

[mryel00]

I saw your addition of camera-streamer and spyglass will not be installed..
Do you think this information is something that needs to be said?

I think a most user would not even know what spyglass and camera-streamer are, so it might be unnecessary more text?
But that's just my pov and I always appreciate feedback 😄

[antoinecellerier]

I felt it was useful for me as in my upgrade path from v4 I'd been using camera-streamer which now no longer was available nor was there an easy way to install it. But I'm just an average end user. I can leave it out if you feel it's better. (I've since learned about spyglass and found it to be an easier alternative to run so my own use case has been addressed separately)

[mryel00]

I'm wondering about your setup now:

upgrade path from v4 I'd been using camera-streamer which now no longer was available

and

running on debian testing where VERSION_ID doesn't exist

Is this a PiOS or something different?
On a non PiOS camera-streamer should have been never installed in the first place, or am I missing something?

[antoinecellerier]

To be honest, I don't recall how I'd set it up originally, likely a vanilla debian + raspi test release install from official raspberry pi images? I've compared it with other systems I run testing (also rpi) or unstable (my main laptop) on and they also lack the VERSION_ID.

The target 3D printer raspberry pi running debian testing:

voron ~% cat /etc/os-release
PRETTY_NAME="Debian GNU/Linux forky/sid"
NAME="Debian GNU/Linux"
VERSION_CODENAME=forky
ID=debian
HOME_URL="https://www.debian.org/"
SUPPORT_URL="https://www.debian.org/support"
BUG_REPORT_URL="https://bugs.debian.org/"

Another one I run also on debian testing:

pizero ~% cat /etc/os-release
PRETTY_NAME="Debian GNU/Linux forky/sid"
NAME="Debian GNU/Linux"
VERSION_CODENAME=forky
ID=debian
HOME_URL="https://www.debian.org/"
SUPPORT_URL="https://www.debian.org/support"
BUG_REPORT_URL="https://bugs.debian.org/"

My main laptop on debian sid / unstable:

yoga ~% cat /etc/os-release
PRETTY_NAME="Debian GNU/Linux forky/sid"
NAME="Debian GNU/Linux"
VERSION_CODENAME=forky
ID=debian
HOME_URL="https://www.debian.org/"
SUPPORT_URL="https://www.debian.org/support"
BUG_REPORT_URL="https://bugs.debian.org/"

[mryel00]

So ig we should maybe add VERSION_CODENAME?
I have to investigate that but we still cannot provide prepackaged builds for it, unless it's using just the same libcamera version as stable 😅
I will definitely look into this in a few weeks. Can you open an issue about it with the 3 entries?

But back to the PR, I would not add this extra information for now, as fixing it would actually make more sense.

[antoinecellerier]

Yeah I guess packaging for debian testing and not just stable would be useful. But it's way more work than this PR is trying to address.

Also from a conventions perspective, distros typically will use the release name and not a version number to version their dists:
e.g. https://archive.raspberrypi.com/debian/dists/ and https://deb.debian.org/debian/dists/
Debian provides both the named releases and the unstable/testing/stable aliases. Raspberry Pi seems to have opted to only have named releases.

I'll open the issues tomorrow.

[mryel00]

Fyi, as you force-pushed a few times.
I will always use squash and merge and remove unnecessary commits from the commit message.
I will only use rebase if the provided commits are actually meant to be like separate PRs but are tied together somehow.
So no need to force push ^^

[mryel00]

This looks good to me now
Thanks a lot for this and all the extra information