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.
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.
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.
(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]
Attachment 87700 [details] did not build on win: Build output: http://queues.webkit.org/results/8314266
(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
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.
Attachment 87961 [details] did not build on qt: Build output: http://queues.webkit.org/results/8315685
Attachment 87961 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8307355
Attachment 87961 [details] did not build on win: Build output: http://queues.webkit.org/results/8320384
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-.
(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.
(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?).
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.
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.
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
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.
(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.
(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.
(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.
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
(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).
(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.
(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.
(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".
(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).
(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).
Created attachment 89034 [details] Test Patch for EWS
(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.
Comment on attachment 89034 [details] Test Patch for EWS Clearing flags on attachment: 89034 Committed r83526: <http://trac.webkit.org/changeset/83526>
All reviewed patches have been landed. Closing bug.