Closed
Bug 551724
Opened 15 years ago
Closed 14 years ago
Disable ctypes on x86/msvc if MASM is unavailable
Categories
(Core :: js-ctypes, defect)
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)
(deleted),
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Callek
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
m_kato
:
review+
benjamin
:
approval2.0+
|
Details | Diff | Splinter Review |
+++ 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•15 years ago
|
Assignee: nobody → dwitte
Status: NEW → ASSIGNED
Comment 1•15 years ago
|
||
<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•15 years ago
|
||
Note also that that page suggests that VC8EE won't work with the W7 SDK...
Assignee | ||
Comment 3•15 years ago
|
||
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•15 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.
Comment 5•15 years ago
|
||
(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).
Updated•15 years ago
|
Attachment #431991 -
Flags: review?(ted.mielczarek) → review+
Assignee | ||
Comment 6•15 years ago
|
||
Comment on attachment 431991 [details] [diff] [review]
patch v1
[Checkin: Comment 6]
http://hg.mozilla.org/mozilla-central/rev/540a1651c059
Assignee | ||
Updated•15 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Updated•15 years ago
|
Target Milestone: --- → mozilla1.9.3a4
Updated•15 years ago
|
Flags: in-testsuite-
Comment 7•15 years ago
|
||
Attachment #433935 -
Flags: review?(bugspam.Callek)
Updated•15 years ago
|
Attachment #433935 -
Flags: review?(bugspam.Callek) → review+
Updated•15 years ago
|
Attachment #431991 -
Attachment description: patch v1 → patch v1
[Checkin: Comment 6]
Comment 8•15 years ago
|
||
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]
Comment 9•15 years ago
|
||
(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•15 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•15 years ago
|
||
Serge, does this get the error message working properly?
Assignee | ||
Comment 12•15 years ago
|
||
Comment on attachment 439296 [details] [diff] [review]
test "ml"
Nevermind, this is totally wrong.
Attachment #439296 -
Attachment is obsolete: true
Updated•14 years ago
|
Assignee | ||
Comment 14•14 years ago
|
||
Status is "patches accepted".
Comment 15•14 years ago
|
||
'$AS' values: "cl" locally, "ml.exe" on Try Server.
Attachment #483408 -
Flags: review?(ted.mielczarek)
Updated•14 years ago
|
Attachment #483408 -
Flags: review?(ted.mielczarek) → review?(sayrer)
Comment 16•14 years ago
|
||
(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.
Comment 17•14 years ago
|
||
(In reply to comment #16)
> Win64 configuration is AS=ml64.exe.
Currently (without my patch), does --enable-ctypes works fine on Win64?
Comment 18•14 years ago
|
||
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•14 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)
Updated•14 years ago
|
Attachment #489560 -
Flags: feedback?(m_kato) → review?(m_kato)
Updated•14 years ago
|
Attachment #489560 -
Flags: review?(m_kato) → review+
Comment 20•14 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]
"approval2.0=?":
Fix broken configure check which was added in 1.9.3a4. No risk.
Attachment #489560 -
Flags: approval2.0?
Comment 21•14 years ago
|
||
(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.
Comment 22•14 years ago
|
||
(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 ])
}
Updated•14 years ago
|
Attachment #489560 -
Flags: approval2.0? → approval2.0+
Comment 23•14 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]
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]
Comment 24•14 years ago
|
||
Tinderboxes are still fine and my local build now errors out at configure time :-)
V.Fixed
Updated•14 years ago
|
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•