469654 - Windows x64 build support

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Makoto Kato [:m_kato]]

Current configure.in works x86 only.  I will merge Windows x64 config.

Comment 1

[Makoto Kato [:m_kato]]

Attachment: patch v0

MOZ_PATH_PROGS cannot handle a path which has space character such as "Program Files (x86)".  So after resolving it, I will send review.

Comment 2

[(not currently active) Ted Mielczarek]

The MOZ_PATH_PROGS bit should be fixed by bug 485264.

Comment 3

[Makoto Kato [:m_kato]]

Thanks, Ted.  unfortunately, this still occurs even if applying bug 485264 (see bug 485264 comment #3).

Comment 4

[Makoto Kato [:m_kato]]

Attachment: patch v1

Comment 5

[(not currently active) Ted Mielczarek]

I like most of this patch, but I'm not sure how I feel about the js6450 bits, as rational as that change is. I know the 32 is purely historical at this point, and our Mac/Linux builds are just libmozjs.

Brendan: is this the right way to go for Windows x64, naming the library js6450.dll?

Comment 6

[Brendan Eich [:brendan]]

Argh, what is there to say? js3250.dll is named based on Netscape 5! Unless there is an ABI issue I don't know about, we should rename to whatever makes sense.

/be

Comment 8

[(not currently active) Ted Mielczarek]

Comment on attachment 374204 [details] [diff] [review]
patch v1

+        if test "$AS_BIN"; then
+            AS="$(basename "$AS_BIN")"
+        fi

What's the purpose of this change?

+            AC_MSG_ERROR([If you build $target, you have to set 32-bit toolchain to your PATH, instead of 64-bit version.])

I think this needs to read like:
"You are targeting i386 but using the 64-bit compiler."

Most of this patch looks good, but per brendan above and bsmedberg (on irc), I'd like you to just rename js3250.dll to mozjs.dll. This should be as simple as removing the LIBRARY_NAME bit from js/src/Makefile.in and fixing up MOZ_JS_LIBS in configure.

Comment 9

[Makoto Kato [:m_kato]]

(In reply to comment #8)
> (From update of attachment 374204 [details] [diff] [review])
> +        if test "$AS_BIN"; then
> +            AS="$(basename "$AS_BIN")"
> +        fi
> 
> What's the purpose of this change?

Because the result of "MOZ_PATH_PROGS(AS, $AS as, $CC)" becomes that AS is "$(CC)".  Even if x86 msvc, this is same result (AS = $(CC)).  (See your autoconf.mk of Win32 target.)  Also, this hack is used on WINCE target, too.

Win32 target of VC++ doesn't use command line assembler (ml.exe), so build error doesn't occur.

> 
> +            AC_MSG_ERROR([If you build $target, you have to set 32-bit
> toolchain to your PATH, instead of 64-bit version.])
> 
> I think this needs to read like:
> "You are targeting i386 but using the 64-bit compiler."

OK. I will change this.
 
> Most of this patch looks good, but per brendan above and bsmedberg (on irc),
> I'd like you to just rename js3250.dll to mozjs.dll. This should be as simple
> as removing the LIBRARY_NAME bit from js/src/Makefile.in and fixing up
> MOZ_JS_LIBS in configure.

Should I change it for WINCE, too?

Comment 10

[(not currently active) Ted Mielczarek]

Ok, sounds reasonable.

(In reply to comment #9)
> Should I change it for WINCE, too?

Yes. The binary should be mozjs on all platforms.

Comment 11

[Makoto Kato [:m_kato]]

Attachment: patch v2

Comment 12

[Phil Ringnalda (:philor)]

Your removed-files.in changes are missing a line to remove the old js3250.dll, and also I suspect you also want to try to remove both jsd3250.dll and jsd6450.dll, so that a 64 bit libxul installer can pave over both a 64 bit and a 32 bit non-libxul install.

Comment 13

[Phil Ringnalda (:philor)]

And for that matter, for non-libxul (and thus Tb/SM) it needs to remove the other jsd, making me think it might be a nice time for jsd to drop the MOZ_BITS bit and just have a single name.

Comment 14

[Brendan Eich [:brendan]]

MOZ_BITS die die die! No mo 16 vs. 32 bit memories. Calgon, take me awayyy! ;-)

/be

Comment 15

[Makoto Kato [:m_kato]]

(In reply to comment #14)
> MOZ_BITS die die die! No mo 16 vs. 32 bit memories. Calgon, take me awayyy! ;-)
> 
> /be

OS_BITS is using filename of self exec installer "firefox-<version>.<lang>.win<OS_BITS>.installer.exe".

If we decide that official filename of installer for x64, (ex. firefox-<version>.<lang>.win-x64.exe), I can change this and may remove OS_BITS.

Comment 16

[Makoto Kato [:m_kato]]

Attachment: patch v2.1

now testing...

Comment 17

[:Mook]

(In reply to comment #16)
> Created an attachment (id=390385) [details]
> patch v2.1

Note that this will fail make check due to changing config/rules.mk but not js/src/config/rules.mk.  Otherwise it seems to build fine for me.  Or maybe it's something else up my patch queue :)

Comment 18

[Makoto Kato [:m_kato]]

(In reply to comment #17)
 
> Note that this will fail make check due to changing config/rules.mk but not
> js/src/config/rules.mk.  Otherwise it seems to build fine for me.  Or maybe
> it's something else up my patch queue :)

Mook, what compiler and version do you use?

Comment 19

[:Mook]

(In reply to comment #18)
> (In reply to comment #17)
> 
> > Note that this will fail make check due to changing config/rules.mk but not
> > js/src/config/rules.mk.  Otherwise it seems to build fine for me.  Or maybe
> > it's something else up my patch queue :)
> 
> Mook, what compiler and version do you use?
mingw-w64 (gcc) off svn trunk, with a few local patches, cross compiled from 32bit msys (but with the binaries copied to look like a native compiler).

I can also try building with msvc8 and msvc9 if desired, but that's not my main working environment :)

Comment 20

[Makoto Kato [:m_kato]]

Attachment: patch v2.2

update by Mook comment.

Comment 21

[Nikolay Shopik]

Makoto, please add review tag and ted as reviewer

Comment 22

[Nikolay Shopik]

Ted, any chances you could review this soon?

Comment 23

[(not currently active) Ted Mielczarek]

Yes, I'll clean out my review queue this week. Sorry for the delay.

Comment 24

[(not currently active) Ted Mielczarek]

Comment on attachment 398102 [details] [diff] [review]
patch v2.2

+++ b/browser/installer/windows/packages-static
 [xpcom]
-bin\js3250.dll
+bin\mozjs.dll

I bitrotted this in bug 511642. On the plus side, all you have to do now is remove the ifdef here:
http://mxr.mozilla.org/mozilla-central/source/browser/installer/package-manifest.in#43

+    case "${host_cpu}" in
+    x86_64)
+        HOST_CFLAGS="$HOST_CFLAGS -D_AMD64_"
+        ;;
+    esac

Is this really necessary? Where are we relying on this define? (And what compilers produce it?)

+    if test "$HAVE_64BIT_OS"; then
+    	AC_DEFINE(_WIN64)
+    fi

I guess this is just for consistency (since we define _WIN32)?

--- a/embedding/config/basebrowser-win

Nice of you to patch it, although I'm guessing it's painfully out of date at this point anyway.

Looks good otherwise, sorry for the delay!

Comment 25

[Makoto Kato [:m_kato]]

This patch depends on comm-central, so after I get approve of all patches (I will post new patch to other bugs soon), I will land this.


(In reply to comment #24)
> (From update of attachment 398102 [details] [diff] [review])
> +++ b/browser/installer/windows/packages-static
>  [xpcom]
> -bin\js3250.dll
> +bin\mozjs.dll
> 
> I bitrotted this in bug 511642. On the plus side, all you have to do now is
> remove the ifdef here:
> http://mxr.mozilla.org/mozilla-central/source/browser/installer/package-manifest.in#43

I will sync this.

> +    case "${host_cpu}" in
> +    x86_64)
> +        HOST_CFLAGS="$HOST_CFLAGS -D_AMD64_"
> +        ;;
> +    esac
> 
> Is this really necessary? Where are we relying on this define? (And what
> compilers produce it?)

This define is for NSPR.  NSPR header needs _AMD64_.

> +    if test "$HAVE_64BIT_OS"; then
> +        AC_DEFINE(_WIN64)
> +    fi
> 
> I guess this is just for consistency (since we define _WIN32)?

If anyone creates IA64 build, this will be needed.  

> 
> --- a/embedding/config/basebrowser-win
> 
> Nice of you to patch it, although I'm guessing it's painfully out of date at
> this point anyway.

I think so.  But, until we remove it, we should consider this.

Comment 26

[Makoto Kato [:m_kato]]

Attachment: patch for checkin

Comment 27

[Nikolay Shopik]

You may need put "checkin-needed" into keywords section to make your patch get checked in.

Comment 28

[Makoto Kato [:m_kato]]

landed
http://hg.mozilla.org/mozilla-central/rev/9c2ef289c411

Comment 29

[Paul Vitale [:iidebyo]]

Attachment: accidental upload

This only replaces the tunnel using a new nr_socket implementation.  The actual
proxy detection code has not been touched.  The proxy url is retrieved but
address resolution is done by necko code in the parent process.  The new socket
wraps a WebrtcProxyChannel which uses necko to handle proxy negotiation.  This
has a happy side effect of enabling all authentication modes that necko already
supports for http proxies.