Update moz.build files to account for mingw-clang

RESOLVED FIXED in Firefox -esr60

Status

enhancement
P2
normal
RESOLVED FIXED
a year ago
5 months ago

People

(Reporter: tjr, Assigned: jacek)

Tracking

(Blocks 2 bugs)

3 Branch
mozilla63
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr60 fixed, firefox63 fixed)

Details

(Whiteboard: [stylo][tor])

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

a year ago
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
(Reporter)

Updated

a year ago
Priority: P1 → P2
(Assignee)

Updated

11 months ago
Blocks: mingw-clang
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 8

11 months ago
(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.
(Reporter)

Comment 9

11 months ago
mozreview-review
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?
(Assignee)

Comment 10

11 months ago
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 hidden (mozreview-request)
(Assignee)

Updated

11 months ago
Attachment #8988182 - Attachment is obsolete: true
Attachment #8988182 - Flags: review?(mh+mozilla)
(Assignee)

Updated

11 months ago
Attachment #8988184 - Attachment is obsolete: true
Attachment #8988184 - Flags: review?(mh+mozilla)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 14

11 months ago
mozreview-review
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 15

11 months ago
mozreview-review
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 16

11 months ago
mozreview-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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 20

10 months ago
mozreview-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+

Comment 21

10 months ago
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
(Reporter)

Comment 22

10 months ago
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?

Comment 23

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6c57a7ace821
https://hg.mozilla.org/mozilla-central/rev/43432e43c201
https://hg.mozilla.org/mozilla-central/rev/a66d67ad3591
Status: NEW → RESOLVED
Last Resolved: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
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.