Skip to content
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

Add b4 Tool #3506

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Add b4 Tool #3506

wants to merge 3 commits into from

Conversation

adityagesh
Copy link
Collaborator

@adityagesh adityagesh commented Nov 12, 2024

Add b4 tool to download upstream patches.
Add am method to git in order to apply this patch.

The usage of these will be raised as a separate PR. A new modifier will be added to kernel_installer to modify code using b4.

@@ -201,6 +201,17 @@ def apply(
)
result.assert_exit_code(message=f"failed on applying patches. {result.stdout}")

def am(self, cwd: pathlib.PurePath, patch: pathlib.PurePath) -> None:
result = self.run(
f"am {patch}",
Copy link
Collaborator

Choose a reason for hiding this comment

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

does git am depend on b4 package?

Copy link
Collaborator Author

@adityagesh adityagesh Nov 12, 2024

Choose a reason for hiding this comment

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

The output of b4 is a .mbx file. You can apply this patch to the repo using git am "filename.mbx"

As per description, git am can be used to apply any email related patch, whereas git apply is for the .patch files

Copy link
Collaborator

Choose a reason for hiding this comment

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

I didn't see the place which uses b4 tool

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will be raising that as a separate PR, added that in the description.
I have created a Modifier in Kernel Install transformer that uses b4 to apply patches

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can add those changes to this PR, if you prefer having it in the same PR

@@ -201,6 +201,17 @@ def apply(
)
result.assert_exit_code(message=f"failed on applying patches. {result.stdout}")

def am(self, cwd: pathlib.PurePath, patch: pathlib.PurePath) -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see you put arg name as "patch" unlike git apply where it has "patches". Does it not work with multiple patches/mboxes? Can you test once if not tested for the tools/library sake?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Interesting, I did not notice that.
Though the parameter is named patches, it accepts a Purepath and not list of Purepath.

I will verify the behavior and fix it along with Chi's comment below

Copy link
Collaborator

Choose a reason for hiding this comment

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

By multiple I meant something like a pattern.
Ref:

file_pattern: str = "*.patch"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is different, it is part of the modifier

lisa/tools/git.py Outdated Show resolved Hide resolved
Update apply method to accept .mbx files. These
are generated by b4 tool.
b4 patch modifier is a modifier for kernel source
installer. It accepts Message_id and apply the .mbx file using git
no_error_log=True,
)
file_extension = patches.suffix
if file_extension == ".mbx":
Copy link
Member

Choose a reason for hiding this comment

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

I just understood the scenario. Because the .mbx is always generated by b4, and the call sequence is always b4.am and git.apply. So the b4 can be called in git, or call git in b4, so it's easier to use.

Copy link
Collaborator Author

@adityagesh adityagesh Nov 18, 2024

Choose a reason for hiding this comment

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

How about something like this?

class Git(Tool):
  def apply_patch(self,....):
      # handles both apply and am
      
class B4(Tool):
  def am(self,....):
    self.run("am {patches}", ....)
    
  def apply(self,....):
      git = node.tool[Git]
      self.am()
      git.apply()

Copy link
Member

Choose a reason for hiding this comment

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

  1. The apply_patch and apply are confusing in the Git tool.
  2. If call B4 in git, so the API can handle different types of patch files automatically. But if call git in B4, it still needs to know the type of patch files, and then call either git.apply or b4.apply.

Copy link
Collaborator Author

@adityagesh adityagesh Nov 25, 2024

Choose a reason for hiding this comment

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

The tool b4 has dependency on Git while pulling patches (taking Message_id as the parameter), so I don't think we can add b4 to Git.

Currently in Git.apply, it takes the path as parameter, which is basically the operation after we download the patch using b4.
B4 after downloading the patch should always call git apply to apply the patch.

The b4 apply that I showed above is : download patch using b4 -> apply downloaded patch using git

Copy link
Member

Choose a reason for hiding this comment

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

  1. I saw the code mark Git as a dependency. It's not necessary, because it's always used after Git tool is called.
  2. The dependencies between b4 and git, are depended by how we manage them. The goal is to provide easy to use interface. If we provide B4 supports in git, the user doesn't need to take care the ext name, just call git to fit different file types. But if the B4 call git, it needs to know ext name firstly.

Copy link
Collaborator Author

@adityagesh adityagesh Nov 26, 2024

Choose a reason for hiding this comment

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

Are you recommending adding something like below in Git?

download_and_apply_patch(self, Message_Id):
 path = b4.am(Message_id)
 git.apply_patch(path)

I saw the code mark Git as a dependency. It's not necessary, because it's always used after Git tool is called.

If we have the tool B4, it is possible someone uses it directly before invoking git. b4 will fail is git is not installed and not set as a dependency

Copy link
Member

Choose a reason for hiding this comment

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

It's like below. There is no conflict to block direct use b4.

    def apply(
        self,
        cwd: pathlib.PurePath,
        patches: pathlib.PurePath,
    ) -> None:
        file_extension = patches.suffix
        if file_extension == ".<b4's ext>"
            from .b4 import B4
            b4 = self.node.tools[B4]
            b4.am(...
            self.run(...
        else:
            self.run(...

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.

4 participants