Bug 94231

Summary: Problems compiling libdrm since glibc 2.23
Product: DRI Reporter: Mike Lothian <mike>
Component: libdrmAssignee: Default DRI bug account <dri-devel>
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: medium CC: mike, nheghathivhistha, vapier
Version: DRI git   
Hardware: Other   
OS: All   
URL: https://bugs.gentoo.org/show_bug.cgi?id=575232
See Also: https://bugs.freedesktop.org/show_bug.cgi?id=94232
Whiteboard:
i915 platform: i915 features:
Attachments:
Description Flags
Patch to include sys/sysmacros.h in libdrm
none
Glibc patches
none
using AC_CHECK_HEADERS()
none
using AC_CHECK_HEADERS none

Description Mike Lothian 2016-02-21 12:31:03 UTC
Created attachment 121865 [details]
Patch to include sys/sysmacros.h in libdrm

Get errors about mkdev, major and minor not being defined when compiling libdrm after upgrading glibc to 2.23

Including sys/sysmacros.h fixes it for me
Comment 1 Emil Velikov 2016-02-22 08:53:50 UTC
Hi Mike,

man makedev/major/minor does not mention anything about this header, although it does mention (the missing) #define _BSD_SOURCE. Does that get thinks working again?

Can you please send a patch (git send-email ideally) to the list. Also please quote the build error and any related information on the topic that you can find.

If you can do the same for mesa that'll be greatly appreciated.

Thank you
Comment 2 Mike Lothian 2016-02-22 10:26:53 UTC
The issue is sys/sysmacros.h has been dropped from sys/types.h in glibc 2.23

Do you know if this can be safely added in for everything or will it need some ifdeffery?

If it's straight forward I'll get the patches out later today (there's one for xorg-server too)
Comment 3 Emil Velikov 2016-02-22 11:03:30 UTC
Are you sure about the removal of "include sys/sysmacros.h" ? Afaics include/sys/types.h includes the posix/sys/types.h with the latter still having the include.

Bth I'm not sure if things can explode with the unconditional include, imho it's better to keep things as documented.

I.e. if the _BSD_SOURCE define does not help, then either glibc needs to be fixed or the manuals should be updated (the glibc fold might have some suggestions) ?
Comment 4 Daniel Stone 2016-02-22 11:39:49 UTC
(In reply to Emil Velikov from comment #3)
> I.e. if the _BSD_SOURCE define does not help, then either glibc needs to be
> fixed or the manuals should be updated (the glibc fold might have some
> suggestions) ?

_BSD_SOURCE gets defined if you call AC_USE_SYSTEM_EXTENSIONS from configure.ac, which gives you the maximal set of defines. It's not safe to just define the _*_SOURCE macros, since whilst glibc etc treat them as additive, BSD libcs treat them as restrictive (e.g. defining _BSD_SOURCE gives you a pure BSD environment without some of the features enabled by, e.g., _XOPEN_SOURCE).
Comment 5 Emil Velikov 2016-02-22 12:28:34 UTC
(In reply to Daniel Stone from comment #4)
> _BSD_SOURCE gets defined if you call AC_USE_SYSTEM_EXTENSIONS

Are you sure about this ? Looking at my autoconf 2.69 (/usr/share/autoconf/autoconf/specific.m4) and it sets the following

__EXTENSIONS__
_ALL_SOURCE
_GNU_SOURCE
_POSIX_PTHREAD_SEMANTICS
_TANDEM_SOURCE

Had no idea about the different interpretation of the defines between GNU vs BSD libc(s). Thanks for that.

We can at check if the explicit define fixes things on Linux and ping the BSD people for more robust solution ?
Comment 6 Mike Lothian 2016-02-22 12:34:57 UTC
Here's the Gentoo bug for this issue - there's a few packages affected

https://bugs.gentoo.org/show_bug.cgi?id=575232
Comment 7 Emil Velikov 2016-02-22 14:03:22 UTC
Hmm... Gentoo devs decided to patch glibc in order to remove the include.

At the same time they lean that applications (25+ based on the incomplete list) should be fixed to use sys/sysmacros.h. There's no justification and the manual does not support this :-(

To make it even more strange the patch isn't alongside the glibc-2.23.ebuild in the repo. Am I looking at the wrong repo ? Can anyone share a link with discussion behind this decision ?
Comment 8 Mike Lothian 2016-02-22 14:09:41 UTC
The patches are bundled up into a tar file: glibc-2.23-patches-1.tar.bz2 which is on the gentoo mirrors
Comment 9 Mike Lothian 2016-02-22 14:10:24 UTC
Created attachment 121891 [details]
Glibc patches

Here's the patch tarball for your convenience
Comment 10 Mike Lothian 2016-02-22 14:16:51 UTC
And the relevant details:

Subject: [PATCH] sys/types.h: drop sys/sysmacros.h include

We want to break apart this include path due to namespace pollution.
https://sourceware.org/ml/libc-alpha/2015-11/msg00253.html
Comment 11 Emil Velikov 2016-02-22 14:47:02 UTC
Thanks for the links Mike.

Not sure about others, but I'm not a happy bunny.

So there's a issue/bug in stdlib.h (or was it g++), that leads to the inclusion of sys/types.h. To 'fix' this lets break the documented behaviour (for those of us that read the manpages). Hmm ... wait what ?

Sorry but this sounds backwards. If we cannot squash the issue, it should be documented properly with list of workarounds (as pointed in the redhat bugzilla).
Yes it sucks, big time. Yet we shouldn't break one thing, in order to workaround another issue, should we ?

If this workaround is needed/applicable with other libc providers like musl (on Linux at least), then there should be a documentation update alongside the deprecation. Followed by a lengthy period _before_ removing things ?
Comment 12 Mike Frysinger 2016-02-22 23:39:33 UTC
it's really not that difficult to test for & include the sys/sysmacros.h header directly when you're using the non-standard functions they provide.  this is the API under BSD systems, and where Linux C libs have already moved/are moving towards.  *this* is the deprecation period.

upstream glibc will take a more measured approach including updating their docs, but realistically, no one is going to read those until they see breakage.  the Gentoo changes are only in an opt-in dev-only build, so it too isn't hitting people randomly.

as the upstream thread shows, if you used the (long time documented) AC_HEADER_MAJOR macro in autoconf, then this wouldn't have impacted drm.
Comment 13 Emil Velikov 2016-02-23 14:03:55 UTC
Your suggestion is not unreasonable my any means. Yes, it's not hard to test (and include) the header, but it's unnecessary according to the documentation.

I'll repeat my request - please update documentation, provide warnings and deprecation _period_. Do not just remove the include and then point fingers at projects because "they're not doing X" when there is no official information that say they should :-)

TL DR; as soon as the manual is updated I'll be more than happy to get this in.
Comment 14 Mike Lothian 2016-02-23 14:24:23 UTC
I'll get the patches created for libdrm, mesa & xorg-server, Mike F please can you provide the location of where the updated docs are and I'll include it in the commit message

Emil is there anything else you'd like? I'm going to mention the I provided above too
Comment 15 Emil Velikov 2016-02-23 14:40:36 UTC
Mike L adding a reference to the updated docs or a mention "since man-pages version X" plus a bugzilla link (if applicable) would be amazing. Thanks !
Comment 16 Mike Frysinger 2016-02-23 17:26:37 UTC
(In reply to Emil Velikov from comment #13)

that is happening in upstream glibc.  i'm not going to bother in Gentoo.

no one is "pointing fingers".  we detected an error in libdrm in Gentoo, hence you guys got a patch.  i don't know why you're upset about this.
Comment 17 Emil Velikov 2016-02-23 20:24:51 UTC
Well I never meant that Gentoo people should update anything bth. As the deprecation is a joint effort (hopefully musl and others are involved) someone 
from the team involved should ping/update the man-pages.

As is, it's a case of the cart going before the horse - fix the code first, then document the new requirement :-\

The "pointing fingers" quote is metaphorical, no offence meant towards anyone.
Comment 18 Matt Turner 2016-04-21 05:10:12 UTC
I guess you can think of this as the deprecation period.


Documentation has been updated:

https://github.com/mkerrisk/man-pages/commit/3350a86413d770198e11fe8df9a3cd5710240dc3
Comment 19 Emil Velikov 2016-04-21 12:26:31 UTC
That was fast, only a few hours ago the commit landed.

No objections about having this in, although let's use AC_CHECK_HEADERS.

I'm wondering if we shouldn't give it a day or two before landing though. Obviously waiting for a man-pages release would be a serious overkill (afaict it will be within ~4 months).
Comment 20 Matt Turner 2016-04-28 19:43:48 UTC
(In reply to Emil Velikov from comment #19)
> That was fast, only a few hours ago the commit landed.
> 
> No objections about having this in, although let's use AC_CHECK_HEADERS.
> 
> I'm wondering if we shouldn't give it a day or two before landing though.
> Obviously waiting for a man-pages release would be a serious overkill
> (afaict it will be within ~4 months).

Do you want to commit it... or do you want me to send it to the list?
Comment 21 Emil Velikov 2016-04-30 19:59:54 UTC
Should have mentioned it earlier - I thought the more portable thing was to use AC_HEADER_MAJOR. Can we do that one as opposed to unconditionally including the header ?
Comment 22 Mike Frysinger 2016-06-21 18:12:02 UTC
Created attachment 124642 [details] [review]
using AC_CHECK_HEADERS()
Comment 23 Mike Frysinger 2016-06-21 18:13:27 UTC
Created attachment 124643 [details] [review]
using AC_CHECK_HEADERS
Comment 24 Emil Velikov 2016-07-06 16:51:54 UTC
Thanks for updating the man pages as well as the patch(es) Mike. Hope you and/or others will get to resolving libstdc++ + _GNU_SOURCE issue that inspired this.

I've pushed the AC_HEADER_MAJOR patch to master.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.