Closed Bug 1443471 Opened 6 years ago Closed 6 years ago

Update moz.build files to account for mingw-clang

Categories

(Firefox Build System :: General, enhancement, P2)

Product:
Firefox Build System
Component:
General
Version:
3 Branch
Type:
enhancement

Tracking

(firefox-esr60 fixed, firefox63 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox-esr60 --- fixed
firefox63 --- fixed

People

(Reporter: tjr, Assigned: jacek)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [stylo][tor])

Attachments

(3 files, 2 obsolete files)

Bug 1443471 - Take clang mingw into account in moz.build files
59 bytes, text/x-review-board-request
glandium
: review+
RyanVM
: approval-mozilla-esr60+
Details
Bug 1443471 - Use correct rust target for mingw clang.
59 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1443471 - Support mingw clang in skia moz.build
59 bytes, text/x-review-board-request
glandium
: review+
Details 
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?
Flags: needinfo?(mh+mozilla)
Flags: needinfo?(dmajor)
(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.
Flags: needinfo?(dmajor)
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.
Flags: needinfo?(mh+mozilla)
No longer blocks: stylo-everywhere
Priority: P1 → P2
Blocks: mingw-clang
(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.
Attachment #8988182 - Attachment is obsolete: true
Attachment #8988182 - Flags: review?(mh+mozilla)
Attachment #8988184 - Attachment is obsolete: true
Attachment #8988184 - Flags: review?(mh+mozilla)
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.
Attachment #8988234 - Flags: review?(mh+mozilla)
Comment on attachment 8988183 [details]
Bug 1443471 - Take clang mingw into account in moz.build files

https://reviewboard.mozilla.org/r/253438/#review260286
Attachment #8988183 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8988235 [details]
Bug 1443471 - Support mingw clang in skia moz.build

https://reviewboard.mozilla.org/r/253466/#review260288
Attachment #8988235 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8988234 [details]
Bug 1443471 - Use correct rust target for mingw clang.

https://reviewboard.mozilla.org/r/253464/#review262698
Attachment #8988234 - Flags: review?(mh+mozilla) → review+
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.
Attachment #8988183 - Flags: approval-mozilla-esr60?
Assignee: nobody → jacek
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.
Attachment #8988183 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Version: Version 3 → 3 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: