Disable ctypes on x86/msvc if MASM is unavailable

VERIFIED FIXED in mozilla2.0b8

Status

()

Core
js-ctypes
VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: dwitte@gmail.com, Assigned: dwitte@gmail.com)

Tracking

Trunk
mozilla2.0b8
x86
Windows XP
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed1.9.3a4: patches v1 and Bv1-CC])

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

7 years ago
+++ This bug was initially created as a clone of Bug #538216 +++

See bug 538216 comment 26 and on. The problem is that MASM is not shipped with VS8 Express, and having Mozilla build with this is important for similarity of toolchain because our release builds are done with VS8 Pro. Numerous options have been debated for resolving this:

1) Require VS9 Express or higher. Untenable for reason mentioned above. (Though this will become tenable when we switch production to VC10.)
2) Require installation of the free MASM download for VC8 Express. Untenable because it is provided under a noncommercial license.
3) Require installation of the Vista SDK, which includes it. Untenable because the Windows 7 SDK should work but does not include it.
4) Rewrite the libffi x86/msvc assembly to use inline asm, which can be built with cl. Untenable without degrading code quality because jump tables are used, and those cannot be implemented with inline asm.
5) Reinstate libffi_msvc, which does use inline asm, with a configure switch to enable it only if MASM is unavailable. Untenable because libffi_msvc is not feature complete, and would result in two differing feature sets, one of which would be untested by our infrastructure.

If any of those are inaccurate, or there are additional options, please speak up now.

Thus we settle on this: throw a configure error if MASM is unavailable, and provide a --disable-ctypes switch to get around it. This way people know they're not getting a feature-complete build, and have some options if they want one.

Once option 1) is available, we can remove the MASM configure check.
(Assignee)

Updated

7 years ago
Assignee: nobody → dwitte
Status: NEW → ASSIGNED
<https://developer.mozilla.org/En/Windows_SDK_versions> might be a good place to document it since it already has a breakdown by Visual C++ version, but "Windows SDK versions" might not be an appropriate name for the page any more. If we decide to rename it we'll either need to add a redirect or change the reference to it in configure.in.

Comment 2

7 years ago
Note also that that page suggests that VC8EE won't work with the W7 SDK...
(Assignee)

Comment 3

7 years ago
Created attachment 431991 [details] [diff] [review]
patch v1
[Checkin: Comment 6]

Something like this. Passes tryserver.

Can someone with VC8EE confirm that this balks with the right message? And that --disable-ctypes then works?
Attachment #431991 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 4

7 years ago
Note to ted: msvcc.sh explicitly calls up 'ml', whereas I'm testing $AS in the configure check. Not sure what you want here, but I could hardcode it to 'ml' if you want.
(In reply to comment #0)
> 3) Require installation of the Vista SDK, which includes it. Untenable because
> the Windows 7 SDK should work but does not include it.

Also of note: If you have VC8EE installed AND the Vista SDK, Mozbuild does not find the ml.exe included in the SDK. Perhaps MozBuild should include both paths, so that we do find ml.exe if the Vista SDK is installed.

(I can't test this because I installed ml.exe manually).
Attachment #431991 - Flags: review?(ted.mielczarek) → review+
(Assignee)

Comment 6

7 years ago
Comment on attachment 431991 [details] [diff] [review]
patch v1
[Checkin: Comment 6]

http://hg.mozilla.org/mozilla-central/rev/540a1651c059
(Assignee)

Updated

7 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Updated

7 years ago
Target Milestone: --- → mozilla1.9.3a4
Flags: in-testsuite-
Created attachment 433935 [details] [diff] [review]
(Bv1-CC) Copy (the useful part of) it to comm-central
[Checkin: Comment 8]
Attachment #433935 - Flags: review?(bugspam.Callek)

Updated

7 years ago
Attachment #433935 - Flags: review?(bugspam.Callek) → review+
Attachment #431991 - Attachment description: patch v1 → patch v1 [Checkin: Comment 6]
Comment on attachment 433935 [details] [diff] [review]
(Bv1-CC) Copy (the useful part of) it to comm-central
[Checkin: Comment 8]


http://hg.mozilla.org/comm-central/rev/a37e03f88d28
Attachment #433935 - Attachment description: (Bv1-CC) Copy (the useful part of) it to comm-central → (Bv1-CC) Copy (the useful part of) it to comm-central [Checkin: Comment 8]
(In reply to comment #3)

> Can someone with VC8EE confirm that this balks with the right message?

It doesn't, compilation will fail later:
{
ml -nologo -safeseh -c -Fosrc/x86/win32.obj src/x86/win32.asm .../js/ctypes/libffi/msvcc.sh: ml: command not found
make[5]: *** [src/x86/win32.lo] Error 1
}

Fwiw, the added configure check does not error out because: $_MSC_VER=1400 and $AS=cl.

NB: I have VS8E and 2003R2 SDK.

> And that --disable-ctypes then works?

It does.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 10

7 years ago
(In reply to comment #9)

> Fwiw, the added configure check does not error out because: $_MSC_VER=1400 and
> $AS=cl.

Lame. :(

We should check that $AS begins with the string "ml", then. Or maybe that "ml" exists and is executable? That would be better, assuming it works if "ml" is in the PATH.
(Assignee)

Comment 11

7 years ago
Created attachment 439296 [details] [diff] [review]
test "ml"

Serge, does this get the error message working properly?
(Assignee)

Comment 12

7 years ago
Comment on attachment 439296 [details] [diff] [review]
test "ml"

Nevermind, this is totally wrong.
Attachment #439296 - Attachment is obsolete: true
(Assignee)

Comment 13

7 years ago
Don't have time to work on this.
Assignee: dwitte → nobody
status2.0: --- → ?
(Assignee)

Comment 14

7 years ago
Status is "patches accepted".
Created attachment 483408 [details] [diff] [review]
(Cv2) Explicitly check that '$AS' is 'ml.exe' (and not 'cl'), Make the error message more explicit

'$AS' values: "cl" locally, "ml.exe" on Try Server.
Attachment #483408 - Flags: review?(ted.mielczarek)
Attachment #483408 - Flags: review?(ted.mielczarek) → review?(sayrer)
Blocks: 538324
(In reply to comment #15)
> Created attachment 483408 [details] [diff] [review]
> (Cv2) Explicitly check that '$AS' is 'ml.exe' (and not 'cl'), Make the error
> message more explicit
> 
> '$AS' values: "cl" locally, "ml.exe" on Try Server.

Serge, Do you consider Win64?  Win64 configuration is AS=ml64.exe.  So please exclude when OS_TEST is x86_64.
(In reply to comment #16)
> Win64 configuration is AS=ml64.exe.

Currently (without my patch), does --enable-ctypes works fine on Win64?
Created attachment 489560 [details] [diff] [review]
(Cv2a) Explicitly check that '$AS' is 'ml.exe'/'ml64.exe' (and not 'cl'), Make the error message more explicit
[Checked in: Comment 23]

Makoto: hopefully, this is enough to support Win64, isn't it?
Attachment #483408 - Attachment is obsolete: true
Attachment #489560 - Flags: review?(sayrer)
Attachment #489560 - Flags: feedback?(m_kato)
Attachment #483408 - Flags: review?(sayrer)

Comment 19

7 years ago
Comment on attachment 489560 [details] [diff] [review]
(Cv2a) Explicitly check that '$AS' is 'ml.exe'/'ml64.exe' (and not 'cl'), Make the error message more explicit
[Checked in: Comment 23]

m_kato's review is sufficient here.
Attachment #489560 - Flags: review?(sayrer)
Attachment #489560 - Flags: feedback?(m_kato) → review?(m_kato)

Updated

7 years ago
Attachment #489560 - Flags: review?(m_kato) → review+
Comment on attachment 489560 [details] [diff] [review]
(Cv2a) Explicitly check that '$AS' is 'ml.exe'/'ml64.exe' (and not 'cl'), Make the error message more explicit
[Checked in: Comment 23]

"approval2.0=?":
Fix broken configure check which was added in 1.9.3a4. No risk.
Attachment #489560 - Flags: approval2.0?
(In reply to comment #15)
> '$AS' values: "cl" locally, "ml.exe" on Try Server.
configure.in has AS=ml so does the .exe come from MOZ_PATH_PROGS?

(In reply to comment #5)
> Also of note: If you have VC8EE installed AND the Vista SDK, Mozbuild does not
> find the ml.exe included in the SDK. Perhaps MozBuild should include both
> paths, so that we do find ml.exe if the Vista SDK is installed.
I believe the current MozBuild prefers the Vista SDK compiler (including ml.exe) if it's present.
(In reply to comment #21)
> (In reply to comment #15)
> > '$AS' values: "cl" locally, "ml.exe" on Try Server.
> configure.in has AS=ml so does the .exe come from MOZ_PATH_PROGS?

Indeed, it looks like so...

http://mxr.mozilla.org/mozilla-central/source/js/src/configure.in
{
175 if test -z "$CROSS_COMPILE"; then
176 case "$target" in
177 *-cygwin*|*-mingw*|*-msvc*|*-mks*)
183     if test -z "$AS"; then
184         case "${target_cpu}" in
185         i*86)
186             AS=ml;
187             ;;
188         x86_64)
189             AS=ml64;
190             ;;
191         esac
192     fi
194     ;;
199 esac
200 fi

304 if test "$target" != "$host"; then
426     MOZ_PATH_PROGS(AS, $AS "${target_alias}-as" "${target}-as", :)
437 else
441     MOZ_PATH_PROGS(AS, $AS as, $CC)
470 fi

986 AS='$(CC)'

1922 *-wince*|*-winmo*)
1929     AS="$AS_BIN"
2044 *-mingw*|*-cygwin*|*-msvc*|*-mks*)
2052     if test -n "$GNU_CC"; then
2073     else
2078         if test "$AS_BIN"; then
2079           AS="$(basename "$AS_BIN")"
2080         fi
2144     fi
}

http://mxr.mozilla.org/mozilla-central/source/js/src/build/autoconf/mozprog.m4
{
62 AC_DEFUN(MOZ_PATH_PROGS,
63 [  AC_PATH_PROGS($1,$2,$3,$4)
64   if test "$msyshost"; then
65     case "[$]$1" in
66     /*)
67       tmp_DIRNAME=`dirname "[$]$1"`
68       tmp_BASENAME=`basename "[$]$1"`
69       tmp_PWD=`cd "$tmp_DIRNAME" && pwd -W`
70       $1="$tmp_PWD/$tmp_BASENAME"
71       if test -e "[$]$1.exe"; then
72         $1="[$]$1.exe"
73       fi
74     esac
75   fi
76 ])
}
Attachment #489560 - Flags: approval2.0? → approval2.0+
Comment on attachment 489560 [details] [diff] [review]
(Cv2a) Explicitly check that '$AS' is 'ml.exe'/'ml64.exe' (and not 'cl'), Make the error message more explicit
[Checked in: Comment 23]

http://hg.mozilla.org/mozilla-central/rev/5832edda3ab3
Attachment #489560 - Attachment description: (Cv2a) Explicitly check that '$AS' is 'ml.exe'/'ml64.exe' (and not 'cl'), Make the error message more explicit → (Cv2a) Explicitly check that '$AS' is 'ml.exe'/'ml64.exe' (and not 'cl'), Make the error message more explicit [Checked in: Comment 23]
Tinderboxes are still fine and my local build now errors out at configure time :-)

V.Fixed
Assignee: nobody → dwitte
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
status2.0: ? → ---
Resolution: --- → FIXED
Whiteboard: [fixed1.9.3a4: patches v1 and Bv1-CC]
Target Milestone: mozilla1.9.3a4 → mozilla2.0b8
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.