Skip to content

Conversation

@JohnTheCoolingFan
Copy link
Contributor

@JohnTheCoolingFan JohnTheCoolingFan commented Sep 20, 2025

Description

  1. Set rootfstype in armbianEnv.txt in the boot from mtd, system on sata/nvme scenario
  2. Add check for btrfs command when the btrfs filesystem is selected, and prompt the user whether to install it or not.

The first fix is an attempt to fix AR-2556 (armbian/config#233) but the original issue doesn't provide a way to reproduce, so i cannot verify if this does actually solve the issue. There's no mention of what boot method is used and where the root partition is installed to. If the rootfstype in /boot/armbianEnv.txt has no effect in the "boot from MTD" scenario, it could be excluded from this PR.

The btrfs check was introduced to make it more obvious what the issue is when the user select btrfs root and btrfs-progs package is not installed, as the script would only fail on "Partition too small" with Available: <empty string> MB as the message.

For the btrfs check, I have several points I'd like to get opinions on:

  1. Is a dialog needed in the first place? Or should the script just install btrfs-progs if the user selected a btrfs filesystem?
  2. Should the dialog stop the install process if the user denied installation?

How Has This Been Tested?

On Radxa Rock 5B+ with a sata ssd

  • armbian-install with boot from MTD, selecting btrfs filesystem type and checking /boot/armbianEnv.txt in the resulting partition for rootfstype=btrfs
  • armbian-install, selecting btrfs filesystem type for the root partition with and without btrfs-progs installed

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings

@JohnTheCoolingFan JohnTheCoolingFan requested a review from a team as a code owner September 20, 2025 12:17
@github-actions github-actions bot added the 11 Milestone: Fourth quarter release label Sep 20, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 20, 2025

Walkthrough

The PR changes packages/bsp/common/usr/bin/armbian-install. For MTD boot paths, the script now sets or appends a rootfstype=<chosen filesystem> line in armbianEnv.txt in addition to updating rootdev. In format_disk, when the user selects btrfs and mkfs.btrfs is missing, the script prompts to run apt update/apt install btrfs-progs; it proceeds if the user agrees and verifies the binary exists, otherwise exits with an error code.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Suggested reviewers

  • igorpecovnik

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Title Check ❓ Inconclusive The PR title "Armbian-install script fixes" is related to the changeset but is overly generic and does not clearly highlight the primary modifications (setting rootfstype for MTD boot and adding a btrfs-progs check/install prompt), so it is inconclusive under the project's title guidelines. Rename the title to a concise, specific line such as "armbian-install: set rootfstype for MTD boot and prompt to install btrfs-progs" so reviewers can quickly see the primary changes.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues Check ✅ Passed The changes implement the coding objectives from AR-2556 by writing rootfstype into /boot/armbianEnv.txt for the MTD boot path and by adding a btrfs-progs presence check with an interactive install flow, which addresses missing tooling and improves diagnostics; however the author could not fully verify the runtime effect on boot and a reviewer requested testing with btrfs as a kernel module, so follow-up verification is recommended before closing the linked issue.
Out of Scope Changes Check ✅ Passed All modifications are confined to the armbian-install script and directly relate to setting rootfstype or handling btrfs tooling; there are no unrelated file changes or functionality outside the linked issue objectives, so there are no out-of-scope changes.
Description Check ✅ Passed The description documents the two code changes, motivation, test steps performed, and open reviewer questions, so it is directly related to the changeset and satisfies this lenient check.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
🧪 Generate unit tests
  • 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 be30ae2 and 6efe6ca.

📒 Files selected for processing (1)
  • packages/bsp/common/usr/bin/armbian-install (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/bsp/common/usr/bin/armbian-install

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

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

@github-actions github-actions bot added size/small PR with less then 50 lines Needs review Seeking for review BSP Board Support Packages labels Sep 20, 2025
@coderabbitai coderabbitai bot requested review from iav and igorpecovnik September 20, 2025 12:18
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

📜 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 5c273e2 and be30ae2.

📒 Files selected for processing (1)
  • packages/bsp/common/usr/bin/armbian-install (2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: tabrisnet
PR: armbian/build#0
File: :0-0
Timestamp: 2025-08-30T06:56:33.372Z
Learning: In Armbian kernel configuration, the BTRFS configuration logic preserves existing settings (whether built-in 'y' or module 'm') and only sets BTRFS_FS to module when it was previously disabled or not set, achieving "allow but not require" flexibility while maintaining backward compatibility.
📚 Learning: 2025-09-01T06:11:43.476Z
Learnt from: wei633
PR: armbian/build#8557
File: config/bootscripts/boot-xpressreal-t3.cmd:0-0
Timestamp: 2025-09-01T06:11:43.476Z
Learning: In Armbian boot scripts without initramfs support, UUID= and LABEL= style device paths cannot be used for rootdev as they require initramfs to resolve during boot. Only direct device paths (/dev/mmcblkXpY) or PARTUUID= references (which U-Boot can resolve) will work.

Applied to files:

  • packages/bsp/common/usr/bin/armbian-install
📚 Learning: 2025-06-04T23:45:38.860Z
Learnt from: djurny
PR: armbian/build#8272
File: config/bootscripts/boot-mvebu.cmd:182-186
Timestamp: 2025-06-04T23:45:38.860Z
Learning: In config/bootscripts/boot-mvebu.cmd, the `fdtfile` variable is mandatory for booting and is pre-set by U-Boot, but can be overridden via armbianEnv.txt. If `fdtfile` is empty, the subsequent device tree file search logic will eventually fail and trigger the critical error "Cannot find DT!" with proper error handling.

Applied to files:

  • packages/bsp/common/usr/bin/armbian-install

@JohnTheCoolingFan JohnTheCoolingFan force-pushed the armbian-install-script-fixes branch from 5bec10d to 6efe6ca Compare September 20, 2025 12:59
@tabrisnet
Copy link
Contributor

this isn't a blocker, just a request: can you test if this boots with btrfs built as a module, in re #8561 (comment)
and let me know?
I do expect it will work, but it'd be great to have confirmation vs #8561 (comment)

@JohnTheCoolingFan
Copy link
Contributor Author

JohnTheCoolingFan commented Sep 20, 2025

@tabrisnet unfortunately, I can't get the board to boot from MTD even with ext4. It's an issue I am also going to try and find the cause of. But right now I can probably only reproduce the boot from SD, root on sata case. If that's enough, I can try it.

@tabrisnet
Copy link
Contributor

@tabrisnet unfortunately, I can't get the board to boot from MTD even with ext4. It's an issue I am also going to try and find the cause of. But right now I can probably only reproduce the boot from SD, root on sata case. If that's enough, I can try it.

sounds reasonable; please be sure to mention which FS are on boot & on root, esp in re this post.
Note that the u-boot Kconfig mentioned is a hair misleading, CMD_BTRFS isn't likely the operative part but rather FS_BTRFS. otoh, CMD_BTRFS implies FS_BTRFS so it's not incorrect.

@JohnTheCoolingFan
Copy link
Contributor Author

Finally managed to install and boot, had to reconfigure u-boot.

Setting rootfstype is important with boot from MTD, which is what this PR fixes too.

Building a custom kernel to make btrfs a module.

@JohnTheCoolingFan
Copy link
Contributor Author

@tabrisnet boots with root as btrfs, btrfs as module, no separate boot partition:

CONFIG_BTRFS_FS=m
CONFIG_BTRFS_FS_POSIX_ACL=y
# CONFIG_BTRFS_FS_RUN_SANITY_TESTS is not set
# CONFIG_BTRFS_DEBUG is not set
# CONFIG_BTRFS_ASSERT is not set
# CONFIG_BTRFS_EXPERIMENTAL is not set
# CONFIG_BTRFS_FS_REF_VERIFY is not set

@tabrisnet
Copy link
Contributor

tabrisnet commented Oct 11, 2025 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

11 Milestone: Fourth quarter release BSP Board Support Packages Needs review Seeking for review size/small PR with less then 50 lines

Development

Successfully merging this pull request may close these issues.

2 participants