Bug 57535 – Fix compilation on Solaris 10 with Sun Studio 12, missing definition for time_t

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Ben Taylor]

Compiling webkit in Qt-4.7.2, we see the following errors due to missing <sys/time.h> includes

"platform/network/ResourceResponseBase.h", line 90: Error: time_t is not defined.
"page/Page.h", line 372: Error: No storage class or type for this declaration.

[Ben Taylor]

Created attachment 87700 [details]
Fix compilation on Solaris 10 with Sun Studio 12, missing definition for time_t

Bug report extracted from the original by Thiago Macieria in https://bugs.webkit.org/show_bug.cgi?id=24932
in patch 04 of 17.  This code compiles fine against webkit in qt-4.7.2, but patch is from webkit head.

[WebKit Review Bot]

Attachment 87700 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/page/Page.h:31:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/loader/icon/IconRecord.h:41:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/loader/icon/IconRecord.h:41:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/network/ResourceResponseBase.h:38:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/platform/network/ResourceResponseBase.h:38:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 5 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ben Taylor]

(In reply to comment #2)
> Attachment 87700 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
> 
> Source/WebCore/page/Page.h:31:  Alphabetical sorting problem.  [build/include_order] [4]
> Source/WebCore/loader/icon/IconRecord.h:41:  One space before end of line comments  [whitespace/comments] [5]
> Source/WebCore/loader/icon/IconRecord.h:41:  Alphabetical sorting problem.  [build/include_order] [4]
> Source/WebCore/platform/network/ResourceResponseBase.h:38:  One space before end of line comments  [whitespace/comments] [5]
> Source/WebCore/platform/network/ResourceResponseBase.h:38:  Alphabetical sorting problem.  [build/include_order] [4]
> Total errors found: 5 in 4 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

They look like false positives to me. Reverting the patch and rerunning check-webkit-style on loader/icon/IconRecord.h emits the following messages.  However, if someone can tell me if the order needs to be changed on the includes, happy to make the change.  The space issue is clearly false positive since the C++ style comments in the lines cause that issue

Source/WebCore/loader/icon/IconRecord.h:34:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/loader/icon/IconRecord.h:36:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/loader/icon/IconRecord.h:78:  The parameter name "data" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/loader/icon/IconRecord.h:107:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebCore/loader/icon/IconRecord.h:114:  Should have a space between // and comment  [whitespace/comments] [4]

[Build Bot]

Attachment 87700 [details] did not build on win:
Build output: http://queues.webkit.org/results/8314266

[Ben Taylor]

(In reply to comment #4)
> Attachment 87700 [details] did not build on win:
> Build output: http://queues.webkit.org/results/8314266

Do I need to resubmit the patch, isolating the includes with an #ifdef/ifndef

like:
#ifdef OS(SOLARIS)
#include <sys/time.h>
#endif 

 or 
#ifndef OS(WINDOWS)
#include <sys/time.h>
#endif

[Ben Taylor]

Created attachment 87961 [details]
Updated patch to isolate includes only for Solaris, since it caused a failure on the windows build bots

Updated the patch to isolate the includes only for Solaris, since it caused a failure on windows build bots.

[Early Warning System Bot]

Attachment 87961 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8315685

[WebKit Review Bot]

Attachment 87961 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8307355

[Build Bot]

Attachment 87961 [details] did not build on win:
Build output: http://queues.webkit.org/results/8320384

[David Levin]

Comment on attachment 87961 [details]
Updated patch to isolate includes only for Solaris, since it caused a failure on the windows build bots

This broke the build on several platforms so r-.

[Ben Taylor]

(In reply to comment #10)
> (From update of attachment 87961 [details])
> This broke the build on several platforms so r-.

What's the recommended way to isolate the header for a specific OS?

I thought the second solution, based on previous patches that I've submitted would have been the right way to solve this problem.  I was very surprised to see the Linux/GTK systems fail the way they did.

[David Levin]

(In reply to comment #11)
> (In reply to comment #10)
> > (From update of attachment 87961 [details] [details])
> > This broke the build on several platforms so r-.
> 
> What's the recommended way to isolate the header for a specific OS?
> 
> I thought the second solution, based on previous patches that I've submitted would have been the right way to solve this problem.  I was very surprised to see the Linux/GTK systems fail the way they did.

It looks like the failures for the QT and cr-linux builds are because the files which include the header must not have the define for OS(). I believe this is in config.h, so I would make sure those files include it.  (The may involve modified a script which generates some files.)

For Windows, I don't quite get what went wrong there (maybe something similar?).

[Ben Taylor]

Created attachment 88917 [details]
This should fix the issue of this patch causing issues with the build bots for linux and windows

This patch includes wtf/Platform.h so that the macro OS(SOLARIS) can be used to isolate the include.

[WebKit Review Bot]

Attachment 88917 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/loader/icon/IconRecord.h:41:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/loader/icon/IconRecord.h:43:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/platform/network/ResourceResponseBase.h:38:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 3 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ben Taylor]

Created attachment 88918 [details]
This should fix the issue of this patch causing issues with the build bots for linux and windows, and clear some of the style bot issues

cosmetic fix, despite having logged bugs against check-webkit-style

[WebKit Review Bot]

Attachment 88918 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/loader/icon/IconRecord.h:41:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/network/ResourceResponseBase.h:38:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ben Taylor]

(In reply to comment #16)
> Attachment 88918 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
> 
> Source/WebCore/loader/icon/IconRecord.h:41:  Alphabetical sorting problem.  [build/include_order] [4]
> Source/WebCore/platform/network/ResourceResponseBase.h:38:  Alphabetical sorting problem.  [build/include_order] [4]
> Total errors found: 2 in 4 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

They are falso positives, as previously indicated with previous versions of this patch series.

[David Levin]

(In reply to comment #15)
> cosmetic fix, despite having logged bugs against check-webkit-style

I see no bugs filed by you about check-webkit-style.

Also I've looked at all issues mentioned in the bug and they are all real problems in your patches.

1. There should only be one space before end of line comments, so this
# "header.h"        // This is for whatever.
doesn't follow the proper style, but this does
# "header.h" // This is for whatever.

2. Headers that aren't ifdef'ed are sorted.  Yours is not. I see
#include <wtf/Platform.h>
   occurring after things like
#include <wtf/Vector.h>

which isn't alphabetical.

3. When you run check-webkit-style on a file without your changes and get other issues, you can actually see that it is working correctly. It did not point out these problems on your patches because they were not in lines that you changed. However, in the patches that you submitted, it is pointing out correct issues on correct lines. :)

PS check-webkit-style does have some issues at times but we strive to have them be pretty rare and none are exhibited in these issues as far as I see.

[Ben Taylor]

(In reply to comment #18)
> (In reply to comment #15)
> > cosmetic fix, despite having logged bugs against check-webkit-style
> I see no bugs filed by you about check-webkit-style.

I know I filed one. Perhaps it was closed.

> Also I've looked at all issues mentioned in the bug and they are all real problems in your patches.
> 1. There should only be one space before end of line comments, so this
> # "header.h"        // This is for whatever.
> doesn't follow the proper style, but this does
> # "header.h" // This is for whatever.

Noted. Will make the fix. There are lots of place in webkit where
this was not followed, specifically made for "formatting" reasons.

> 2. Headers that aren't ifdef'ed are sorted.  Yours is not. I see
> #include <wtf/Platform.h>
>    occurring after things like
> #include <wtf/Vector.h>
> which isn't alphabetical.

Noted.  Will make the fix.  Obviously, sys/time.h will have to come after
wtf/Platform.h, since the macro protecting the include requires wtf/Platform.h

> 3. When you run check-webkit-style on a file without your changes and get other issues, you can actually see that it is working correctly. It did not point out these problems on your patches because they were not in lines that you changed. However, in the patches that you submitted, it is pointing out correct issues on correct lines. :)

Noted.

> PS check-webkit-style does have some issues at times but we strive to have them be pretty rare and none are exhibited in these issues as far as I see.

Will resubmit shortly.

[Ben Taylor]

Created attachment 88949 [details]
Patch to fix a compile issue on Solaris 10 with Sun Studio 12, which includes <sys/time.h>, updated per bug comments

Patch resubmitted with include files ordered and comments structured. check-webkit-style run against the patch produced:

Total errors found: 0 in 1 files

This should resolve the remaining issues

[David Levin]

(In reply to comment #20)
> Created an attachment (id=88949) [details]
> Patch to fix a compile issue on Solaris 10 with Sun Studio 12, which includes <sys/time.h>, updated per bug comments

OK the patch is well structured. Looking at the patch, I still believe what I said before is correct:

"It looks like the failures for the QT and cr-linux builds are because the files which include the header must not have the define for OS(). I believe this is in config.h, so I would make sure those files include it.  (The may involve modified a script which generates some files.)"

Specifically, I'm saying that the real problem is that there are cpp files which aren't doing #include "config.h" and fixing that is what should be done as opposed to including wtf/Platform.h

So I can't r+ this, but I'll leave this open (without an r- just in case some other reviewer really believes this is the correct way to address the issue).

[Ben Taylor]

(In reply to comment #21)
> (In reply to comment #20)
> > Created an attachment (id=88949) [details] [details]
> > Patch to fix a compile issue on Solaris 10 with Sun Studio 12, which includes <sys/time.h>, updated per bug comments
> 
> OK the patch is well structured. Looking at the patch, I still believe what I said before is correct:
> 
> "It looks like the failures for the QT and cr-linux builds are because the files which include the header must not have the define for OS(). I believe this is in config.h, so I would make sure those files include it.  (The may involve modified a script which generates some files.)"

The only time the QT and cr-linux tests failed were when I isolated the sys/time.h include with #if OS(SOLARIS), which failed because QT and cr-linux 
because the OS macro was not available there. 

> Specifically, I'm saying that the real problem is that there are cpp files which aren't doing #include "config.h" and fixing that is what should be done as opposed to including wtf/Platform.h

so including a "config.h" file which includes a pile of stuff is preferable to a surgical include with wtf/Platform.h?  Whatever.  I'll test your method and if it works, great, and I can be done with this patch and this disagreement.

> So I can't r+ this, but I'll leave this open (without an r- just in case some other reviewer really believes this is the correct way to address the issue).

I hope someone else can review and either confirm your suspicion or confirm my patch.

[David Levin]

(In reply to comment #22) 
> so including a "config.h" file which includes a pile of stuff is preferable to a surgical include with wtf/Platform.h?  Whatever.  I'll test your method and if it works, great, and I can be done with this patch and this disagreement.

Note including config.h in the cpp's not the header file.

"All implementation files must #include "config.h" first. Header files should never include "config.h"." -- http://www.webkit.org/coding/coding-style.html

Really the include of wtf/Platform.h is working around a problem in a cpp file. No header should have to include wtf/Platform.h The only thing I don't know is if we make exceptions in some odd cases but I'm not aware of them.

fwiw, I'm trying to help you. I'll admit that my note about sorting headers was off. At that point I wasn't looking at the patch. Only looking at and reacting to the notes about check-webkit-style giving false positives and a quick glance showed me this wasn't true.

if you feel that I'm not being helpful, I'm willing to go away and not touch this bug anymore.

[Ben Taylor]

(In reply to comment #23)
> (In reply to comment #22) 
> > so including a "config.h" file which includes a pile of stuff is preferable to a surgical include with wtf/Platform.h?  Whatever.  I'll test your method and if it works, great, and I can be done with this patch and this disagreement.
> 
> Note including config.h in the cpp's not the header file.

I'm assuming (other than the style issues) that you meant that I should be able to replace <wtf/Platform.h> and expect that <sys/time.h> would be included on SOLARIS systems only.  (Which is exactly what I've done on my
local base for compile testing) 

> "All implementation files must #include "config.h" first. Header files should never include "config.h"." -- http://www.webkit.org/coding/coding-style.html

indirect includes are always bad..

> Really the include of wtf/Platform.h is working around a problem in a cpp file. No header should have to include wtf/Platform.h The only thing I don't know is if we make exceptions in some odd cases but I'm not aware of them.

oh, I'm starting to see where you're going here.  So any cpp file that includes those three patched includes, would require the config.h, if they don't. I bet this patch now explodes with the number of files now required to be modified.  

> fwiw, I'm trying to help you. I'll admit that my note about sorting headers was off. At that point I wasn't looking at the patch. Only looking at and reacting to the notes about check-webkit-style giving false positives and a quick glance showed me this wasn't true.

Consider that running check-webkit-style against the files I'm working on,
and having it spit out a bunch of errors prior to modification, and then be told you're not doing it the right way.  Seems a bit hypocritical. But I can also see that the style engine may have come later and other files may still have style issues that won't be fixed, just for style issues.   I'm ok with that.  Just confusing..

> if you feel that I'm not being helpful, I'm willing to go away and not touch this bug anymore.

I'm happy for reviews, just having a hard time trying to get this patch into shape given all the "constraints".

[David Levin]

(In reply to comment #24)
> oh, I'm starting to see where you're going here.  So any cpp file that includes those three patched includes, would require the config.h, if they don't.

Yes.

> I bet this patch now explodes with the number of files now required to be modified.  

Hopefully not. We're pretty good about this, but I bet you're hitting a problem in the autogenerated files. It is unfortunate if they don't properly include config.h, but I guess no one noticed and since they are autogenerated our style checker wouldn't have caught that like it normally would.

 Perhaps I can work up a fix for that (if you don't get to it first... I think the files are here: http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/scripts (Well on the web but you can find them locally as well).

[David Levin]

(In reply to comment #25)
> (In reply to comment #24)
>  Perhaps I can work up a fix for that (if you don't get to it first... I think the files are here: http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/scripts (Well on the web but you can find them locally as well).

Hmm I started looking into this and it isn't obvious to me that I pointed out the correct place. (This isn't an area that I deal with a lot.) I need to attend to some other items at the moment, but I'll try to fix the include config.h issue for the v8 files at least this evening (in about 5 hours).

[David Levin]

Created attachment 89034 [details]
Test Patch for EWS

[David Levin]

(In reply to comment #27)
> Created an attachment (id=89034) [details]
> Patch

Ok, the problem in the original patches appears to have been the use of #ifdef instead of #if.

I missed that and unfortunately brought config.h into the picture which just confused things.

Waiting a bit longer on the win build to make sure it passes before r+'ing.

[WebKit Commit Bot]

Comment on attachment 89034 [details]
Test Patch for EWS


Clearing flags on attachment: 89034

Committed r83526: <http://trac.webkit.org/changeset/83526>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.