In Bug 1390583, Georg is working on a mingw-clang build. In several places through our moz.build files, I used the following type of idiom to detect mingw(-gcc): > elif CONFIG['FFI_TARGET'] == 'X86_WIN64': > ffi_srcs = ['ffi.c'] > if CONFIG['CC_TYPE'] == 'gcc': > if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'windows': > if CONFIG['CC_TYPE'] != 'gcc': That's no longer going to work. And we can't add 'clang' in there, because of clang-cl builds on Windows. We could say something like 'Host OS is Linux and Target is Windows'... BUT it might be good to future-proof ourselves against a future 'clang on Linux for Windows but not MinGW' build. I could make CONFIG['CC_TYPE'] = 'mingw'.... BUT it's likely there will be a place where mingw-gcc differs from mingw-clang. So the ideas I have are: Ideas: - Make CONFIG['CC_TYPE'] equal mingw-gcc or mingw-clang - Make CONFIG['MINGW_BUILD'] - Something else What do people think?

(In reply to Tom Ritter [:tjr] from comment #0) > - Make CONFIG['CC_TYPE'] equal mingw-gcc or mingw-clang I personally like this approach, in that it forces us to be explicit everywhere and there's no room for doubt in casual reading of the source. But I feel like I'm not really an authority on these things. Let's see what glandium says.

If we're going to wind up with a lot of "this is specific to mingw regardless of whether we're using gcc or clang" then adding something like `CONFIG['MINGW']` should be fine. FYI, we do differentiate between clang and clang-cl in `CC_TYPE`: https://dxr.mozilla.org/mozilla-central/rev/0ef34a9ec4fbfccd03ee0cfb26b182c03e28133a/python/mozbuild/mozbuild/configure/constants.py#11

I vote for something like Ted's suggestion of `CONFIG['MINGW']` ('MINGW_BUILD', whatever) rather than overloading CONFIG['CC_TYPE'] with the system that we're running on.

As noted by Ted, CC_TYPE already distinguishes between clang-cl and clang. I did that explicitly to support an hypothetical mingw-clang. As for CONFIG['MINGW'], I'd say it depends how many things would need something like that. Checking whether the target is windows and the compiler is either ('msvc', 'clang-cl') or ('gcc', 'clang') could be enough.

(In reply to Mike Hommey [:glandium] from comment #4) > As for CONFIG['MINGW'], I'd say it depends how many things would need > something like that. Checking whether the target is windows and the compiler > is either ('msvc', 'clang-cl') or ('gcc', 'clang') could be enough. Patches that I submitted do just that. Supporting 32-bit clang would require a few more of such changes, but it looks like it rather a short list of required changes.

Comment on attachment 8988183 [details] Bug 1443471 - Take clang mingw into account in moz.build files https://reviewboard.mozilla.org/r/253438/#review260022 ::: media/libpng/moz.build:68 (Diff revision 1) > FINAL_LIBRARY = 'gkmedias' > > # We allow warnings for third-party code that can be updated from upstream. > AllowCompilerWarnings() > > -if CONFIG['CC_TYPE'] in ('clang', 'gcc'): > +if CONFIG['CC_TYPE'] in ('gcc'): Won't this affect clang-on-mac and clang-on-linux too?

It's needed because clang doesn't support 'inline' keyword in c89 mode and mingw-w64 headers don't expect that. Could we just always set it to gnu89 instead? It's enough for mingw clang, let's see about other platforms: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c5ac4177edb5e64d3e08bd80adcb5dc93a9725b I will resubmit previous patch without that change and post new one for libpng once it's ready.

Comment on attachment 8988234 [details] Bug 1443471 - Use correct rust target for mingw clang. https://reviewboard.mozilla.org/r/253464/#review260284 ::: build/moz.configure/rust.configure:152 (Diff revision 1) > `host_or_target` is either `host` or `target` (the @depends functions > from init.configure). > """ > assert host_or_target in (host, target) > > - @depends(rustc, host_or_target, building_with_gcc, rust_supported_targets, > + @depends(rustc, host_or_target, c_compiler, rust_supported_targets, python/mozbuild/mozbuild/test/configure/test_toolchain_configure.py will need some tweaking.

Comment on attachment 8988183 [details] Bug 1443471 - Take clang mingw into account in moz.build files https://reviewboard.mozilla.org/r/253438/#review260286

Comment on attachment 8988235 [details] Bug 1443471 - Support mingw clang in skia moz.build https://reviewboard.mozilla.org/r/253466/#review260288

Comment on attachment 8988234 [details] Bug 1443471 - Use correct rust target for mingw clang. https://reviewboard.mozilla.org/r/253464/#review262698

Pushed by jacek@codeweavers.com: https://hg.mozilla.org/integration/autoland/rev/6c57a7ace821 Use correct rust target for mingw clang. r=glandium https://hg.mozilla.org/integration/autoland/rev/43432e43c201 Take clang mingw into account in moz.build files r=glandium https://hg.mozilla.org/integration/autoland/rev/a66d67ad3591 Support mingw clang in skia moz.build r=glandium

Comment on attachment 8988183 [details] Bug 1443471 - Take clang mingw into account in moz.build files [Approval Request Comment] This is one of several MinGW Build patches I'd like to land in esr60 for Tor. It will prevent them from carrying their own patches for the lifetime of esr60 and will enable us to keep the MinGW build functioning and know if/when/how it was broken by new commits into esr60. This commit affects only MinGW builds, so it is low risk.

https://hg.mozilla.org/mozilla-central/rev/6c57a7ace821 https://hg.mozilla.org/mozilla-central/rev/43432e43c201 https://hg.mozilla.org/mozilla-central/rev/a66d67ad3591

Comment on attachment 8988183 [details] Bug 1443471 - Take clang mingw into account in moz.build files Makes downstream maintenance easier for Tor. Approved for ESR 60.2.

https://hg.mozilla.org/releases/mozilla-esr60/rev/0aeece86e02d https://hg.mozilla.org/releases/mozilla-esr60/rev/a43554005a5a https://hg.mozilla.org/releases/mozilla-esr60/rev/16ed2ac69014