Opened 12 months ago

Closed 3 months ago

#23561 closed task (fixed)

Fix nsis builds for Windows 64

Reported by: boklm Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-rbm, TorBrowserTeam201806
Cc: tbb-team, sukhbir Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

We need to do Windows 64 builds of nsis.

Child Tickets

Change History (32)

comment:1 Changed 12 months ago by boklm

I tried a build with the following patch:

diff --git a/projects/nsis/build b/projects/nsis/build
index af19dd7..c75dced 100755
--- a/projects/nsis/build
+++ b/projects/nsis/build
@@ -16,7 +16,7 @@ do
     [ -f debian/patches/$patch ] && patch -p1 < debian/patches/$patch
 done
 [% SET scons_args = 'APPEND_CCFLAGS="-fgnu89-inline" VERSION=' _ c("version") 
-        _ " SKIPUTILS='NSIS Menu' XGCC_W32_PREFIX=i686-w64-mingw32-"
+        _ " SKIPUTILS='NSIS Menu' XGCC_W32_PREFIX=" _ c("arch") _ "-w64-mingw32-"
         _ ' PREFIX=/var/tmp/dist/nsis' -%]
 scons [% scons_args %]
 scons [% scons_args %] install

Which failed with the following error:

x86_64-w64-mingw32-g++ -s -mwindows -Wl,--file-alignment,512 -Wl,-Map,build/release/Banner/Banner.map -static-libgcc -static-libstdc++ -Wl,-e_DllMain@12 -nostdlib -Wl,--
exclude-libs,msvcrt.a -shared -o build/release/Banner/Banner.dll build/release/Banner/Banner.o -Lbuild/release/api/nsis -lpluginapi -lkernel32 -luser32 -Wl,--out-implib,
build/release/Banner/libBanner.a -Wl,--output-def,build/release/Banner/Banner.def
/var/tmp/dist/mingw-w64/lib/gcc/x86_64-w64-mingw32/5.4.0/../../../../x86_64-w64-mingw32/bin/ld: warning: cannot find entry symbol _DllMain@12; defaulting to 000000006cf0
1000
x86_64-w64-mingw32-g++ -o build/release/BgImage/BgImage.o -c -fgnu89-inline -Os -Wall -fno-strict-aliasing -DNSISCALL='__attribute__((__stdcall__))' -Ibuild/release/api 
Contrib/BgImage/BgImage.cpp
cc1plus: warning: command line option '-fgnu89-inline' is valid for C/ObjC but not for C++
Contrib/BgImage/BgImage.cpp: In function 'void SetBg(HWND, int, char*, stack_t**, extra_parameters*)':
Contrib/BgImage/BgImage.cpp:134:21: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
       (LPSTR)(DWORD)atomClass,
                     ^
Contrib/BgImage/BgImage.cpp:152:49: error: 'GWL_WNDPROC' was not declared in this scope
     oldProc = (void *)SetWindowLong(hwndParent, GWL_WNDPROC, (long)WndProc);
                                                 ^
Contrib/BgImage/BgImage.cpp:152:68: error: cast from 'LRESULT (*)(HWND, UINT, WPARAM, LPARAM) {aka long long int (*)(HWND__*, unsigned int, long long unsigned int, long 
long int)}' to 'long int' loses precision [-fpermissive]
     oldProc = (void *)SetWindowLong(hwndParent, GWL_WNDPROC, (long)WndProc);
                                                                    ^
Contrib/BgImage/BgImage.cpp: In function 'void AddText(HWND, int, char*, stack_t**, extra_parameters*)':
Contrib/BgImage/BgImage.cpp:296:39: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
   newImg->hFont = (HFONT)myatoi(szTemp);
                                       ^
Contrib/BgImage/BgImage.cpp: In function 'void Destroy(HWND, int, char*, stack_t**, extra_parameters*)':
Contrib/BgImage/BgImage.cpp:348:31: error: 'GWL_WNDPROC' was not declared in this scope
     SetWindowLong(hwndParent, GWL_WNDPROC, (long)oldProc);
                               ^
Contrib/BgImage/BgImage.cpp:348:50: error: cast from 'void*' to 'long int' loses precision [-fpermissive]
     SetWindowLong(hwndParent, GWL_WNDPROC, (long)oldProc);
                                                  ^
Contrib/BgImage/BgImage.cpp: In function 'LRESULT WndProc(HWND, UINT, WPARAM, LPARAM)':
Contrib/BgImage/BgImage.cpp:394:5: error: invalid conversion from 'long int (*)(HWND, unsigned int, unsigned int, long int) {aka long int (*)(HWND__*, unsigned int, unsi
gned int, long int)}' to 'WNDPROC {aka long long int (*)(HWND__*, unsigned int, long long unsigned int, long long int)}' [-fpermissive]
     );
     ^
In file included from /var/tmp/dist/mingw-w64/x86_64-w64-mingw32/include/windows.h:72:0,
                 from Contrib/BgImage/BgImage.cpp:1:
/var/tmp/dist/mingw-w64/x86_64-w64-mingw32/include/winuser.h:2092:29: note:   initializing argument 1 of 'LRESULT CallWindowProcA(WNDPROC, HWND, UINT, WPARAM, LPARAM)'
   WINUSERAPI LRESULT WINAPI CallWindowProcA (WNDPROC lpPrevWndFunc, HWND hWnd, UINT Msg, WPARAM wParam, LPARAM lParam);
                             ^
scons: *** [build/release/BgImage/BgImage.o] Error 1
scons: building terminated because of errors.

comment:2 Changed 12 months ago by gk

Keywords: TorBrowserTeam201710 added; TorBrowserTeam201709 removed

Items for October 2017

comment:3 Changed 11 months ago by boklm

As a workaround for this issue, we can use a 32bit nsis installer to install the 64bit Tor Browser.

comment:4 Changed 11 months ago by gk

Keywords: TorBrowserTeam201711 added; TorBrowserTeam201710 removed

Moving tickets over to November.

comment:5 Changed 11 months ago by boklm

Status: assignedneeds_review

The branch bug_23561 in my git repo has a patch for review:
https://gitweb.torproject.org/user/boklm/tor-browser-build.git/commit/?h=bug_23561&id=03a5abef9e76f9e7464e0110f068b9e65e9a841d

With this patch we use a 32bit nsis for the 64bit bundle. This create a 32bit installer to install the 64bit bundle.

comment:6 Changed 11 months ago by boklm

I pushed a more simple version of the patch in branch bug_23561_v2:
https://gitweb.torproject.org/user/boklm/tor-browser-build.git/commit/?h=bug_23561_v2&id=2740676bf72c1bd41cbef098a1018a31eaf3be87

The tor-browser output filename is the same in both branches, so the result should be the same:

./rbm/rbm showconf tor-browser filename --target torbrowser-windows-i686 --target alpha
tor-browser-7.5a7-windows-i686-328442

comment:7 in reply to:  6 Changed 11 months ago by boklm

Replying to boklm:

I pushed a more simple version of the patch in branch bug_23561_v2:
https://gitweb.torproject.org/user/boklm/tor-browser-build.git/commit/?h=bug_23561_v2&id=2740676bf72c1bd41cbef098a1018a31eaf3be87

Actually this patch doesn't work as expected. This windows-x86_64: windows-i686 line only apply inside projects/nsis, but not on the dependencies, such as the compiler, so it will be using the 64bit mingw-w64.

So we should use the patch from bug_23561. Sorry for the confusion.

The tor-browser output filename is the same in both branches, so the result should be the same:

./rbm/rbm showconf tor-browser filename --target torbrowser-windows-i686 --target alpha
tor-browser-7.5a7-windows-i686-328442

I used --target torbrowser-windows-i686 instead of --target torbrowser-windows-x86_64 here. If I use --target torbrowser-windows-x86_64 then I get a different tor-browser filename in the 2 branches.

comment:8 Changed 10 months ago by gk

Keywords: TorBrowserTeam201711R added; TorBrowserTeam201711 removed

comment:9 Changed 10 months ago by gk

Keywords: TorBrowserTeam201711 added; TorBrowserTeam201711R removed
Owner: changed from boklm to tbb-team
Parent ID: #20636
Status: needs_reviewassigned

Alright, pushed the workaround to master (commit 51e32622701c743727d9903c67e38579c1bc7b8b). I leave this ticket open and on our radar for the right fix, though.

comment:10 Changed 10 months ago by gk

Keywords: TorBrowserTeam201712 added; TorBrowserTeam201711 removed

Moving win64 tickets to December.

comment:11 Changed 10 months ago by boklm

We are currently using nsis version 2.51. We should check if this issue also affects version 3.02.1, which is the latest nsis version.

comment:12 Changed 8 months ago by gk

Keywords: TorBrowserTeam201801 added; TorBrowserTeam201712 removed

Moving tickets to 2018.

comment:13 Changed 8 months ago by gk

Keywords: TorBrowserTeam201802 added; TorBrowserTeam201801 removed

Moving tickets to Feb

comment:14 Changed 7 months ago by gk

Keywords: TorBrowserTeam201803 added; TorBrowserTeam201802 removed

Adding to our March plate.

comment:15 Changed 7 months ago by gk

If we bump our NSIS version to something else we should keep in mind to check whether our pe_checksum_fix.py hack is still needed/still working.

comment:16 Changed 5 months ago by gk

Keywords: TorBrowserTeam201804 added; TorBrowserTeam201803 removed

Moving our tickets to April.

comment:17 Changed 4 months ago by gk

Keywords: TorBrowserTeam201805 added; TorBrowserTeam201804 removed

Moving remaining tickets to May.

comment:18 Changed 4 months ago by gk

Cc: sukhe added

comment:19 Changed 4 months ago by gk

Cc: sukhbir added; sukhe removed

CCing the real sukhe

comment:20 Changed 3 months ago by gk

Keywords: TorBrowserTeam201806 added; TorBrowserTeam201805 removed

Moving our tickets to June 2018

comment:21 Changed 3 months ago by gk

sukhbir: once you are done with the Windows bundles for esr60, this would be a worthwhile bug spending your time on as we still need to workaround it which is annoying, see: #26363.

comment:22 Changed 3 months ago by sukhbir

Status: assignedneeds_review

For review:

https://github.com/azadi/tor-browser-build-1/commits/bug-23561

From the commit message:

Bug 23561: Fix NSIS builds for Windows 64

This commit adds support for building the 64-bit version of NSIS, and
also bumps the version to 3.03. Doing this enables us to build MAR files
in a 64-bit container for the 64-bit version of Tor Browser; see bug
26363 and bug 24477.

The pe_checksum_fix.py doesn't work in a 64-bit container with the
bundled python-pefile version so we build its latest version to fix this
issue. This change is borrowed from commit bb32ec9 and updates
python-pefile to 2017.11.5.

The Debian package and the patches are no longer required as all changes
were merged upstream in 3.01-1. (See the nsis changelog in Debian.)

After building Tor Browser nightly 32- and 64-bit, we get the respective NSIS installers. (Tested on Windows 10.)

Last edited 3 months ago by sukhbir (previous) (diff)

comment:23 Changed 3 months ago by boklm

Status: needs_reviewneeds_revision

In projects/nsis/build there is a line [% IF c("var/windows") %], but this IF is always true as we only build nsis for Windows. So I think the IF can be removed.

Otherwise this commit looks good to me. I have started builds on 2 machines to see if I get the same result.

comment:24 in reply to:  23 ; Changed 3 months ago by boklm

Replying to boklm:

In projects/nsis/build there is a line [% IF c("var/windows") %], but this IF is always true as we only build nsis for Windows. So I think the IF can be removed.

In projects/nsis/config the line enable: '[% c("var/windows") %]' can also be removed.

comment:25 in reply to:  24 Changed 3 months ago by sukhbir

Status: needs_revisionneeds_review

Replying to boklm:

Replying to boklm:

In projects/nsis/build there is a line [% IF c("var/windows") %], but this IF is always true as we only build nsis for Windows. So I think the IF can be removed.

In projects/nsis/config the line enable: '[% c("var/windows") %]' can also be removed.

Thanks, I removed the c("var/windows") guard for zlib in both the build and the config files. (Same branch for review.)

https://github.com/azadi/tor-browser-build-1/commits/bug-23561

comment:26 Changed 3 months ago by gk

One thing I wondered on IRC is how to avoid depending on another external party, files.pythonhosted.org, not being down once we build + about whether rebuilding python-future and python-pefile every time is a smart idea.

I guess we could try at least for python-future the 0.15.2 package from Debian. Which leaves the other one. FWIW: Do we have a policy for when we build own projects and when it is okay to build a project during another one? I.e.: should be build python-future and python-pefile in own projects to avoid building them over and over again (in case they indeed need to get built)?

comment:27 in reply to:  26 Changed 3 months ago by boklm

Replying to gk:

One thing I wondered on IRC is how to avoid depending on another external party, files.pythonhosted.org, not being down once we build + about whether rebuilding python-future and python-pefile every time is a smart idea.

I think we could mirror those files on some reliable hosting. Related to that I opened #25835 as we have the same issue when people.tpo is down.

I guess we could try at least for python-future the 0.15.2 package from Debian.

It looks like a good idea to try that. I think this package didn't exist in the Ubuntu version we were using when we started doing this.

Which leaves the other one. FWIW: Do we have a policy for when we build own projects and when it is okay to build a project during another one? I.e.: should be build python-future and python-pefile in own projects to avoid building them over and over again (in case they indeed need to get built)?

I can think of the following reasons for wanting to move builds into a separate project (maybe there are more):

  • the build of that project takes some time, and the other project in which we could include it is something we rebuild more often (such as firefox)
  • we need to clone a git repository (we can have only one per project)
  • moving it to a separate project makes things more simple or more clear

Running time python setup.py install --user for python-future and python-pefile on my laptop, I get this:

future:
real    0m0.811s
user    0m0.535s
sys     0m0.238s

pefile:
real    0m0.261s
user    0m0.140s
sys     0m0.110s

Building them takes less than 2 seconds, so it doesn't seem worth moving them to a separate project.

comment:28 in reply to:  26 Changed 3 months ago by boklm

Replying to gk:

I guess we could try at least for python-future the 0.15.2 package from Debian.

This fixup commit is using the python-future package from jessie-backports:
https://gitweb.torproject.org/user/boklm/tor-browser-build.git/commit/?h=bug_23561_v4&id=8ecea4a935a8f03754234656c94b18ff3b5af568

I have two matching x86_64 builds, and am waiting for i686 builds to finish to check if they match too.

comment:29 Changed 3 months ago by boklm

While looking at the commit again, I noticed a small issue: the pe_checksum_fix.py file is no longer included only in the Windows builds. This is fixed by this fixup commit:
https://gitweb.torproject.org/user/boklm/tor-browser-build.git/commit/?h=bug_23561_v4&id=e2a135f648cbd0ce3dbc366c62a431f42efdf2b8

comment:30 Changed 3 months ago by boklm

Branch bug_23561_v5 has the commit with the two fixups:
https://gitweb.torproject.org/user/boklm/tor-browser-build.git/commit/?h=bug_23561_v5&id=0b884dc0c06b5467ef91e23ea0128c9e2c0169bd

My two builds for x86_64 and i686 are matching, so it seems the build is reproducible.

comment:32 Changed 3 months ago by boklm

Resolution: fixed
Status: needs_reviewclosed

I pushed bug_23561_v6 to master as commit 951b1a7e01000f3dc1529c497b64d32cc0e58339.

Note: See TracTickets for help on using tickets.