Skip to content

Conversation

@iav
Copy link
Contributor

@iav iav commented Oct 27, 2025

After #8751 ODroid-M1 not boots.
With this string it boots again.

How Has This Been Tested?

Build image for ODroid-M1 with root on btrfs without patch. System not boots from image.
Build image for ODroid-M1 with root on btrfs with patch. System boots from image.

Checklist:

  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

@iav iav requested a review from rpardini as a code owner October 27, 2025 04:09
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 27, 2025

Walkthrough

Added a new board configuration variable BOOTFS_TYPE="ext4" in config/boards/odroidm1.conf. Modified boot partition creation logic in lib/functions/image/partitioning.sh: a boot partition is now created when BOOTSIZE != "0" and either BOOTFS_TYPE != ROOTFS_TYPE or BOOTPART_REQUIRED == "yes" (previously the condition checked for presence/non-empty BOOTFS_TYPE instead of comparing it to ROOTFS_TYPE). No other functional or control-flow changes detected.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Verify naming consistency and expected default value in config/boards/odroidm1.conf.
  • Check the new conditional in lib/functions/image/partitioning.sh for edge cases (empty or unset BOOTFS_TYPE/ROOTFS_TYPE) and correct quoting/word-splitting in shell.
  • Search for other code or docs that relied on the previous presence-check behavior for BOOTFS_TYPE.
  • Confirm interactions with BOOTPART_REQUIRED and any callers that set it.

Possibly related PRs

Suggested reviewers

  • chraac
  • igorpecovnik
  • amazingfate
  • catalinii

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title Check ✅ Passed The title "ODroidM1: add BOOTFS_TYPE=ext4 into board config to fix after #8751" directly and accurately describes the primary change in the pull request. The raw summary confirms that the main modification is adding the BOOTFS_TYPE variable with value "ext4" to the ODroid-M1 board configuration file. The title is concise, specific, and clearly communicates both what was changed and why (fixing a regression from PR #8751). The title is fully related to the main change in the changeset.
Description Check ✅ Passed The pull request description is related to the changeset and provides relevant context. It explains that the change fixes an ODroid-M1 boot regression introduced by PR #8751, describes the testing performed (building with and without the patch on btrfs root), and confirms that no new warnings were generated. While the description is brief, it clearly communicates the purpose and validation of the changes rather than being vague or completely off-topic.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ 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 121b463 and d313443.

📒 Files selected for processing (1)
  • lib/functions/image/partitioning.sh (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • lib/functions/image/partitioning.sh

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 11 Milestone: Fourth quarter release Needs review Seeking for review Hardware Hardware related like kernel, U-Boot, ... labels Oct 27, 2025
@github-actions github-actions bot added the Ready to merge Reviewed, tested and ready for merge label Oct 27, 2025
@github-actions
Copy link
Contributor

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

@github-actions github-actions bot removed the Needs review Seeking for review label Oct 27, 2025
@rpardini
Copy link
Member

This looks like it's forcing a separate /boot partition to address the btrfs case.
Simple builds with full-ext4 shouldn't need it.
I'd fix whatever is broken with the btrfs instead of crippling ext4 rootfs.

@igorpecovnik igorpecovnik added Work in progress Unfinished / work in progress Discussion Being discussed - Voice your opinions :) and removed Ready to merge Reviewed, tested and ready for merge labels Oct 27, 2025
@iav iav requested a review from a team as a code owner October 27, 2025 19:51
@github-actions github-actions bot added Needs review Seeking for review Framework Framework components labels Oct 27, 2025
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: 1

📜 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 22a8ffd and 121b463.

📒 Files selected for processing (1)
  • lib/functions/image/partitioning.sh (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
lib/functions/image/partitioning.sh (1)
extensions/cloud-init/cloud-init.sh (1)
  • extension_prepare_config__ci_compatibility_check (30-40)
🪛 GitHub Actions: Shellcheck - PR #8829 ("ODroidM1: add BOOTFS_TYPE=ext4 into board config to fix after #8751")
lib/functions/image/partitioning.sh

[error] 116-116: ShellCheck SC2053: Quote the right-hand side of != in [[ ]] to prevent glob matching. (lib/ code detected as critical; exit code 1)

@iav iav requested a review from amazingfate October 27, 2025 19:55
@iav iav force-pushed the odroidn2bootfrom branch from 121b463 to d313443 Compare October 27, 2025 20:04
# Check if we need boot partition
# Specialized storage extensions like cryptroot or lvm may require a boot partition
if [[ $BOOTSIZE != "0" && (-n $BOOTFS_TYPE || $BOOTPART_REQUIRED == yes) ]]; then
if [[ $BOOTSIZE != "0" && ( $BOOTFS_TYPE != "$ROOTFS_TYPE" || $BOOTPART_REQUIRED == "yes" ) ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

What about the condition that user just want a seperate boot partition with same filesystem type as root? There are other declarations like BOOTFS_TYPE="ext4" in config/boards.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

User just want a separate boot partition with same filesystem can use BOOTPART_REQUIRED == "yes".
Other declarations like BOOTFS_TYPE="ext4" in config/boards will work, I suppose.
BOOTFS_TYPE I test, it works. If you worries about other declarations — just say, will think about it.

BOOTFS_TYPE have to be filled in boadrconfigs for boards that can boot only from ext4 or fat fs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would like to remind you that sometimes, albeit rarely, not only ext4 and btrfs fs are used.

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

Labels

11 Milestone: Fourth quarter release Discussion Being discussed - Voice your opinions :) Framework Framework components Hardware Hardware related like kernel, U-Boot, ... Needs review Seeking for review size/small PR with less then 50 lines Work in progress Unfinished / work in progress

Development

Successfully merging this pull request may close these issues.

5 participants