Skip to content

Move magiskboot cli to argh #8766

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Move magiskboot cli to argh #8766

wants to merge 4 commits into from

Conversation

yujincheng08
Copy link
Collaborator

No description provided.

@yujincheng08 yujincheng08 changed the title Upgrade argh to stable release More more cli to argh Feb 6, 2025
@yujincheng08 yujincheng08 force-pushed the argh branch 2 times, most recently from 173b8a3 to b26d5b0 Compare February 6, 2025 07:24
@topjohnwu
Copy link
Owner

@yujincheng08 cuttlefish starts failing after b26d5b0

@yujincheng08 yujincheng08 marked this pull request as draft February 12, 2025 00:51
@yujincheng08 yujincheng08 force-pushed the argh branch 4 times, most recently from 6e4df50 to ecbc7a1 Compare February 23, 2025 09:42
@yujincheng08 yujincheng08 marked this pull request as ready for review February 23, 2025 10:16
@yujincheng08
Copy link
Collaborator Author

@topjohnwu fixed

@topjohnwu topjohnwu force-pushed the argh branch 2 times, most recently from c5a58bc to 9833a89 Compare March 7, 2025 10:39
@yujincheng08

This comment was marked as resolved.

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request refactors various boot-related modules to adopt a new CLI interface and update type signatures for improved consistency. Key changes include:

  • Changing the parameter types for payload and patch extraction functions from specialized UTF-8 types to standard string types.
  • Refactoring the dtb and cpio command functions to return LoggedResult values instead of calling process exit.
  • Updating the ffi bridge in lib.rs with new function signatures and removing unused functions.

Reviewed Changes

Copilot reviewed 6 out of 10 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
native/src/boot/payload.rs Updated extract_boot_from_payload signature and removed Utf8CStr references.
native/src/boot/patch.rs Modified hexpatch to accept mutable String and Utf8CStr for from/to values.
native/src/boot/lib.rs Adjusted ffi bindings by removing unused imports and functions.
native/src/boot/dtb.rs Changed dtb_commands to use a more structured argument approach.
native/src/boot/cpio.rs Refactored cpio_commands to accept explicit arguments and return LoggedResult.
Files not reviewed (4)
  • native/src/Android.mk: Language not supported
  • native/src/boot/compress.hpp: Language not supported
  • native/src/boot/magiskboot.hpp: Language not supported
  • native/src/boot/main.cpp: Language not supported
Comments suppressed due to low confidence (2)

native/src/boot/payload.rs:31

  • Changing the parameter types from Utf8CStr to &str removes implicit UTF-8 validation; consider adding explicit checks or conversion to ensure the input is valid UTF-8.
pub fn extract_boot_from_payload( in_path: &str, partition_name: Option<&str>, out_path: Option<&str>, ) -> LoggedResult<()> {

native/src/boot/patch.rs:105

  • Changing the file parameter to a mutable String and converting it with Utf8CStr::from_string(file) may fail if the string is not valid UTF-8; ensure that invalid inputs are handled appropriately.
pub fn hexpatch(file: &mut String, from: &Utf8CStr, to: &Utf8CStr) -> bool {

CpioAction::Restore(_) => cpio.restore()?,
CpioAction::Patch(_) => cpio.patch(),
CpioAction::Exists(Exists { path }) => {
return Ok(cpio.exists(path));
Copy link
Preview

Copilot AI Apr 2, 2025

Choose a reason for hiding this comment

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

Using an early return within the loop may prevent processing of subsequent commands; clarify if early exit is intentional or if all commands should be processed sequentially.

Suggested change
return Ok(cpio.exists(path));
exists_result = Some(cpio.exists(path));

Copilot uses AI. Check for mistakes.

@yujincheng08 yujincheng08 changed the title More more cli to argh Move magiskboot cli to argh Apr 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants