Opened 8 months ago

Closed 8 months ago

#29104 closed task (fixed)

Rebase Tor Browser patches against 60.5.0esr

Reported by: gk Owned by: tbb-team
Priority: Very High Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: TorBrowserTeam201901R, GeorgKoppen201901
Cc: mcs, brade Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

I make a ticket this time for the rebase for the upcoming release because I like to have someone review the result. The reason for that is that Mozilla reformatted the whole ESR60 tree to adhere to Google's coding style (see: bug 1513900) and we now have a ton of fallout from that.

Child Tickets

Change History (5)

comment:1 Changed 8 months ago by mcs

I did not realize that the reformatting was applied to the ESR code, but it make sense from Mozilla's perspective to do so. What is the status of automated tools to help with the rebase (after the reformatting)? I know Mozilla created some tools for hg and git, but I did not follow the conversation closely on their dev-platform list.

comment:2 Changed 8 months ago by gk

Keywords: TorBrowserTeam201901R added; TorBrowserTeam201901 removed
Status: newneeds_review

Replying to mcs:

I did not realize that the reformatting was applied to the ESR code, but it make sense from Mozilla's perspective to do so. What is the status of automated tools to help with the rebase (after the reformatting)? I know Mozilla created some tools for hg and git, but I did not follow the conversation closely on their dev-platform list.

That's been a good point which I had forgotten about.

https://groups.google.com/forum/#!topic/mozilla.dev.platform/kQ0q29dlmJQ

is relevant here and contains two tools for Git. I used https://github.com/emilio/clang-format-merge which I had to patch: ./mach clang-format did not work for me (not sure if that's expected for esr60), instead I made sure that the wrapper hit cat "$1" | clang-format "-assume-filename=$REPO_PATH" > "$1.tmp" (needs to have clang-format installed).

After that the rewriting mostly worked. There are five patches (IIRC) which I needed to adapt manually. bug_29104_rebase_6050esr_v2 (https://gitweb.torproject.org/user/gk/tor-browser.git/log/?h=bug_29104_rebase_6050esr_v2) in my tor-browser repo is the result. Let me know if that makes sense (it's not the final rebase for the release but I plan to use it for comparison to make sure the final rebase is correct, both for the alpha and the stable branch).

I additionally filed #29141 to track a clean-up of our search engine list patch.

comment:3 Changed 8 months ago by gk

Cc: mcs brade added

tor-browser-60.5.0esr-8.0-1 (https://gitweb.torproject.org/user/gk/tor-browser.git/log/?h=tor-browser-60.5.0esr-8.0-1) in my public tor-browser repo is what could be the branch for the 8.0.5 stable.

Assuming you have git-wrapper in your path you should get it by doing git-wrapper rebase -i --autosquash 5305958d84a17c0203074504874350683476b8de and git-wrapper rebase --continue after each of the conflicts got resolved, starting from tor-browser.60.4.0esr-8.0-1.

comment:4 Changed 8 months ago by mcs

r=brade, r=mcs
Everything looks good to us. As a double-check against the work of the automated reformatting tools, we quickly compared each of the rebased patches against each of the tor-browser-60.4.0esr-8.0-1 patches. The only thing we noticed that was a little surprising was that for some patches reformatting was done that was unrelated to the original patch, e.g., note the whitespace changes in the second of these two patches:

  • 432f8e57e03a3be2d4e5932eb61a6b3b88168d14 (60.4.0esr patch for #18900)
  • b81a07a586f7a2312645d35fd5a08b94fda919f5 (60.5.0esr patch for #18900)

Presumably slightly different clang-format rules were used. It looks like the format in the rebased patches matches that used on mozilla-central so it seems fine.

comment:5 in reply to:  4 Changed 8 months ago by gk

Resolution: fixed
Status: needs_reviewclosed

Replying to mcs:

r=brade, r=mcs
Everything looks good to us. As a double-check against the work of the automated reformatting tools, we quickly compared each of the rebased patches against each of the tor-browser-60.4.0esr-8.0-1 patches. The only thing we noticed that was a little surprising was that for some patches reformatting was done that was unrelated to the original patch, e.g., note the whitespace changes in the second of these two patches:

  • 432f8e57e03a3be2d4e5932eb61a6b3b88168d14 (60.4.0esr patch for #18900)
  • b81a07a586f7a2312645d35fd5a08b94fda919f5 (60.5.0esr patch for #18900)

Presumably slightly different clang-format rules were used. It looks like the format in the rebased patches matches that used on mozilla-central so it seems fine.

Yeah, I think the whitespace changes are okay. I took the rebased 8.0 branch for 8.0.5 build1 and will use the 8.5 one as a baseline for the final branch for the alpha. I think we are done here, thanks!

Note: See TracTickets for help on using tickets.