After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 777030 - build error where minor() and major() cant be resolved in gio/gdbusmessage.c
build error where minor() and major() cant be resolved in gio/gdbusmessage.c
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: build
unspecified
Other other
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2017-01-09 08:43 UTC by declan
Modified: 2017-05-02 08:41 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch with proposed fix (3.11 KB, patch)
2017-01-09 11:02 UTC, declan
needs-work Details | Review
new pacth with comments removed and #define renamed as requested (2.22 KB, patch)
2017-04-28 13:49 UTC, declan
none Details | Review
gdbusmessage: Don’t use major()/minor() if they’re unavailable (2.22 KB, patch)
2017-04-28 15:17 UTC, declan
none Details | Review

Description declan 2017-01-09 08:43:10 UTC
The functions functions minor() and major() are called in gio/gdbusmessage.c but might have never been declared/defined.

The functions are usually declared and defined inline by the inclusion of
sys/mkdev.h or sys/sysmacros.h


However this inclusion is conditional and can fail. It does for example on android when using crystax ndk. 
In fact it probably only succeeds if types.h (included further above) happens to include sysmacros.h as happens unconditionally on many systems. (eg android using google's ndk). So the conditional inclusion doesnt kick in anyways and is just confusing.

This can lead to build errors. Worse it can lead to just warnings (implied declaration)  that go unnoticed, and run time errors that are very hard to find the cause of.

The call to minor()/major() should not be made if the inline definitions have not been included.

The pull request here provides a possible fix:

https://github.com/GNOME/glib/pull/14
Comment 1 Emmanuele Bassi (:ebassi) 2017-01-09 10:14:20 UTC
Thanks for your contribution.

Could you please attach the patch to fix this issue directly to Bugzilla? We cannot merge pull requests on the GitHub mirror, as it's read-only.

Thanks again!
Comment 2 declan 2017-01-09 11:02:20 UTC
Created attachment 343144 [details] [review]
patch with proposed fix

Here is a  patch from the commit, as requested
Comment 3 declan 2017-04-28 08:15:16 UTC
Any update on this?
Will you apply the patch?
Thanks
Comment 4 Philip Withnall 2017-04-28 11:10:46 UTC
Review of attachment 343144 [details] [review]:

Thanks for the patch. A few comments.

Please also shorten the commit message subject — git commit message subjects are conventionally 80 characters long at most. How about “gdbusmessage: Don’t use major()/minor() if they’re unavailable”.

::: gio/gdbusmessage.c
@@ +36,3 @@
+//-----------------------------------------
+// guard against caling minor() and minor() below if not defined (as is the case with on android with crystax ndk..verified with version 10.3.2)
+// note: on some systems (eg android with google ndk) the sys/types.h included above also #includes sys/sysmacros.h

Drop all the `//` comment lines; they’re unnecessary (and our coding style uses /* C-style comments */ not // C++-style comments).

@@ +37,3 @@
+// guard against caling minor() and minor() below if not defined (as is the case with on android with crystax ndk..verified with version 10.3.2)
+// note: on some systems (eg android with google ndk) the sys/types.h included above also #includes sys/sysmacros.h
+#define NO_MINOR_MAJOR_MAKEDEV_DEFS_FOUND

I’d call this `MAJOR_MINOR_NOT_FOUND` for brevity and define its value to 1 (just in case someone does `#if MAJOR_MINOR_NOT_FOUND` in future).
Comment 5 declan 2017-04-28 13:49:25 UTC
Created attachment 350645 [details] [review]
new pacth with comments removed and #define renamed as requested

This fixes the bug
https://bugzilla.gnome.org/show_bug.cgi?id=777030
and does so as requested by the reviewer Philip Withnall in
https://bugzilla.gnome.org/show_bug.cgi?id=777030#c4
Comment 6 declan 2017-04-28 13:50:07 UTC
Done as requested
Comment 7 Philip Withnall 2017-04-28 14:21:05 UTC
Review of attachment 350645 [details] [review]:

This is close enough; I’ll fix it up and push it to git master. Thanks.

::: gio/gdbusmessage.c
@@ +34,3 @@
 #include <sys/sysmacros.h>
+#else
+#define MAJOR_MINOR_NOT_FOUND=1

This is not valid C. There should be a space instead of an equals.
Comment 8 declan 2017-04-28 15:17:05 UTC
Created attachment 350651 [details] [review]
gdbusmessage: Don’t use major()/minor() if they’re unavailable

fixed typo in prev patch
Comment 9 declan 2017-04-28 15:17:44 UTC
sorry about the typo
Comment 10 Philip Withnall 2017-04-28 15:47:40 UTC
(In reply to declan from comment #9)
> sorry about the typo

Don’t worry; I already pushed a fixed version of the patch to git master as commit aefffa3.
Comment 11 declan 2017-05-02 08:41:26 UTC
Thank you.