Closed Bug 1353541 Opened 7 years ago Closed 7 years ago

MinGW build broken by rustc

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: jdm, Assigned: tjr)

References

(Blocks 1 open bug)

Details

(Whiteboard: [tor])

Attachments

(2 files)

Build error
1.55 KB, text/plain
Details
Bug 1353541 Fix rustc in MinGW build
59 bytes, text/x-review-board-request
froydnj
: review+
ted
: review+
Details
Attached file Build error — 
I heard from a contributor with a mingw build that they were stuck on the configure step. The log shows that the Rust compiler can't find the standard library since it's looking for a different target than the one that's installed.
I think this aligns with a bug I have not formalized but am vaguely aware of.  A patch that may fix it is:


--- a/build/moz.configure/rust.configure
+++ b/build/moz.configure/rust.configure
@@ -165,8 +165,8 @@ def rust_triple_alias(host_or_target):
             # Windows
             # XXX better detection of CXX needed here, to figure out whether
             # we need i686-pc-windows-gnu instead, since mingw32 builds work.
-            ('x86', 'WINNT'): 'i686-pc-windows-msvc',
-            ('x86_64', 'WINNT'): 'x86_64-pc-windows-msvc',
+            ('x86', 'WINNT'): 'i686-pc-windows-gnu',
+            ('x86_64', 'WINNT'): 'x86_64-pc-windows-gnu',
         }.get((host_or_target.cpu, os_or_kernel), None)
 
         if rustc_target is None:


But I don't know if this is a complete fix, I had a different rust error after applying it:


make[5]: *** No rule to make target '../../toolkit/library/rust/../i686-pc-windows-gnu/release/libgkrust.a', needed by 'xul.dll'.  Stop.
/home/tom/Documents/moz/mingw-work/just-build/config/recurse.mk:71: recipe for target 'toolkit/library/target' failed


I haven't investigated any of this very deeply.
Blocks: 1330608
Whiteboard: [tor]
Comment on attachment 8859653 [details]
Bug 1353541 Fix rustc in MinGW build

https://reviewboard.mozilla.org/r/131668/#review134430

::: build/moz.configure/rust.configure:168
(Diff revision 1)
>              ('arm', 'Android'): 'armv7-linux-androideabi',
>              ('x86', 'Android'): 'i686-linux-android',
>              # Windows
>              # XXX better detection of CXX needed here, to figure out whether
>              # we need i686-pc-windows-gnu instead, since mingw32 builds work.
> -            ('x86', 'WINNT'): 'i686-pc-windows-msvc',
> +            ('x86', 'WINNT'): 'i686-pc-windows-gnu',

This is temporary, need to fix in next rev.
Besides the target being incorrect, rustc generates .lib files, when MinGW expects .a.  See https://github.com/rust-lang/rust/pull/29520 for the rustc discussion.
Summary: Mingw build passes msvc target to Rust compiler → MinGW build broken by rustc
Assignee: nobody → tom
Attachment #8859653 - Flags: review?(ted)
Attachment #8859653 - Flags: review?(nfroyd)
Comment on attachment 8859653 [details]
Bug 1353541 Fix rustc in MinGW build

https://reviewboard.mozilla.org/r/131670/#review134520

Everything else seems fine, but the comment below is a blocker, I think.  Or are we just ignoring the possibility of things like clang-cl cross compilation in other mingw patches that you're writing?

::: build/moz.configure/rust.configure:133
(Diff revision 2)
> +        # If we are targetting Windows but the host is Linux, we need to
> +        # use a different rustc target
> +        os_or_kernel = 'WINNT-MINGW' if host.kernel == "Linux" and host_or_target.kernel == "WINNT" else os_or_kernel

This seems like the wrong check.  What about the (admittedly hypothetical at the moment) world where somebody wants to cross-compile to Windows using clang-cl?  That's going to want to use the windows-msvc target, rather than the windows-gnu target.

I think you want to check `c_compiler` or similar here, if that's at all possible.
Attachment #8859653 - Flags: review?(nfroyd) → review-
Comment on attachment 8859653 [details]
Bug 1353541 Fix rustc in MinGW build

https://reviewboard.mozilla.org/r/131670/#review134522

::: build/moz.configure/rust.configure:133
(Diff revision 2)
> +        # If we are targetting Windows but the host is Linux, we need to
> +        # use a different rustc target
> +        os_or_kernel = 'WINNT-MINGW' if host.kernel == "Linux" and host_or_target.kernel == "WINNT" else os_or_kernel

We do not support nor intend to support compiling with clang-cl on Linux for Windows, no. I believe this statement is true not just for 'Tor/MinGW stuff' but all of Mozilla Windows builds. 

I've talked with the clang-cl team at Google and they are treating 'building for Windows on Linux for clang-cl' as a hobby project and it's not really doable right now.

Down the road we might try to do that but it would be a whole new project. Again, I believe this is true for all of Mozilla.
Comment on attachment 8859653 [details]
Bug 1353541 Fix rustc in MinGW build

https://reviewboard.mozilla.org/r/131670/#review134640
Attachment #8859653 - Flags: review- → review+
Comment on attachment 8859653 [details]
Bug 1353541 Fix rustc in MinGW build

https://reviewboard.mozilla.org/r/131670/#review135854

This needs a small change, but otherwise looks good.

::: build/moz.configure/rust.configure:133
(Diff revision 4)
>          # https://github.com/rust-lang/rust/blob/master/src/librustc_back/target/mod.rs
>  
>          # Avoid having to write out os+kernel for all the platforms where
>          # they don't differ.
>          os_or_kernel = host_or_target.kernel if host_or_target.kernel == 'Linux' and host_or_target.os != 'Android' else host_or_target.os
> +        # If we are targetting Windows but the host is Linux, we need to

I'm going to concur with froydnj here--this isn't the right check. You'd hit this same issue if you were building on Windows using a mingw toolchain. The right thing to check is whether we're targeting Windows but building with GCC. We already have a `building_with_gcc`:
https://dxr.mozilla.org/mozilla-central/rev/e17cbb839dd225a2da7e5d5bec43cf94e11749d8/build/moz.configure/toolchain.configure#835

...so you can put that in `@depends` and instead write this as:
```
os_or_kernel = 'WINNT-MINGW' if building_with_gcc and host_or_target.kernel == "WINNT" else os_or_kernel
```
Attachment #8859653 - Flags: review?(ted) → review-
Comment on attachment 8859653 [details]
Bug 1353541 Fix rustc in MinGW build

https://reviewboard.mozilla.org/r/131670/#review136860

Thanks, this looks sensible.

::: build/moz.configure/rust.configure:135
(Diff revision 5)
>          # Avoid having to write out os+kernel for all the platforms where
>          # they don't differ.
>          os_or_kernel = host_or_target.kernel if host_or_target.kernel == 'Linux' and host_or_target.os != 'Android' else host_or_target.os
> +        # If we are targetting Windows but the compiler is gcc, we need to
> +        # use a different rustc target
> +        os_or_kernel = 'WINNT-MINGW' if building_with_gcc and host_or_target.kernel == "WINNT" else os_or_kernel

nit: we use single quotes for strings in these files.
Attachment #8859653 - Flags: review?(ted) → review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/51edd4c0ae9f
Fix rustc in MinGW build r=froydnj,ted
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/51edd4c0ae9f
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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: