Closed Bug 551724 Opened 14 years ago Closed 14 years ago

Disable ctypes on x86/msvc if MASM is unavailable

Categories

(Core :: js-ctypes, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla2.0b8

People

(Reporter: dwitte, Assigned: dwitte)

References

Details

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

Attachments

(3 files, 2 obsolete files)

+++ 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: 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.
Note also that that page suggests that VC8EE won't work with the W7 SDK...
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)
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+
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a4
Flags: in-testsuite-
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 → ---
(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.
Attached patch test "ml" (obsolete) — Splinter Review
Serge, does this get the error message working properly?
Comment on attachment 439296 [details] [diff] [review]
test "ml"

Nevermind, this is totally wrong.
Attachment #439296 - Attachment is obsolete: true
Don't have time to work on this.
Assignee: dwitte → nobody
Status is "patches accepted".
'$AS' values: "cl" locally, "ml.exe" on Try Server.
Attachment #483408 - Flags: review?(ted.mielczarek)
Attachment #483408 - Flags: review?(ted.mielczarek) → review?(sayrer)
(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?
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 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)
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
Closed: 14 years ago14 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.

Attachment

General

Created:
Updated:
Size: