Closed
Bug 1329798
Opened 6 years ago
Closed 5 years ago
Use of major & minor macros without including sys/sysmacros.h
Categories
(Core :: XPCOM, defect)
Tracking
()
RESOLVED
FIXED
mozilla54
People
(Reporter: anthonyryan1, Assigned: mgorny)
References
Details
Attachments
(1 file, 1 obsolete file)
977 bytes,
patch
|
glandium
:
review+
jcristau
:
approval-mozilla-aurora+
jcristau
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
User Agent: Build ID: 20161228001411 Steps to reproduce: An upcoming version of glibc intends to stop including sys/sysmacros.h automatically with sys/type.h and require that it be included explicitly for the use of the major, minor & makedev macros. Compiling firefox against a version of glibc with this implicit inclusion removed causes a compiler error. Actual results: Compiler error: /var/tmp/portage/www-client/firefox-50.1.0-r1/work/firefox-50.1.0/xpcom/io/nsLocalFileUnix.cpp:1380:46: error: 'major' was not declared in this scope if (!GetDeviceName(major(mCachedStat.st_dev), ^ /var/tmp/portage/www-client/firefox-50.1.0-r1/work/firefox-50.1.0/xpcom/io/nsLocalFileUnix.cpp:1381:46: error: 'minor' was not declared in this scope minor(mCachedStat.st_dev), ^ Expected results: The file should #include <sys/sysmacros.h> This will also improve compatibility with alternative libc implementations.
Assignee | ||
Comment 1•6 years ago
|
||
Here's a patch adding the #include conditionally to a configure check, and adding the file to system-includes. I have confirmed that it fixes the build on Linux with patched glibc.
Updated•6 years ago
|
Attachment #8832788 -
Flags: review?(mh+mozilla)
Comment 2•5 years ago
|
||
Comment on attachment 8832788 [details] [diff] [review] 0001-Include-sys-sysmacros.h-for-major-minor-when-availab.patch Review of attachment 8832788 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/io/nsLocalFileUnix.cpp @@ +30,5 @@ > #define BLOCK_SIZE 1024 /* kernel block size */ > #endif > #endif > +#if defined(HAVE_SYS_SYSMACROS_H) > +#include <sys/sysmacros.h> The macros major and minor are only used in portions of the code that require linux/quota.h. Which pretty much assumes a Linux target system. Looking at the most popular non-glibc Linux libcs (bionic, musl, uclibc), they all provide sys/sysmacros.h. So let's just not check it exists. We can pretty much assumes it does. Just place the include in the same #if as the sys/quota.h include.
Attachment #8832788 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 3•5 years ago
|
||
Attachment #8832788 -
Attachment is obsolete: true
Updated•5 years ago
|
Attachment #8837466 -
Flags: review?(mh+mozilla)
Updated•5 years ago
|
Attachment #8837466 -
Flags: review?(mh+mozilla) → review+
Updated•5 years ago
|
Keywords: checkin-needed
Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/b9a1da5e4f89 Include sys/sysmacros.h for major(), minor() on Linux. r=glandium
Keywords: checkin-needed
Comment 5•5 years ago
|
||
[Tracking Requested - why for this release]: As many distros are already moving to glibc-2.24 support would be nice to get this landed to be a part of esr and aurora, there are no downsides to uplifting to beta and aurora at this point.
status-firefox52:
--- → affected
status-firefox53:
--- → affected
tracking-firefox52:
--- → ?
tracking-firefox53:
--- → ?
Comment 6•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b9a1da5e4f89
Status: UNCONFIRMED → RESOLVED
Closed: 5 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment 7•5 years ago
|
||
Michal, Mike, want to request uplift to aurora/beta for this one?
Assignee: nobody → mgorny
Flags: needinfo?(mh+mozilla)
Flags: needinfo?(mgorny)
Assignee | ||
Comment 8•5 years ago
|
||
(In reply to Julien Cristau [:jcristau] from comment #7) > Michal, Mike, want to request uplift to aurora/beta for this one? Yes, please uplift it. It would certainly be helpful to have this fix around before we unmask new glibc on our users.
Updated•5 years ago
|
Flags: needinfo?(mh+mozilla)
Comment 9•5 years ago
|
||
Comment on attachment 8837466 [details] [diff] [review] 0001-Include-sys-sysmacros.h-for-major-minor-on-Linux.patch Approval Request Comment [Feature/Bug causing the regression]: Header changes in newer glibc [User impact if declined]: Build failure against newer glibc (recent distros) [Is this code covered by automated tests?]: N/A [Has the fix been verified in Nightly?]: N/A [Needs manual test from QE? If yes, steps to reproduce]: N/A [List of other uplifts needed for the feature/fix]: N/A [Is the change risky?]: No [Why is the change risky/not risky?]: Adds a #include of a header that has existed since the dawn of times, but happened to be #included indirectly, which is what ceases to happen with newer glibc. [String changes made/needed]: N/A
Attachment #8837466 -
Flags: approval-mozilla-beta?
Attachment #8837466 -
Flags: approval-mozilla-aurora?
Updated•5 years ago
|
Comment 10•5 years ago
|
||
Comment on attachment 8837466 [details] [diff] [review] 0001-Include-sys-sysmacros.h-for-major-minor-on-Linux.patch build fix for new glibc, aurora53+, beta52+ Let's get this in ESR52 so linux distros don't need to patch it in.
Attachment #8837466 -
Flags: approval-mozilla-beta?
Attachment #8837466 -
Flags: approval-mozilla-beta+
Attachment #8837466 -
Flags: approval-mozilla-aurora?
Attachment #8837466 -
Flags: approval-mozilla-aurora+
Updated•5 years ago
|
Flags: needinfo?(mgorny)
Comment 11•5 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/d4e8a7b1832d
Comment 12•5 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/5326967d9271
Comment 13•5 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-esr52/rev/5326967d9271
status-firefox-esr52:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•