Closed Bug 469654 Opened 11 years ago Closed 10 years ago

Windows x64 build support

Categories

(Firefox Build System :: General, defect)

x86_64
Windows XP
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9.3a1

People

(Reporter: m_kato, Assigned: m_kato)

References

()

Details

Attachments

(2 files, 5 obsolete files)

Current configure.in works x86 only.  I will merge Windows x64 config.
Attached patch patch v0 (obsolete) — Splinter Review
MOZ_PATH_PROGS cannot handle a path which has space character such as "Program Files (x86)".  So after resolving it, I will send review.
The MOZ_PATH_PROGS bit should be fixed by bug 485264.
Thanks, Ted.  unfortunately, this still occurs even if applying bug 485264 (see bug 485264 comment #3).
Attached patch patch v1 (obsolete) — Splinter Review
Attachment #370579 - Attachment is obsolete: true
Attachment #374204 - Flags: review?(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?
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
Duplicate of this bug: 440964
Attachment #374204 - Flags: review?(ted.mielczarek) → review-
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.
(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?
Ok, sounds reasonable.

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

Yes. The binary should be mozjs on all platforms.
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #374204 - Attachment is obsolete: true
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.
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.
MOZ_BITS die die die! No mo 16 vs. 32 bit memories. Calgon, take me awayyy! ;-)

/be
Blocks: 504028
Blocks: 482143
(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.
Hardware: x86 → x86_64
Attached patch patch v2.1 (obsolete) — Splinter Review
now testing...
Attachment #387824 - Attachment is obsolete: true
(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 :)
(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?
(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 :)
Attached patch patch v2.2Splinter Review
update by Mook comment.
Attachment #390385 - Attachment is obsolete: true
Makoto, please add review tag and ted as reviewer
Attachment #398102 - Flags: review?(ted.mielczarek)
Ted, any chances you could review this soon?
Yes, I'll clean out my review queue this week. Sorry for the delay.
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!
Attachment #398102 - Flags: review?(ted.mielczarek) → review+
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.
Blocks: 504027
You may need put "checkin-needed" into keywords section to make your patch get checked in.
landed
http://hg.mozilla.org/mozilla-central/rev/9c2ef289c411
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Flags: in-testsuite-
Target Milestone: --- → mozilla1.9.3a1
Version: unspecified → Trunk
Product: Core → Firefox Build System
Attached patch accidental upload (obsolete) — Splinter Review
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.
Attachment #8984333 - Attachment description: part 2. replace proxy tunnel with new ipc-based tunnel → accidental upload
Attachment #8984333 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.