Opened 6 months ago

Closed 5 months ago

#26203 closed task (fixed)

Adapt tor-browser-build firefox and tor-browser project for ESR 60 Windows build

Reported by: gk Owned by: tbb-team
Priority: Very High Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: ff60-esr, TorBrowserTeam201806R
Cc: sukhbir, arthuredelstein, boklm Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

We need to do a bunch of changes to the firefox and tor-browser projects in tor-browser-build to get the ESR60-based Windows builds going.

Child Tickets

TicketStatusOwnerSummaryComponent
#23231closedtbb-teamError in STL wrappers when building Firefox 64-bit for WindowsApplications/Tor Browser
#25554closedtbb-teamBump mingw-w64 version for ESR 60Applications/Tor Browser
#25837closedtbb-teamIntegrate fxc2 into our build setup for Windows Tor Browser buildsApplications/Tor Browser
#26204closedtbb-teamBundle d3dcompiler_47.dll for Tor Browser 8Applications/Tor Browser
#26205closedtbb-teamDon't build the uninstaller for Windows during Firefox compilationApplications/Tor Browser
#26206closedtbb-teamShip pthread related dll where neededApplications/Tor Browser
#26239closedtbb-teamBackport patch for bug 1448748 to fix Windows 64bit buildsApplications/Tor Browser
#26326closedtbb-teamwine error when building Firefox ESR60 Windows x86_64Applications/Tor Browser
#26329closedtbb-teamRust invocation for Firefox on 32bit Windows is failingApplications/Tor Browser

Change History (16)

comment:1 Changed 6 months ago by gk

Cc: sukhbir added; sukhe removed

CCing the real sukhe

comment:2 Changed 6 months ago by arthuredelstein

Cc: arthuredelstein added

comment:3 Changed 6 months ago by gk

Keywords: TorBrowserTeam201806 added; TorBrowserTeam201805 removed

Moving our tickets to June 2018

comment:4 Changed 5 months ago by sukhbir

Status: newneeds_review

For review:

https://github.com/azadi/tor-browser-build-1/tree/bug-26203

This should address #26205, #26206, #25837, #26326, #26204.

Some observations:

  1. We have a working build of Windows x86_64 with this branch. I verified it on Windows 10.
  1. I think we can do better for #26205 (commit 93f89a735792e329bd7853bcbd0db056426e32d6). Please review and suggest. Also, should this patch go in Firefox (as part of git_url) or as separate patches which we add during the build (current approach)?
  1. For fxc2, we are pointing to my fork of the project. We will switch to mozilla/fxc2 when https://github.com/mozilla/fxc2/pull/1 is merged.
  1. For Rust, I made the change below; let me know if we should remove the guards altogether:
-[% IF ! c("var/windows") %]
+[% IF ! c("var/windows-i686") %]
  1. For Firefox, I based gecko-dev/tree/win-ff60 (commit be179ca1a0c7812a332b16a4f1b87eb68c629d01) on tor-browser-60.0.1esr-8.0-1 and tjr's patches, including the ones from https://treeherder.mozilla.org/#/jobs?repo=try&revision=781b545807dfa9d3de23054bf413ec232a9ce1c1 (Sandbox).

comment:5 Changed 5 months ago by gk

Cc: boklm added
Keywords: TorBrowserTeam201806R added; TorBrowserTeam201806 removed

comment:6 in reply to:  4 Changed 5 months ago by boklm

Replying to sukhbir:

For review:

https://github.com/azadi/tor-browser-build-1/tree/bug-26203

Thanks. I will review, and suggest some fixup commits if needed.

  1. I think we can do better for #26205 (commit 93f89a735792e329bd7853bcbd0db056426e32d6). Please review and suggest. Also, should this patch go in Firefox (as part of git_url) or as separate patches which we add during the build (current approach)?

I think the patch should go in tor-browser.git (but adding it as a patch file is useful for testing before it is merged in tor-browser.git).

  1. For Rust, I made the change below; let me know if we should remove the guards altogether:
-[% IF ! c("var/windows") %]
+[% IF ! c("var/windows-i686") %]

I think we can remove the guards altogether, as we will want it at some point, and the build fails without rust.

comment:7 Changed 5 months ago by gk

We actually need libwinpthread-1.dll in Browser\TorBrowser\Tor it seems and not in Browser. Which means we need it for the expert bundle as well.

comment:8 in reply to:  7 Changed 5 months ago by cypherpunks

Replying to gk:

We actually need libwinpthread-1.dll in Browser\TorBrowser\Tor it seems and not in Browser. Which means we need it for the expert bundle as well.

This is #26206. And you need to ship it with every executable you built with your patched gcc. But that's not great, and you don't do that for msvcr100.dll, fwiw.

comment:9 Changed 5 months ago by gk

sukhbir: Why do we need pre_pkginst: dpkg --add-architecture i386 for the 64bit case? Compiling without it works for me.

comment:10 in reply to:  9 ; Changed 5 months ago by sukhbir

Replying to gk:

sukhbir: Why do we need pre_pkginst: dpkg --add-architecture i386 for the 64bit case? Compiling without it works for me.

Without adding it, I was getting something like it looks like multiarch needs to be enabled. Also, I based this on https://wiki.debian.org/Wine, which says:

On 64-bit systems you should enable a 32-bit architecture for multiarch. This is needed for running 32-bit Windows applications (many modern apps are still 32-bit), but also for large parts of the Windows subsystem itself. If in doubt, you do need it!

But maybe Wine64 is all we need if it works without that.

comment:11 in reply to:  10 ; Changed 5 months ago by boklm

Replying to sukhbir:

Replying to gk:

sukhbir: Why do we need pre_pkginst: dpkg --add-architecture i386 for the 64bit case? Compiling without it works for me.

Without adding it, I was getting something like it looks like multiarch needs to be enabled. Also, I based this on https://wiki.debian.org/Wine, which says:

On 64-bit systems you should enable a 32-bit architecture for multiarch. This is needed for running 32-bit Windows applications (many modern apps are still 32-bit), but also for large parts of the Windows subsystem itself. If in doubt, you do need it!

But maybe Wine64 is all we need if it works without that.

Isn't that the error that you were having when you were trying to use the 32bit d3dcompiler_47.dll?
When using the 64bit version of d3dcompiler_47.dll I don't think we need the 32bit version of wine.

comment:12 in reply to:  11 ; Changed 5 months ago by sukhbir

Replying to boklm:

Replying to sukhbir:

Replying to gk:

sukhbir: Why do we need pre_pkginst: dpkg --add-architecture i386 for the 64bit case? Compiling without it works for me.

Without adding it, I was getting something like it looks like multiarch needs to be enabled. Also, I based this on https://wiki.debian.org/Wine, which says:

On 64-bit systems you should enable a 32-bit architecture for multiarch. This is needed for running 32-bit Windows applications (many modern apps are still 32-bit), but also for large parts of the Windows subsystem itself. If in doubt, you do need it!

But maybe Wine64 is all we need if it works without that.

Isn't that the error that you were having when you were trying to use the 32bit d3dcompiler_47.dll?
When using the 64bit version of d3dcompiler_47.dll I don't think we need the 32bit version of wine.

OK, if that's the case and it does compile without it, we should remove it. Should I update the patch?

comment:13 in reply to:  12 Changed 5 months ago by boklm

Replying to sukhbir:

OK, if that's the case and it does compile without it, we should remove it. Should I update the patch?

I posted a new version of the patch, including this change and a few other changes in #25837 for review.

comment:14 Changed 5 months ago by gk

I looked over https://github.com/azadi/gecko-dev and all the cherry-picked patches. I omitted the following patches when pushing the changes to tor-browser-60.0.1esr-8.0-1:

1411401: That's only for Mozilla infra and we are essentially getting this patch by using mingw-w64's ee9fc3d0b8c8868280e38384edd968067b8e71fc.
1434316: That's only for Mozilla infra.
1460620: Let's leave this out for now and bundle the dll ourselves (as your patch is doing)
1461421: just cherry-pick the actual fix (the other two commits are stricly speaking not needed and I want to keep the patch number as small as possible)

comment:15 Changed 5 months ago by boklm

I pushed an updated version of the tor-browser-build patches in branch bug_26203_v6:
https://gitweb.torproject.org/user/boklm/tor-browser-build.git/log/?h=bug_26203_v6

comment:16 Changed 5 months ago by gk

Resolution: fixed
Status: needs_reviewclosed

We are done here, thanks all!

Note: See TracTickets for help on using tickets.