Bug 81482 - Cross-platform processor core counter: fix build on FreeBSD.
Summary: Cross-platform processor core counter: fix build on FreeBSD.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Raphael Kubo da Costa (:rakuco)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-18 20:12 PDT by Raphael Kubo da Costa (:rakuco)
Modified: 2012-03-29 01:56 PDT (History)
3 users (show)

See Also:


Attachments
Patch (1.52 KB, patch)
2012-03-18 20:13 PDT, Raphael Kubo da Costa (:rakuco)
no flags Details | Formatted Diff | Diff
Same patch with some additional comments in the code (1.71 KB, patch)
2012-03-19 09:13 PDT, Raphael Kubo da Costa (:rakuco)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Raphael Kubo da Costa (:rakuco) 2012-03-18 20:12:24 PDT
Cross-platform processor core counter: fix build on FreeBSD.
Comment 1 Raphael Kubo da Costa (:rakuco) 2012-03-18 20:13:22 PDT
Created attachment 132530 [details]
Patch
Comment 2 WebKit Review Bot 2012-03-18 20:15:14 PDT
Attachment 132530 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/JavaScriptCore/wtf/NumberOfCores.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Raphael Kubo da Costa (:rakuco) 2012-03-18 20:19:04 PDT
(In reply to comment #2)
> Attachment 132530 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
> Source/JavaScriptCore/wtf/NumberOfCores.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
> Total errors found: 1 in 2 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

This is a false positive, sys/types.h must come before sys/sysctl.h.
Comment 4 Zoltan Herczeg 2012-03-19 00:16:56 PDT
There is no other OS where this include order caused any trouble. Where is this documentation (a link please)? If this is only FreeBSD specific then add a comment please.
Comment 5 Raphael Kubo da Costa (:rakuco) 2012-03-19 07:46:26 PDT
(In reply to comment #4)
> There is no other OS where this include order caused any trouble.

It may have broken OpenBSD and NetBSD too, since the requirements are pretty much the same. I don't have access to an OS X machine to check what they do differently for this include order reversal to work (this part of their libraries comes from FreeBSD).

> Where is this documentation (a link please)? If this is only FreeBSD specific then add a comment please.

The man pages for NetBSD, OpenBSD, FreeBSD and Darwin all show that sys/sysctl.h must be included after either sys/param.h or sys/types.h:

 - http://netbsd.gw.com/cgi-bin/man-cgi?sysctl+3+NetBSD-current
 - http://www.openbsd.org/cgi-bin/man.cgi?query=sysctl&sektion=3&arch=i386&apropos=0&manpath=OpenBSD+Current
 - http://www.freebsd.org/cgi/man.cgi?query=sysctl&sektion=3&apropos=0&manpath=FreeBSD+9.0-RELEASE
 - https://developer.apple.com/library/mac/#documentation/Darwin/Reference/Manpages/man3/sysctl.3.html#//apple_ref/doc/man/3/sysctl

u_int is a typedef defined in sys/types.h on FreeBSD (and probably other BSDs too). And including sys/types.h or sys/param.h before is part of the OpenBSD and FreeBSD style guides:

 - http://www.freebsd.org/cgi/man.cgi?query=style&apropos=0&sektion=0&manpath=FreeBSD+9.0-RELEASE&arch=default&format=html
 - http://www.openbsd.org/cgi-bin/man.cgi?query=style&apropos=0&sektion=0&manpath=OpenBSD+Current&arch=i386&format=html
Comment 6 Zoltan Herczeg 2012-03-19 08:00:39 PDT
Poor design :( Headers should include what they need. Do you think this will be fixed in the future?

Ok, if noone objects I can give you an r+ but please upload a new patch with a comment in the header file which clearly states why we break the WK style guides.
Comment 7 Raphael Kubo da Costa (:rakuco) 2012-03-19 09:13:48 PDT
Created attachment 132594 [details]
Same patch with some additional comments in the code
Comment 8 Raphael Kubo da Costa (:rakuco) 2012-03-19 09:14:41 PDT
(In reply to comment #6)
> Poor design :( Headers should include what they need. Do you think this will be fixed in the future?

I don't think so, I guess it's part of the design. See select(2) or stat(2) on Linux as well -- they too need additional headers in a certain order.

> Ok, if noone objects I can give you an r+ but please upload a new patch with a comment in the header file which clearly states why we break the WK style guides.

Done.
Comment 9 WebKit Review Bot 2012-03-19 09:17:49 PDT
Attachment 132594 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/JavaScriptCore/wtf/NumberOfCores.cpp:33:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Zoltan Herczeg 2012-03-19 09:40:10 PDT
Comment on attachment 132594 [details]
Same patch with some additional comments in the code

r=me
Comment 11 Raphael Kubo da Costa (:rakuco) 2012-03-19 09:48:49 PDT
Comment on attachment 132594 [details]
Same patch with some additional comments in the code

Clearing flags on attachment: 132594

Committed r111195: <http://trac.webkit.org/changeset/111195>
Comment 12 Raphael Kubo da Costa (:rakuco) 2012-03-19 09:48:58 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Landry Breuil 2012-03-29 01:28:59 PDT
Note that on OpenBSD it also needs sys/param.h before sys/sysctl.h and sys/types.h, otherwise some #defines are missing (as stated in comment #5). Should i reopen this bug or file a new one ?
Comment 14 Zoltan Herczeg 2012-03-29 01:48:33 PDT
File a new one.
Comment 15 Landry Breuil 2012-03-29 01:56:22 PDT
#82585.