Closed Bug 1508316 Opened 6 years ago Closed 6 years ago

esr60 build of mingw-clang requires fiddling with math include

Categories

(Firefox Build System :: General: Unsupported Platforms, enhancement, P5)

enhancement

Tracking

(firefox-esr60 fixed, firefox65 unaffected, firefox66 unaffected)

RESOLVED FIXED
Tracking Status
firefox-esr60 --- fixed
firefox65 --- unaffected
firefox66 --- unaffected

People

(Reporter: tjr, Assigned: tjr)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

As of this bug, this try run represents the patches needed for a successful esr60 build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d2aff9d1bb5f5d73699ee839c04f529e1ae05536 The two I don't understand why are needed are: - _finite is not defined by MinGW - We need to explicitly specify -D_USE_MATH_DEFINES in our CFLAGS We don't need these in -central. I swear I encountered this error before; Without these patches, the errors look like this: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a0bd84fbc092389f9bdc0562acfa59f2e5271f00 and aside from _finite seem mostly to be in rust compilation.
Hey Jacek, Do you have any idea what might cause this? (With the NSS bugs out of the way I can finally work on landing in esr60 again...)
Flags: needinfo?(jacek)
It's no longer possible to see details of those tasks, but it looks like strict mode that I described in bug 1390583 comment 104. Those are declared in mingw-w64 math.h, not not when strict mode is used. We really should fix it in mingw-w64... but a workaround is to not use strict mode (if my theory is right).
Flags: needinfo?(jacek)
Attached patch math.diff (deleted) — Splinter Review
The attached (not tested yet) mingw-w64 patch should help.
That patch did fix it, thanks. Do you think that should land in MinGW, be a local MinGW patch for Mozilla, or if I should dig into how to get rust compilation to pass gnu++ instead of c++?
Flags: needinfo?(jacek)
(In reply to Tom Ritter [:tjr] from comment #4) > That patch did fix it, thanks. Do you think that should land in MinGW, Yes, I plan to submit that to mingw-w64 > be a local MinGW patch for Mozilla or if I should dig into how to get rust compilation to pass gnu++ instead of c++? For m-c it was changed by: https://hg.mozilla.org/mozilla-central/rev/978b64d7fe7b#l1.7
Flags: needinfo?(jacek)
> > be a local MinGW patch for Mozilla or if I should dig into how to get rust compilation to pass gnu++ instead of c++? > > For m-c it was changed by: > https://hg.mozilla.org/mozilla-central/rev/978b64d7fe7b#l1.7 Oh! We don't have that in esr60! But the build still works?? Even without the [build."os=windows"."env=gnu"] part? I don't understand why that doesn't need to be backported. I don't see a difference from my vantage point as to whether we backport that one or use the MinGW patch; but I am confused why we don't need that one...
I sent the patch to mingw-w64. Looking at why it woks, I can see that the patch changed three things: - Using gnu++14 standard mode With the fix to mingw-w64, it will no longer be needed. Still, I think that it's a good idea to use the same mode there as is used for the rest codebase. But changing standard mode is probably an unnecessary risk for esr60, so I'd skip that part and use mingw-w64 patch. - Adding -DOS_WIN=1 and -DWIN32=1 for all Windows targets (not only MSVC) I don't remember details. It generally looks suspicious that it needs explicit defines. Those are pretty important macros, but there is a number of similar other macros that don't need to be defined explicitly. Maybe it's something not needed anymore? Why does MSVC veriant need it? Maybe it doesn't need it? - Specify target via --target It was needed at some point, but we ended up specifying it in BINDGEN_CFLAGS anyway. Maybe it's no loner needed in ServoBindings.toml.
Hey Nathan - Tom Prince and I would like to include this patch into the build for now until it's upstreamed to keep the esr60 mingw-clang train rolling.
Attachment #9034026 - Flags: review?(nfroyd)
Comment on attachment 9034026 [details] [diff] [review] Temporarily patch mingw in esr60 hile it's being upstreamed Review of attachment 9034026 [details] [diff] [review]: ----------------------------------------------------------------- This seems OK. Is this patch actively being upstreamed? Can we put a description + link in the patch file itself or a comment in the script?
Attachment #9034026 - Flags: review?(nfroyd) → review+
Flags: needinfo?(mozilla)
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: needinfo?(mozilla)
Resolution: --- → FIXED
Blocks: 1522595
Priority: -- → P5
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: