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)
Firefox Build System
General: Unsupported Platforms
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)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•6 years ago
|
||
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)
Comment 2•6 years ago
|
||
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)
Comment 3•6 years ago
|
||
The attached (not tested yet) mingw-w64 patch should help.
Assignee | ||
Comment 4•6 years ago
|
||
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)
Comment 5•6 years ago
|
||
(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)
Assignee | ||
Comment 6•6 years ago
|
||
> > 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...
Comment 7•6 years ago
|
||
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.
Assignee | ||
Comment 8•6 years ago
|
||
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 9•6 years ago
|
||
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+
Assignee | ||
Comment 10•6 years ago
|
||
It's being discussed here: https://sourceforge.net/p/mingw-w64/mailman/message/36505053/
Updated•6 years ago
|
Flags: needinfo?(mozilla)
Comment 11•6 years ago
|
||
bugherder uplift |
status-firefox-esr60:
--- → fixed
Updated•6 years ago
|
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox65:
--- → unaffected
status-firefox66:
--- → unaffected
Flags: needinfo?(mozilla)
Resolution: --- → FIXED
Updated•6 years ago
|
Priority: -- → P5
You need to log in
before you can comment on or make changes to this bug.
Description
•