Summary: | TestWebKitAPI has issues due to FastMalloc incompatibility | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dmitry Lomov <dslomov> | ||||||||||||||
Component: | Tools / Tests | Assignee: | Dmitry Lomov <dslomov> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | aroben, bfulgham, ddkilzer, fpizlo, levin, mrowe, sam, simon.fraser, webkit-bug-importer, webkit.review.bot | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Bug Depends on: | 66607, 75153 | ||||||||||||||||
Bug Blocks: | |||||||||||||||||
Attachments: |
|
Description
Dmitry Lomov
2011-08-18 19:30:11 PDT
Created attachment 104438 [details]
This patch ensures that gtest uses new and delete operators that are defined in JavaScriptCore.
Also some cleanup of gtest.xcodeproj
Comment on attachment 104438 [details]
This patch ensures that gtest uses new and delete operators that are defined in JavaScriptCore.
So your clean up consisted of getting rid of the code which tests gtest itself?
It seems like it would be good to keep this running in case anyone does local changes to gtest, they can still be verified.
(In reply to comment #2) > (From update of attachment 104438 [details]) > So your clean up consisted of getting rid of the code which tests gtest itself? > > It seems like it would be good to keep this running in case anyone does local changes to gtest, they can still be verified. Yes, good point. Created attachment 104531 [details]
Recovered gtest unit-tests
Comment on attachment 104531 [details] Recovered gtest unit-tests Seems fine except it would be good to modify the readme (http://trac.webkit.org/browser/trunk/Source/ThirdParty/gtest/README.WebKit) to keep track of the local changes done. It would be good to include info about the last changes as well. The purpose is so that when someone wants to pull down a new version of gtest they can modify it in the same way that we have done locally. Created attachment 104540 [details]
README fixed up
Landed in http://trac.webkit.org/changeset/93426. Closing bug. Some patches collided in mid-air causing MacOS build of TestWebKitAPI to fail. (In reply to comment #7) > Landed in http://trac.webkit.org/changeset/93426. Closing bug. This was rolled out in r93451 by Bug 66607. <http://trac.webkit.org/changeset/93451> Comment on attachment 104540 [details] README fixed up Obsoleting this attachment since it was rolled out in r93451. *** Bug 69942 has been marked as a duplicate of this bug. *** Created attachment 120211 [details]
Patch v4
Comment on attachment 120211 [details]
Patch v4
I am rebasing this patch to a more recent trunk. The changes in Tools/TestWebKitAPI/config.h have been replaced by some new headers in JavaScriptCore, and I need to make similar changes to gtest-port.h.
Created attachment 120373 [details]
Patch v5
(In reply to comment #15) > Created an attachment (id=120373) [details] > Patch v5 Changes since Patch v4: - Removed Tools/TestWebKitAPI/config.h changes as they were no longer needed. - Replaced various macro definitions in gtest-port.h with wtf headers (much cleaner). Attachment 120373 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/ThirdParty/ChangeLog', u'Source/Thi..." exit_code: 1
Source/ThirdParty/gtest/include/gtest/internal/gtest-port.h:181: Alphabetical sorting problem. [build/include_order] [4]
Total errors found: 1 in 6 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 120373 [details]
Patch v5
The patch looks good to me, other than style issue.
I confirmed it builds and runs on Windows.
(In reply to comment #17) > Attachment 120373 [details] did not pass style-queue: > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/ThirdParty/ChangeLog', u'Source/Thi..." exit_code: 1 > > Source/ThirdParty/gtest/include/gtest/internal/gtest-port.h:181: Alphabetical sorting problem. [build/include_order] [4] > Total errors found: 1 in 6 files 1. gtest-port.h is ThirdParty source. I don't think we apply webkit style rules to third-party source, do we? (Running check-webkit-style with no arguments shows gtest-port.h has 420 style violations.) 2. It's complaining that <wtf/Platform.h> appears before <wtf/ExportMacros.h>, but <wtf/Platform.h> must appear first in that list. I think this style warning is a non-issue for this patch. (In reply to comment #19) > (In reply to comment #17) > > Attachment 120373 [details] [details] did not pass style-queue: > > > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/ThirdParty/ChangeLog', u'Source/Thi..." exit_code: 1 > > > > Source/ThirdParty/gtest/include/gtest/internal/gtest-port.h:181: Alphabetical sorting problem. [build/include_order] [4] > > Total errors found: 1 in 6 files > > 1. gtest-port.h is ThirdParty source. I don't think we apply webkit style rules to third-party source, do we? (Running check-webkit-style with no arguments shows gtest-port.h has 420 style violations.) > > 2. It's complaining that <wtf/Platform.h> appears before <wtf/ExportMacros.h>, but <wtf/Platform.h> must appear first in that list. > > I think this style warning is a non-issue for this patch. Yes - ok looked though this - I think it is ok too. Comment on attachment 120373 [details]
Patch v5
Clearing the review flag.
The way that the WebCore headers are referenced will cause Apple internal builds to fail because WebCore.framework won't be at $(BUILT_PRODUCTS_DIR). We need to add a Production target to gtest to make it work, then redefine the variable where the WebCore headers are searched for in that configuration.
I will do this for the next patch.
(In reply to comment #21) > (From update of attachment 120373 [details]) > Clearing the review flag. > > The way that the WebCore headers are referenced will cause Apple internal builds to fail because WebCore.framework won't be at $(BUILT_PRODUCTS_DIR). We need to add a Production target to gtest to make it work, then redefine the variable where the WebCore headers are searched for in that configuration. > > I will do this for the next patch. Actually, I should add the Production configuration in a separate patch first. See Bug 75153. Created attachment 121167 [details]
Patch v6
Attachment 121167 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/ThirdParty/ChangeLog', u'Source/Thi..." exit_code: 1
Source/ThirdParty/gtest/include/gtest/internal/gtest-port.h:181: Alphabetical sorting problem. [build/include_order] [4]
Total errors found: 1 in 9 files
If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #24) > Attachment 121167 [details] did not pass style-queue: > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/ThirdParty/ChangeLog', u'Source/Thi..." exit_code: 1 > > Source/ThirdParty/gtest/include/gtest/internal/gtest-port.h:181: Alphabetical sorting problem. [build/include_order] [4] > Total errors found: 1 in 9 files > > > If any of these errors are false positives, please file a bug against check-webkit-style. See Comment #17, Comment #19, Comment #20. The headers must be in this order. (In reply to comment #23) > Created an attachment (id=121167) [details] > Patch v6 Changes since v5: - Defined HEADER_SEARCH_PATHS in terms of WEBCORE_PRIVATE_HEADERS_DIR. - Added WEBCORE_PRIVATE_HEADERS_DIR variable to [Debug|Release|Production]Project.xcconfig so that the correct path would be used on Production builds. I verified that Release (-) versus Production (+) builds set the correct HEADER_SEARCH_PATHS and WEBCORE_PRIVATE_HEADERS_DIR paths: - setenv HEADER_SEARCH_PATHS "/Volumes/Data/Build/Release/WebCore.framework/PrivateHeaders/ForwardingHeaders ../ ../include/" + setenv HEADER_SEARCH_PATHS "/System/Library/Frameworks/WebKit.framework/Versions/A/Frameworks/WebCore.framework/PrivateHeaders/ForwardingHeaders ../ ../include/" - setenv WEBCORE_PRIVATE_HEADERS_DIR /Volumes/Data/Build/Release/WebCore.framework/PrivateHeaders + setenv WEBCORE_PRIVATE_HEADERS_DIR /System/Library/Frameworks/WebKit.framework/Versions/A/Frameworks/WebCore.framework/PrivateHeaders Committed r104091: <http://trac.webkit.org/changeset/104091> (In reply to comment #27) > Committed r104091: <http://trac.webkit.org/changeset/104091> Horray! (In reply to comment #27) > Committed r104091: <http://trac.webkit.org/changeset/104091> Follow-up build fix for internal Safari builds: Committed r104118: <http://trac.webkit.org/changeset/104118> *** Bug 74127 has been marked as a duplicate of this bug. *** |