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

Pipeline to generate and apply whistium patches #7498

Open
wants to merge 6 commits into
base: dev
Choose a base branch
from

Conversation

sardination
Copy link
Contributor

Ticket(s) Closed

  • N/A

Description
./generate_patches.sh <INSTANCE_IP> <PRIVATE_KEY> <INSTANCE_CHROMIUM_SRC_PATH> to generate patches and copy newly added files into the monorepo.
Example: ./generate_patches.sh 3.237.13.4 /Users/whist/Documents/suriya-keypair.pem /home/ubuntu/chromium/src

./apply_patches.sh <INSTANCE_IP> <PRIVATE_KEY> <INSTANCE_CHROMIUM_SRC_PATH> to apply patches and copy custom files from the monorepo into the remote Chromium repo.
Example: ./apply_patches.sh 3.237.13.4 /Users/whist/Documents/suriya-keypair.pem /home/ubuntu/chromium/src

Implementation
Use ssh and scp and built-in git utilities to generate and apply patches and to copy newly added files between the monorepo and the remote Chromium repository.

Documentation & Tests Added
N/A

Testing Instructions
Check the description to see how usage instructions. Try it out on your own remote Chromium repository!

PR Checklist

  • Did the PR author fully test this PR end-to-end?
  • Did one PR reviewer fully test this PR end-to-end?
  • Did one PR reviewer conduct a thorough code design review?

@github-actions github-actions bot added the 📁 Repo: mandelboxes This PR/Issue modifies /mandelboxes code label Nov 9, 2022
@sardination sardination force-pushed the suriya/generate-whistium-patches branch from 06631df to 84251b1 Compare November 9, 2022 23:23
Copy link
Contributor

@jkarthic jkarthic left a comment

Choose a reason for hiding this comment

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

Say if I am working on the whistium repo, is there an easy way to check the diff on the current working directory? Running the script to generate new patches and then checking the diff on a patch is very painful and difficult to understand. Also switching between branches in this method is very confusing.

Is there any issue with the git commit followed by git format-patch and git am approach that I outlined in the Slack channel. In that way, we could have the working directory clean when we start making our changes.

Does this per-file patch approach similar to brave have any significant advantage over the per-commit patch approach?

@sardination
Copy link
Contributor Author

Say if I am working on the whistium repo, is there an easy way to check the diff on the current working directory? Running the script to generate new patches and then checking the diff on a patch is very painful and difficult to understand. Also switching between branches in this method is very confusing.

This is a good point. The only way to get around this would be to commit in Chromium after applying patches, but then the generated patches won't be complete because they won't contain all of the changes that were committed.

Is there any issue with the git commit followed by git format-patch and git am approach that I outlined in the Slack channel. In that way, we could have the working directory clean when we start making our changes.

Does this per-file patch approach similar to brave have any significant advantage over the per-commit patch approach?

Here was my thinking: having each full update patch is like having version control within version control (history of patches within Github, which itself is version control). But having each individual patch as a separate file allows us to use GIthub as version control to see where changes have been made in each file. I personally prefer it this way to be able to more easily see the history of and review individual files, but I could be convinced otherwise.

@jkarthic
Copy link
Contributor

jkarthic commented Nov 10, 2022

The only way to get around this would be to commit in Chromium after applying patches, but then the generated patches won't be complete because they won't contain all of the changes that were committed.

Yes. This issue will be solved by the per-commit patch approach.

But having each individual patch as a separate file allows us to use GIthub as version control to see where changes have been made in each file. I personally prefer it this way to be able to more easily see the history of and review individual files, but I could be convinced otherwise.

In the per-commit patch approach you could easily see the per-file change history in your working repo after applying patches with the git am command. And this history will be much more easier to understand, since this will be a history on the actual file, rather the history of the patch file.

@sardination
Copy link
Contributor Author

The only way to get around this would be to commit in Chromium after applying patches, but then the generated patches won't be complete because they won't contain all of the changes that were committed.

Yes. This issue will be solved by the per-commit patch approach.

I've updated my scripts to commit right after applying patches - see if this fixes the problem?

But having each individual patch as a separate file allows us to use GIthub as version control to see where changes have been made in each file. I personally prefer it this way to be able to more easily see the history of and review individual files, but I could be convinced otherwise.

In the per-commit patch approach you could easily see the per-file change history in your working repo after applying patches with the git am command. And this history will be much more easier to understand, since this will be a history on the actual file, rather the history of the patch file.

But my issue with this approach is that you have to apply the patches to see the history in the same way as you would see when going through commit history on Github. For example, if I'm looking at the history of a change on a specific file in Github, I would like to use blame and go backward, which I can't do with the per-commit patch approach.

Copy link
Contributor

@jkarthic jkarthic left a comment

Choose a reason for hiding this comment

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

I've updated my scripts to commit right after applying patches - see if this fixes the problem?

Yes, this should address my primary concern.

@rpadaki
Copy link
Contributor

rpadaki commented Nov 11, 2022

Can we move the patches to whist/browsers/whistium instead of the mandelboxes folder? I think it will be useful to have these side-by-side

Copy link
Owner

@philippemnoel philippemnoel left a comment

Choose a reason for hiding this comment

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

deferring

Copy link
Contributor

@jkarthic jkarthic left a comment

Choose a reason for hiding this comment

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

Today I faced one frustrating issue with this per-file patch in the client side brave browser. Merge conflicts with these per-file patches are super difficult to resolve manually as it contains checksums and the +/- signs which are not easy to edit manually.

Here is my workflow, where I faced the above issue.

  • I had raised a PR for branch1. This PR is opened for review and was waiting for review comments.
  • Meanwhile I started doing my work on top of branch1.
  • Now I have received review comments for my PR.
  • I save my new changes as branch2. I switch to branch1. Addressed the review comments on branch1 and update the PR.
  • Now I switch to branch2. I rebase/cherry-pick branch2's changes on top of latest branch1. This shows merge conflict in the patch files, which was impossible to resolve manually.

In the per-commit patch approach, if we face any merge conflicts then we will have to resolve them on the actual source file. But in this approach we will have to resolve merge conflicts on the patch file, which is super difficult.

If we cannot agree on a patching approach, can we choose option1 and fork the entire chromium repo and get back to just simple git commits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📁 Repo: mandelboxes This PR/Issue modifies /mandelboxes code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants