Bug 66521 – TestWebKitAPI has issues due to FastMalloc incompatibility

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Dmitry Lomov]

The fix for  https://bugs.webkit.org/show_bug.cgi?id=61812 was incomplete.

[Dmitry Lomov]

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

[David Levin]

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.

[Dmitry Lomov]

(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.

[Dmitry Lomov]

Created attachment 104531 [details]
Recovered gtest unit-tests

[David Levin]

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.

[Dmitry Lomov]

Created attachment 104540 [details]
README fixed up

[Dmitry Lomov]

Landed in http://trac.webkit.org/changeset/93426. Closing bug.

[Dmitry Lomov]

Some patches collided in mid-air causing MacOS build of TestWebKitAPI to fail.

[David Kilzer (:ddkilzer)]

(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>

[David Kilzer (:ddkilzer)]

Comment on attachment 104540 [details]
README fixed up

Obsoleting this attachment since it was rolled out in r93451.

[David Kilzer (:ddkilzer)]

*** Bug 69942 has been marked as a duplicate of this bug. ***

[David Kilzer (:ddkilzer)]

<rdar://problem/10607911>

[David Kilzer (:ddkilzer)]

Created attachment 120211 [details]
Patch v4

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

Created attachment 120373 [details]
Patch v5

[David Kilzer (:ddkilzer)]

(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).

[WebKit Review Bot]

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.

[Dmitry Lomov]

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.

[David Kilzer (:ddkilzer)]

(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.

[Dmitry Lomov]

(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.

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

(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.

[David Kilzer (:ddkilzer)]

Created attachment 121167 [details]
Patch v6

[WebKit Review Bot]

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.

[David Kilzer (:ddkilzer)]

(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.

[David Kilzer (:ddkilzer)]

(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

[David Kilzer (:ddkilzer)]

Committed r104091: <http://trac.webkit.org/changeset/104091>

[Dmitry Lomov]

(In reply to comment #27)
> Committed r104091: <http://trac.webkit.org/changeset/104091>

Horray!

[David Kilzer (:ddkilzer)]

(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>

[David Kilzer (:ddkilzer)]

*** Bug 74127 has been marked as a duplicate of this bug. ***