Closed Bug 1314979 Opened 8 years ago Closed 7 years ago

windows.configure doesn't support cross compilation

Categories

(Firefox Build System :: General, defect)

Product:
Firefox Build System
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: jacek, Unassigned)

References

(Blocks 1 open bug)

Details

(Whiteboard: [tor 1314979])

Attachments

(2 files)

hacks
3.36 KB, patch
Details | Diff | Splinter Review
Bug 1314979 Support cross compilation with MINGW in windows.configure
59 bytes, text/x-review-board-request
glandium
: review+
Details
Attached patch hacks — — Splinter Review 
When cross compiling (and generally compiling with mingw), most checks in windows.configure are not relevant. mingw comes with its own headers and libraries, so it shouldn't look for PSDK dirs. Also it shouldn't mess with PATH in this case.

I'm attaching a very hackish diff. It allowed me to get past the problem and I'm attaching it because it spots problems, but it's just a quick hack.
No longer depends on: 1263876
I guess not finding the proper windres binary falls into this ticket as well:

Creating Resource file: module.res
: -O coff --use-temp-file -DNDEBUG=1 -DTRIMMED=1 --include-dir /home/firefox/win52/mozilla-central/dom/media/gmp-plugin-openh264 --include-dir /home/firefox/win52/mozilla-central/obj-mingw/dom/media/gmp-plugin-openh264 --include-dir /home/firefox/win52/mozilla-central/obj-mingw/dist/include -o module.res /home/firefox/win52/mozilla-central/obj-mingw/dom/media/gmp-plugin-openh264/module.rc
Whiteboard: [tor]
Pushed an initial version of a commit that solves the configure problem (but not the WINDRES AND RANLIB problem.) Happy to hear how I should change it so that it's acceptable.
The WINDRES and RANLIB problem can be fixed by adding `ac_add_options --with-toolchain-prefix=i686-w64-mingw32-` to the mozconfig (instead of defining environment variables for them).
I tested your patch and it works great for me. I have a few comments:

- Skipping makensis checks is fine with me, but ideally we should be able to use it for mingw as well. AFAIR the old configure searched for it, but it didn't complain about it missing if mingw build was found.

- I'd remove |if test "$target" != "$host"; then| check from ANGLE changes. I don't build using mingw on Windows myself, but I think we should allow missing Windows SDK in this case as well.

- I like the idea of skipping windows.configure as whole. (I'd say the file should be named msvs.configure).
Comment on attachment 8830902 [details]
Bug 1314979 Support cross compilation with MINGW in windows.configure

https://reviewboard.mozilla.org/r/107568/#review131118

::: build/moz.configure/toolchain.configure:976
(Diff revision 8)
> +# Windows, for Windows. We observe that the target i686-w64-mingw32
> +# is different from i686-pc-mingw32 - the former is MinGW on Linux
> +# for Windows, the latter is building for Windows on Windows.
>  @depends(target)
>  def is_windows(target):
> -    return target.kernel == 'WINNT'
> +    return target.kernel == 'WINNT' and "-w64-" not in target.alias

I think a better test would be to add a dependency on host, and check that host.kernel == 'WINNT' too.

::: moz.configure:306
(Diff revision 8)
> +    # We observe that the target i686-w64-mingw32 is different
> +    # from i686-pc-mingw32 - the former is MinGW on Linux
> +    # for Windows, the latter is building for Windows on Windows.
> +    elif target.kernel == 'WINNT' and "-w64-" in target.alias:
> +        return

This should go in a separate bug which should be more along the lines of "make nsis an optional build dependency" than "disable nsis for mingw", because the latter prevents using nsis entirely, while it should actually work to create an installer.

::: old-configure.in:3253
(Diff revision 8)
>      else
>        AC_MSG_RESULT([Windows SDK not found.])
>      fi
>    else
> +    case "$target" in
> +    *-mingw*)

that's going to match normal windows builds...
Attachment #8830902 - Flags: review?(mh+mozilla)
Comment on attachment 8830902 [details]
Bug 1314979 Support cross compilation with MINGW in windows.configure

https://reviewboard.mozilla.org/r/107568/#review131118

> This should go in a separate bug which should be more along the lines of "make nsis an optional build dependency" than "disable nsis for mingw", because the latter prevents using nsis entirely, while it should actually work to create an installer.

Opened https://bugzilla.mozilla.org/show_bug.cgi?id=1355584 for this.
Comment on attachment 8830902 [details]
Bug 1314979 Support cross compilation with MINGW in windows.configure

https://reviewboard.mozilla.org/r/107568/#review131722

::: old-configure.in:3254
(Diff revision 9)
>        AC_MSG_RESULT([Windows SDK not found.])
>      fi
>    else
> +    if test "$target" != "$host"; then
> +        case "$target" in
> +        *-mingw*)

This combination of conditions still can match non-mingw builds. example: host is 64-bits windows, target is 32-bits windows. You should use the "same" condition as for including windows.configure. So, you want to check $OS_ARCH and $HOST_OS_ARCH. That being said, does any of the tests in this block actually set anything? If not, you could just skip the whole MOZ_ANGLE_RENDERER block?
Attachment #8830902 - Flags: review?(mh+mozilla)
Comment on attachment 8830902 [details]
Bug 1314979 Support cross compilation with MINGW in windows.configure

https://reviewboard.mozilla.org/r/107568/#review133258

::: old-configure.in:3133
(Diff revision 10)
>  MOZ_D3DCOMPILER_VISTA_DLL_PATH=
>  
>  if test "$COMPILE_ENVIRONMENT" ; then
>  case "$target_os" in
>  *mingw*)
> +    if test -z "$CROSS_COMPILE"; then

It feels like this really should be same kind of test as in toolchain.configure, cf. comment 16. Technically, CROSS_COMPILE is also true when host is windows 64-bits and target is windows 32-bits.

Please also add an explanation of the rationale for this change in the commit message (essentially an answer to the question in the last sentence from comment 16 is what I'm looking for).
Attachment #8830902 - Flags: review?(mh+mozilla)
Comment on attachment 8830902 [details]
Bug 1314979 Support cross compilation with MINGW in windows.configure

https://reviewboard.mozilla.org/r/107568/#review134066

::: old-configure.in:3133
(Diff revision 11)
>  MOZ_D3DCOMPILER_VISTA_DLL_PATH=
>  
>  if test "$COMPILE_ENVIRONMENT" ; then
>  case "$target_os" in
>  *mingw*)
> +    if test "$OS_ARCH" == "$HOST_OS_ARCH"; then

== is a bashism. Please use =.
Attachment #8830902 - Flags: review?(mh+mozilla) → review+
I'm r+'ing, but come to think of it, this may not be adressing the reason this bug was filed. Maybe the right check is whether the compiler is gcc... but that doesn't help with windows.configure :-/

I'd suggesting cloning this bug to keep it open for mingw builds on windows.
I added the concern to https://bugzilla.mozilla.org/show_bug.cgi?id=1355586 which has another MinGW on Windows issue.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c943802a5256
Support cross compilation with MINGW in windows.configure r=glandium
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c943802a5256
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Whiteboard: [tor] → [tor 1314979]
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: